On Mon, Sep 8, 2014 at 8:04 PM, Heikki Linnakangas
<[email protected]> 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 int pqSocketPoll(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();
return dgettext(PG_TEXTDOMAIN("libpq"), msgid);
}
+ char *
+ libpq_ngettext(const char *msgid, const char *msgid_plural, unsigned long n)
+ {
+ libpq_bindomain();
+ return dngettext(PG_TEXTDOMAIN("libpq"), msgid, msgid_plural, n);
+ }
+
#endif /* ENABLE_NLS */
diff --git a/src/interfaces/libpq/fe-secure-openssl.c
b/src/interfaces/libpq/fe-secure-openssl.c
new file mode 100644
index f950fc3..c51e8ff
*** a/src/interfaces/libpq/fe-secure-openssl.c
--- b/src/interfaces/libpq/fe-secure-openssl.c
***************
*** 60,68 ****
--- 60,72 ----
#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 *);
*************** wildcard_certificate_match(const char *p
*** 473,570 ****
/*
! * 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
! * 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;
}
#ifdef ENABLE_THREAD_SAFETY
--- 477,704 ----
/*
! * 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 int
! verify_peer_name_matches_certificate_name(PGconn *conn, ASN1_STRING
*name_entry,
!
char **store_name)
{
int len;
! char *name;
! unsigned char *namedata;
! int result;
! *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;
! }
/*
! * GEN_DNS can be only IA5String, equivalent to US ASCII.
*
! * There is no guarantee the string returned from the certificate is
! * NULL-terminated, so make a copy that is.
*/
! namedata = 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 -1;
}
! 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("SSL certificate's name contains embedded
null\n"));
! return -1;
}
! if (pg_strcasecmp(name, conn->pghost) == 0)
{
! /* Exact name match */
! result = 1;
! }
! 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;
/*
! * 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;
!
! /* Check that we have a hostname to compare with. */
! 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;
}
/*
! * First, get the Subject Alternative Names (SANs) from the certificate,
! * and compare them against the originally given hostname.
*/
! peer_san = (STACK_OF(GENERAL_NAME) *)
! X509_get_ext_d2i(conn->peer, NID_subject_alt_name, NULL, NULL);
!
! if (peer_san)
{
! int san_len = sk_GENERAL_NAME_num(peer_san);
!
! for (i = 0; i < san_len; 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);
! }
! }
! if (found_match || got_error)
! break;
! }
! sk_GENERAL_NAME_free(peer_san);
}
!
! /*
! * If no match to any of the SANs, check the Common Name.
! */
! if (!found_match && !got_error)
{
! 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_ngettext("server certificate for \"%s\" (and %d other name) does not
match host name \"%s\"\n",
!
"server certificate for \"%s\" (and %d other names) does not match
host name \"%s\"\n",
!
names_examined - 1),
! 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("could
not get server's hostname from server certificate\n"));
}
}
! /* clean up */
! if (first_name)
! free(first_name);
!
! return found_match && !got_error;
}
#ifdef ENABLE_THREAD_SAFETY
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
new file mode 100644
index 6032904..9808ab4
*** a/src/interfaces/libpq/libpq-int.h
--- b/src/interfaces/libpq/libpq-int.h
*************** extern ssize_t pgtls_write(PGconn *conn,
*** 653,660 ****
--- 653,664 ----
extern char *
libpq_gettext(const char *msgid)
__attribute__((format_arg(1)));
+ extern char *
+ libpq_ngettext(const char *msgid, const char *msgid_plural, unsigned long n)
+ __attribute__((format_arg(1))) __attribute__((format_arg(2)));
#else
#define libpq_gettext(x) (x)
+ #define libpq_ngettext(s,p,n) ((n) == 1 ? (s) : (p))
#endif
/*
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
