On Mon, Sep 1, 2014 at 10:39 AM, Alexey Klyukin <al...@hintbits.com> wrote:
> On Fri, Aug 29, 2014 at 11:22 AM, Heikki Linnakangas
> <hlinnakan...@vmware.com> wrote:
>> Yeah, I think a certificate without CN should be supported. See also RFC
>> 6125, section 4.1. "Rules" [for issuers of certificates]:
>>
>>> 5. Even though many deployed clients still check for the CN-ID
>>> within the certificate subject field, certification authorities
>>> are encouraged to migrate away from issuing certificates that
>>> represent the server's fully qualified DNS domain name in a
>>> CN-ID. Therefore, the certificate SHOULD NOT include a CN-ID
>>> unless the certification authority issues the certificate in
>>> accordance with a specification that reuses this one and that
>>> explicitly encourages continued support for the CN-ID identifier
>>> type in the context of a given application technology.
>>
>>
>> Certificates without a CN-ID are probably rare today, but they might start
>> to appear in the future.
>
> Ok, I will change a patch to add support for this clause.
Attached is a new version. I've changed the logic to check for the SAN
names first, and only check the common name if there is no match. The
error when the common name is missing is only shown if SAN section
does not contain any DNS names as well. The tricky part is the error
message if no match was found: initially, it only listed a single
common name, but now tracking all DNS names just for the sake of the
error message makes the code more bloated, so I'm wondering if simply
stating that there was no match, as implemented in the attached patch,
would be good enough?
--
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..a4e3fc6
*** 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,
+
char *name,
+
unsigned int len,
+
bool *match);
static void destroy_ssl_system(void);
static int initialize_SSL(PGconn *conn);
static PostgresPollingStatusType open_client_SSL(PGconn *);
*************** wildcard_certificate_match(const char *p
*** 471,487 ****
return 1;
}
/*
! * 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
--- 476,525 ----
return 1;
}
+ /*
+ * Validate a single 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, char *name, unsigned int
len, bool *match)
+ {
+ /* There is no guarantee the string returned from the certificate is
NULL-terminated */
+ name[len] = '\0';
+ *match = false;
+ /*
+ * 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
common name contains embedded null\n"));
+ 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;
+ 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;
! bool result;
! bool san_has_dns_names;
! STACK_OF(GENERAL_NAME) *peer_san;
/*
* If told not to verify the peer name, don't do it. Return true
*************** verify_peer_name_matches_certificate(PGc
*** 491,569 ****
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"));
! 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;
}
--- 529,646 ----
return true;
/*
! * 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;
}
! /* 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;
! san_has_dns_names = false;
/*
! * 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)
! {
! int reported_len;
! char *dns_namedata,
! *dns_name;
! san_has_dns_names = true;
! reported_len = ASN1_STRING_length(name->d.dNSName);
! /* GEN_DNS can be only IA5String, equivalent to US
ASCII */
! dns_namedata = (char *)
ASN1_STRING_data(name->d.dNSName);
!
! dns_name = malloc(reported_len + 1);
! if (dns_name == NULL)
! {
! printfPQExpBuffer(&conn->errorMessage,
! libpq_gettext("out of
memory\n"));
! return false;
! }
! memcpy(dns_name, dns_namedata, reported_len);
! /* bail out immediately if an error happened during
validation */
! if (certificate_name_entry_validate_match(conn,
dns_name, reported_len, &result) == 0)
! {
! free(dns_name);
! sk_GENERAL_NAMES_free(peer_san);
! return false;
! }
! free(dns_name);
! }
! if (result)
! break;
}
! if (peer_san != NULL)
! sk_GENERAL_NAMES_free(peer_san);
!
! if (!result)
{
! int r;
! int len;
! char *peer_cn;
!
! /*
! * Extract the common name from the certificate.
! */
! /* 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 && !san_has_dns_names)
{
printfPQExpBuffer(&conn->errorMessage,
! libpq_gettext("could
not get server name from server certificate\n"));
! return false;
}
! else if (len != -1)
! {
! 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 && !san_has_dns_names)
! {
! /* Got different length than on the first call.
Shouldn't happen. */
! printfPQExpBuffer(&conn->errorMessage,
!
libpq_gettext("could not get server name from server certificate\n"));
! free(peer_cn);
! return false;
! }
! else if (r == len)
! {
! if (certificate_name_entry_validate_match(conn,
peer_cn, len, &result) == 0)
! {
! free(peer_cn);
! return false;
! }
! }
! free(peer_cn);
! }
! if (!result)
! {
! /* Common name did not match and there are no
alternative names */
! printfPQExpBuffer(&conn->errorMessage,
! libpq_gettext("server
names from server certficate do not match host name \"%s\"\n"),
! conn->pghost);
! }
! }
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
