Remove arbitrary limitation on length of common name in SSL certificates.

Both libpq and the backend would truncate a common name extracted from a
certificate at 32 bytes.  Replace that fixed-size buffer with dynamically
allocated string so that there is no hard limit.  While at it, remove the
code for extracting peer_dn, which we weren't using for anything; and
don't bother to store peer_cn longer than we need it in libpq.

This limit was not so terribly unreasonable when the code was written,
because we weren't using the result for anything critical, just logging it.
But now that there are options for checking the common name against the
server host name (in libpq) or using it as the user's name (in the server),
this could result in undesirable failures.  In the worst case it even seems
possible to spoof a server name or user name, if the correct name is
exactly 32 bytes and the attacker can persuade a trusted CA to issue a
certificate in which that string is a prefix of the certificate's common
name.  (To exploit this for a server name, he'd also have to send the
connection astray via phony DNS data or some such.)  The case that this is
a realistic security threat is a bit thin, but nonetheless we'll treat it
as one.

Back-patch to 8.4.  Older releases contain the faulty code, but it's not
a security problem because the common name wasn't used for anything
interesting.

Reported and patched by Heikki Linnakangas

Security: CVE-2012-0867

diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c
index 3f4c15dd259132f3d85ab7cb460d3c8548fc3f1d..f3e9bf732a84cf4e32a11683120e6b027850f475 100644 (file)
--- a/src/backend/libpq/be-secure.c
+++ b/src/backend/libpq/be-secure.c
@@ -73,6 +73,7 @@
 
 #include "libpq/libpq.h"
 #include "tcop/tcopprot.h"
+#include "utils/memutils.h"
 
 
 #ifdef USE_SSL
@@ -950,44 +951,54 @@ aloop:
 
        port->count = 0;
 
-       /* get client certificate, if available. */
+       /* Get client certificate, if available. */
        port->peer = SSL_get_peer_certificate(port->ssl);
-       if (port->peer == NULL)
-       {
-               strlcpy(port->peer_dn, "(anonymous)", sizeof(port->peer_dn));
-               strlcpy(port->peer_cn, "(anonymous)", sizeof(port->peer_cn));
-       }
-       else
+
+       /* and extract the Common Name from it. */
+       port->peer_cn = NULL;
+       if (port->peer != NULL)
        {
-               X509_NAME_oneline(X509_get_subject_name(port->peer),
-                                                 port->peer_dn, sizeof(port->peer_dn));
-               port->peer_dn[sizeof(port->peer_dn) - 1] = '\0';
-               r = X509_NAME_get_text_by_NID(X509_get_subject_name(port->peer),
-                                          NID_commonName, port->peer_cn, sizeof(port->peer_cn));
-               port->peer_cn[sizeof(port->peer_cn) - 1] = '\0';
-               if (r == -1)
-               {
-                       /* Unable to get the CN, set it to blank so it can't be used */
-                       port->peer_cn[0] = '\0';
-               }
-               else
+               int             len;
+
+               len = X509_NAME_get_text_by_NID(X509_get_subject_name(port->peer),
+                                                                               NID_commonName, NULL, 0);
+               if (len != -1)
                {
+                       char       *peer_cn;
+
+                       peer_cn = MemoryContextAlloc(TopMemoryContext, len + 1);
+                       r = X509_NAME_get_text_by_NID(X509_get_subject_name(port->peer),
+                                                                                 NID_commonName, peer_cn, len + 1);
+                       peer_cn[len] = '\0';
+                       if (r != len)
+                       {
+                               /* shouldn't happen */
+                               pfree(peer_cn);
+                               close_SSL(port);
+                               return -1;
+                       }
+
                        /*
-                        * Reject embedded NULLs in certificate common name to prevent attacks like
-                        * CVE-2009-4034.
+                        * Reject embedded NULLs in certificate common name to prevent
+                        * attacks like CVE-2009-4034.
                         */
-                       if (r != strlen(port->peer_cn))
+                       if (len != strlen(peer_cn))
                        {
                                ereport(COMMERROR,
                                                (errcode(ERRCODE_PROTOCOL_VIOLATION),
                                                 errmsg("SSL certificate's common name contains embedded null")));
+                               pfree(peer_cn);
                                close_SSL(port);
                                return -1;
                        }
+
+                       port->peer_cn = peer_cn;
                }
        }
+
        ereport(DEBUG2,
-                       (errmsg("SSL connection from \"%s\"", port->peer_cn)));
+                       (errmsg("SSL connection from \"%s\"",
+                                       port->peer_cn ? port->peer_cn : "(anonymous)")));
 
        /* set up debugging/info callback */
        SSL_CTX_set_info_callback(SSL_context, info_cb);
@@ -1013,6 +1024,12 @@ close_SSL(Port *port)
                X509_free(port->peer);
                port->peer = NULL;
        }
