libpq: Allow IP address SANs in server certificates

The current implementation supports exactly one IP address in a server
certificate's Common Name, which is brittle (the strings must match
exactly).  This patch adds support for IPv4 and IPv6 addresses in a
server's Subject Alternative Names.

Per discussion on-list:

- If the client's expected host is an IP address, we allow fallback to
  the Subject Common Name if an iPAddress SAN is not present, even if
  a dNSName is present.  This matches the behavior of NSS, in
  violation of the relevant RFCs.

- We also, counter-intuitively, match IP addresses embedded in dNSName
  SANs.  From inspection this appears to have been the behavior since
  the SAN matching feature was introduced in acd08d76.

- Unlike NSS, we don't map IPv4 to IPv6 addresses, or vice-versa.

Author: Jacob Champion <pchampion@vmware.com>
Co-authored-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Co-authored-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://www.postgresql.org/message-id/flat/9f5f20974cd3a4091a788cf7f00ab663d5fcdffe.camel@vmware.com

1 files changed, 130 insertions, 13 deletions

diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 0cba5c5cf2f..24a598b6e41 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -72,6 +72,9 @@ static int	verify_cb(int ok, X509_STORE_CTX *ctx);
 static int	openssl_verify_peer_name_matches_certificate_name(PGconn *conn,
 															  ASN1_STRING *name,
 															  char **store_name);
+static int	openssl_verify_peer_name_matches_certificate_ip(PGconn *conn,
+															ASN1_OCTET_STRING *addr_entry,
+															char **store_name);
 static void destroy_ssl_system(void);
 static int	initialize_SSL(PGconn *conn);
 static PostgresPollingStatusType open_client_SSL(PGconn *);
@@ -510,6 +513,56 @@ openssl_verify_peer_name_matches_certificate_name(PGconn *conn, ASN1_STRING *nam
 }
 
 /*
+ * OpenSSL-specific wrapper around
+ * pq_verify_peer_name_matches_certificate_ip(), converting the
+ * ASN1_OCTET_STRING into a plain C string.
+ */
+static int
+openssl_verify_peer_name_matches_certificate_ip(PGconn *conn,
+												ASN1_OCTET_STRING *addr_entry,
+												char **store_name)
+{
+	int			len;
+	const unsigned char *addrdata;
+
+	/* Should not happen... */
+	if (addr_entry == NULL)
+	{
+		appendPQExpBufferStr(&conn->errorMessage,
+							 libpq_gettext("SSL certificate's address entry is missing\n"));
+		return -1;
+	}
+
+	/*
+	 * GEN_IPADD is an OCTET STRING containing an IP address in network byte
+	 * order.
+	 */
+#ifdef HAVE_ASN1_STRING_GET0_DATA
+	addrdata = ASN1_STRING_get0_data(addr_entry);
+#else
+	addrdata = ASN1_STRING_data(addr_entry);
+#endif
+	len = ASN1_STRING_length(addr_entry);
+
+	return pq_verify_peer_name_matches_certificate_ip(conn, addrdata, len, store_name);
+}
+
+static bool
+is_ip_address(const char *host)
+{
+	struct in_addr dummy4;
+#ifdef HAVE_INET_PTON
+	struct in6_addr dummy6;
+#endif
+
+	return inet_aton(host, &dummy4)
+#ifdef HAVE_INET_PTON
+		|| (inet_pton(AF_INET6, host, &dummy6) == 1)
+#endif
+		;
+}
+
+/*
  *	Verify that the server certificate matches the hostname we connected to.
  *
  * The certificate's Common Name and Subject Alternative Names are considered.
@@ -522,6 +575,36 @@ pgtls_verify_peer_name_matches_certificate_guts(PGconn *conn,
 	STACK_OF(GENERAL_NAME) * peer_san;
 	int			i;
 	int			rc = 0;
+	char	   *host = conn->connhost[conn->whichhost].host;
+	int			host_type;
+	bool		check_cn = true;
+
+	Assert(host && host[0]);	/* should be guaranteed by caller */
+
+	/*
+	 * We try to match the NSS behavior here, which is a slight departure from
+	 * the spec but seems to make more intuitive sense:
+	 *
+	 * If connhost contains a DNS name, and the certificate's SANs contain any
+	 * dNSName entries, then we'll ignore the Subject Common Name entirely;
+	 * otherwise, we fall back to checking the CN. (This behavior matches the
+	 * RFC.)
+	 *
+	 * If connhost contains an IP address, and the SANs contain iPAddress
+	 * entries, we again ignore the CN. Otherwise, we allow the CN to match,
+	 * EVEN IF there is a dNSName in the SANs. (RFC 6125 prohibits this: "A
+	 * client MUST NOT seek a match for a reference identifier of CN-ID if the
+	 * presented identifiers include a DNS-ID, SRV-ID, URI-ID, or any
+	 * application-specific identifier types supported by the client.")
+	 *
+	 * NOTE: Prior versions of libpq did not consider iPAddress entries at
+	 * all, so this new behavior might break a certificate that has different
+	 * IP addresses in the Subject CN and the SANs.
+	 */
+	if (is_ip_address(host))
+		host_type = GEN_IPADD;
+	else
+		host_type = GEN_DNS;
 
 	/*
 	 * First, get the Subject Alternative Names (SANs) from the certificate,
@@ -537,38 +620,62 @@ pgtls_verify_peer_name_matches_certificate_guts(PGconn *conn,
 		for (i = 0; i < san_len; i++)
 		{
 			const GENERAL_NAME *name = sk_GENERAL_NAME_value(peer_san, i);
+			char	   *alt_name = NULL;
 
-			if (name->type == GEN_DNS)
+			if (name->type == host_type)
 			{
-				char	   *alt_name;
+				/*
+				 * This SAN is of the same type (IP or DNS) as our host name,
+				 * so don't allow a fallback check of the CN.
+				 */
+				check_cn = false;
+			}
 
