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.
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.
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).
Regards,
--
Alexey Klyukin
diff --git a/src/interfaces/libpq/fe-secure-openssl.c
b/src/interfaces/libpq/fe-secure-openssl.c
new file mode 100644
index f950fc3..a4ad305
*** a/src/interfaces/libpq/fe-secure-openssl.c
--- b/src/interfaces/libpq/fe-secure-openssl.c
***************
*** 60,68 ****
--- 60,73 ----
#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 certificate_name_entry_validate_match(PGconn *conn,
+
ASN1_STRING *name,
+
bool *match,
+
char **store_name);
static void destroy_ssl_system(void);
static int initialize_SSL(PGconn *conn);
static PostgresPollingStatusType open_client_SSL(PGconn *);
*************** wildcard_certificate_match(const char *p
*** 473,540 ****
/*
! * Verify that common name resolves to peer.
*/
! 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.
! */
! 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
--- 478,573 ----
/*
! * Extract the name from the certificate name entry and match it against the
pghost.
! * Returns 0 if the certificate name is invalid (contains embedded NULLs), 1
otherwise.
*/
! static int
! certificate_name_entry_validate_match(PGconn *conn, ASN1_STRING *name_entry,
! bool
*match, char **store_name)
{
! int len;
! char *name,
! *namedata;
! /* Should not happen... */
! if (name_entry == NULL)
{
printfPQExpBuffer(&conn->errorMessage,
! libpq_gettext("SSL
certificate's name entry is missing"));
! return 0;
}
! /* GEN_DNS can be only IA5String, equivalent to US ASCII */
! namedata = (char *) ASN1_STRING_data(name_entry);
! len = ASN1_STRING_length(name_entry);
! name = malloc(len + 1);
! if (name == NULL)
{
printfPQExpBuffer(&conn->errorMessage,
! libpq_gettext("out of memory\n"));
! return 0;
}
! memcpy(name, namedata, len);
! /* There is no guarantee the string returned from the certificate is
NULL-terminated */
! name[len] = '\0';
/*
! * Reject embedded NULLs in certificate common or alternative name to
prevent attacks
* like CVE-2009-4034.
*/
! if (len != strlen(name))
{
printfPQExpBuffer(&conn->errorMessage,
! libpq_gettext("SSL certificate's name
contains embedded null\n"));
! free(name);
! return 0;
}
+ if (pg_strcasecmp(name, conn->pghost) == 0)
+ /* Exact name match */
+ *match = true;
+ else if (wildcard_certificate_match(name, conn->pghost))
+ /* Matched wildcard certificate */
+ *match = true;
+ else
+ {
+ *match = false;
+ /* store the non-matching certificate name for further
reporting */
+ if (*store_name == NULL)
+ {
+ *store_name = malloc(len + 1);
+ if (*store_name == NULL)
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("out of
memory\n"));
+ return 0;
+ }
+ memcpy(*store_name, name, len + 1);
+ }
+ }
+ free(name);
+ return 1;
+ }
+
+ /*
+ * Verify that common name or any of the alternative DNS names resolves to
peer.
+ * Names in Subject Alternative Names and Common Name if present are
considered.
+ */
+ static bool
+ verify_peer_name_matches_certificate(PGconn *conn)
+ {
+ int i;
+ int san_len;
+ int names_examined;
+ bool result;
+ STACK_OF(GENERAL_NAME) *peer_san;
+ char *reported_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;
/*
* We got the peer's common name. Now compare it against the originally
*************** verify_peer_name_matches_certificate(PGc
*** 544,569 ****
{
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("host name must
be specified for a verified SSL connection\n"));
! result = false;
}
! else
{
! 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;
! else
{
! printfPQExpBuffer(&conn->errorMessage,
! libpq_gettext("server
common name \"%s\" does not match host name \"%s\"\n"),
! peer_cn,
conn->pghost);
! result = false;
}
}
! free(peer_cn);
return result;
}
--- 577,671 ----
{
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("host name must
be specified for a verified SSL connection\n"));
! return false;
}
!
! /* Get the list and the total number of subject alternative names
(SANs). */
! peer_san = (STACK_OF(GENERAL_NAME) *) X509_get_ext_d2i(conn->peer,
NID_subject_alt_name, NULL, NULL);
! san_len = peer_san ? sk_GENERAL_NAME_num(peer_san) : 0;
! names_examined = 0;
!
! /*
! * Compare the alternative names dnsNames identifies against
! * the originally given hostname.
! */
! for (i = 0; i < san_len; i++)
{
! const GENERAL_NAME *name = sk_GENERAL_NAME_value(peer_san, i);
! if (name->type == GEN_DNS)
{
! names_examined++;
!
! /* bail out immediately if an error happened during
validation */
! if (certificate_name_entry_validate_match(conn,
name->d.dNSName, &result, &reported_name) == 0)
! {
! sk_GENERAL_NAMES_free(peer_san);
! free(reported_name);
! return false;
! }
}
+ if (result)
+ break;
}
+ if (peer_san != NULL)
+ sk_GENERAL_NAMES_free(peer_san);
! if (!result)
! {
! X509_NAME *subject_name;
!
! /*
! * Extract the common name from the certificate.
! */
! subject_name = X509_get_subject_name(conn->peer);
! if (subject_name == NULL && names_examined == 0)
! {
! printfPQExpBuffer(&conn->errorMessage,
! libpq_gettext("could
not get server name from server SSL certificate\n"));
! free(reported_name);
! return false;
! }
! else if (subject_name != NULL)
! {
! /* First find out the name's length and allocate a
buffer for it. */
! int cn_index = X509_NAME_get_index_by_NID(subject_name,
!
NID_commonName, -1);
!
! if (cn_index < 0 && names_examined == 0)
! {
! printfPQExpBuffer(&conn->errorMessage,
! libpq_gettext("could
not get server name from server SSL certificate\n"));
! free(reported_name);
! return false;
! }
! else if (cn_index >= 0)
! {
! /* The name for the potential error report will
be copied from the common name */
! free(reported_name);
! reported_name = NULL;
! names_examined++;
! if (certificate_name_entry_validate_match(conn,
!
X509_NAME_ENTRY_get_data(X509_NAME_get_entry(subject_name,
cn_index)),
!
&result, &reported_name) == 0)
! return false;
! }
! }
!
! if (!result)
! {
! /* Common name did not match and there are no
alternative names */
! if (names_examined > 1)
! printfPQExpBuffer(&conn->errorMessage,
!
libpq_gettext("server name \"%s\" and %d other names from server SSL
certificate do not match host name \"%s\"\n"),
!
reported_name, names_examined - 1, conn->pghost);
! else
! printfPQExpBuffer(&conn->errorMessage,
!
libpq_gettext("server name \"%s\" from server SSL certificate does not match
host name \"%s\"\n"),
!
reported_name, conn->pghost);
!
! }
! }
! free(reported_name);
return result;
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
