Re: [HACKERS] implement subject alternative names support for SSL connections
On Fri, Sep 12, 2014 at 4:20 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: Hmm. If that's what the browsers do, I think we should also err on the side of caution here. Ignoring the CN is highly unlikely to cause anyone a problem; a CA worth its salt should not issue a certificate with a CN that's not also listed in the SAN section. But if you have such a certificate anyway for some reason, it shouldn't be too difficult to get a new certificate. Certificates expire every 1-3 years anyway, so there must be a procedure to renew them anyway. Committed, with that change, ie. the CN is not checked if SANs are present. Thanks for bearing through all these iterations! Great news! Thank you very much for devoting your time and energy to the review and providing such a useful feedback! On the CN thing, I don't have particularly strong arguments for either of the possible behaviors, so sticking to RFC makes sense here Sincerely, -- Alexey Klyukin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] implement subject alternative names support for SSL connections
On Mon, Sep 15, 2014 at 10:23 AM, Alexey Klyukin al...@hintbits.com wrote:
On Fri, Sep 12, 2014 at 4:20 PM, Heikki Linnakangas
hlinnakan...@vmware.com wrote:
Hmm. If that's what the browsers do, I think we should also err on the
side of caution here. Ignoring the CN is highly unlikely to cause anyone
a problem; a CA worth its salt should not issue a certificate with a CN
that's not also listed in the SAN section. But if you have such a
certificate anyway for some reason, it shouldn't be too difficult to get
a new certificate. Certificates expire every 1-3 years anyway, so there
must be a procedure to renew them anyway.
Committed, with that change, ie. the CN is not checked if SANs are present.
Actually, I disagree with the way the patch ignores the CN. Currently,
it skips the
CN unconditionally if the SubjectAltName section is present. But what
RFC 6125 says
is:
If a subjectAltName extension of type dNSName is present, that MUST
be used as the identity. Otherwise, the (most specific) Common Name
field in the Subject field of the certificate MUST be used.
This means that we have to check that at least one dNSName resource is
present before
rejecting to examine the CN. Attached is a one-liner (excluding
comments) that fixes this.
Regards,
Alexey
diff --git a/src/interfaces/libpq/fe-secure-openssl.c
b/src/interfaces/libpq/fe-secure-openssl.c
new file mode 100644
index 98d02b6..dd4fab8
*** a/src/interfaces/libpq/fe-secure-openssl.c
--- b/src/interfaces/libpq/fe-secure-openssl.c
*** verify_peer_name_matches_certificate(PGc
*** 626,637
sk_GENERAL_NAME_free(peer_san);
}
/*
!* If there is no subjectAltName extension, check the Common Name.
*
!* (Per RFC 2818 and RFC 6125, if the subjectAltName extension is
present,
* the CN must be ignored.)
*/
! else
{
X509_NAME *subject_name;
--- 626,637
sk_GENERAL_NAME_free(peer_san);
}
/*
!* If there is no subjectAltName extension of type dNSName, check the
Common Name.
*
!* (Per RFC 2818 and RFC 6125, if the subjectAltName extension of type
dNSName is present,
* the CN must be ignored.)
*/
! if (names_examined == 0)
{
X509_NAME *subject_name;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] implement subject alternative names support for SSL connections
On 09/15/2014 01:44 PM, Alexey Klyukin wrote: Committed, with that change, ie. the CN is not checked if SANs are present. Actually, I disagree with the way the patch ignores the CN. Currently, it skips the CN unconditionally if the SubjectAltName section is present. But what RFC 6125 says is: If a subjectAltName extension of type dNSName is present, that MUST be used as the identity. Otherwise, the (most specific) Common Name field in the Subject field of the certificate MUST be used. This means that we have to check that at least one dNSName resource is present before rejecting to examine the CN. Attached is a one-liner (excluding comments) that fixes this. Ok, good catch. Fixed. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] implement subject alternative names support for SSL connections
On 09/11/2014 08:46 PM, Alexey Klyukin wrote: On Mon, Sep 8, 2014 at 8:04 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: 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? At least Firefox and Safari seem to follow RFC strictly, according to some anecdotical evidences (I haven't got enough time to dig into the source code yet): http://superuser.com/questions/230136/primary-common-name-in-subjectaltname https://bugzilla.mozilla.org/show_bug.cgi?id=238142 Chrome seem to follow them, so the major open-source browsers are ignoring the Common Name if the SubjectAltName is present. Still, I see no win in ignoring CN (except for the shorter code), it just annoys some users that forgot to put the CN name also in the SAN section, so my 5 cents is that we should check both. Hmm. If that's what the browsers do, I think we should also err on the side of caution here. Ignoring the CN is highly unlikely to cause anyone a problem; a CA worth its salt should not issue a certificate with a CN that's not also listed in the SAN section. But if you have such a certificate anyway for some reason, it shouldn't be too difficult to get a new certificate. Certificates expire every 1-3 years anyway, so there must be a procedure to renew them anyway. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] implement subject alternative names support for SSL connections
On 09/12/2014 01:30 PM, Heikki Linnakangas wrote: On 09/11/2014 08:46 PM, Alexey Klyukin wrote: On Mon, Sep 8, 2014 at 8:04 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: 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? At least Firefox and Safari seem to follow RFC strictly, according to some anecdotical evidences (I haven't got enough time to dig into the source code yet): http://superuser.com/questions/230136/primary-common-name-in-subjectaltname https://bugzilla.mozilla.org/show_bug.cgi?id=238142 Chrome seem to follow them, so the major open-source browsers are ignoring the Common Name if the SubjectAltName is present. Still, I see no win in ignoring CN (except for the shorter code), it just annoys some users that forgot to put the CN name also in the SAN section, so my 5 cents is that we should check both. Hmm. If that's what the browsers do, I think we should also err on the side of caution here. Ignoring the CN is highly unlikely to cause anyone a problem; a CA worth its salt should not issue a certificate with a CN that's not also listed in the SAN section. But if you have such a certificate anyway for some reason, it shouldn't be too difficult to get a new certificate. Certificates expire every 1-3 years anyway, so there must be a procedure to renew them anyway. Committed, with that change, ie. the CN is not checked if SANs are present. Thanks for bearing through all these iterations! - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] implement subject alternative names support for SSL connections
On Mon, Sep 8, 2014 at 8:04 PM, Heikki Linnakangas
hlinnakan...@vmware.com wrote:
On 09/05/2014 07:30 PM, Alexey Klyukin wrote:
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.
+1
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.
Well, the translation guidelines in the documentation suggest avoiding
plurals if possible, but I don't like rephrasing the sentence with
'names total: %d', so attached is my attempt to add the function.
I have also moved the one-time library binding code to the function of its own.
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?
Thank you, I like your approach of unconditionally allocating and
freeing memory, it makes the code easier to read.
2 minor changes I've made in v7 (in addition to adding the libpq_ngettext):
- the verify_peer_name_matches_certificate does not try to return -1
(since it returns bool, it would be misinterpreted as true)
- removed the !got_error !found_match check from the for loop
header to the loop body per style comment from Tom in the beginning of
this thread.
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?
At least Firefox and Safari seem to follow RFC strictly, according to
some anecdotical evidences (I haven't got enough time to dig into the
source code yet):
http://superuser.com/questions/230136/primary-common-name-in-subjectaltname
https://bugzilla.mozilla.org/show_bug.cgi?id=238142
Chrome seem to follow them, so the major open-source browsers are
ignoring the Common Name if the SubjectAltName is present.
Still, I see no win in ignoring CN (except for the shorter code), it
just annoys some users that forgot to put the CN name also in the SAN
section, so my 5 cents is that we should check both.
Regards,
--
Alexey Klyukin
diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
new file mode 100644
index fc930bd..a082268
*** a/src/interfaces/libpq/fe-misc.c
--- b/src/interfaces/libpq/fe-misc.c
*** static int pqSendSome(PGconn *conn, int
*** 64,69
--- 64,71
static int pqSocketCheck(PGconn *conn, int forRead, int forWrite,
time_t end_time);
static intpqSocketPoll(int sock, int forRead, int forWrite, time_t
end_time);
+ static void libpq_bindomain();
+
/*
* PQlibVersion: return the libpq version number
*** PQenv2encoding(void)
*** 1210,1223
#ifdef ENABLE_NLS
! char *
! libpq_gettext(const char *msgid)
{
static bool already_bound = false;
if (!already_bound)
{
! /* dgettext() preserves errno, but bindtextdomain() doesn't */
#ifdef WIN32
int save_errno = GetLastError();
#else
--- 1212,1225
#ifdef ENABLE_NLS
! static void
! libpq_bindomain()
{
static bool already_bound = false;
if (!already_bound)
{
! /* bindtextdomain() does not preserve errno */
#ifdef WIN32
int save_errno = GetLastError();
#else
*** libpq_gettext(const char *msgid)
*** 1237,1244
--- 1239,1258
errno = save_errno;
#endif
}
+ }
+ char *
+ libpq_gettext(const char *msgid)
+ {
+ libpq_bindomain();
Re: [HACKERS] implement subject alternative names support for SSL connections
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
Re: [HACKERS] implement subject alternative names support for SSL connections
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 intverify_cb(int ok, X509_STORE_CTX *ctx);
+ static intcertificate_name_entry_validate_match(PGconn *conn,
+
ASN1_STRING *name,
+
bool *match,
+
char **store_name);
static void destroy_ssl_system(void);
static intinitialize_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;
! boolresult;
!
! /*
!* 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,
Re: [HACKERS] implement subject alternative names support for SSL connections
On Wed, Sep 3, 2014 at 11:50 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: * It's ugly that the caller does the malloc and memcpy, and the certificate_name_entry_validate_match function then modifies its name argument. Move the malloc+memcpy inside the function. For the case of CN the caller has to do the malloc anyway, since X509_NAME_get_text_by_NID expects the already allocated buffer. This means that 'movable' malloc + memcpy occurs only once, and while it would still make sense to move it into the function, it would also mean we would do an unnecessary malloc for the case of CN. * The error message in certificate_name_entry_validate_match says SSL certificate's common name contains embedded null even though it's also used for SANs. Will fix, thank you. 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? Hmm. It would still be nice to say something about the certificate that was received. How about: server certificate with common name %s does not match host name %s We cannot guarantee at this point that the common name is present in the certificate. And if it is not, which name should we pick instead? One way to solve this is to take the first non-NULL name, but it if there is no common name, but then the wording of the error message would depend on the availability of CN. Another is to show all available names, but I do not like collecting them just for the sake of displaying in the error message. And last one is to just show the error without mentioning names, that's what I've chosen to be the most consistent. Regards, -- Alexey Klyukin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] implement subject alternative names support for SSL connections
On 09/04/2014 10:33 AM, Alexey Klyukin wrote: On Wed, Sep 3, 2014 at 11:50 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: * It's ugly that the caller does the malloc and memcpy, and the certificate_name_entry_validate_match function then modifies its name argument. Move the malloc+memcpy inside the function. For the case of CN the caller has to do the malloc anyway, since X509_NAME_get_text_by_NID expects the already allocated buffer. This means that 'movable' malloc + memcpy occurs only once, and while it would still make sense to move it into the function, it would also mean we would do an unnecessary malloc for the case of CN. 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. 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? Hmm. It would still be nice to say something about the certificate that was received. How about: server certificate with common name %s does not match host name %s We cannot guarantee at this point that the common name is present in the certificate. And if it is not, which name should we pick instead? One way to solve this is to take the first non-NULL name, but it if there is no common name, but then the wording of the error message would depend on the availability of CN. Another is to show all available names, but I do not like collecting them just for the sake of displaying in the error message. And last one is to just show the error without mentioning names, that's what I've chosen to be the most consistent. 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. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] implement subject alternative names support for SSL connections
On 09/01/2014 09:14 PM, Alexey Klyukin wrote: 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. * It's ugly that the caller does the malloc and memcpy, and the certificate_name_entry_validate_match function then modifies its name argument. Move the malloc+memcpy inside the function. * The error message in certificate_name_entry_validate_match says SSL certificate's common name contains embedded null even though it's also used for SANs. 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? Hmm. It would still be nice to say something about the certificate that was received. How about: server certificate with common name %s does not match host name %s ? - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] implement subject alternative names support for SSL connections
On Fri, Aug 29, 2014 at 11:22 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 08/28/2014 07:28 PM, Alexey Klyukin wrote: On Mon, Aug 25, 2014 at 12:02 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 08/24/2014 03:11 PM, Alexey Klyukin wrote: On Wed, Aug 20, 2014 at 11:53 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: The patch doesn't seem to support wildcards in alternative names. Is that on purpose? Not really, we just did not have any use case for them, but it seems that RFC 5280 does not disallow them: Finally, the semantics of subject alternative names that include wildcard characters (e.g., as a placeholder for a set of names) are not addressed by this specification. Applications with specific requirements MAY use such names, but they must define the semantics. I've added support for them in the next iteration of the patch attached to this email. Hmm. So wildcards MAY be supported, but should we? I think we should follow the example of common browsers here, or OpenSSL or other SSL libraries; what do they do? Yes, they seem to be supported. The function you've mentioned above (X509_check_host) specifically mentions wildcards in SANs at https://www.openssl.org/docs/crypto/X509_check_host.html: 'X509_check_host() checks if the certificate Subject Alternative Name (SAN) or Subject CommonName (CN) matches the specified host name, which must be encoded in the preferred name syntax described in section 3.5 of RFC 1034. By default, wildcards are supported and they match only in the left-most label; but they may match part of that label with an explicit prefix or suffix. For example, by default, the host name ``www.example.com'' would match a certificate with a SAN or CN value of ``*.example.com'', ``w*.example.com'' or ``*w.example.com''.' RFC 6125 section 6.4.4 Checking of Common Names says: As noted, a client MUST NOT seek a match for a reference identifier of CN-ID if the presented identifiers include a DNS-ID, SRV-ID, URI-ID, or any application-specific identifier types supported by the client. So, to conform to that we shouldn't check the Common Name at all, if an alternative subject field is present. While the RFC indeed says so, the OpenSSL implementation includes X509_CHECK_FLAG_ALWAYS_CHECK_SUBJECT flag (which, as far as I can see, is set to 1 by default), which makes it check for the CN even if DNS names in SAN are present. I'm not sure what is the reason behind section 6.4.4, and I think it makes sense to check CN as well, since it does not lead to the case of false matches. 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. BTW, should we also support alternative subject names in the server, in client certificates? And if there are multiple names, which one takes effect? Perhaps better to leave that for a separate patch. Good question. The RFC 5280 section 4.2.1.6 does not restrict the certificates to be used only server-side, so the same rules should be applicable to the client certs. I'm wondering if there is an equivalent of RFC 6125 for the clients? PostgreSQL, however, checks different things on the backends and the clients, the formers are checked against the DNS name, while on the clients only the user name is checked. For this, I think, a SAN section with some custom identity type should be issued (the 4.2.1.6 does not list user names as a pre-defined identify type). Note that PostgreSQL can already support clients with multiple names via the user maps, so support for SAN is not that urgent there. On the other hand, should we, in addition to verification of client user names, verify the client names supplied during connections against the DNS entries in their certificates? Are there use cases for this? I do agree this should be the subject of a separate patch. Regards, Alexey -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] implement subject alternative names support for SSL connections
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 intverify_cb(int ok, X509_STORE_CTX *ctx);
+ static intcertificate_name_entry_validate_match(PGconn *conn,
+
char *name,
+
unsigned int len,
+
bool *match);
static void destroy_ssl_system(void);
static intinitialize_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;
! boolresult;
/*
* 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)
{
! inti;
! intsan_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
Re: [HACKERS] implement subject alternative names support for SSL connections
On 08/28/2014 07:28 PM, Alexey Klyukin wrote: On Mon, Aug 25, 2014 at 12:02 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 08/24/2014 03:11 PM, Alexey Klyukin wrote: On Wed, Aug 20, 2014 at 11:53 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: The patch doesn't seem to support wildcards in alternative names. Is that on purpose? Not really, we just did not have any use case for them, but it seems that RFC 5280 does not disallow them: Finally, the semantics of subject alternative names that include wildcard characters (e.g., as a placeholder for a set of names) are not addressed by this specification. Applications with specific requirements MAY use such names, but they must define the semantics. I've added support for them in the next iteration of the patch attached to this email. Hmm. So wildcards MAY be supported, but should we? I think we should follow the example of common browsers here, or OpenSSL or other SSL libraries; what do they do? RFC 6125 section 6.4.4 Checking of Common Names says: As noted, a client MUST NOT seek a match for a reference identifier of CN-ID if the presented identifiers include a DNS-ID, SRV-ID, URI-ID, or any application-specific identifier types supported by the client. So, to conform to that we shouldn't check the Common Name at all, if an alternative subject field is present. (Relying on OpenSSL's hostname-checking function is starting feel more and more appetizing. At least it won't be our problem then.) It would be good to add a little helper function that does the NULL-check, straight comparison, and wildcard check, for a single name. And then use that for the Common Name and all the Alternatives. That'll ensure that all the same rules apply whether the name is the Common Name or an Alternative (assuming that the rules are supposed to be the same; I don't know if that's true). Thanks, common code has been moved into a separate new function. Another question is how should we treat the certificates with no CN and non-empty SAN? Current code just bails out right after finding no CN section present , but the RFC (5280) says: Further, if the only subject identity included in the certificate is an alternative name form (e.g., an electronic mail address), then the subject distinguished name MUST be empty (an empty sequence), and the subjectAltName extension MUST be present. which to me sounds like the possibility of coming across such certificates in the wild, although I personally see little use in them. 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. BTW, should we also support alternative subject names in the server, in client certificates? And if there are multiple names, which one takes effect? Perhaps better to leave that for a separate patch. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] implement subject alternative names support for SSL connections
On Mon, Aug 25, 2014 at 12:02 PM, Heikki Linnakangas
hlinnakan...@vmware.com wrote:
On 08/24/2014 03:11 PM, Alexey Klyukin wrote:
On Wed, Aug 20, 2014 at 11:53 AM, Heikki Linnakangas
hlinnakan...@vmware.com wrote:
The patch doesn't seem to support wildcards in alternative names. Is
that on purpose?
Not really, we just did not have any use case for them, but it seems that
RFC 5280 does not disallow them:
Finally, the semantics of subject alternative names that include
wildcard characters (e.g., as a placeholder for a set of names) are
not addressed by this specification. Applications with specific
requirements MAY use such names, but they must define the semantics.
I've added support for them in the next iteration of the patch attached to
this email.
It would be good to add a little helper function that does the NULL-check,
straight comparison, and wildcard check, for a single name. And then use
that for the Common Name and all the Alternatives. That'll ensure that all
the same rules apply whether the name is the Common Name or an Alternative
(assuming that the rules are supposed to be the same; I don't know if
that's true).
Thanks, common code has been moved into a separate new function.
Another question is how should we treat the certificates with no CN and
non-empty SAN?
Current code just bails out right after finding no CN section present , but
the RFC (5280) says:
Further, if the only subject identity included in the certificate is
an alternative name form (e.g., an electronic mail address), then the
subject distinguished name MUST be empty (an empty sequence), and the
subjectAltName extension MUST be present.
which to me sounds like the possibility of coming across such certificates
in the wild, although I personally see little use in them.
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..394a66f
*** 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 intverify_cb(int ok, X509_STORE_CTX *ctx);
+ static intcertificate_name_entry_validate_match(PGconn *conn,
+
char *name,
+
unsigned int len,
+
bool *match);
static void destroy_ssl_system(void);
static intinitialize_SSL(PGconn *conn);
static PostgresPollingStatusType open_client_SSL(PGconn *);
*** wildcard_certificate_match(const char *p
*** 471,479
return 1;
}
/*
! *Verify that common name resolves to peer.
*/
static bool
verify_peer_name_matches_certificate(PGconn *conn)
--- 476,515
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 dNSNames resolves to
peer.
*/
static bool
verify_peer_name_matches_certificate(PGconn *conn)
*** verify_peer_name_matches_certificate(PGc
*** 492,499
/*
* 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),
--- 528,533
*** verify_peer_name_matches_certificate(PGc
*** 522,540
Re: [HACKERS] implement subject alternative names support for SSL connections
On Mon, Aug 25, 2014 at 12:33 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 08/25/2014 01:07 PM, Andres Freund wrote: On 2014-08-25 13:02:50 +0300, Heikki Linnakangas wrote: But actually, I wonder if we should delegate the whole hostname matching to OpenSSL? There's a function called X509_check_host for that, although it's new in OpenSSL 1.1.0 so we'd need to add a configure test for that and keep the current code to handle older versions. Given that we're about to add support for other SSL implementations I'm not sure that that's a good idea. IIRC there exist quite a bit of different interpretations about what denotes a valid cert between the libraries. As long as just this patch is concerned, I agree it's easier to just implement it ourselves, but if we want to start implementing more complicated rules, then I'd rather not get into that business at all, and let the SSL library vendor deal with the bugs and CVEs. Sounds reasonable. I guess we'll go ahead with this patch for now, but keep this in mind if someone wants to complicate the rules further in the future. +1 -- Regards, Alexey Klyukin
Re: [HACKERS] implement subject alternative names support for SSL connections
On 08/24/2014 03:11 PM, Alexey Klyukin wrote: On Wed, Aug 20, 2014 at 11:53 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 07/25/2014 07:10 PM, Alexey Klyukin wrote: Greetings, I'd like to propose a patch for checking subject alternative names entry in the SSL certificate for DNS names during SSL authentication. Thanks! I just ran into this missing feature last week, while working on my SSL test suite. So +1 for having the feature. This patch needs to be rebased over current master branch, thanks to my refactoring that moved all OpenSSL-specific stuff to be-secure-openssl.c. The patch is rebased against fe-secure-openssl.c (that's where verify_peer_name_matches_certificate appeared in the master branch), I've changed the condition in the for loop to be less confusing (thanks to comments from Magnus and Tom), making an explicit break once a match is detected. The patch doesn't seem to support wildcards in alternative names. Is that on purpose? It would be good to add a little helper function that does the NULL-check, straight comparison, and wildcard check, for a single name. And then use that for the Common Name and all the Alternatives. That'll ensure that all the same rules apply whether the name is the Common Name or an Alternative (assuming that the rules are supposed to be the same; I don't know if that's true). But actually, I wonder if we should delegate the whole hostname matching to OpenSSL? There's a function called X509_check_host for that, although it's new in OpenSSL 1.1.0 so we'd need to add a configure test for that and keep the current code to handle older versions. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] implement subject alternative names support for SSL connections
On 2014-08-25 13:02:50 +0300, Heikki Linnakangas wrote: But actually, I wonder if we should delegate the whole hostname matching to OpenSSL? There's a function called X509_check_host for that, although it's new in OpenSSL 1.1.0 so we'd need to add a configure test for that and keep the current code to handle older versions. Given that we're about to add support for other SSL implementations I'm not sure that that's a good idea. IIRC there exist quite a bit of different interpretations about what denotes a valid cert between the libraries. Doesn't sound fun to me. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] implement subject alternative names support for SSL connections
On 08/25/2014 01:07 PM, Andres Freund wrote: On 2014-08-25 13:02:50 +0300, Heikki Linnakangas wrote: But actually, I wonder if we should delegate the whole hostname matching to OpenSSL? There's a function called X509_check_host for that, although it's new in OpenSSL 1.1.0 so we'd need to add a configure test for that and keep the current code to handle older versions. Given that we're about to add support for other SSL implementations I'm not sure that that's a good idea. IIRC there exist quite a bit of different interpretations about what denotes a valid cert between the libraries. Really? That sounds scary. I can imagine that some libraries support more complicated stuff like Internationalized Domain Names, while others don't, but as long as they all behave the same with the basic stuff, I think that's acceptable. Doesn't sound fun to me. As long as just this patch is concerned, I agree it's easier to just implement it ourselves, but if we want to start implementing more complicated rules, then I'd rather not get into that business at all, and let the SSL library vendor deal with the bugs and CVEs. I guess we'll go ahead with this patch for now, but keep this in mind if someone wants to complicate the rules further in the future. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] implement subject alternative names support for SSL connections
On Wed, Aug 20, 2014 at 11:53 AM, Heikki Linnakangas
hlinnakan...@vmware.com wrote:
On 07/25/2014 07:10 PM, Alexey Klyukin wrote:
Greetings,
I'd like to propose a patch for checking subject alternative names entry
in
the SSL certificate for DNS names during SSL authentication.
Thanks! I just ran into this missing feature last week, while working on
my SSL test suite. So +1 for having the feature.
This patch needs to be rebased over current master branch, thanks to my
refactoring that moved all OpenSSL-specific stuff to be-secure-openssl.c.
The patch is rebased against fe-secure-openssl.c (that's where
verify_peer_name_matches_certificate appeared in the master branch), I've
changed the condition in the for loop to be less confusing (thanks to
comments from Magnus and Tom), making an explicit break once a match is
detected.
Note that It generates a lot of OpenSSL related warnings on my system (66
total) with clang, complaining about
$X is deprecated: first deprecated in OS X 10.7
[-Wdeprecated-declarations], but it does so for most other SSL functions,
so I don't think it's a problem introduced by this patch.
Sincerely,
Alexey.
diff --git a/src/interfaces/libpq/fe-secure-openssl.c
b/src/interfaces/libpq/fe-secure-openssl.c
new file mode 100644
index f950fc3..b4f6bc9
*** a/src/interfaces/libpq/fe-secure-openssl.c
--- b/src/interfaces/libpq/fe-secure-openssl.c
***
*** 60,65
--- 60,66
#ifdef USE_SSL_ENGINE
#include openssl/engine.h
#endif
+ #include openssl/x509v3.h
static bool verify_peer_name_matches_certificate(PGconn *);
static intverify_cb(int ok, X509_STORE_CTX *ctx);
*** wildcard_certificate_match(const char *p
*** 473,479
/*
! *Verify that common name resolves to peer.
*/
static bool
verify_peer_name_matches_certificate(PGconn *conn)
--- 474,480
/*
! *Verify that common name or any of the alternative dNSNames resolves to
peer.
*/
static bool
verify_peer_name_matches_certificate(PGconn *conn)
*** verify_peer_name_matches_certificate(PGc
*** 492,499
/*
* 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),
--- 493,498
*** verify_peer_name_matches_certificate(PGc
*** 556,565
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;
}
}
--- 555,627
result = true;
else
{
+ inti;
+ intalt_names_total;
+ STACK_OF(GENERAL_NAME) *alt_names;
+
+ /* Get the list and the total number of alternative
names. */
+ alt_names = (STACK_OF(GENERAL_NAME) *)
X509_get_ext_d2i(conn-peer, NID_subject_alt_name, NULL, NULL);
+ if (alt_names != NULL)
+ alt_names_total =
sk_GENERAL_NAME_num(alt_names);
+ else
+ alt_names_total = 0;
+
+ result = false;
+
+ /*
+* Compare the alternative names dnsNames identifies
against
+* the originally given hostname.
+*/
+ for (i = 0; i alt_names_total; i++)
+ {
+ const GENERAL_NAME *name =
sk_GENERAL_NAME_value(alt_names, i);
+ if (name-type == GEN_DNS)
+ {
+ intreported_len;
+ intactual_len;
+ char *dns_namedata,
+ *dns_name;
+
+ 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);
+ memcpy(dns_name, dns_namedata,
reported_len);
+ dns_name[reported_len] = '\0';
+
+
Re: [HACKERS] implement subject alternative names support for SSL connections
On 07/25/2014 07:10 PM, Alexey Klyukin wrote: Greetings, I'd like to propose a patch for checking subject alternative names entry in the SSL certificate for DNS names during SSL authentication. Thanks! I just ran into this missing feature last week, while working on my SSL test suite. So +1 for having the feature. This patch needs to be rebased over current master branch, thanks to my refactoring that moved all OpenSSL-specific stuff to be-secure-openssl.c. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] implement subject alternative names support for SSL connections
Greetings,
I'd like to propose a patch for checking subject alternative names entry in
the SSL certificate for DNS names during SSL authentication.
When the client PGSSLMODE is set to verify-full, the common name (CN) of
the PostgreSQL server in the certificate is matched against the actual
host name supplied by the client. A successful connection is only
established when those names match.
If a failover schema with a floating IP is used, a single DNS name may
point to different hosts after failover. In order to maintain the correct
pair of (client connection FQDN, FQDN in the certificate) the certificate
from the master should also be transferred to the slave. Unfortunately, it
makes failover more complicated, since the server restart is required when
the new certificate is installed.
The other option is to issue a single certificate covering all the hosts
that may participate in the failover.
So far the only way to cover more than one server name with a single
certificate is to use wildcards in the server common name, i.e.
*.db.example com. Unfortunately, this schema only works for names like
master.db.example.com, slave.db.example.com, but not for the example.com
and db-master.example.com and db-slave.example.com or other more complex
naming schemas.
The other way to issue a single certificate for multiple names is to set
the subject alternative names, described in the RFC 3280 4.2.1.7. SAN
allows binding multiple identities to the certificate, including DNS names.
For the names above the SAN with look like this:
X509v3 Subject Alternative Name:
DNS:db-master.example.com, DNS:db-slave.example.com.
At the moment PostgreSQL doesn't support SANs, but should, according to the
following comment in the code in verify_peer_name_matches_certificate:
/*
* Extract the common name from the certificate.
*
* XXX: Should support alternate names here
*/
The attached patch adds support for checking the client supplied names
against the dNSName-s in SAN, making it easier to set secure HA
environments using PostgreSQL. If SAN section is present, the DNS names
from that section are checked against the peer name in addition to checking
the common name (CN) from the certificate. The match is successful if at
least one of those names match the name supplied by the peer.
I gave it a spin and it works in our environment (Linux database servers,
Linux and Mac clients). I don't have either Windows or old versions of
OpenSSL, it's not tested against those systems.
I'd appreciate your feedback.
Sincerely,
--
Alexey Klyukin
diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
new file mode 100644
index 9ba3567..64eab27
*** a/src/interfaces/libpq/fe-secure.c
--- b/src/interfaces/libpq/fe-secure.c
***
*** 64,69
--- 64,70
#ifdef USE_SSL_ENGINE
#include openssl/engine.h
#endif
+ #include openssl/x509v3.h
#ifndef WIN32
*** wildcard_certificate_match(const char *p
*** 754,760
/*
! *Verify that common name resolves to peer.
*/
static bool
verify_peer_name_matches_certificate(PGconn *conn)
--- 755,761
/*
! *Verify that common name or any of the alternative dNSNames resolves to
peer.
*/
static bool
verify_peer_name_matches_certificate(PGconn *conn)
*** verify_peer_name_matches_certificate(PGc
*** 773,780
/*
* 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),
--- 774,779
*** verify_peer_name_matches_certificate(PGc
*** 837,846
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;
}
}
--- 836,904
result = true;
else
{
+ inti;
+ intalt_names_total;
+ STACK_OF(GENERAL_NAME) *alt_names;
+
+ /* Get the list and the total number of alternative
names. */
+ alt_names = (STACK_OF(GENERAL_NAME) *)
X509_get_ext_d2i(conn-peer, NID_subject_alt_name, NULL, NULL);
+ if (alt_names != NULL)
+ alt_names_total =
sk_GENERAL_NAME_num(alt_names);
+ else
+ alt_names_total = 0;
+
+ result = false;
+
+ /*
+
Re: [HACKERS] implement subject alternative names support for SSL connections
On Fri, Jul 25, 2014 at 6:10 PM, Alexey Klyukin al...@hintbits.com wrote: Greetings, I'd like to propose a patch for checking subject alternative names entry in the SSL certificate for DNS names during SSL authentication. When the client PGSSLMODE is set to verify-full, the common name (CN) of the PostgreSQL server in the certificate is matched against the actual host name supplied by the client. A successful connection is only established when those names match. If a failover schema with a floating IP is used, a single DNS name may point to different hosts after failover. In order to maintain the correct pair of (client connection FQDN, FQDN in the certificate) the certificate from the master should also be transferred to the slave. Unfortunately, it makes failover more complicated, since the server restart is required when the new certificate is installed. The other option is to issue a single certificate covering all the hosts that may participate in the failover. So far the only way to cover more than one server name with a single certificate is to use wildcards in the server common name, i.e. *.db.example com. Unfortunately, this schema only works for names like master.db.example.com, slave.db.example.com, but not for the example.com and db-master.example.com and db-slave.example.com or other more complex naming schemas. The other way to issue a single certificate for multiple names is to set the subject alternative names, described in the RFC 3280 4.2.1.7. SAN allows binding multiple identities to the certificate, including DNS names. For the names above the SAN with look like this: X509v3 Subject Alternative Name: DNS:db-master.example.com, DNS:db-slave.example.com. At the moment PostgreSQL doesn't support SANs, but should, according to the following comment in the code in verify_peer_name_matches_certificate: /* * Extract the common name from the certificate. * * XXX: Should support alternate names here */ I believe that comment is mine, and yes, it should definitely be done. So absolutely +1 on the feature :) The attached patch adds support for checking the client supplied names against the dNSName-s in SAN, making it easier to set secure HA environments using PostgreSQL. If SAN section is present, the DNS names from that section are checked against the peer name in addition to checking the common name (CN) from the certificate. The match is successful if at least one of those names match the name supplied by the peer. I gave it a spin and it works in our environment (Linux database servers, Linux and Mac clients). I don't have either Windows or old versions of OpenSSL, it's not tested against those systems. We definitely need testing for older openssl. I don't think Windows will make a difference, but openssl definitely may - there is a fairly large chance we need a configure check. I'd appreciate your feedback. I just took a very quick look at the code, and just noticed one thing: Why keep looping once you've found a match? When you set result=true you should break; from the loop I think. Not necessarily for performance, but there might be something about a different extension we can't parse for example, no need to fail in that case. Please add it to the next CF - this was just a very quick review, and it needs a proper one along with openssl version testing :) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] implement subject alternative names support for SSL connections
On Fri, Jul 25, 2014 at 6:34 PM, Magnus Hagander mag...@hagander.net wrote: I just took a very quick look at the code, and just noticed one thing: Why keep looping once you've found a match? When you set result=true you should break; from the loop I think. Not necessarily for performance, but there might be something about a different extension we can't parse for example, no need to fail in that case. The for loop header is for (i = 0; i alt_names_total !result; i++), so the loop should terminate right when the result becomes true, which happens if the pg_strcasecmp finds a match between the given dNSName and the name supplied by the client. Please add it to the next CF - this was just a very quick review, and it needs a proper one along with openssl version testing :) Done. Sincerely, -- Alexey Klyukin
Re: [HACKERS] implement subject alternative names support for SSL connections
On Fri, Jul 25, 2014 at 7:15 PM, Alexey Klyukin al...@hintbits.com wrote: On Fri, Jul 25, 2014 at 6:34 PM, Magnus Hagander mag...@hagander.net wrote: I just took a very quick look at the code, and just noticed one thing: Why keep looping once you've found a match? When you set result=true you should break; from the loop I think. Not necessarily for performance, but there might be something about a different extension we can't parse for example, no need to fail in that case. The for loop header is for (i = 0; i alt_names_total !result; i++), so the loop should terminate right when the result becomes true, which happens if the pg_strcasecmp finds a match between the given dNSName and the name supplied by the client. oh, ha. So yeah, that was too quick to count as a review - clearly :) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] implement subject alternative names support for SSL connections
Magnus Hagander mag...@hagander.net writes: On Fri, Jul 25, 2014 at 7:15 PM, Alexey Klyukin al...@hintbits.com wrote: On Fri, Jul 25, 2014 at 6:34 PM, Magnus Hagander mag...@hagander.net Why keep looping once you've found a match? When you set result=true you should break; from the loop I think. Not necessarily for performance, but there might be something about a different extension we can't parse for example, no need to fail in that case. The for loop header is for (i = 0; i alt_names_total !result; i++), so the loop should terminate right when the result becomes true, which happens if the pg_strcasecmp finds a match between the given dNSName and the name supplied by the client. oh, ha. So yeah, that was too quick to count as a review - clearly :) FWIW, I find that type of loop coding to be extremely poor style, precisely because it's not too readable. A break in the loop body is *far* more obvious to the reader. (Not to mention that it doesn't add overhead to the loop on iterations where you can't break.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