+
+       if (port->peer_cn)
+       {
+               pfree(port->peer_cn);
+               port->peer_cn = NULL;
+       }
 }
 
 /*

diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 539826d65542109ea6eb2c7bd5a529e5388035a7..000e658a646f4ca153413029fc198e5dee0987d5 100644 (file)
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -166,8 +166,7 @@ typedef struct Port
 #ifdef USE_SSL
        SSL                *ssl;
        X509       *peer;
-       char            peer_dn[128 + 1];
-       char            peer_cn[SM_USER + 1];
+       char       *peer_cn;
        unsigned long count;
 #endif
 } Port;

diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index e4d5307141bca6429e5cb5994ab0f60c0b5c283c..f5328e4c94a351adee55b936f09dac87dc1539e3 100644 (file)
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -675,6 +675,11 @@ wildcard_certificate_match(const char *pattern, const char *string)
 static bool
 verify_peer_name_matches_certificate(PGconn *conn)
 {
+       char       *peer_cn;
+       int                     r;
+       int                     len;
+       bool            result;
+
        /*
         * If told not to verify the peer name, don't do it. Return true indicating
         * that the verification was successful.
@@ -682,33 +687,81 @@ verify_peer_name_matches_certificate(PGconn *conn)
        if (strcmp(conn->sslmode, "verify-full") != 0)
                return true;
 
+       /*
+        * Extract the common name from the certificate.
+        *
+        * XXX: Should support alternate names here
+        */
+       /* First find out the name's length and allocate a buffer for it. */
+       len = X509_NAME_get_text_by_NID(X509_get_subject_name(conn->peer),
+                                                                       NID_commonName, NULL, 0);
+       if (len == -1)
+       {
+               printfPQExpBuffer(&conn->errorMessage,
+                                                 libpq_gettext("could not get server common name from server certificate\n"));
+               return false;
+       }
+       peer_cn = malloc(len + 1);
+       if (peer_cn == NULL)
+       {
+               printfPQExpBuffer(&conn->errorMessage,
+                                                 libpq_gettext("out of memory\n"));
+               return false;
+       }
+
+       r = X509_NAME_get_text_by_NID(X509_get_subject_name(conn->peer),
+                                                                 NID_commonName, peer_cn, len + 1);
+       if (r != len)
+       {
+               /* Got different length than on the first call. Shouldn't happen. */
+               printfPQExpBuffer(&conn->errorMessage,
+                                                 libpq_gettext("could not get server common name from server certificate\n"));
+               free(peer_cn);
+               return false;
+       }
+       peer_cn[len] = '\0';
+
+       /*
+        * Reject embedded NULLs in certificate common name to prevent attacks
+        * like CVE-2009-4034.
+        */
+       if (len != strlen(peer_cn))
+       {
+               printfPQExpBuffer(&conn->errorMessage,
+                                                 libpq_gettext("SSL certificate's common name contains embedded null\n"));
+               free(peer_cn);
+               return false;
+       }
+
+       /*
+        * We got the peer's common name. Now compare it against the originally
+        * given hostname.
+        */
        if (!(conn->pghost && conn->pghost[0] != '\0'))
        {
                printfPQExpBuffer(&conn->errorMessage,
                                                  libpq_gettext("host name must be specified for a verified SSL connection\n"));
-               return false;
+               result = false;
        }
        else
        {
-               /*
-                * Connect by hostname.
-                *
-                * XXX: Should support alternate names here
-                */
-               if (pg_strcasecmp(conn->peer_cn, conn->pghost) == 0)
+               if (pg_strcasecmp(peer_cn, conn->pghost) == 0)
                        /* Exact name match */
-                       return true;
-               else if (wildcard_certificate_match(conn->peer_cn, conn->pghost))
+                       result = true;
+               else if (wildcard_certificate_match(peer_cn, conn->pghost))
                        /* Matched wildcard certificate */
-                       return true;
+                       result = true;
                else
                {
                        printfPQExpBuffer(&conn->errorMessage,
                                                          libpq_gettext("server common name \"%s\" does not match host name \"%s\"\n"),
-                                                         conn->peer_cn, conn->pghost);
-                       return false;
+                                                         peer_cn, conn->pghost);
+                       result = false;
                }
        }
+
+       free(peer_cn);
+       return result;
 }
 
 /*
@@ -1337,7 +1390,7 @@ open_client_SSL(PGconn *conn)
         * SSL_CTX_set_verify() if root.crt exists.
         */
 
-       /* pull out server distinguished and common names */
+       /* get server certificate */
        conn->peer = SSL_get_peer_certificate(conn->ssl);
        if (conn->peer == NULL)
        {
@@ -1351,33 +1404,6 @@ open_client_SSL(PGconn *conn)
                return PGRES_POLLING_FAILED;
        }
 
-       X509_NAME_oneline(X509_get_subject_name(conn->peer),
-                                         conn->peer_dn, sizeof(conn->peer_dn));
-       conn->peer_dn[sizeof(conn->peer_dn) - 1] = '\0';
-
-       r = X509_NAME_get_text_by_NID(X509_get_subject_name(conn->peer),
-                                                         NID_commonName, conn->peer_cn, SM_USER);
-       conn->peer_cn[SM_USER] = '\0'; /* buffer is SM_USER+1 chars! */
-       if (r == -1)
-       {
-               /* Unable to get the CN, set it to blank so it can't be used */
-               conn->peer_cn[0] = '\0';
-       }
-       else
-       {
-               /*
-                * Reject embedded NULLs in certificate common name to prevent attacks like
-                * CVE-2009-4034.
-                */
-               if (r != strlen(conn->peer_cn))
-               {
-                       printfPQExpBuffer(&conn->errorMessage,
-                                                         libpq_gettext("SSL certificate's common name contains embedded null\n"));
-                       close_SSL(conn);
-                       return PGRES_POLLING_FAILED;
-               }
-       }
-
        if (!verify_peer_name_matches_certificate(conn))
        {
                close_SSL(conn);

diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index bf918e323023b3bcb5944a78c6eb0a5b3dd7dbc3..ace3a405ab37088ed90e0fe02c9c6a881c971637 100644 (file)
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -385,8 +385,6 @@ struct pg_conn
                                                                 * attempting normal connection */
        SSL                *ssl;                        /* SSL status, if have SSL connection */
        X509       *peer;                       /* X509 cert of server */
-       char            peer_dn[256 + 1];               /* peer distinguished name */
-       char            peer_cn[SM_USER + 1];   /* peer common name */
 #ifdef USE_SSL_ENGINE
        ENGINE     *engine;                     /* SSL engine, if any */
 #else