Skip to content

Commit 4ee9db0

Browse files
tglsfdctglsfdc
committed
Remove arbitrary limitation on length of common name in SSL certificates.
Both libpq and the backend would truncate a common name extracted from a certificate at 32 bytes. Replace that fixed-size buffer with dynamically allocated string so that there is no hard limit. While at it, remove the code for extracting peer_dn, which we weren't using for anything; and don't bother to store peer_cn longer than we need it in libpq. This limit was not so terribly unreasonable when the code was written, because we weren't using the result for anything critical, just logging it. But now that there are options for checking the common name against the server host name (in libpq) or using it as the user's name (in the server), this could result in undesirable failures. In the worst case it even seems possible to spoof a server name or user name, if the correct name is exactly 32 bytes and the attacker can persuade a trusted CA to issue a certificate in which that string is a prefix of the certificate's common name. (To exploit this for a server name, he'd also have to send the connection astray via phony DNS data or some such.) The case that this is a realistic security threat is a bit thin, but nonetheless we'll treat it as one. Back-patch to 8.4. Older releases contain the faulty code, but it's not a security problem because the common name wasn't used for anything interesting. Reported and patched by Heikki Linnakangas Security: CVE-2012-0867
1 parent 993b3e5 commit 4ee9db0

File tree

4 files changed

+107
-67
lines changed

4 files changed

+107
-67
lines changed

‎src/backend/libpq/be-secure.c

Lines changed: 40 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@
7373

7474
#include "libpq/libpq.h"
7575
#include "tcop/tcopprot.h"
76+
#include "utils/memutils.h"
7677

7778

7879
#ifdef USE_SSL
@@ -950,44 +951,54 @@ open_server_SSL(Port *port)
950951

951952
port->count = 0;
952953

953-
/* get client certificate, if available. */
954+
/* Get client certificate, if available. */
954955
port->peer = SSL_get_peer_certificate(port->ssl);
955-
if (port->peer == NULL)
956-
{
957-
strlcpy(port->peer_dn, "(anonymous)", sizeof(port->peer_dn));
958-
strlcpy(port->peer_cn, "(anonymous)", sizeof(port->peer_cn));
959-
}
960-
else
956+
957+
/* and extract the Common Name from it. */
958+
port->peer_cn = NULL;
959+
if (port->peer != NULL)
961960
{
962-
X509_NAME_oneline(X509_get_subject_name(port->peer),
963-
port->peer_dn, sizeof(port->peer_dn));
964-
port->peer_dn[sizeof(port->peer_dn) - 1] = '\0';
965-
r = X509_NAME_get_text_by_NID(X509_get_subject_name(port->peer),
966-
NID_commonName, port->peer_cn, sizeof(port->peer_cn));
967-
port->peer_cn[sizeof(port->peer_cn) - 1] = '\0';
968-
if (r == -1)
969-
{
970-
/* Unable to get the CN, set it to blank so it can't be used */
971-
port->peer_cn[0] = '\0';
972-
}
973-
else
961+
int len;
962+
963+
len = X509_NAME_get_text_by_NID(X509_get_subject_name(port->peer),
964+
NID_commonName, NULL, 0);
965+
if (len != -1)
974966
{
967+
char *peer_cn;
968+
969+
peer_cn = MemoryContextAlloc(TopMemoryContext, len + 1);
970+
r = X509_NAME_get_text_by_NID(X509_get_subject_name(port->peer),
971+
NID_commonName, peer_cn, len + 1);
972+
peer_cn[len] = '\0';
973+
if (r != len)
974+
{
975+
/* shouldn't happen */
976+
pfree(peer_cn);
977+
close_SSL(port);
978+
return -1;
979+
}
980+
975981
/*
976-
* Reject embedded NULLs in certificate common name to prevent attacks like
977-
* CVE-2009-4034.
982+
* Reject embedded NULLs in certificate common name to prevent
983+
* attacks like CVE-2009-4034.
978984
*/
979-
if (r != strlen(port->peer_cn))
985+
if (len != strlen(peer_cn))
980986
{
981987
ereport(COMMERROR,
982988
(errcode(ERRCODE_PROTOCOL_VIOLATION),
983989
errmsg("SSL certificate's common name contains embedded null")));
990+
pfree(peer_cn);
984991
close_SSL(port);
985992
return -1;
986993
}
994+
995+
port->peer_cn = peer_cn;
987996
}
988997
}
998+
989999
ereport(DEBUG2,
990-
(errmsg("SSL connection from \"%s\"", port->peer_cn)));
1000+
(errmsg("SSL connection from \"%s\"",
1001+
port->peer_cn ? port->peer_cn : "(anonymous)")));
9911002

