since-v22.diff

text/x-patch

Download

Patch

Format: unified

Series: patch v22

File	+	−
src/backend/libpq/be-secure-nss.c	108	14

commit cf6a3e49488b9e795e28713f81f6a4d32702de5d
Author: Jacob Champion <pchampion@vmware.com>
Date:   Tue Jan 19 10:32:27 2021 -0800

    NSS: handle client cert Common Names like be-secure-openssl
    
    Store an unquoted Common Name into port->peer_cn. This is the bulk of
    the patch, because CERT_GetCommonName() always performs RFC 4514
    quoting, and the authentication code in the core expects raw data for
    direct string comparison.
    
    These changes allow us to handle
    - usernames with quotable characters, such as backslashes
    - certificates with other name attributes besides Common Name (country
      code, organizational unit, etc.)
    - certificates without a Common Name
    
    Certificates with embedded NULL bytes in the Common Name will be
    rejected in the same manner as the OpenSSL implementation.

diff --git a/src/backend/libpq/be-secure-nss.c b/src/backend/libpq/be-secure-nss.c
index 111b46dea8..80d7598662 100644
--- a/src/backend/libpq/be-secure-nss.c
+++ b/src/backend/libpq/be-secure-nss.c
@@ -1051,6 +1051,69 @@ pg_bad_cert_handler(void *arg, PRFileDesc * fd)
 	return SECFailure;
 }
 
+/*
+ * Returns the Subject Common Name for the given certificate as a raw char
+ * buffer (that is, without any form of escaping for unprintable characters or
+ * embedded nulls), with the length of the buffer returned in the len parameter.
+ * The buffer is allocated in the TopMemoryContext and is given a NULL
+ * terminator so that callers are safe to call strlen() on it.
+ *
+ * This is used instead of CERT_GetCommonName(), which always performs quoting
+ * and/or escaping. NSS doesn't appear to give us a way to easily unescape the
+ * result, and we need to store the raw CN into port->peer_cn for compatibility
+ * with the OpenSSL implementation.
+ */
+static char *
+raw_subject_common_name(CERTCertificate *cert, unsigned int *len)
+{
+	CERTName	subject = cert->subject;
+	CERTRDN	  **rdn;
+
+	for (rdn = subject.rdns; *rdn; rdn++)
+	{
+		CERTAVA	  **ava;
+
+		for (ava = (*rdn)->avas; *ava; ava++)
+		{
+			SECItem	   *buf;
+			char	   *cn;
+
+			if (CERT_GetAVATag(*ava) != SEC_OID_AVA_COMMON_NAME)
+				continue;
+
+			/* Found a CN. Decode and copy it into a newly allocated buffer. */
+			buf = CERT_DecodeAVAValue(&(*ava)->value);
+			if (!buf)
+			{
+				/*
+				 * This failure case is difficult to test. (Since this code runs
+				 * after certificate authentication has otherwise succeeded,
+				 * you'd need to convince a CA implementation to sign a
+				 * corrupted certificate in order to get here.)
+				 *
+				 * Follow the behavior of CERT_GetCommonName() in this case and
+				 * simply return NULL, as if a Common Name had not been found.
+				 */
+				goto fail;
+			}
+
+			cn = MemoryContextAlloc(TopMemoryContext, buf->len + 1);
+			memcpy(cn, buf->data, buf->len);
+			cn[buf->len] = '\0';
+
+			*len = buf->len;
+
+			SECITEM_FreeItem(buf, PR_TRUE);
+			return cn;
+		}
+	}
+
+fail:
+	/* Not found. */
+	*len = 0;
+	return NULL;
+}
+
 static SECStatus
 pg_cert_auth_handler(void *arg, PRFileDesc * fd, PRBool checksig, PRBool isServer)
 {
@@ -1058,28 +1121,59 @@ pg_cert_auth_handler(void *arg, PRFileDesc * fd, PRBool checksig, PRBool isServe
 	Port	   *port = (Port *) arg;
 	CERTCertificate *cert;
 	char	   *peer_cn;
-	int			len;
+	unsigned int len;
 
 	status = SSL_AuthCertificate(CERT_GetDefaultCertDB(), port->pr_fd, checksig, PR_TRUE);
-	if (status == SECSuccess)
+	if (status != SECSuccess)
+		return status;
+
+	port->peer_cn = NULL;
+	port->peer_cert_valid = false;
+
+	cert = SSL_PeerCertificate(port->pr_fd);
+	if (!cert)
 	{
-		cert = SSL_PeerCertificate(port->pr_fd);
-		len = strlen(cert->subjectName);
-		peer_cn = MemoryContextAllocZero(TopMemoryContext, len + 1);
+		/* Shouldn't be possible; why did we get a client cert callback? */
+		Assert(cert != NULL);
 
-		/*
-		 * Skip over the key= portion of the key=value containing the peer CN.
-		 */
-		if (strncmp(cert->subjectName, "CN=", 3) == 0)
-			strlcpy(peer_cn, cert->subjectName + strlen("CN="), len + 1);
-		else
-			strlcpy(peer_cn, cert->subjectName, len + 1);
-		CERT_DestroyCertificate(cert);
+		PR_SetError(SEC_ERROR_LIBRARY_FAILURE, 0);
+		return SECFailure;
+	}
 
-		port->peer_cn = peer_cn;
+	peer_cn = raw_subject_common_name(cert, &len);
+	if (!peer_cn)
+	{
+		/* No Common Name, but the certificate otherwise checks out. */
 		port->peer_cert_valid = true;
+
+		status = SECSuccess;
+		goto cleanup;
 	}
 
+	/*
+	 * Reject embedded NULLs in certificate common name to prevent attacks like
+	 * CVE-2009-4034.
+	 */
+	if (len != strlen(peer_cn))
+	{
+		ereport(COMMERROR,
+				(errcode(ERRCODE_PROTOCOL_VIOLATION),
+				 errmsg("SSL certificate's common name contains embedded null")));
+
+		pfree(peer_cn);
+		PR_SetError(SEC_ERROR_CERT_NOT_VALID, 0);
+
+		status = SECFailure;
+		goto cleanup;
+	}
+
+	port->peer_cn = peer_cn;
+	port->peer_cert_valid = true;
+
+	status = SECSuccess;
+
+cleanup:
+	CERT_DestroyCertificate(cert);
 	return status;
 }