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