Refactor client-side SSL certificate checking code
authorPeter Eisentraut <peter_e@gmx.net>
Sat, 27 Jan 2018 18:47:52 +0000 (13:47 -0500)
committerPeter Eisentraut <peter_e@gmx.net>
Wed, 31 Jan 2018 03:56:24 +0000 (22:56 -0500)
Separate the parts specific to the SSL library from the general logic.

The previous code structure was

open_client_SSL()
calls verify_peer_name_matches_certificate()
calls verify_peer_name_matches_certificate_name()
calls wildcard_certificate_match()

and was completely in fe-secure-openssl.c.  The new structure is

open_client_SSL() [openssl]
calls pq_verify_peer_name_matches_certificate() [generic]
calls pgtls_verify_peer_name_matches_certificate_guts() [openssl]
calls openssl_verify_peer_name_matches_certificate_name() [openssl]
calls pq_verify_peer_name_matches_certificate_name() [generic]
calls wildcard_certificate_match() [generic]

Move the generic functions into a new file fe-secure-common.c, so the
calls generally go fe-connect.c -> fe-secure.c -> fe-secure-${impl}.c ->
fe-secure-common.c, although there is a bit of back-and-forth between
the last two.

Reviewed-by: Michael Paquier <michael.paquier@gmail.com>
src/interfaces/libpq/Makefile
src/interfaces/libpq/fe-secure-common.c [new file with mode: 0644]
src/interfaces/libpq/fe-secure-common.h [new file with mode: 0644]
src/interfaces/libpq/fe-secure-openssl.c
src/interfaces/libpq/libpq-int.h
src/interfaces/libpq/nls.mk
src/tools/msvc/Mkvcbuild.pm

index 0bf1e7ef041a7ec450b8dd097d2cb54f21dc321b..abe0a50e9854b49c5f6344869272d6ae9fdb71a1 100644 (file)
@@ -52,7 +52,7 @@ OBJS += encnames.o wchar.o
 OBJS += base64.o ip.o md5.o scram-common.o saslprep.o unicode_norm.o
 
 ifeq ($(with_openssl),yes)
-OBJS += fe-secure-openssl.o sha2_openssl.o
+OBJS += fe-secure-openssl.o fe-secure-common.o sha2_openssl.o
 else
 OBJS += sha2.o
 endif