9921003
/* set up debugging/info callback */
9931004
SSL_CTX_set_info_callback(SSL_context, info_cb);
@@ -1013,6 +1024,12 @@ close_SSL(Port *port)
10131024
X509_free(port->peer);
10141025
port->peer = NULL;
10151026
}
1027+
1028+
if (port->peer_cn)
1029+
{
1030+
pfree(port->peer_cn);
1031+
port->peer_cn = NULL;
1032+
}
10161033
}
10171034

10181035
/*

‎src/include/libpq/libpq-be.h

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -166,8 +166,7 @@ typedef struct Port
166166
#ifdef USE_SSL
167167
SSL *ssl;
168168
X509 *peer;
169-
char peer_dn[128 + 1];
170-
char peer_cn[SM_USER + 1];
169+
char *peer_cn;
171170
unsigned long count;
172171
#endif
173172
} Port;

‎src/interfaces/libpq/fe-secure.c

Lines changed: 66 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -675,40 +675,93 @@ wildcard_certificate_match(const char *pattern, const char *string)
675675
static bool
676676
verify_peer_name_matches_certificate(PGconn *conn)
677677
{
678+
char *peer_cn;
679+
int r;
680+
int len;
681+
bool result;
682+
678683
/*
679684
* If told not to verify the peer name, don't do it. Return true indicating
680685
* that the verification was successful.
681686
*/
682687
if (strcmp(conn->sslmode, "verify-full") != 0)
683688
return true;
684689

690+
/*
691+
* Extract the common name from the certificate.
692+
*
693+
* XXX: Should support alternate names here
694+
*/
695+
/* First find out the name's length and allocate a buffer for it. */
696+
len = X509_NAME_get_text_by_NID(X509_get_subject_name(conn->peer),
697+
NID_commonName, NULL, 0);
698+
if (len == -1)
699+
{
700+
printfPQExpBuffer(&conn->errorMessage,
701+
libpq_gettext("could not get server common name from server certificate\n"));
702+
return false;
703+
}
704+
peer_cn = malloc(len + 1);
705+
if (peer_cn == NULL)
706+
{
707+
printfPQExpBuffer(&conn->errorMessage,
708+
libpq_gettext("out of memory\n"));
709+
return false;
710+
}
711+
712+
r = X509_NAME_get_text_by_NID(X509_get_subject_name(conn->peer),
713+
NID_commonName, peer_cn, len + 1);
714+
if (r != len)
715+
{
716+
/* Got different length than on the first call. Shouldn't happen. */
717+
printfPQExpBuffer(&conn->errorMessage,
718+
libpq_gettext("could not get server common name from server certificate\n"));
719+
free(peer_cn);
720+
return false;
721+
}
722+
peer_cn[len] = '\0';
723+
724+
/*
725+
* Reject embedded NULLs in certificate common name to prevent attacks
726+
* like CVE-2009-4034.
727+
*/
728+
if (len != strlen(peer_cn))
729+
{
730+
printfPQExpBuffer(&conn->errorMessage,
731+
libpq_gettext("SSL certificate's common name contains embedded null\n"));
732+
free(peer_cn);
733+
return false;
734+
}
735+
736+
/*
737+
* We got the peer's common name. Now compare it against the originally
738+
* given hostname.
739+
*/
685740
if (!(conn->pghost && conn->pghost[0] != '\0'))
686741
{
687742
printfPQExpBuffer(&conn->errorMessage,
688743
libpq_gettext("host name must be specified for a verified SSL connection\n"));
689-
return false;
744+
result = false;
690745
}
691746
else
692747
{
693-
/*
694-
* Connect by hostname.
695-
*
696-
* XXX: Should support alternate names here
697-
*/
698-
if (pg_strcasecmp(conn->peer_cn, conn->pghost) == 0)
748+
if (pg_strcasecmp(peer_cn, conn->pghost) == 0)
699749
/* Exact name match */
700-
return true;
701-
else if (wildcard_certificate_match(conn->peer_cn, conn->pghost))
750+
result = true;
751+
else if (wildcard_certificate_match(peer_cn, conn->pghost))
702752
/* Matched wildcard certificate */
703-
return true;
753+
result = true;
704754
else
705755
{
706756
printfPQExpBuffer(&conn->errorMessage,
707757
libpq_gettext("server common name \"%s\" does not match host name \"%s\"\n"),
708-
conn->peer_cn, conn->pghost);
709-
return false;
758+
peer_cn, conn->pghost);
759+
result = false;
710760
}
711761
}
762+
763+
free(peer_cn);
764+
return result;
712765
}
713766

