From 8915c3fb4fa40743bdddf23013a63e014d03d02c Mon Sep 17 00:00:00 2001 From: Tjerk Meesters Date: Sat, 21 Sep 2013 16:45:20 +0800 Subject: [PATCH 1/6] added better wildcard matching for CN --- ext/openssl/openssl.c | 35 +++++++++++++++++++-------- ext/openssl/tests/bug65729.pem | 28 ++++++++++++++++++++++ ext/openssl/tests/bug65729.phpt | 42 +++++++++++++++++++++++++++++++++ 3 files changed, 95 insertions(+), 10 deletions(-) create mode 100644 ext/openssl/tests/bug65729.pem create mode 100644 ext/openssl/tests/bug65729.phpt diff --git a/ext/openssl/openssl.c b/ext/openssl/openssl.c index 4aac4e3137cad..5460f3a6e1131 100644 --- a/ext/openssl/openssl.c +++ b/ext/openssl/openssl.c @@ -4829,6 +4829,30 @@ static int verify_callback(int preverify_ok, X509_STORE_CTX *ctx) /* {{{ */ } /* }}} */ +static int php_openssl_match_cn(const char *subjectname, const char *certname) +{ + int match = strcmp(subjectname, certname) == 0; + + if (!match) { + char *wildcard = strchr(certname, '*'); + int prefix_len = wildcard - certname; + + /* 1) prefix, if not empty, must match */ + if (wildcard && (prefix_len == 0 || strncmp(subjectname, certname, prefix_len) == 0)) { + const char *suffix = subjectname + strlen(subjectname) - strlen(wildcard + 1); + + /* + * 2) suffix must match + * 3) no period between prefix and suffix + **/ + match = strcmp(wildcard + 1, suffix) == 0 && + memchr(subjectname + prefix_len, '.', suffix - subjectname - prefix_len) == NULL; + } + } + + return match; +} + int php_openssl_apply_verification_policy(SSL *ssl, X509 *peer, php_stream *stream TSRMLS_DC) /* {{{ */ { zval **val = NULL; @@ -4881,16 +4905,7 @@ int php_openssl_apply_verification_policy(SSL *ssl, X509 *peer, php_stream *stre return FAILURE; } - match = strcmp(cnmatch, buf) == 0; - if (!match && strlen(buf) > 3 && buf[0] == '*' && buf[1] == '.') { - /* Try wildcard */ - - if (strchr(buf+2, '.')) { - char *tmp = strstr(cnmatch, buf+1); - - match = tmp && strcmp(tmp, buf+2) && tmp == strchr(cnmatch, '.'); - } - } + match = php_openssl_match_cn(cnmatch, buf); if (!match) { /* didn't match */ diff --git a/ext/openssl/tests/bug65729.pem b/ext/openssl/tests/bug65729.pem new file mode 100644 index 0000000000000..dbeed6efd3011 --- /dev/null +++ b/ext/openssl/tests/bug65729.pem @@ -0,0 +1,28 @@ +-----BEGIN CERTIFICATE----- +MIICCTCCAXICCQDNMI29sowT7TANBgkqhkiG9w0BAQUFADBJMQswCQYDVQQGEwJT +RzESMBAGA1UECBMJVGVzdHZpbGxlMREwDwYDVQQKEwhkYXRpYmJhdzETMBEGA1UE +AxQKKi50ZXN0LmNvbTAeFw0xMzA5MjEwNzUyMjRaFw0xNDA5MjEwNzUyMjRaMEkx +CzAJBgNVBAYTAlNHMRIwEAYDVQQIEwlUZXN0dmlsbGUxETAPBgNVBAoTCGRhdGli +YmF3MRMwEQYDVQQDFAoqLnRlc3QuY29tMIGfMA0GCSqGSIb3DQEBAQUAA4GNADCB +iQKBgQCdzVnic8K5W4SVbwVuqezcTjeqVLoQ91vVNZB0Jnsuz6q3DoK03oAd1jTe +Vd0k+MQDbXpHoc37lA4+8z/g5Bs0UXxNx+nkbFTE7Ba2/G24caI9/cOXZPG3UViD +rtqXKL6h5/umqRG9Dt5liF2MVP9XFAesVC7B8+Ca+PbPlQoYzwIDAQABMA0GCSqG +SIb3DQEBBQUAA4GBAAS07u/Ke+EhEHidz6CG3Qcr+zg483JKRgZFyGz+YUKyyKKy +fmLs7JieGJxYQjOmIpj/6X9Gnb2HjIPDnI6A+MV1emXDTnnmsgf2/lZGcthhpZn2 +rMbj9bI0iH6HwOVGtp4ZJA5fB7nj3J+gWNTCQzDDOxwX36d2LL9ua+UMnk/g +-----END CERTIFICATE----- +-----BEGIN RSA PRIVATE KEY----- +MIICXQIBAAKBgQCdzVnic8K5W4SVbwVuqezcTjeqVLoQ91vVNZB0Jnsuz6q3DoK0 +3oAd1jTeVd0k+MQDbXpHoc37lA4+8z/g5Bs0UXxNx+nkbFTE7Ba2/G24caI9/cOX +ZPG3UViDrtqXKL6h5/umqRG9Dt5liF2MVP9XFAesVC7B8+Ca+PbPlQoYzwIDAQAB +AoGAeyzTwKPDl5QMRejHQL57GOwlH1vLcXrjv+VzwHZZKQ0IoKM++5fCQYf29KXp +XPahaluGW2u9sWa8R/7wGcd0Q4RtquGzsgT3+AQsIc5KfIamyOyDaRVM/ymX3fWg +gHIU7OOzB+ihOU8sHyRIwfbk01/kmrBXLRj8E31sy3i3PIECQQDQQYE+aN7Acrdt +yN5CaqvbkiCGjRvASlemiTzPosgOtndyp21w1gakJwKYhYDk1N6A6Qb8REMZqM/U +wFypldV/AkEAwfq6NFuhpGL6hDA7MvlyY1KiZ0cHetPUX+PgdNqy2DA+1Sv4i7gm +Wd/uA651K7aPXuUaf9dKtPCmZwI4M6SEsQJBALW89HTqP7niYoDEEnITdPaghxHk +gptERUln6lGo1L1CLus3gSI/JHyMLo+7scgAnEwTD62GRKhX0Ubwt+ymfTECQAY5 +fHYnppU20+EgBxZIqOIFCc8UmWnYmE0Ha/Fz/x8u1SVUBuK84wYpSGL32yyu7ATY +hzQo/W229zABAzqtAdECQQCUdB7IBFpPnsfv/EUBFX7X/7zAc9JpACmu9It5ju8C +KIsMuz/02D+TQoJNjdAngBM+4AJDIaGFgTMIfaDMh5L7 +-----END RSA PRIVATE KEY----- diff --git a/ext/openssl/tests/bug65729.phpt b/ext/openssl/tests/bug65729.phpt new file mode 100644 index 0000000000000..d4645d9f5b432 --- /dev/null +++ b/ext/openssl/tests/bug65729.phpt @@ -0,0 +1,42 @@ +--TEST-- +Bug #65729: CN_match gives false positive when wildcard is used +--SKIPIF-- + array( + 'verify_peer' => true, + 'allow_self_signed' => true, + 'CN_match' => 'foo.test.com.sg', + ) + ) + ); + var_dump(stream_socket_client("ssl://127.0.0.1:64321", $errno, $errstr, 1, + STREAM_CLIENT_CONNECT, $contextC)); +} else { + @pcntl_wait($status); + @stream_socket_accept($server, 1); +} +--EXPECTF-- +Warning: stream_socket_client(): Peer certificate CN=`*.test.com' did not match expected CN=`foo.test.com.sg' in %s on line %d + +Warning: stream_socket_client(): Failed to enable crypto in %s on line %d + +Warning: stream_socket_client(): unable to connect to ssl://127.0.0.1:64321 (Unknown error) in %s on line %d +bool(false) + From 8e847b5845b85c080295aea60c20869973c09a15 Mon Sep 17 00:00:00 2001 From: Tjerk Meesters Date: Sat, 21 Sep 2013 19:38:09 +0800 Subject: [PATCH 2/6] Fixed bug that would lead to out of bounds memory access --- ext/openssl/openssl.c | 46 ++++++++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/ext/openssl/openssl.c b/ext/openssl/openssl.c index 5460f3a6e1131..1c367df0815d0 100644 --- a/ext/openssl/openssl.c +++ b/ext/openssl/openssl.c @@ -4831,26 +4831,36 @@ static int verify_callback(int preverify_ok, X509_STORE_CTX *ctx) /* {{{ */ static int php_openssl_match_cn(const char *subjectname, const char *certname) { - int match = strcmp(subjectname, certname) == 0; - - if (!match) { - char *wildcard = strchr(certname, '*'); - int prefix_len = wildcard - certname; - - /* 1) prefix, if not empty, must match */ - if (wildcard && (prefix_len == 0 || strncmp(subjectname, certname, prefix_len) == 0)) { - const char *suffix = subjectname + strlen(subjectname) - strlen(wildcard + 1); - - /* - * 2) suffix must match - * 3) no period between prefix and suffix - **/ - match = strcmp(wildcard + 1, suffix) == 0 && - memchr(subjectname + prefix_len, '.', suffix - subjectname - prefix_len) == NULL; - } + char *wildcard; + int prefix_len, suffix_len, subject_len; + + if (strcmp(subjectname, certname) == 0) { + return 1; } - return match; + if (!(wildcard = strchr(certname, '*'))) { + return 0; + } + + // 1) prefix, if not empty, must match subject + prefix_len = wildcard - certname; + if (prefix_len && strncmp(subjectname, certname, prefix_len) != 0) { + return 0; + } + + suffix_len = strlen(wildcard + 1); + subject_len = strlen(subjectname); + if (suffix_len <= subject_len) { + const char *suffix = subjectname + subject_len - suffix_len; + + /* 2) suffix must match + * 3) no . between prefix and suffix + **/ + return strcmp(wildcard + 1, suffix) == 0 && + memchr(subjectname + prefix_len, '.', suffix - subjectname - prefix_len) == NULL; + } + + return 0; } int php_openssl_apply_verification_policy(SSL *ssl, X509 *peer, php_stream *stream TSRMLS_DC) /* {{{ */ From a820c3d6baac945ead4a5fe6e54d0a04b02620de Mon Sep 17 00:00:00 2001 From: Tjerk Meesters Date: Sat, 21 Sep 2013 20:42:52 +0800 Subject: [PATCH 3/6] yay, reduced one variable --- ext/openssl/openssl.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/ext/openssl/openssl.c b/ext/openssl/openssl.c index 1c367df0815d0..2aa850ad1c0db 100644 --- a/ext/openssl/openssl.c +++ b/ext/openssl/openssl.c @@ -4851,13 +4851,11 @@ static int php_openssl_match_cn(const char *subjectname, const char *certname) suffix_len = strlen(wildcard + 1); subject_len = strlen(subjectname); if (suffix_len <= subject_len) { - const char *suffix = subjectname + subject_len - suffix_len; - /* 2) suffix must match * 3) no . between prefix and suffix **/ - return strcmp(wildcard + 1, suffix) == 0 && - memchr(subjectname + prefix_len, '.', suffix - subjectname - prefix_len) == NULL; + return strcmp(wildcard + 1, subjectname + subject_len - suffix_len) == 0 && + memchr(subjectname + prefix_len, '.', subject_len - suffix_len - prefix_len) == NULL; } return 0; From 674dd73f8c34b9faf1e777a301e5302348b48b9d Mon Sep 17 00:00:00 2001 From: Tjerk Meesters Date: Mon, 7 Oct 2013 22:10:05 +0800 Subject: [PATCH 4/6] Added two more test cases for CN matching. --- ext/openssl/tests/bug65729.phpt | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/ext/openssl/tests/bug65729.phpt b/ext/openssl/tests/bug65729.phpt index d4645d9f5b432..7008f3c7b56c4 100644 --- a/ext/openssl/tests/bug65729.phpt +++ b/ext/openssl/tests/bug65729.phpt @@ -13,24 +13,28 @@ stream_context_set_option($context, 'ssl', 'allow_self_signed', true); $server = stream_socket_server('ssl://127.0.0.1:64321', $errno, $errstr, STREAM_SERVER_BIND|STREAM_SERVER_LISTEN, $context); +$expected_names = array('foo.test.com.sg', 'foo.test.com', 'foo.bar.test.com'); + $pid = pcntl_fork(); if ($pid == -1) { die('could not fork'); } else if ($pid) { - $contextC = stream_context_create( - array( + foreach ($expected_names as $expected_name) { + $contextC = stream_context_create(array( 'ssl' => array( 'verify_peer' => true, 'allow_self_signed' => true, - 'CN_match' => 'foo.test.com.sg', + 'CN_match' => $expected_name, ) - ) - ); - var_dump(stream_socket_client("ssl://127.0.0.1:64321", $errno, $errstr, 1, + )); + var_dump(stream_socket_client("ssl://127.0.0.1:64321", $errno, $errstr, 1, STREAM_CLIENT_CONNECT, $contextC)); + } } else { @pcntl_wait($status); - @stream_socket_accept($server, 1); + foreach ($expected_names as $name) { + @stream_socket_accept($server, 1); + } } --EXPECTF-- Warning: stream_socket_client(): Peer certificate CN=`*.test.com' did not match expected CN=`foo.test.com.sg' in %s on line %d @@ -39,4 +43,11 @@ Warning: stream_socket_client(): Failed to enable crypto in %s on line %d Warning: stream_socket_client(): unable to connect to ssl://127.0.0.1:64321 (Unknown error) in %s on line %d bool(false) +resource(%d) of type (stream) + +Warning: stream_socket_client(): Peer certificate CN=`*.test.com' did not match expected CN=`foo.bar.test.com' in %s on line %d + +Warning: stream_socket_client(): Failed to enable crypto in %s on line %d +Warning: stream_socket_client(): unable to connect to ssl://127.0.0.1:64321 (Unknown error) in %s on line %d +bool(false) From 39c0daeb71f76ce22dc604bda8a063319fd55e59 Mon Sep 17 00:00:00 2001 From: Tjerk Meesters Date: Mon, 7 Oct 2013 23:04:24 +0800 Subject: [PATCH 5/6] Use zend_bool as return value for _match() --- ext/openssl/openssl.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/ext/openssl/openssl.c b/ext/openssl/openssl.c index 2aa850ad1c0db..2b345700ff853 100644 --- a/ext/openssl/openssl.c +++ b/ext/openssl/openssl.c @@ -4829,7 +4829,7 @@ static int verify_callback(int preverify_ok, X509_STORE_CTX *ctx) /* {{{ */ } /* }}} */ -static int php_openssl_match_cn(const char *subjectname, const char *certname) +static zend_bool php_openssl_match_cn(const char *subjectname, const char *certname) { char *wildcard; int prefix_len, suffix_len, subject_len; @@ -4902,7 +4902,6 @@ int php_openssl_apply_verification_policy(SSL *ssl, X509 *peer, php_stream *stre /* Does the common name match ? (used primarily for https://) */ GET_VER_OPT_STRING("CN_match", cnmatch); if (cnmatch) { - int match = 0; int name_len = X509_NAME_get_text_by_NID(name, NID_commonName, buf, sizeof(buf)); if (name_len == -1) { @@ -4913,9 +4912,7 @@ int php_openssl_apply_verification_policy(SSL *ssl, X509 *peer, php_stream *stre return FAILURE; } - match = php_openssl_match_cn(cnmatch, buf); - - if (!match) { + if (!php_openssl_match_cn(cnmatch, buf)) { /* didn't match */ php_error_docref(NULL TSRMLS_CC, E_WARNING, "Peer certificate CN=`%.*s' did not match expected CN=`%s'", name_len, buf, cnmatch); return FAILURE; From 6106896440572dd8093acdd11ea691a07d9b169c Mon Sep 17 00:00:00 2001 From: datibbaw Date: Tue, 8 Oct 2013 10:07:54 +0800 Subject: [PATCH 6/6] DNS name comparison is now case insensitive. --- ext/openssl/openssl.c | 6 +++--- ext/openssl/tests/bug65729.phpt | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/ext/openssl/openssl.c b/ext/openssl/openssl.c index 2b345700ff853..15cf798ddba33 100644 --- a/ext/openssl/openssl.c +++ b/ext/openssl/openssl.c @@ -4834,7 +4834,7 @@ static zend_bool php_openssl_match_cn(const char *subjectname, const char *certn char *wildcard; int prefix_len, suffix_len, subject_len; - if (strcmp(subjectname, certname) == 0) { + if (strcasecmp(subjectname, certname) == 0) { return 1; } @@ -4844,7 +4844,7 @@ static zend_bool php_openssl_match_cn(const char *subjectname, const char *certn // 1) prefix, if not empty, must match subject prefix_len = wildcard - certname; - if (prefix_len && strncmp(subjectname, certname, prefix_len) != 0) { + if (prefix_len && strncasecmp(subjectname, certname, prefix_len) != 0) { return 0; } @@ -4854,7 +4854,7 @@ static zend_bool php_openssl_match_cn(const char *subjectname, const char *certn /* 2) suffix must match * 3) no . between prefix and suffix **/ - return strcmp(wildcard + 1, subjectname + subject_len - suffix_len) == 0 && + return strcasecmp(wildcard + 1, subjectname + subject_len - suffix_len) == 0 && memchr(subjectname + prefix_len, '.', subject_len - suffix_len - prefix_len) == NULL; } diff --git a/ext/openssl/tests/bug65729.phpt b/ext/openssl/tests/bug65729.phpt index 7008f3c7b56c4..c0ee4443ebee5 100644 --- a/ext/openssl/tests/bug65729.phpt +++ b/ext/openssl/tests/bug65729.phpt @@ -13,7 +13,7 @@ stream_context_set_option($context, 'ssl', 'allow_self_signed', true); $server = stream_socket_server('ssl://127.0.0.1:64321', $errno, $errstr, STREAM_SERVER_BIND|STREAM_SERVER_LISTEN, $context); -$expected_names = array('foo.test.com.sg', 'foo.test.com', 'foo.bar.test.com'); +$expected_names = array('foo.test.com.sg', 'foo.test.com', 'FOO.TEST.COM', 'foo.bar.test.com'); $pid = pcntl_fork(); if ($pid == -1) { @@ -44,6 +44,7 @@ Warning: stream_socket_client(): Failed to enable crypto in %s on line %d Warning: stream_socket_client(): unable to connect to ssl://127.0.0.1:64321 (Unknown error) in %s on line %d bool(false) resource(%d) of type (stream) +resource(%d) of type (stream) Warning: stream_socket_client(): Peer certificate CN=`*.test.com' did not match expected CN=`foo.bar.test.com' in %s on line %d