diff --git a/src/interfaces/libpq/fe-secure-common.c b/src/interfaces/libpq/fe-secure-common.c
new file mode 100644 (file)
index 0000000..40203f3
--- /dev/null
@@ -0,0 +1,204 @@
+/*-------------------------------------------------------------------------
+ *
+ * fe-secure-common.c
+ *
+ * common implementation-independent SSL support code
+ *
+ * While fe-secure.c contains the interfaces that the rest of libpq call, this
+ * file contains support routines that are used by the library-specific
+ * implementations such as fe-secure-openssl.c.
+ *
+ * Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * IDENTIFICATION
+ *   src/interfaces/libpq/fe-secure-common.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#include "postgres_fe.h"
+
+#include "fe-secure-common.h"
+
+#include "libpq-int.h"
+#include "pqexpbuffer.h"
+
+/*
+ * Check if a wildcard certificate matches the server hostname.
+ *
+ * The rule for this is:
+ * 1. We only match the '*' character as wildcard
+ * 2. We match only wildcards at the start of the string
+ * 3. The '*' character does *not* match '.', meaning that we match only
+ *    a single pathname component.
+ * 4. We don't support more than one '*' in a single pattern.
+ *
+ * This is roughly in line with RFC2818, but contrary to what most browsers
+ * appear to be implementing (point 3 being the difference)
+ *
+ * Matching is always case-insensitive, since DNS is case insensitive.
+ */
+static bool
+wildcard_certificate_match(const char *pattern, const char *string)
+{
+   int         lenpat = strlen(pattern);
+   int         lenstr = strlen(string);
+
+   /* If we don't start with a wildcard, it's not a match (rule 1 & 2) */
+   if (lenpat < 3 ||
+       pattern[0] != '*' ||
+       pattern[1] != '.')
+       return false;
+
+   /* If pattern is longer than the string, we can never match */
+   if (lenpat > lenstr)
+       return false;
+
+   /*
+    * If string does not end in pattern (minus the wildcard), we don't match
+    */
+   if (pg_strcasecmp(pattern + 1, string + lenstr - lenpat + 1) != 0)
+       return false;
+
+   /*
+    * If there is a dot left of where the pattern started to match, we don't
+    * match (rule 3)
+    */
+   if (strchr(string, '.') < string + lenstr - lenpat)
+       return false;
+
+   /* String ended with pattern, and didn't have a dot before, so we match */
+   return true;
+}
+
+/*
+ * Check if a name from a server's certificate matches the peer's hostname.
+ *
+ * Returns 1 if the name matches, and 0 if it does not. On error, returns
+ * -1, and sets the libpq error message.
+ *
+ * The name extracted from the certificate is returned in *store_name. The
+ * caller is responsible for freeing it.
+ */
+int
+pq_verify_peer_name_matches_certificate_name(PGconn *conn,
+                                            const char *namedata, size_t namelen,
+                                            char **store_name)
+{
+   char       *name;
+   int         result;
+   char       *host = PQhost(conn);
+
+   *store_name = NULL;
+
+   /*
+    * There is no guarantee the string returned from the certificate is
+    * NULL-terminated, so make a copy that is.
+    */
+   name = malloc(namelen + 1);
+   if (name == NULL)
+   {
+       printfPQExpBuffer(&conn->errorMessage,
+                         libpq_gettext("out of memory\n"));
+       return -1;
+   }
+   memcpy(name, namedata, namelen);
+   name[namelen] = '\0';
+
+   /*
+    * Reject embedded NULLs in certificate common or alternative name to
+    * prevent attacks like CVE-2009-4034.
+    */
+   if (namelen != strlen(name))
+   {
+       free(name);
+       printfPQExpBuffer(&conn->errorMessage,
+                         libpq_gettext("SSL certificate's name contains embedded null\n"));
+       return -1;
+   }
+
+   if (pg_strcasecmp(name, host) == 0)
+   {
+       /* Exact name match */
+       result = 1;
+   }
+   else if (wildcard_certificate_match(name, host))
+   {
+       /* Matched wildcard name */
+       result = 1;
+   }
+   else
+   {
+       result = 0;
+   }
+
+   *store_name = name;
+   return result;
+}
+
+/*
+ * Verify that the server certificate matches the hostname we connected to.
+ *
+ * The certificate's Common Name and Subject Alternative Names are considered.
+ */
+bool
+pq_verify_peer_name_matches_certificate(PGconn *conn)
+{
+   char       *host = PQhost(conn);
+   int         rc;
+   int         names_examined = 0;
+   char       *first_name = NULL;
+
+   /*
+    * If told not to verify the peer name, don't do it. Return true
+    * indicating that the verification was successful.
+    */
+   if (strcmp(conn->sslmode, "verify-full") != 0)
+       return true;
+
+   /* Check that we have a hostname to compare with. */
+   if (!(host && host[0] != '\0'))
+   {
+       printfPQExpBuffer(&conn->errorMessage,
+                         libpq_gettext("host name must be specified for a verified SSL connection\n"));
+       return false;
+   }
+
+   rc = pgtls_verify_peer_name_matches_certificate_guts(conn, &names_examined, &first_name);
+
+   if (rc == 0)
+   {
+       /*
+        * No match. Include the name from the server certificate in the error
+        * message, to aid debugging broken configurations. If there are
+        * multiple names, only print the first one to avoid an overly long
+        * error message.
+        */
+       if (names_examined > 1)
+       {
+           printfPQExpBuffer(&conn->errorMessage,
+                             libpq_ngettext("server certificate for \"%s\" (and %d other name) does not match host name \"%s\"\n",
+                                            "server certificate for \"%s\" (and %d other names) does not match host name \"%s\"\n",
+                                            names_examined - 1),
+                             first_name, names_examined - 1, host);
+       }
+       else if (names_examined == 1)
+       {
+           printfPQExpBuffer(&conn->errorMessage,
+                             libpq_gettext("server certificate for \"%s\" does not match host name \"%s\"\n"),
+                             first_name, host);
+       }
+       else
+       {
+           printfPQExpBuffer(&conn->errorMessage,
+                             libpq_gettext("could not get server's host name from server certificate\n"));
+       }
+   }
+
+   /* clean up */
+   if (first_name)
+       free(first_name);
+
+   return (rc == 1);
+}
diff --git a/src/interfaces/libpq/fe-secure-common.h b/src/interfaces/libpq/fe-secure-common.h
new file mode 100644 (file)
index 0000000..980a58a
--- /dev/null
@@ -0,0 +1,26 @@
+/*-------------------------------------------------------------------------
+ *
+ * fe-secure-common.h
+ *
+ * common implementation-independent SSL support code
+ *
+ * Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * IDENTIFICATION
+ *   src/interfaces/libpq/fe-secure-common.h
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#ifndef FE_SECURE_COMMON_H
+#define FE_SECURE_COMMON_H
+
+#include "libpq-fe.h"
+
+extern int pq_verify_peer_name_matches_certificate_name(PGconn *conn,
+                                            const char *namedata, size_t namelen,
+                                            char **store_name);
+extern bool pq_verify_peer_name_matches_certificate(PGconn *conn);
+
+#endif                         /* FE_SECURE_COMMON_H */
index 9ab317320a2ee9f008f5fdf77bf23337fda5cf98..cade4e157cbd04e3e27fef56f73aa6feccc0712d 100644 (file)
@@ -28,6 +28,7 @@
 
 #include "libpq-fe.h"
 #include "fe-auth.h"