714767
/*
@@ -1337,7 +1390,7 @@ open_client_SSL(PGconn *conn)
13371390
* SSL_CTX_set_verify() if root.crt exists.
13381391
*/
13391392

1340-
/* pull out server distinguished and common names */
1393+
/* get server certificate */
13411394
conn->peer = SSL_get_peer_certificate(conn->ssl);
13421395
if (conn->peer == NULL)
13431396
{
@@ -1351,33 +1404,6 @@ open_client_SSL(PGconn *conn)
13511404
return PGRES_POLLING_FAILED;
13521405
}
13531406

1354-
X509_NAME_oneline(X509_get_subject_name(conn->peer),
1355-
conn->peer_dn, sizeof(conn->peer_dn));
1356-
conn->peer_dn[sizeof(conn->peer_dn) - 1] = '\0';
1357-
1358-
r = X509_NAME_get_text_by_NID(X509_get_subject_name(conn->peer),
1359-
NID_commonName, conn->peer_cn, SM_USER);
1360-
conn->peer_cn[SM_USER] = '\0'; /* buffer is SM_USER+1 chars! */
1361-
if (r == -1)
1362-
{
1363-
/* Unable to get the CN, set it to blank so it can't be used */
1364-
conn->peer_cn[0] = '\0';
1365-
}
1366-
else
1367-
{
1368-
/*
1369-
* Reject embedded NULLs in certificate common name to prevent attacks like
1370-
* CVE-2009-4034.
1371-
*/
1372-
if (r != strlen(conn->peer_cn))
1373-
{
1374-
printfPQExpBuffer(&conn->errorMessage,
1375-
libpq_gettext("SSL certificate's common name contains embedded null\n"));
1376-
close_SSL(conn);
1377-
return PGRES_POLLING_FAILED;
1378-
}
1379-
}
1380-
13811407
if (!verify_peer_name_matches_certificate(conn))
13821408
{
13831409
close_SSL(conn);

‎src/interfaces/libpq/libpq-int.h

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -385,8 +385,6 @@ struct pg_conn
385385
* attempting normal connection */
386386
SSL *ssl; /* SSL status, if have SSL connection */
387387
X509 *peer; /* X509 cert of server */
388-
char peer_dn[256 + 1]; /* peer distinguished name */
389-
char peer_cn[SM_USER + 1]; /* peer common name */
390388
#ifdef USE_SSL_ENGINE
391389
ENGINE *engine; /* SSL engine, if any */
392390
#else

0 commit comments

Comments
 (0)