Bug 5363: Handle IP-based X.509 SANs better

CONTRIBUTORS

@@ -521,6 +521,7 @@ Thank you!
     Tomas Hozza <thozza@redhat.com>
     tomofumi-yoshida <51390036+tomofumi-yoshida@users.noreply.github.com>
     Tony Lorimer <tlorimer@au.mdis.com>
+    Tony Walker <tony.walker@twosigma.com>
     trapexit <trapexit@spawn.link>
     Trever Adams <trever@middleearth.sapphiresunday.org>
     Tsantilas Christos <chtsanti@users.sourceforge.net>

src/acl/ServerName.cc

@@ -11,9 +11,11 @@
 #include "squid.h"
 #include "acl/FilledChecklist.h"
 #include "acl/ServerName.h"
+#include "anyp/Host.h"
 #include "client_side.h"
 #include "http/Stream.h"
 #include "HttpRequest.h"
+#include "sbuf/Stream.h"
 #include "ssl/bio.h"
 #include "ssl/ServerBump.h"
 #include "ssl/support.h"
@@ -45,31 +47,50 @@ ACLServerNameData::match(const char *host)
 
 }
 
-/// A helper function to be used with Ssl::matchX509CommonNames().
-/// \retval 0 when the name (cn or an alternate name) matches acl data
-/// \retval 1 when the name does not match
-template<class MatchType>
-int
-check_cert_domain( void *check_data, ASN1_STRING *cn_data)
+namespace Acl {
+
+/// GeneralNameMatcher for matching configured ACL parameters
+class ServerNameMatcher: public Ssl::GeneralNameMatcher
 {
-    char cn[1024];
-    ACLData<MatchType> * data = (ACLData<MatchType> *)check_data;
-
-    if (cn_data->length > (int)sizeof(cn) - 1)
-        return 1; // ignore data that does not fit our buffer
-
-    char *s = reinterpret_cast<char *>(cn_data->data);
-    char *d = cn;
-    for (int i = 0; i < cn_data->length; ++i, ++d, ++s) {
-        if (*s == '\0')
-            return 1; // always a domain mismatch. contains 0x00
-        *d = *s;
-    }
-    cn[cn_data->length] = '\0';
-    debugs(28, 4, "Verifying certificate name/subjectAltName " << cn);
-    if (data->match(cn))
-        return 0;
-    return 1;
+public:
+    explicit ServerNameMatcher(ServerNameCheck::Parameters &p): parameters(p) {}
+
+protected:
+    /* GeneralNameMatcher API */
+    bool matchDomainName(const Dns::DomainName &) const override;
+    bool matchIp(const Ip::Address &) const override;
+
+private:
+    // TODO: Make ServerNameCheck::Parameters::match() and this reference constant.
+    ServerNameCheck::Parameters &parameters; ///< configured ACL parameters
+};
+
+} // namespace Acl
+
+bool
+Acl::ServerNameMatcher::matchDomainName(const Dns::DomainName &domain) const
+{
+    return parameters.match(SBuf(domain).c_str()); // TODO: Upgrade string-matching ACLs to SBuf
+}
+
+bool
+Acl::ServerNameMatcher::matchIp(const Ip::Address &ip) const
+{
+    // We are given an Ip::Address, but our ACL parameters use case-insensitive
+    // string equality (::matchDomainName) or regex string matches. There are
+    // many ways to convert an IPv6 address to a string, but only one format can
+    // correctly match certain configured parameters. Our ssl::server_name docs
+    // request the following ACL parameter formatting (that this to-string
+    // conversion code produces): IPv6 addresses use "::" notation (where
+    // applicable) and are not bracketed.
+    //
+    // Similar problems affect dstdomain ACLs. TODO: Instead of relying on users
+    // reading docs and following their inet_ntop(3) implementation to match
+    // IPv6 addresses handled by matchDomainName(), enhance matchDomainName()
+    // code and ACL parameter storage to support Ip::Address objects.
+    char hostStr[MAX_IPSTRLEN];
+    (void)ip.toStr(hostStr, sizeof(hostStr)); // no brackets
+    return parameters.match(hostStr);
 }
 
 int
@@ -79,37 +100,41 @@ Acl::ServerNameCheck::match(ACLChecklist * const ch)
 
     assert(checklist != nullptr && checklist->request != nullptr);
 
