On 09/05/2014 07:30 PM, Alexey Klyukin wrote:
On Thu, Sep 4, 2014 at 10:23 AM, Heikki Linnakangas
<hlinnakan...@vmware.com> wrote:
Hmm. Perhaps we should use X509_NAME_get_index_by_NID + X509_NAME_get_entry
instead of X509_NAME_get_text_by_NID. You could then pass the ASN1_STRING
object to the certificate_name_entry_validate_match() function, and have it
do the ASN1_STRING_length() and ASN1_STRING_data() calls too.
...
I think we should:
1. Check if there's a common name, and if so, print that
2. Check if there is exactly one SAN, and if so, print that
3. Just print an error without mentioning names.
There's a lot of value in printing the name if possible, so I'd really like
to keep that. But I agree that printing all the names if there are several
would get complicated and the error message could become very long. Yeah,
the error message might need to be different for cases 1 and 2. Or maybe
phrase it "server certificate's name \"%s\" does not match host name
\"%s\"", which would be reasonable for both 1. and 2.
Thank you, I've implemented both suggestions in the attached new
version of the patch.
On the error message, I've decided to show either a single name, or
the first examined name and the number of other names present in the
certificate, i.e:
psql: server name "example.com" and 2 other names from server SSL certificate do not
match host name "example-foo.com"
The error does not state whether the names comes from the CN or from
the SAN section.
I'd reword that slightly, to:
psql: server certificate for "example.com" (and 2 other names) does not
match host name "example-foo.com"
I never liked the current wording, "server name "foo"" very much. I
think it's important to use the word "server certificate" in the error
message, to make it clear where the problem is.
For translations, that message should be "pluralized", but there is no
libpq_ngettext macro equivalent to ngettext(), like libpq_gettext. I
wonder if that was left out on purpose, or if we just haven't needed
that in libpq before. Anyway, I think we need to add that for this.
This version also checks for the availability of the subject name, it
looks like RFC 5280 does not require it at all.
4.1.2.6. Subject
The subject field identifies the entity associated with the public
key stored in the subject public key field. The subject name MAY be
carried in the subject field and/or the subjectAltName extension.
Ok.
The pattern of allocating the name in the subroutine and then
reporting it (and releasing when necessary) in the main function is a
little bit ugly, but it looks to me the least ugly of anything else I
could come up (i.e. extracting those names at the time the error
message is shown).
I reworked that a bit, see attached. What do you think?
I think this is ready for commit, except for two things:
1. The pluralization of the message for translation
2. I still wonder if we should follow the RFC 6125 and not check the
Common Name at all, if SANs are present. I don't really understand the
point of that rule, and it seems unlikely to pose a security issue in
practice, because surely a CA won't sign a certificate with a
bogus/wrong CN, because an older client that doesn't look at the SANs at
all would use the CN anyway. But still... what does a Typical Web
Browser do?
- Heikki
diff --git a/src/interfaces/libpq/fe-secure-openssl.c
b/src/interfaces/libpq/fe-secure-openssl.c
index f950fc3..4f6f324 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -60,9 +60,13 @@
#ifdef USE_SSL_ENGINE
#include <openssl/engine.h>
#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,
+
ASN1_STRING *name,
+
char **store_name);
static void destroy_ssl_system(void);
static int initialize_SSL(PGconn *conn);
static PostgresPollingStatusType open_client_SSL(PGconn *);
@@ -473,98 +477,229 @@ wildcard_certificate_match(const char *pattern, const
char *string)
/*
- * Verify that common name resolves to peer.
+ * 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.
*/
-static bool
-verify_peer_name_matches_certificate(PGconn *conn)
+static int
+verify_peer_name_matches_certificate_name(PGconn *conn, ASN1_STRING
*name_entry,
+
char **store_name)
{
- char *peer_cn;
- int r;
int len;
- bool result;
+ char *name;
+ unsigned char *namedata;
+ int result;
- /*
- * 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;
+ *store_name = NULL;
+
+ /* Should not happen... */
+ if (name_entry == NULL)
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("SSL certificate's name entry
is missing\n"));
+ return -1;
+ }
/*
- * Extract the common name from the certificate.
+ * GEN_DNS can be only IA5String, equivalent to US ASCII.
*
- * XXX: Should support alternate names here
+ * There is no guarantee the string returned from the certificate is
+ * NULL-terminated, so make a copy that is.
*/
- /* 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)
+ namedata = ASN1_STRING_data(name_entry);
+ len = ASN1_STRING_length(name_entry);
+ name = malloc(len + 1);
+ if (name == NULL)
{
printfPQExpBuffer(&conn->errorMessage,
- libpq_gettext("could not get
server common name from server certificate\n"));
- return false;
+ libpq_gettext("out of
memory\n"));
+ return -1;
}
- peer_cn = malloc(len + 1);
- if (peer_cn == NULL)
+ 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("out of
memory\n"));
- return false;
+ libpq_gettext("SSL certificate's name contains embedded
null\n"));
+ return -1;
}
- r = X509_NAME_get_text_by_NID(X509_get_subject_name(conn->peer),
-
NID_commonName, peer_cn, len + 1);
- if (r != len)
+ if (pg_strcasecmp(name, conn->pghost) == 0)
{
- /* 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;
+ /* Exact name match */
+ result = 1;
}
- peer_cn[len] = '\0';
+ else if (wildcard_certificate_match(name, conn->pghost))
+ {
+ /* Matched wildcard certificate */
+ 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.
+ */
+static bool
+verify_peer_name_matches_certificate(PGconn *conn)
+{
+ 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;
/*
- * Reject embedded NULLs in certificate common name to prevent attacks
- * like CVE-2009-4034.
+ * If told not to verify the peer name, don't do it. Return true
+ * indicating that the verification was successful.
*/
- if (len != strlen(peer_cn))
+ if (strcmp(conn->sslmode, "verify-full") != 0)
+ return true;
+
+ /* Check that we have a hostname to compare with. */
+ if (!(conn->pghost && conn->pghost[0] != '\0'))
{
printfPQExpBuffer(&conn->errorMessage,
- libpq_gettext("SSL
certificate's common name contains embedded null\n"));
- free(peer_cn);
+ libpq_gettext("host name must
be specified for a verified SSL connection\n"));
return false;
}
/*
- * We got the peer's common name. Now compare it against the originally
- * given hostname.
+ * First, get the Subject Alternative Names (SANs) from the certificate,
+ * and compare them against the originally given hostname.
*/
- if (!(conn->pghost && conn->pghost[0] != '\0'))
+ peer_san = (STACK_OF(GENERAL_NAME) *)
+ X509_get_ext_d2i(conn->peer, NID_subject_alt_name, NULL, NULL);
+
+ if (peer_san)
{
- printfPQExpBuffer(&conn->errorMessage,
- libpq_gettext("host name must
be specified for a verified SSL connection\n"));
- result = false;
+ int san_len = sk_GENERAL_NAME_num(peer_san);
+
+ for (i = 0; i < san_len && !found_match && !got_error; i++)
+ {
+ const GENERAL_NAME *name =
sk_GENERAL_NAME_value(peer_san, i);
+
+ if (name->type == GEN_DNS)
+ {
+ char *alt_name;
+
+ names_examined++;
+ rc =
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;
+ else
+ free(alt_name);
+ }
+ }
+ }
+ sk_GENERAL_NAME_free(peer_san);
}
- else
+
+ /*
+ * If no match to any of the SANs, check the Common Name.
+ */
+ if (!found_match && !got_error)
{
- if (pg_strcasecmp(peer_cn, conn->pghost) == 0)
- /* Exact name match */
- result = true;
- else if (wildcard_certificate_match(peer_cn, conn->pghost))
- /* Matched wildcard certificate */
- result = true;
+ X509_NAME *subject_name;
+
+ subject_name = X509_get_subject_name(conn->peer);
+
+ if (subject_name != NULL)
+ {
+ int cn_index;
+
+ cn_index = X509_NAME_get_index_by_NID(subject_name,
+
NID_commonName, -1);
+ if (cn_index >= 0)
+ {
+ /*
+ * We prefer the Common Name over SANs in the
error message,
+ * if both are present.
+ */
+ if (first_name)
+ free(first_name);
+
+ names_examined++;
+ rc = 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;
+ }
+ }
+ }
+
+ if (!found_match && !got_error)
+ {
+ /*
+ * No match. Include the host 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_gettext("server
certificate for \"%s\" (and %d other names) does not match host name \"%s\"\n"),
+ first_name,
names_examined - 1, conn->pghost);
+ }
+ else if (names_examined == 1)
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("server
certificate for \"%s\" does not match host name \"%s\"\n"),
+ first_name,
conn->pghost);
+ }
else
{
printfPQExpBuffer(&conn->errorMessage,
- libpq_gettext("server
common name \"%s\" does not match host name \"%s\"\n"),
- peer_cn,
conn->pghost);
- result = false;
+ libpq_gettext("could
not get server's hostname from server certificate\n"));
}
}
- free(peer_cn);
- return result;
+ /* clean up */
+ if (first_name)
+ free(first_name);
+
+ if (found_match)
+ return 1;
+ else if (got_error)
+ return -1;
+ else
+ return 0;
}
#ifdef ENABLE_THREAD_SAFETY
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