+			if (name->type == GEN_DNS)
+			{
 				(*names_examined)++;
 				rc = openssl_verify_peer_name_matches_certificate_name(conn,
 																	   name->d.dNSName,
 																	   &alt_name);
+			}
+			else if (name->type == GEN_IPADD)
+			{
+				(*names_examined)++;
+				rc = openssl_verify_peer_name_matches_certificate_ip(conn,
+																	 name->d.iPAddress,
+																	 &alt_name);
+			}
 
-				if (alt_name)
-				{
-					if (!*first_name)
-						*first_name = alt_name;
-					else
-						free(alt_name);
-				}
+			if (alt_name)
+			{
+				if (!*first_name)
+					*first_name = alt_name;
+				else
+					free(alt_name);
 			}
+
 			if (rc != 0)
+			{
+				/*
+				 * Either we hit an error or a match, and either way we should
+				 * not fall back to the CN.
+				 */
+				check_cn = false;
 				break;
+			}
 		}
 		sk_GENERAL_NAME_pop_free(peer_san, GENERAL_NAME_free);
 	}
 
 	/*
-	 * If there is no subjectAltName extension of type dNSName, check the
+	 * If there is no subjectAltName extension of the matching type, check the
 	 * Common Name.
 	 *
 	 * (Per RFC 2818 and RFC 6125, if the subjectAltName extension of type
-	 * dNSName is present, the CN must be ignored.)
+	 * dNSName is present, the CN must be ignored. We break this rule if host
+	 * is an IP address; see the comment above.)
 	 */
-	if (*names_examined == 0)
+	if (check_cn)
 	{
 		X509_NAME  *subject_name;
 
@@ -581,10 +688,20 @@ pgtls_verify_peer_name_matches_certificate_guts(PGconn *conn,
 												  NID_commonName, -1);
 			if (cn_index >= 0)
 			{
+				char	   *common_name = NULL;
+
 				(*names_examined)++;
 				rc = openssl_verify_peer_name_matches_certificate_name(conn,
 																	   X509_NAME_ENTRY_get_data(X509_NAME_get_entry(subject_name, cn_index)),
-																	   first_name);
+																	   &common_name);
+
+				if (common_name)
+				{
+					if (!*first_name)
+						*first_name = common_name;
+					else
+						free(common_name);
+				}
 			}
 		}
 	}