+#include "fe-secure-common.h"
 #include "libpq-int.h"
 
 #ifdef WIN32
@@ -60,9 +61,8 @@
 #endif
 #include <openssl/x509v3.h>
 
-static bool verify_peer_name_matches_certificate(PGconn *);
 static int verify_cb(int ok, X509_STORE_CTX *ctx);
-static int verify_peer_name_matches_certificate_name(PGconn *conn,
+static int openssl_verify_peer_name_matches_certificate_name(PGconn *conn,
                                          ASN1_STRING *name,
                                          char **store_name);
 static void destroy_ssl_system(void);
@@ -492,76 +492,16 @@ verify_cb(int ok, X509_STORE_CTX *ctx)
 
 
 /*
- * Check if a wildcard certificate matches the server hostname.
- *
- * The rule for this is:
- * 1. We only match the '*' character as wildcard
- * 2. We match only wildcards at the start of the string
- * 3. The '*' character does *not* match '.', meaning that we match only
- *    a single pathname component.
- * 4. We don't support more than one '*' in a single pattern.
- *
- * This is roughly in line with RFC2818, but contrary to what most browsers
- * appear to be implementing (point 3 being the difference)
- *
- * Matching is always case-insensitive, since DNS is case insensitive.
- */
-static int
-wildcard_certificate_match(const char *pattern, const char *string)
-{
-   int         lenpat = strlen(pattern);
-   int         lenstr = strlen(string);
-
-   /* If we don't start with a wildcard, it's not a match (rule 1 & 2) */
-   if (lenpat < 3 ||
-       pattern[0] != '*' ||
-       pattern[1] != '.')
-       return 0;
-
-   if (lenpat > lenstr)
-       /* If pattern is longer than the string, we can never match */
-       return 0;
-
-   if (pg_strcasecmp(pattern + 1, string + lenstr - lenpat + 1) != 0)
-
-       /*
-        * If string does not end in pattern (minus the wildcard), we don't
-        * match
-        */
-       return 0;
-
-   if (strchr(string, '.') < string + lenstr - lenpat)
-
-       /*
-        * If there is a dot left of where the pattern started to match, we
-        * don't match (rule 3)
-        */
-       return 0;
-
-   /* String ended with pattern, and didn't have a dot before, so we match */
-   return 1;
-}
-
-/*
- * Check if a name from a server's certificate matches the peer's hostname.
- *
- * Returns 1 if the name matches, and 0 if it does not. On error, returns
- * -1, and sets the libpq error message.
- *
- * The name extracted from the certificate is returned in *store_name. The
- * caller is responsible for freeing it.
+ * OpenSSL-specific wrapper around
+ * pq_verify_peer_name_matches_certificate_name(), converting the ASN1_STRING
+ * into a plain C string.
  */
 static int
-verify_peer_name_matches_certificate_name(PGconn *conn, ASN1_STRING *name_entry,
-                                         char **store_name)
+openssl_verify_peer_name_matches_certificate_name(PGconn *conn, ASN1_STRING *name_entry,
+                                                 char **store_name)
 {
    int         len;
-   char       *name;
    const unsigned char *namedata;
-   int         result;
-   char       *host = PQhost(conn);
-
-   *store_name = NULL;
 
    /* Should not happen... */
    if (name_entry == NULL)
@@ -573,9 +513,6 @@ verify_peer_name_matches_certificate_name(PGconn *conn, ASN1_STRING *name_entry,
 
    /*
     * GEN_DNS can be only IA5String, equivalent to US ASCII.
-    *
-    * There is no guarantee the string returned from the certificate is
-    * NULL-terminated, so make a copy that is.
     */
 #ifdef HAVE_ASN1_STRING_GET0_DATA
    namedata = ASN1_STRING_get0_data(name_entry);
@@ -583,45 +520,9 @@ verify_peer_name_matches_certificate_name(PGconn *conn, ASN1_STRING *name_entry,
    namedata = ASN1_STRING_data(name_entry);
 #endif
    len = ASN1_STRING_length(name_entry);
-   name = malloc(len + 1);
-   if (name == NULL)
-   {
-       printfPQExpBuffer(&conn->errorMessage,
-                         libpq_gettext("out of memory\n"));
-       return -1;
-   }
-   memcpy(name, namedata, len);
-   name[len] = '\0';
-
-   /*
-    * Reject embedded NULLs in certificate common or alternative name to
-    * prevent attacks like CVE-2009-4034.
-    */
-   if (len != strlen(name))
-   {
-       free(name);
-       printfPQExpBuffer(&conn->errorMessage,
-                         libpq_gettext("SSL certificate's name contains embedded null\n"));
-       return -1;
-   }
 
-   if (pg_strcasecmp(name, host) == 0)
-   {
-       /* Exact name match */
-       result = 1;
-   }
-   else if (wildcard_certificate_match(name, host))
-   {
-       /* Matched wildcard name */
-       result = 1;
-   }
-   else
-   {
-       result = 0;
-   }
-
-   *store_name = name;
-   return result;
+   /* OK to cast from unsigned to plain char, since it's all ASCII. */
+   return pq_verify_peer_name_matches_certificate_name(conn, (const char *) namedata, len, store_name);
 }
 
 /*
@@ -629,33 +530,14 @@ verify_peer_name_matches_certificate_name(PGconn *conn, ASN1_STRING *name_entry,
  *
  * The certificate's Common Name and Subject Alternative Names are considered.
  */
-static bool
-verify_peer_name_matches_certificate(PGconn *conn)
+int
+pgtls_verify_peer_name_matches_certificate_guts(PGconn *conn,
+                                               int *names_examined,
+                                               char **first_name)
 {
-   int         names_examined = 0;
-   bool        found_match = false;
-   bool        got_error = false;
-   char       *first_name = NULL;
-
    STACK_OF(GENERAL_NAME) *peer_san;
    int         i;
-   int         rc;
-   char       *host = PQhost(conn);
-
-   /*
-    * If told not to verify the peer name, don't do it. Return true
-    * indicating that the verification was successful.
-    */
-   if (strcmp(conn->sslmode, "verify-full") != 0)
-       return true;
-
-   /* Check that we have a hostname to compare with. */
-   if (!(host && host[0] != '\0'))
-   {
-       printfPQExpBuffer(&conn->errorMessage,
-                         libpq_gettext("host name must be specified for a verified SSL connection\n"));
-       return false;
-   }
+   int         rc = 0;
 
    /*
     * First, get the Subject Alternative Names (SANs) from the certificate,
@@ -676,24 +558,20 @@ verify_peer_name_matches_certificate(PGconn *conn)
            {
                char       *alt_name;
 
-               names_examined++;
-               rc = verify_peer_name_matches_certificate_name(conn,
+               (*names_examined)++;
+               rc = openssl_verify_peer_name_matches_certificate_name(conn,
                                                               name->d.dNSName,
                                                               &alt_name);
-               if (rc == -1)
-                   got_error = true;
-               if (rc == 1)
-                   found_match = true;
 
                if (alt_name)
                {
-                   if (!first_name)
-                       first_name = alt_name;
+                   if (!*first_name)
+                       *first_name = alt_name;
                    else
                        free(alt_name);
                }
            }
-           if (found_match || got_error)
+           if (rc != 0)
                break;
        }
        sk_GENERAL_NAME_free(peer_san);
@@ -706,7 +584,7 @@ verify_peer_name_matches_certificate(PGconn *conn)
     * (Per RFC 2818 and RFC 6125, if the subjectAltName extension of type
     * dNSName is present, the CN must be ignored.)
     */
-   if (names_examined == 0)
+   if (*names_examined == 0)
    {
        X509_NAME  *subject_name;
 
@@ -719,55 +597,17 @@ verify_peer_name_matches_certificate(PGconn *conn)
                                                  NID_commonName, -1);
            if (cn_index >= 0)
            {
-               names_examined++;
-               rc = verify_peer_name_matches_certificate_name(
+               (*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);
-
-               if (rc == -1)
-                   got_error = true;
-               else if (rc == 1)
-                   found_match = true;
+                                                              first_name);
            }
        }
    }
 
-   if (!found_match && !got_error)
-   {
-       /*
-        * No match. Include the name from the server certificate in the error
-        * message, to aid debugging broken configurations. If there are
-        * multiple names, only print the first one to avoid an overly long
-        * error message.
-        */
-       if (names_examined > 1)
-       {
-           printfPQExpBuffer(&conn->errorMessage,
-                             libpq_ngettext("server certificate for \"%s\" (and %d other name) does not match host name \"%s\"\n",
-                                            "server certificate for \"%s\" (and %d other names) does not match host name \"%s\"\n",
-                                            names_examined - 1),
-                             first_name, names_examined - 1, host);
-       }
-       else if (names_examined == 1)
-       {
-           printfPQExpBuffer(&conn->errorMessage,
-                             libpq_gettext("server certificate for \"%s\" does not match host name \"%s\"\n"),
-                             first_name, host);
-       }
-       else
-       {
-           printfPQExpBuffer(&conn->errorMessage,
-                             libpq_gettext("could not get server's host name from server certificate\n"));
-       }
-   }
-
-   /* clean up */
-   if (first_name)
-       free(first_name);
-
-   return found_match && !got_error;
+   return rc;
 }
 
 #if defined(ENABLE_THREAD_SAFETY) && defined(HAVE_CRYPTO_LOCK)
@@ -1441,7 +1281,7 @@ open_client_SSL(PGconn *conn)
        return PGRES_POLLING_FAILED;
    }
 