-    const char *serverName = nullptr;
-    SBuf clientSniKeeper; // because c_str() is not constant
+    std::optional<AnyP::Host> serverNameFromConn;
     if (ConnStateData *conn = checklist->conn()) {
-        const char *clientRequestedServerName = nullptr;
-        clientSniKeeper = conn->tlsClientSni();
-        if (clientSniKeeper.isEmpty()) {
-            const char *host = checklist->request->url.host();
-            if (host && *host) // paranoid first condition: host() is never nil
-                clientRequestedServerName = host;
-        } else
-            clientRequestedServerName = clientSniKeeper.c_str();
+        std::optional<AnyP::Host> clientRequestedServerName;
+        const auto &clientSni = conn->tlsClientSni();
+        if (clientSni.isEmpty()) {
+            clientRequestedServerName = checklist->request->url.parsedHost();
+        } else {
+            // RFC 6066: "The hostname is represented as a byte string using
+            // ASCII encoding"; "Literal IPv4 and IPv6 addresses are not
+            // permitted". TODO: Store TlsDetails::serverName and similar
+            // domains using a new domain-only type instead of SBuf.
+            clientRequestedServerName = AnyP::Host::ParseSimpleDomainName(clientSni);
+        }
 
         if (useConsensus) {
             X509 *peer_cert = conn->serverBump() ? conn->serverBump()->serverCert.get() : nullptr;
             // use the client requested name if it matches the server
             // certificate or if the certificate is not available
-            if (!peer_cert || Ssl::checkX509ServerValidity(peer_cert, clientRequestedServerName))
-                serverName = clientRequestedServerName;
+            if (!peer_cert || !clientRequestedServerName ||
+                    Ssl::HasSubjectName(*peer_cert, *clientRequestedServerName))
+                serverNameFromConn = clientRequestedServerName;
         } else if (useClientRequested)
-            serverName = clientRequestedServerName;
+            serverNameFromConn = clientRequestedServerName;
         else { // either no options or useServerProvided
             if (X509 *peer_cert = (conn->serverBump() ? conn->serverBump()->serverCert.get() : nullptr))
-                return Ssl::matchX509CommonNames(peer_cert, data.get(), check_cert_domain<const char*>);
+                return Ssl::HasMatchingSubjectName(*peer_cert, ServerNameMatcher(*data));
             if (!useServerProvided)
-                serverName = clientRequestedServerName;
+                serverNameFromConn = clientRequestedServerName;
         }
     }
 
-    if (!serverName)
-        serverName = "none";
-
+    std::optional<SBuf> printedServerName;
+    if (serverNameFromConn)
+        printedServerName = ToSBuf(*serverNameFromConn); // no brackets
+    const auto serverName = printedServerName ? printedServerName->c_str() : "none";
     return data->match(serverName);
 }
 

src/anyp/Host.cc

@@ -0,0 +1,113 @@
+/*
+ * Copyright (C) 1996-2023 The Squid Software Foundation and contributors
+ *
+ * Squid software is distributed under GPLv2+ license and includes
+ * contributions from numerous individuals and organizations.
+ * Please see the COPYING and CONTRIBUTORS files for details.
+ */
+
+#include "squid.h"
+#include "anyp/Host.h"
+
+#include <iostream>
+
+std::optional<AnyP::Host>
+AnyP::Host::ParseIp(const Ip::Address &ip)
+{
+    // any preparsed IP value is acceptable
+    debugs(23, 7, ip);
+    return Host(ip);
+}
+
+/// common parts of FromSimpleDomain() and FromWildDomain()
+std::optional<AnyP::Host>
+AnyP::Host::ParseDomainName(const SBuf &rawName)
+{
+    if (rawName.isEmpty()) {
+        debugs(23, 3, "rejecting empty name");
+        return std::nullopt;
+    }
+
+    // Reject bytes incompatible with rfc1035NamePack() and ::matchDomainName()
+    // implementations (at least). Such bytes can come from percent-encoded HTTP
+    // URIs or length-based X.509 fields, for example. Higher-level parsers must
+    // reject or convert domain name encodings like UTF-16, but this low-level
+    // check works as an additional (albeit unreliable) layer of defense against
+    // those (unsupported by Squid DNS code) encodings.
+    if (rawName.find('\0') != SBuf::npos) {
+        debugs(83, 3, "rejecting ASCII NUL character in " << rawName);
+        return std::nullopt;
+    }
+
+    // TODO: Consider rejecting names with isspace(3) bytes.
+
+    debugs(23, 7, rawName);
+    return Host(rawName);
+}
+
+std::optional<AnyP::Host>
+AnyP::Host::ParseSimpleDomainName(const SBuf &rawName)
+{
+    if (rawName.find('*') != SBuf::npos) {
+        debugs(23, 3, "rejecting wildcard in " << rawName);
+        return std::nullopt;
+    }
+    return ParseDomainName(rawName);
+}
+
+std::optional<AnyP::Host>
+AnyP::Host::ParseWildDomainName(const SBuf &rawName)
+{
+    const static SBuf wildcardLabel("*.");
+    if (rawName.startsWith(wildcardLabel)) {
+        if (rawName.find('*', 2) != SBuf::npos) {
+            debugs(23, 3, "rejecting excessive wildcards in " << rawName);
+            return std::nullopt;
+        }
+        // else: fall through to final checks
+    } else {
+        if (rawName.find('*', 0) != SBuf::npos) {
+            // this case includes "*" and "example.*" input
+            debugs(23, 3, "rejecting unsupported wildcard in " << rawName);
+            return std::nullopt;
+        }
+        // else: fall through to final checks
+    }
+    return ParseDomainName(rawName);
+}
+
+std::ostream &
+AnyP::operator <<(std::ostream &os, const Host &host)
+{
+    if (const auto ip = host.ip()) {
+        char buf[MAX_IPSTRLEN];
+        (void)ip->toStr(buf, sizeof(buf)); // no brackets
+        os << buf;
+    } else {
+        // If Host object creators start applying Uri::Decode() to reg-names,
+        // then we must start applying Uri::Encode() here, but only to names
+        // that require it. See "The reg-name syntax allows percent-encoded
+        // octets" paragraph in RFC 3986.
+        const auto domainName = host.domainName();
+        Assure(domainName);
+        os << *domainName;
+    }
+    return os;
+}
+
+std::ostream &
+AnyP::operator <<(std::ostream &os, const Bracketed &hostWrapper)
+{
+    bool addBrackets = false;
+    if (const auto ip = hostWrapper.host.ip())
+        addBrackets = ip->isIPv6();
+
+    if (addBrackets)
+        os << '[';
+    os << hostWrapper.host;
+    if (addBrackets)
+        os << ']';
+
+    return os;
+}
+

