8000 Remove arbitrary limitation on length of common name in SSL certifica… · jandas/postgres@850d341 · GitHub
[go: up one dir, main page]
Skip to content

Commit 850d341

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 de323d5 commit 850d341

File tree

4 files changed

+105
-65
lines changed

4 files changed

+105
-65
lines changed

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

Lines changed: 38 additions & 21 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
@@ -964,44 +965,54 @@ open_server_SSL(Port *port)
964965

965966
port->count = 0;
966967

967-
/* get client certificate, if available. */
968+
/* Get client certificate, if available. */
968969
port->peer = SSL_get_peer_certificate(port->ssl);
969-
if (port->peer == NULL)
970-
{
971-
strlcpy(port->peer_dn, "(anonymous)", sizeof(port->peer_dn));
972-
strlcpy(port->peer_cn, "(anonymous)", sizeof(port->peer_cn));
973-
}
974-
else
970+
971+
/* and extract the Common Name from it. */
972+
port->peer_cn = NULL;
973+
if (port->peer != NULL)
975974
{
976-
X509_NAME_oneline(X509_get_subject_name(port->peer),
977-
port->peer_dn, sizeof(port 8000 ->peer_dn));
978-
port->peer_dn[sizeof(port->peer_dn) - 1] = '\0';
979-
r = X509_NAME_get_text_by_NID(X509_get_subject_name(port->peer),
980-
NID_commonName, port->peer_cn, sizeof(port->peer_cn));
981-
port->peer_cn[sizeof(port->peer_cn) - 1] = '\0';
982-
if (r == -1)
983-
{
984-
/* Unable to get the CN, set it to blank so it can't be used */
985-
port->peer_cn[0] = '\0';
986-
}
987-
else
975+
int len;
976+
977+
len = X509_NAME_get_text_by_NID(X509_get_subject_name(port->peer),
978+
NID_commonName, NULL, 0);
979+
if (len != -1)
988980
{
981+
char *peer_cn;
982+
983+
peer_cn = MemoryContextAlloc(TopMemoryContext, len + 1);
984+
r = X509_NAME_get_text_by_NID(X509_get_subject_name(port->peer),
985+
NID_commonName, peer_cn, len + 1);
986+
peer_cn[len] = '\0';
987+
if (r != len)
988+
{
989+
/* shouldn't happen */
990+
pfree(peer_cn);
991+
close_SSL(port);
992+
return -1;
993+
}
994+
989995
/*
990996
* Reject embedded NULLs in certificate common name to prevent
991997
* attacks like CVE-2009-4034.
992998
*/
993-
if (r != strlen(port->peer_cn))
999+
if (len != strlen(peer_cn))
9941000
{
9951001
ereport(COMMERROR,
9961002
(errcode(ERRCODE_PROTOCOL_VIOLATION),
9971003
errmsg("SSL certificate's common name contains embedded null")));
1004+
pfree(peer_cn);
9981005
close_SSL(port);
9991006
return -1;
10001007
}
1008+
1009+
port->peer_cn = peer_cn;
10011010
}
10021011
}
1012+
10031013
ereport(DEBUG2,
1004-
(errmsg("SSL connection from \"%s\"", port->peer_cn)));
1014+
(errmsg("SSL connection from \"%s\"",
1015+
port->peer_cn ? port->peer_cn : "(anonymous)")));
10051016

10061017
/* set up debugging/info callback */
10071018
SSL_CTX_set_info_callback(SSL_context, info_cb);
@@ -1027,6 +1038,12 @@ close_SSL(Port *port)
10271038
X509_free(port->peer);
10281039
port->peer = NULL;
10291040
}
1041+
1042+
if (port->peer_cn)
1043+
{
1044+
pfree(port->peer_cn);
1045+
port->peer_cn = NULL;
1046+
}
10301047
}
10311048

10321049
/*

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

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

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

Lines changed: 66 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -733,40 +733,93 @@ wildcard_certificate_match(const char *pattern, const char *string)
733733
static bool
734734
verify_peer_name_matches_certificate(PGconn *conn)
735735
{
736+
char *peer_cn;
737+
int r;
738+
int len;
739+
bool result;
740+
736741
/*
737742
* If told not to verify the peer name, don't do it. Return true indicating
738743
* that the verification was successful.
739744
*/
740745
if (strcmp(conn->sslmode, "verify-full") != 0)
741746
return true;
742747

