From 792ef23e6e1c05780fe17f733859eef6eb8c8be3 Mon Sep 17 00:00:00 2001
From: Andreas Weigel <andreas.weigel@securepoint.de>
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<const char *>(a);
const char *d = static_cast<const char *>(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<char*>(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