From ab0bd8f655a5c75c8040b462a9d2c0111fbf323a Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Sat, 27 Jan 2018 13:47:52 -0500 Subject: [PATCH] Refactor client-side SSL certificate checking code 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] --- src/interfaces/libpq/fe-secure-openssl.c | 209 ++++--------------------------- src/interfaces/libpq/fe-secure.c | 185 ++++++++++++++++++++++++++- src/interfaces/libpq/libpq-int.h | 20 +++ 3 files changed, 228 insertions(+), 186 deletions(-) diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c index 9ab317320a..228ea5897a 100644 --- a/src/interfaces/libpq/fe-secure-openssl.c +++ b/src/interfaces/libpq/fe-secure-openssl.c @@ -60,9 +60,8 @@ #endif #include -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 +491,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 +512,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 +519,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 +529,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 +557,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 +583,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 +596,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 +1280,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; diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c index cfb77f6d85..72bb8ba01c 100644 --- a/src/interfaces/libpq/fe-secure.c +++ b/src/interfaces/libpq/fe-secure.c @@ -389,7 +389,190 @@ pqsecure_raw_write(PGconn *conn, const void *ptr, size_t len) return n; } -/* Dummy versions of SSL info functions, when built without SSL support */ +/* + * Common helper functions + */ +#ifdef USE_SSL +/* + * 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); +} +#endif /* USE_SSL */ + +/* + * Dummy versions of SSL info functions, when built without SSL support + */ #ifndef USE_SSL void * diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index b3492b033a..d511203c4e 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -661,6 +661,13 @@ extern void pq_reset_sigpipe(sigset_t *osigset, bool sigpipe_pending, bool got_epipe); #endif +#ifdef USE_SSL +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 + /* === SSL === */ /* @@ -732,6 +739,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 === */ /* -- 2.16.1