From 792ef23e6e1c05780fe17f733859eef6eb8c8be3 Mon Sep 17 00:00:00 2001 From: Andreas Weigel Date: Wed, 18 Oct 2023 04:14:31 +0000 Subject: [PATCH] Fix validation of certificates with CN=* (#1523) The bug was discovered and detailed by Joshua Rogers at https://megamansec.github.io/Squid-Security-Audit/ where it was filed as "Buffer UnderRead in SSL CN Parsing". Additionally includes upstream commit abbd782505c760279a8bd86a45c6095428f10752. --- src/URL.h | 39 ++++++++++++++++++++++++--------------- src/acl/ServerName.cc | 2 +- src/ssl/support.cc | 9 ++++++--- src/url.cc | 37 ++++++++++++++++++++++++++++++++----- 4 files changed, 63 insertions(+), 24 deletions(-) diff --git a/src/URL.h b/src/URL.h index 7076958..9a6c091 100644 --- a/src/URL.h +++ b/src/URL.h @@ -73,23 +73,29 @@ char *urlMakeAbsolute(const HttpRequest *, const char *); char *urlRInternal(const char *host, unsigned short port, const char *dir, const char *name); char *urlInternal(const char *dir, const char *name); +enum MatchDomainNameFlags { + mdnNone = 0, + mdnHonorWildcards = 1 << 0, + mdnRejectSubsubDomains = 1 << 1 +}; + /** - * matchDomainName() compares a hostname (usually extracted from traffic) - * with a domainname (usually from an ACL) according to the following rules: + * matchDomainName() matches a hostname (usually extracted from traffic) + * with a domainname when mdnNone or mdnRejectSubsubDomains flags are used + * according to the following rules: * - * HOST | DOMAIN | MATCH? - * -------------|-------------|------ - * foo.com | foo.com | YES - * .foo.com | foo.com | YES - * x.foo.com | foo.com | NO - * foo.com | .foo.com | YES - * .foo.com | .foo.com | YES - * x.foo.com | .foo.com | YES + * HOST | DOMAIN | mdnNone | mdnRejectSubsubDomains + * -------------|-------------|-----------|----------------------- + * foo.com | foo.com | YES | YES + * .foo.com | foo.com | YES | YES + * x.foo.com | foo.com | NO | NO + * foo.com | .foo.com | YES | YES + * .foo.com | .foo.com | YES | YES + * x.foo.com | .foo.com | YES | YES + * .x.foo.com | .foo.com | YES | NO + * y.x.foo.com | .foo.com | YES | NO * - * We strip leading dots on hosts (but not domains!) so that - * ".foo.com" is always the same as "foo.com". - * - * if honorWildcards is true then the matchDomainName() also accepts + * if mdnHonorWildcards flag is set then the matchDomainName() also accepts * optional wildcards on hostname: * * HOST | DOMAIN | MATCH? @@ -99,11 +105,14 @@ char *urlInternal(const char *dir, const char *name); * *.foo.com | .foo.com | YES * *.foo.com | foo.com | NO * + * The combination of mdnHonorWildcards and mdnRejectSubsubDomains flags is + * supported. + * * \retval 0 means the host matches the domain * \retval 1 means the host is greater than the domain * \retval -1 means the host is less than the domain */ -int matchDomainName(const char *host, const char *domain, bool honorWildcards = false); +int matchDomainName(const char *host, const char *domain, uint flags = mdnNone); int urlCheckRequest(const HttpRequest *); int urlDefaultPort(AnyP::ProtocolType p); char *urlHostname(const char *url); diff --git a/src/acl/ServerName.cc b/src/acl/ServerName.cc index 9cc08b2..f49209b 100644 --- a/src/acl/ServerName.cc +++ b/src/acl/ServerName.cc @@ -30,7 +30,7 @@ aclHostDomainCompare( char *const &a, char * const &b) const char *h = static_cast(a); const char *d = static_cast(b); debugs(28, 7, "Match:" << h << " <> " << d); - return matchDomainName(h, d, true); + return matchDomainName(h, d, mdnHonorWildcards); } bool diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 6f01a94..5e33447 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -199,9 +199,12 @@ static int check_domain( void *check_data, ASN1_STRING *cn_data) char cn[1024]; const char *server = (const char *)check_data; - if (cn_data->length > (int)sizeof(cn) - 1) { + if (cn_data->length == 0) + return 1; // zero length cn, ignore + + if (cn_data->length > (int)sizeof(cn) - 1) return 1; //if does not fit our buffer just ignore - } + char *s = reinterpret_cast(cn_data->data); char *d = cn; for (int i = 0; i < cn_data->length; ++i, ++d, ++s) { @@ -211,7 +214,7 @@ static int check_domain( void *check_data, ASN1_STRING *cn_data) } cn[cn_data->length] = '\0'; debugs(83, 4, "Verifying server domain " << server << " to certificate name/subjectAltName " << cn); - return matchDomainName(server, cn[0] == '*' ? cn + 1 : cn); + return matchDomainName(server, (cn[0] == '*' ? cn + 1 : cn), mdnRejectSubsubDomains); } bool Ssl::checkX509ServerValidity(X509 *cert, const char *server) diff --git a/src/url.cc b/src/url.cc index 1e14d05..0bddd0e 100644 --- a/src/url.cc +++ b/src/url.cc @@ -54,6 +54,7 @@ urlInitialize(void) assert(0 == matchDomainName("foo.com", ".foo.com")); assert(0 == matchDomainName(".foo.com", ".foo.com")); assert(0 == matchDomainName("x.foo.com", ".foo.com")); + assert(0 == matchDomainName("y.x.foo.com", ".foo.com")); assert(0 != matchDomainName("x.foo.com", "foo.com")); assert(0 != matchDomainName("foo.com", "x.foo.com")); assert(0 != matchDomainName("bar.com", "foo.com")); @@ -66,6 +67,17 @@ urlInitialize(void) assert(0 < matchDomainName("bfoo.com", "afoo.com")); assert(0 > matchDomainName("afoo.com", "bfoo.com")); assert(0 < matchDomainName("x-foo.com", ".foo.com")); + + assert(0 == matchDomainName(".foo.com", ".foo.com", mdnRejectSubsubDomains)); + assert(0 == matchDomainName("x.foo.com", ".foo.com", mdnRejectSubsubDomains)); + assert(0 != matchDomainName("y.x.foo.com", ".foo.com", mdnRejectSubsubDomains)); + assert(0 != matchDomainName(".x.foo.com", ".foo.com", mdnRejectSubsubDomains)); + + assert(0 == matchDomainName("*.foo.com", "x.foo.com", mdnHonorWildcards)); + assert(0 == matchDomainName("*.foo.com", ".x.foo.com", mdnHonorWildcards)); + assert(0 == matchDomainName("*.foo.com", ".foo.com", mdnHonorWildcards)); + assert(0 != matchDomainName("*.foo.com", "foo.com", mdnHonorWildcards)); + /* more cases? */ } @@ -680,16 +692,20 @@ urlMakeAbsolute(const HttpRequest * req, const char *relUrl) } int -matchDomainName(const char *h, const char *d, bool honorWildcards) +matchDomainName(const char *h, const char *d, uint flags) { int dl; int hl; + const bool hostIncludesSubdomains = (*h == '.'); while ('.' == *h) ++h; hl = strlen(h); + if (hl == 0) + return -1; + dl = strlen(d); /* @@ -727,9 +743,20 @@ matchDomainName(const char *h, const char *d, bool honorWildcards) * is a leading '.'. */ - if ('.' == d[0]) - return 0; - else + if ('.' == d[0]) { + if (flags & mdnRejectSubsubDomains) { + // Check for sub-sub domain and reject + while(--hl >= 0 && h[hl] != '.'); + if (hl < 0) { + // No sub-sub domain found, but reject if there is a + // leading dot in given host string (which is removed + // before the check is started). + return hostIncludesSubdomains ? 1 : 0; + } else + return 1; // sub-sub domain, reject + } else + return 0; + } else return 1; } } @@ -741,7 +768,7 @@ matchDomainName(const char *h, const char *d, bool honorWildcards) // If the h has a form of "*.foo.com" and d has a form of "x.foo.com" // then the h[hl] points to '*', h[hl+1] to '.' and d[dl] to 'x' // The following checks are safe, the "h[hl + 1]" in the worst case is '\0'. - if (honorWildcards && h[hl] == '*' && h[hl + 1] == '.') + if ((flags & mdnHonorWildcards) && h[hl] == '*' && h[hl + 1] == '.') return 0; /* -- 2.43.0