src/anyp/Host.h

@@ -0,0 +1,81 @@
+/*
+ * Copyright (C) 1996-2023 The Squid Software Foundation and contributors
+ *
+ * Squid software is distributed under GPLv2+ license and includes
+ * contributions from numerous individuals and organizations.
+ * Please see the COPYING and CONTRIBUTORS files for details.
+ */
+
+#ifndef SQUID_SRC_ANYP_HOST_H
+#define SQUID_SRC_ANYP_HOST_H
+
+#include "dns/forward.h"
+#include "ip/Address.h"
+#include "sbuf/SBuf.h"
+
+#include <iosfwd>
+#include <optional>
+#include <variant>
+
+namespace AnyP
+{
+
+/// either a domain name (as defined in DNS RFC 1034) or an IP address
+class Host
+{
+public:
+    /// converts an already parsed IP address to a Host object
+    static std::optional<Host> ParseIp(const Ip::Address &);
+
+    /// Parses input as a literal ASCII domain name (A-labels OK; see RFC 5890).
+    /// Does not allow wildcards; \sa ParseWildDomainName().
+    static std::optional<Host> ParseSimpleDomainName(const SBuf &);
+
+    /// Same as ParseSimpleDomainName() but allows the first label to be a
+    /// wildcard (RFC 9525 Section 6.3).
+    static std::optional<Host> ParseWildDomainName(const SBuf &);
+
+    // Accessor methods below are mutually exclusive: Exactly one method is
+    // guaranteed to return a result other than std::nullopt.
+
+    /// stored IPv or IPv6 address (if any)
+    ///
+    /// Ip::Address::isNoAddr() may be true for the returned address.
+    /// Ip::Address::isAnyAddr() may be true for the returned address.
+    auto ip() const { return std::get_if<Ip::Address>(&raw_); }
+
+    /// stored domain name (if any)
+    auto domainName() const { return std::get_if<SBuf>(&raw_); }
+
+private:
+    using Storage = std::variant<Ip::Address, Dns::DomainName>;
+
+    static std::optional<Host> ParseDomainName(const SBuf &);
+
+    // use a Parse*() function to create Host objects
+    Host(const Storage &raw): raw_(raw) {}
+
+    Storage raw_; ///< the host we are providing access to
+};
+
+/// helps print Host value in RFC 3986 Section 3.2.2 format, with square
+/// brackets around an IPv6 address (if the Host value is an IPv6 address)
+class Bracketed
+{
+public:
+    explicit Bracketed(const Host &aHost): host(aHost) {}
+    const Host &host;
+};
+
+/// prints Host value _without_ square brackets around an IPv6 address (even
+/// when the Host value is an IPv6 address); \sa Bracketed
+std::ostream &operator <<(std::ostream &, const Host &);
+
+/// prints Host value _without_ square brackets around an IPv6 address (even
+/// when the Host value is an IPv6 address); \sa Bracketed
+std::ostream &operator <<(std::ostream &, const Bracketed &);
+
+} // namespace Anyp
+
+#endif /* SQUID_SRC_ANYP_HOST_H */
+

src/anyp/Makefile.am

@@ -10,6 +10,8 @@ include $(top_srcdir)/src/Common.am
 noinst_LTLIBRARIES = libanyp.la
 
 libanyp_la_SOURCES = \
+	Host.cc \
+	Host.h \
 	PortCfg.cc \
 	PortCfg.h \
 	ProtocolType.cc \