748+
/*
749+
* Extract the common name from the certificate.
750+
*
751+
* XXX: Should support alternate names here
752+
*/
753+
/* First find out the name's length and allocate a buffer for it. */
754+
len = X509_NAME_get_text_by_NID(X509_get_subject_name(conn->peer),
755+
NID_commonName, NULL, 0);
756+
if (len == -1)
757+
{
758+
printfPQExpBuffer(&conn->errorMessage,
759+
libpq_gettext("could not get server common name from server certificate\n"));
760+
return false;
761+
}
762+
peer_cn = malloc(len + 1);
763+
if (peer_cn == NULL)
764+
{
765+
printfPQExpBuffer(&conn->errorMessage,
766+
libpq_gettext("out of memory\n"));
767+
return false;
768+
}
769+
770+
r = X509_NAME_get_text_by_NID(X509_get_subject_name(conn->peer),
771+
NID_commonName, peer_cn, len + 1);
772+
if (r != len)
773+
{
774+
/* Got different length than on the first call. Shouldn't happen. */
775+
printfPQExpBuffer(&conn->errorMessage,
776+
libpq_gettext("could not get server common name from server certificate\n"));
777+
free(peer_cn);
778+
return false;
779+
}
780+
peer_cn[len] = '\0';
781+
782+
/*
783+
* Reject embedded NULLs in certificate common name to prevent attacks
784+
* like CVE-2009-4034.
785+
*/
786+
if (len != strlen(peer_cn))
787+
{
788+
printfPQExpBuffer(&conn->errorMessage,
789+
libpq_gettext("SSL certificate's common name contains embedded null\n"));
790+
free(peer_cn);
791+
return false;
792+
}
793+
794+
/*
795+
* We got the peer's common name. Now compare it against the originally
796+
* given hostname.
797+
*/
743798
if (!(conn->pghost && conn->pghost[0] != '\0'))
744799
{
745800
printfPQExpBuffer(&conn->errorMessage,
746801
libpq_gettext("host name must be specified for a verified SSL connection\n"));
747-
return false;
802+
result = false;
748803
}
749804
else
750805
{
751-
/*
752-
* Compare CN to originally given hostname.
753-
*
754-
* XXX: Should support alternate names here
755-
*/
756-
if (pg_strcasecmp(conn->peer_cn, conn->pghost) == 0)
806+
if (pg_strcasecmp(peer_cn, conn->pghost) == 0)
757807
/* Exact name match */
758-
return true;
759-
else if (wildcard_certificate_match(conn->peer_cn, conn->pghost))
808+
result = true;
809+
else if (wildcard_certificate_match(peer_cn, conn->pghost))
760810
/* Matched wildcard certificate */
761-
return true;
811+
result = true;
762812
else
763813
{
764814
printfPQExpBuffer(&conn->errorMessage,
765815
libpq_gettext("server common name \"%s\" does not match host name \"%s\"\n"),
766-
conn->peer_cn, conn->pghost);
767-
return false;
816+
peer_cn, conn->pghost);
817+
result = false;
768818
}
769819
}
820+
821+
free(peer_cn);
822+
return result;
770823
}
771824

772825
#ifdef ENABLE_THREAD_SAFETY
@@ -1362,7 +1415,7 @@ open_client_SSL(PGconn *conn)
13621415
* SSL_CTX_set_verify(), if root.crt exists.
13631416
*/
13641417

1365-
/* pull out server distinguished and common names */
1418+
/* get server certificate */
13661419
conn->peer = SSL_get_peer_certificate(conn->ssl);
13671420
if (conn->peer == NULL)
13681421
{
@@ -1376,33 +1429,6 @@ open_client_SSL(PGconn *conn)
13761429
return PGRES_POLLING_FAILED;
13771430
}
13781431

1379-
X509_NAME_oneline(X509_get_subject_name(conn->peer),
1380-
conn->peer_dn, sizeof(conn->peer_dn));
1381-
conn->peer_dn[sizeof(conn->peer_dn) - 1] = '\0';
1382-
1383-
r = X509_NAME_get_text_by_NID(X509_get_subject_name(conn->peer),
1384-
NID_commonName, conn->peer_cn, SM_USER);
1385-
conn->peer_cn[SM_USER] = '\0'; /* buffer is SM_USER+1 chars! */
1386-
if (r == -1)
1387-
{
1388-
/* Unable to get the CN, set it to blank so it can't be used */
1389-
conn->peer_cn[0] = '\0';
1390-
}
1391-
else
1392-
{
1393-
/*
1394-
* Reject embedded NULLs in certificate common name to prevent attacks
1395-
* like CVE-2009-4034.
1396-
*/
1397-
if (r != strlen(conn->peer_cn))
1398-
{
1399-
printfPQExpBuffer(&conn->errorMessage,
1400-
libpq_gettext("SSL certificate's common name contains embedded null\n"));
1401-
close_SSL(conn);
1402-
return PGRES_POLLING_FAILED;
1403-
}
1404-
}
1405-
14061432
if (!verify_peer_name_matches_certificate(conn))
14071433
{
14081434
close_SSL(conn);

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -397,8 +397,6 @@ struct pg_conn
397397
* attempting normal connection */
398398
SSL *ssl; /* SSL status, if have SSL connection */
399399
X509 *peer; /* X509 cert of server */
400-
char peer_dn[256 + 1]; /* peer distinguished name */
401-
char peer_cn[SM_USER + 1]; /* peer common name */
402400
#ifdef USE_SSL_ENGINE
403401
ENGINE *engine; /* SSL engine, if any */
404402
#else

0 commit comments

Comments
 (0)
0