From c0e8bed5bf7a7e56897e492a4dcc399621939995 Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Sat, 1 Aug 2009 21:56:59 +0000 Subject: [PATCH] - Scott Cantor posted the bug report #2829955 (http://curl.haxx.se/bug/view.cgi?id=2829955) mentioning the recent SSL cert verification flaw found and exploited by Moxie Marlinspike. The presentation he did at Black Hat is available here: https://www.blackhat.com/html/bh-usa-09/bh-usa-09-archives.html#Marlinspike Apparently at least one CA allowed a subjectAltName or CN that contain a zero byte, and thus clients that assumed they would never have zero bytes were exploited to OK a certificate that didn't actually match the site. Like if the name in the cert was "example.com\0theatualsite.com", libcurl would happily verify that cert for example.com. libcurl now better use the length of the extracted name, not assuming it is zero terminated. --- CHANGES | 15 +++++++++++++++ RELEASE-NOTES | 3 ++- lib/ssluse.c | 33 ++++++++++++++++++++++----------- 3 files changed, 39 insertions(+), 12 deletions(-) diff --git a/CHANGES b/CHANGES index d2b194ffb..e03f92c88 100644 --- a/CHANGES +++ b/CHANGES @@ -7,6 +7,21 @@ Changelog Daniel Stenberg (1 Aug 2009) +- Scott Cantor posted the bug report #2829955 + (http://curl.haxx.se/bug/view.cgi?id=2829955) mentioning the recent SSL cert + verification flaw found and exploited by Moxie Marlinspike. The presentation + he did at Black Hat is available here: + https://www.blackhat.com/html/bh-usa-09/bh-usa-09-archives.html#Marlinspike + + Apparently at least one CA allowed a subjectAltName or CN that contain a + zero byte, and thus clients that assumed they would never have zero bytes + were exploited to OK a certificate that didn't actually match the site. Like + if the name in the cert was "example.com\0theatualsite.com", libcurl would + happily verify that cert for example.com. + + libcurl now better use the length of the extracted name, not assuming it is + zero terminated. + - Tanguy Fautre pointed out that OpenSSL's function RAND_screen() (present only in some OpenSSL installs - like on Windows) isn't thread-safe and we agreed that moving it to the global_init() function is a decent way to deal diff --git a/RELEASE-NOTES b/RELEASE-NOTES index bd5700f77..b019bbc74 100644 --- a/RELEASE-NOTES +++ b/RELEASE-NOTES @@ -40,6 +40,7 @@ This release includes the following bugfixes: o missing algorithms in libcurl+OpenSSL o with noproxy set you could still get a proxy if a proxy env was set o rand seeding on libcurl on windows built with OpenSSL was not thread-safe + o fixed the zero byte inserted in cert name flaw in libcurl+OpenSSL This release includes the following known bugs: @@ -53,6 +54,6 @@ advice from friends like these: Aaron Oneal, Igor Novoseltsev, Eric Wong, Bill Hoffman, Daniel Steinberg, Fabian Keil, Michal Marek, Reuven Wachtfogel, Markus Koetter, Constantine Sapuntzakis, David Binderman, Johan van Selst, Alexander Beedie, - Tanguy Fautre + Tanguy Fautre, Scott Cantor Thanks! (and sorry if I forgot to mention someone) diff --git a/lib/ssluse.c b/lib/ssluse.c index ffc1fbd96..324b05d47 100644 --- a/lib/ssluse.c +++ b/lib/ssluse.c @@ -990,14 +990,19 @@ static int asn1_output(const ASN1_UTCTIME *tm, #define HOST_NOMATCH 0 #define HOST_MATCH 1 -static int hostmatch(const char *hostname, const char *pattern) +static int hostmatch(const char *hostname, const char *pattern, size_t plen) { while(1) { char c = *pattern++; + plen--; - if(c == '\0') + if(!plen) return (*hostname ? HOST_NOMATCH : HOST_MATCH); + if(!c) + /* an embedded zero in the pattern can't match a host name */ + return HOST_NOMATCH; + if(c == '*') { c = *pattern; if(c == '\0') /* "*\0" matches anything remaining */ @@ -1005,7 +1010,7 @@ static int hostmatch(const char *hostname, const char *pattern) while(*hostname) { /* The only recursive function in libcurl! */ - if(hostmatch(hostname++,pattern) == HOST_MATCH) + if(hostmatch(hostname++, pattern, plen) == HOST_MATCH) return HOST_MATCH; } break; @@ -1018,17 +1023,20 @@ static int hostmatch(const char *hostname, const char *pattern) } static int -cert_hostcheck(const char *match_pattern, const char *hostname) +cert_hostcheck(const char *match_pattern, size_t mlen, const char *hostname) { + size_t hlen = strlen(hostname); if(!match_pattern || !*match_pattern || - !hostname || !*hostname) /* sanity check */ + !hostname || !*hostname) /* sanity check */ return 0; - if(Curl_raw_equal(hostname, match_pattern)) /* trivial case */ + if((hlen == mlen) && !memcmp(hostname, match_pattern, hlen)) + /* trivial case */ return 1; - if(hostmatch(hostname,match_pattern) == HOST_MATCH) + if(hostmatch(hostname, match_pattern, mlen) == HOST_MATCH) return 1; + return 0; } @@ -1101,7 +1109,7 @@ static CURLcode verifyhost(struct connectdata *conn, if(check->type == target) { /* get data and length */ const char *altptr = (char *)ASN1_STRING_data(check->d.ia5); - size_t altlen; + size_t altlen = (size_t) ASN1_STRING_length(check->d.ia5); switch(target) { case GEN_DNS: /* name/pattern comparison */ @@ -1114,15 +1122,17 @@ static CURLcode verifyhost(struct connectdata *conn, Gisle researched the OpenSSL sources: "I checked the 0.9.6 and 0.9.8 sources before my patch and it always 0-terminates an IA5String." + + To reduce the risk of an embedded zero before the final zero + causing us trouble, we use the length OpenSSL reports! */ - if(cert_hostcheck(altptr, conn->host.name)) + if(cert_hostcheck(altptr, altlen, conn->host.name)) matched = TRUE; break; case GEN_IPADD: /* IP address comparison */ /* compare alternative IP address if the data chunk is the same size our server IP address is */ - altlen = (size_t) ASN1_STRING_length(check->d.ia5); if((altlen == addrlen) && !memcmp(altptr, &addr, altlen)) matched = TRUE; break; @@ -1196,7 +1206,8 @@ static CURLcode verifyhost(struct connectdata *conn, "SSL: unable to obtain common name from peer certificate"); return CURLE_PEER_FAILED_VERIFICATION; } - else if(!cert_hostcheck((const char *)peer_CN, conn->host.name)) { + else if(!cert_hostcheck((const char *)peer_CN, strlen((char *)peer_CN), + conn->host.name)) { if(data->set.ssl.verifyhost > 1) { failf(data, "SSL: certificate subject name '%s' does not match " "target host name '%s'", peer_CN, conn->host.dispname);