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

3 files changed, 238 insertions, 13 deletions

diff --git a/src/interfaces/libpq/fe-secure-common.c b/src/interfaces/libpq/fe-secure-common.c
index bd46f08faee..165a6ed9b7b 100644
--- a/src/interfaces/libpq/fe-secure-common.c
+++ b/src/interfaces/libpq/fe-secure-common.c
@@ -19,6 +19,8 @@
 
 #include "postgres_fe.h"
 
+#include <arpa/inet.h>
+
 #include "fe-secure-common.h"
 
 #include "libpq-int.h"
@@ -145,6 +147,108 @@ pq_verify_peer_name_matches_certificate_name(PGconn *conn,
 }
 
 /*
+ * Check if an IP address from a server's certificate matches the peer's
+ * hostname (which must itself be an IPv4/6 address).
+ *
+ * Returns 1 if the address matches, and 0 if it does not. On error, returns
+ * -1, and sets the libpq error message.
+ *
+ * A string representation of the certificate's IP address is returned in
+ * *store_name. The caller is responsible for freeing it.
+ */
+int
+pq_verify_peer_name_matches_certificate_ip(PGconn *conn,
+										   const unsigned char *ipdata,
+										   size_t iplen,
+										   char **store_name)
+{
+	char	   *addrstr;
+	int			match = 0;
+	char	   *host = conn->connhost[conn->whichhost].host;
+	int			family;
+	char		tmp[sizeof "ffff:ffff:ffff:ffff:ffff:ffff:255.255.255.255"];
+	char		sebuf[PG_STRERROR_R_BUFLEN];
+
+	*store_name = NULL;
+
+	if (!(host && host[0] != '\0'))
+	{
+		appendPQExpBufferStr(&conn->errorMessage,
+							 libpq_gettext("host name must be specified\n"));
+		return -1;
+	}
+
+	/*
+	 * The data from the certificate is in network byte order. Convert our
+	 * host string to network-ordered bytes as well, for comparison. (The host
+	 * string isn't guaranteed to actually be an IP address, so if this
+	 * conversion fails we need to consider it a mismatch rather than an
+	 * error.)
+	 */
+	if (iplen == 4)
+	{
+		/* IPv4 */
+		struct in_addr addr;
+
+		family = AF_INET;
+
+		/*
+		 * The use of inet_aton() is deliberate; we accept alternative IPv4
+		 * address notations that are accepted by inet_aton() but not
+		 * inet_pton() as server addresses.
+		 */
+		if (inet_aton(host, &addr))
+		{
+			if (memcmp(ipdata, &addr.s_addr, iplen) == 0)
+				match = 1;
+		}
+	}
+	/*
+	 * If they don't have inet_pton(), skip this.  Then, an IPv6 address in a
+	 * certificate will cause an error.
+	 */
+#ifdef HAVE_INET_PTON
+	else if (iplen == 16)
+	{
+		/* IPv6 */
+		struct in6_addr addr;
+
+		family = AF_INET6;
+
+		if (inet_pton(AF_INET6, host, &addr) == 1)
+		{
+			if (memcmp(ipdata, &addr.s6_addr, iplen) == 0)
+				match = 1;
+		}
+	}
+#endif
+	else
+	{
+		/*
+		 * Not IPv4 or IPv6. We could ignore the field, but leniency seems
+		 * wrong given the subject matter.
+		 */
+		appendPQExpBuffer(&conn->errorMessage,
+						  libpq_gettext("certificate contains IP address with invalid length %lu\n"),
+						  (unsigned long) iplen);
+		return -1;
+	}
+
+	/* Generate a human-readable representation of the certificate's IP. */
+	addrstr = pg_inet_net_ntop(family, ipdata, 8 * iplen, tmp, sizeof(tmp));
+	if (!addrstr)
+	{
+		appendPQExpBuffer(&conn->errorMessage,
+						  libpq_gettext("could not convert certificate's IP address to string: %s\n"),
+						  strerror_r(errno, sebuf, sizeof(sebuf)));
+		return -1;
+	}
+
+	*store_name = strdup(addrstr);
+	return match;
+}
+
+/*
  * Verify that the server certificate matches the hostname we connected to.
  *
  * The certificate's Common Name and Subject Alternative Names are considered.
diff --git a/src/interfaces/libpq/fe-secure-common.h b/src/interfaces/libpq/fe-secure-common.h
index 1cca6d785ac..d18db7138cc 100644
--- a/src/interfaces/libpq/fe-secure-common.h
+++ b/src/interfaces/libpq/fe-secure-common.h
@@ -21,6 +21,10 @@
 extern int	pq_verify_peer_name_matches_certificate_name(PGconn *conn,
 														 const char *namedata, size_t namelen,
 														 char **store_name);
+extern int	pq_verify_peer_name_matches_certificate_ip(PGconn *conn,
+													   const unsigned char *addrdata,
+													   size_t addrlen,
+													   char **store_name);
 extern bool pq_verify_peer_name_matches_certificate(PGconn *conn);
 
 #endif							/* FE_SECURE_COMMON_H */
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);
+				}
 			}
 		}
 	}