Don't reflect unescaped cert data to the logs

Commit 3a0e385048 introduced a new path for unauthenticated bytes from
the client certificate to be printed unescaped to the logs. There are a
handful of these already, but it doesn't make sense to keep making the
problem worse. \x-escape any unprintable bytes.

The test case introduces a revoked UTF-8 certificate. This requires the
addition of the `-utf8` flag to `openssl req`. Since the existing
certificates all use an ASCII subset, this won't modify the existing
certificates' subjects if/when they get regenerated; this was verified
experimentally with

    $ make sslfiles-clean
    $ make sslfiles

Unfortunately the test can't be run in the CI yet due to a test timing
issue; see 55828a6b60.

Author: Jacob Champion <jchampion@timescale.com>
Discussion: https://www.postgresql.org/message-id/CAAWbhmgsvHrH9wLU2kYc3pOi1KSenHSLAHBbCVmmddW6-mc_=w@mail.gmail.com

1 files changed, 31 insertions, 26 deletions

diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 55d4b29f7e1..03565573876 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -27,12 +27,14 @@
 #include <netinet/tcp.h>
 #include <arpa/inet.h>
 
+#include "common/string.h"
 #include "libpq/libpq.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "storage/fd.h"
 #include "storage/latch.h"
 #include "tcop/tcopprot.h"
+#include "utils/builtins.h"
 #include "utils/memutils.h"
 
 /*
@@ -1080,16 +1082,16 @@ dummy_ssl_passwd_cb(char *buf, int size, int rwflag, void *userdata)
 }
 
 /*
- * Examines the provided certificate name, and if it's too long to log, modifies
- * and truncates it. The return value is NULL if no truncation was needed; it
- * otherwise points into the middle of the input string, and should not be
- * freed.
+ * Examines the provided certificate name, and if it's too long to log or
+ * contains unprintable ASCII, escapes and truncates it. The return value is
+ * always a new palloc'd string. (The input string is still modified in place,
+ * for ease of implementation.)
  */
 static char *
-truncate_cert_name(char *name)
+prepare_cert_name(char *name)
 {
 	size_t		namelen = strlen(name);
-	char	   *truncated;
+	char	   *truncated = name;
 
 	/*
 	 * Common Names are 64 chars max, so for a common case where the CN is the
@@ -1099,19 +1101,20 @@ truncate_cert_name(char *name)
 	 */
 #define MAXLEN 71
 
-	if (namelen <= MAXLEN)
-		return NULL;
-
-	/*
-	 * Keep the end of the name, not the beginning, since the most specific
-	 * field is likely to give users the most information.
-	 */
-	truncated = name + namelen - MAXLEN;
-	truncated[0] = truncated[1] = truncated[2] = '.';
+	if (namelen > MAXLEN)
+	{
+		/*
+		 * Keep the end of the name, not the beginning, since the most specific
+		 * field is likely to give users the most information.
+		 */
+		truncated = name + namelen - MAXLEN;
+		truncated[0] = truncated[1] = truncated[2] = '.';
+		namelen = MAXLEN;
+	}
 
 #undef MAXLEN
 
-	return truncated;
+	return pg_clean_ascii(truncated, 0);
 }
 
 /*
@@ -1154,21 +1157,24 @@ verify_cb(int ok, X509_STORE_CTX *ctx)
 	{
 		char	   *subject,
 				   *issuer;
-		char	   *sub_truncated,
-				   *iss_truncated;
+		char	   *sub_prepared,
+				   *iss_prepared;
 		char	   *serialno;
 		ASN1_INTEGER *sn;
 		BIGNUM	   *b;
 
 		/*
 		 * Get the Subject and Issuer for logging, but don't let maliciously
-		 * huge certs flood the logs.
+		 * huge certs flood the logs, and don't reflect non-ASCII bytes into it
+		 * either.
 		 */
 		subject = X509_NAME_to_cstring(X509_get_subject_name(cert));
-		sub_truncated = truncate_cert_name(subject);
+		sub_prepared = prepare_cert_name(subject);
+		pfree(subject);
 
 		issuer = X509_NAME_to_cstring(X509_get_issuer_name(cert));
-		iss_truncated = truncate_cert_name(issuer);
+		iss_prepared = prepare_cert_name(issuer);
+		pfree(issuer);
 
 		/*
 		 * Pull the serial number, too, in case a Subject is still ambiguous.
@@ -1181,14 +1187,13 @@ verify_cb(int ok, X509_STORE_CTX *ctx)
 		appendStringInfoChar(&str, '\n');
 		appendStringInfo(&str,
 						 _("Failed certificate data (unverified): subject \"%s\", serial number %s, issuer \"%s\"."),
-						 sub_truncated ? sub_truncated : subject,
-						 serialno ? serialno : _("unknown"),
-						 iss_truncated ? iss_truncated : issuer);
+						 sub_prepared, serialno ? serialno : _("unknown"),
+						 iss_prepared);
 
 		BN_free(b);
 		OPENSSL_free(serialno);
-		pfree(issuer);
-		pfree(subject);
+		pfree(iss_prepared);
+		pfree(sub_prepared);
 	}
 
 	/* Store our detail message to be logged later. */