-   if (!verify_peer_name_matches_certificate(conn))
+   if (!pq_verify_peer_name_matches_certificate(conn))
    {
        pgtls_close(conn);
        return PGRES_POLLING_FAILED;
index b3492b033a6c7c0d718e43e78b1b99123477117a..eba23dcecc8dca030526c7d0142b5908dc294850 100644 (file)
@@ -732,6 +732,19 @@ extern char *pgtls_get_finished(PGconn *conn, size_t *len);
  */
 extern char *pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len);
 
+/*
+ * Verify that the server certificate matches the host name we connected to.
+ *
+ * The certificate's Common Name and Subject Alternative Names are considered.
+ *
+ * Returns 1 if the name matches, and 0 if it does not. On error, returns
+ * -1, and sets the libpq error message.
+ *
+ */
+extern int pgtls_verify_peer_name_matches_certificate_guts(PGconn *conn,
+                                                          int *names_examined,
+                                                          char **first_name);
+
 /* === miscellaneous macros === */
 
 /*
index 2c5659e2622c41fc72b8567bbb7884d2b46edf16..4196870b496101385b619fcc403069be62eeb0f3 100644 (file)
@@ -1,6 +1,6 @@
 # src/interfaces/libpq/nls.mk
 CATALOG_NAME     = libpq
 AVAIL_LANGUAGES  = cs de es fr he it ja ko pl pt_BR ru sv tr zh_CN zh_TW
-GETTEXT_FILES    = fe-auth.c fe-auth-scram.c fe-connect.c fe-exec.c fe-lobj.c fe-misc.c fe-protocol2.c fe-protocol3.c fe-secure.c fe-secure-openssl.c win32.c
+GETTEXT_FILES    = fe-auth.c fe-auth-scram.c fe-connect.c fe-exec.c fe-lobj.c fe-misc.c fe-protocol2.c fe-protocol3.c fe-secure.c fe-secure-common.c fe-secure-openssl.c win32.c
 GETTEXT_TRIGGERS = libpq_gettext pqInternalNotice:2
 GETTEXT_FLAGS    = libpq_gettext:1:pass-c-format pqInternalNotice:2:c-format
index 93f364a9f2184fcfe8f646711730a7f3d8cf53aa..d8c279ab92642d5a26366a293771f72b72c708d3 100644 (file)
@@ -242,6 +242,7 @@ sub mkvcbuild
    # building with OpenSSL.
    if (!$solution->{options}->{openssl})
    {
+       $libpq->RemoveFile('src/interfaces/libpq/fe-secure-common.c');
        $libpq->RemoveFile('src/interfaces/libpq/fe-secure-openssl.c');
        $libpq->RemoveFile('src/common/sha2_openssl.c');
    }