From 9724436af701a247f3c8f4d2cbf8b2635a090c0b Mon Sep 17 00:00:00 2001 From: Tony Walker Date: Thu, 29 Feb 2024 10:53:36 +0000 Subject: [PATCH 01/38] Ensure bumped SSL certs get IP SANs if available --- src/ssl/gadgets.cc | 33 +++++++++++++++++++++++++++++++-- src/ssl/gadgets.h | 3 +++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/src/ssl/gadgets.cc b/src/ssl/gadgets.cc index 1f8ac9d76d3..c4823854364 100644 --- a/src/ssl/gadgets.cc +++ b/src/ssl/gadgets.cc @@ -5,7 +5,9 @@ * contributions from numerous individuals and organizations. * Please see the COPYING and CONTRIBUTORS files for details. */ - +#include +#include +#include #include "squid.h" #include "base/IoManip.h" #include "error/SysErrorDetail.h" @@ -467,6 +469,25 @@ mimicExtensions(Security::CertPointer & cert, Security::CertPointer const &mimic return added; } +/// Checks to see whether the address is an IP address and configure +/// the passed sockaddr_storage appropriately +int Ssl::is_ip_address(const unsigned char* ip, struct sockaddr_storage* ss) { + ss->ss_family = AF_UNSPEC; + + // Try and convert as IPv4 + if (inet_pton(AF_INET, (char*)ip, &((struct sockaddr_in *)ss)->sin_addr) == 1) { + ss->ss_family = AF_INET; + return 1; + } + // Try and convert as IPv6 + if (inet_pton(AF_INET6, (char*)ip, &((struct sockaddr_in6 *)ss)->sin6_addr) == 1) { + ss->ss_family = AF_INET6; + return 1; + } + // Otherwise, it's not an IP + return 0; +} + /// Adds a new subjectAltName extension contining Subject CN or returns false /// expects the caller to check for the existing subjectAltName extension static bool @@ -484,8 +505,16 @@ addAltNameWithSubjectCn(Security::CertPointer &cert) if (!cn_data) return false; + sockaddr_storage socket_info; + const char* altname_type; + const int is_ip = Ssl::is_ip_address(cn_data->data, &socket_info); + if (is_ip == 1) { + altname_type = "IP"; + } else { + altname_type = "DNS"; + } char dnsName[1024]; // DNS names are limited to 256 characters - const int res = snprintf(dnsName, sizeof(dnsName), "DNS:%*s", cn_data->length, cn_data->data); + const int res = snprintf(dnsName, sizeof(dnsName), "%s:%.*s", altname_type, cn_data->length, cn_data->data); if (res <= 0 || res >= static_cast(sizeof(dnsName))) return false; diff --git a/src/ssl/gadgets.h b/src/ssl/gadgets.h index 72da1781d68..a4a3029e0ff 100644 --- a/src/ssl/gadgets.h +++ b/src/ssl/gadgets.h @@ -293,6 +293,9 @@ bool CertificatesCmp(const Security::CertPointer &cert1, const Security::CertPoi /// portability issues with older OpenSSL versions const ASN1_BIT_STRING *X509_get_signature(const Security::CertPointer &); +/// Checks to see whether the address is an IP address and configure +/// the passed sockaddr_storage appropriately +int is_ip_address(const unsigned char* ip, struct sockaddr_storage* ss); } // namespace Ssl #endif // USE_OPENSSL From e24d6801fa13b62d3256a379472a360af1c7853b Mon Sep 17 00:00:00 2001 From: Tony Walker Date: Thu, 29 Feb 2024 10:54:32 +0000 Subject: [PATCH 02/38] Enable matchX509CommonNames to match on IP --- src/ssl/support.cc | 57 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 49 insertions(+), 8 deletions(-) diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 753d491e6c0..2167c2cdee1 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -15,6 +15,7 @@ */ #if USE_OPENSSL +#include #include "acl/FilledChecklist.h" #include "anyp/PortCfg.h" #include "anyp/Uri.h" @@ -192,6 +193,36 @@ int Ssl::asn1timeToString(ASN1_TIME *tm, char *buf, int len) return write; } +// Compare 2 IP addresses +int compare_ip_addresses(void *check_data, ASN1_OCTET_STRING *altname_ip) { + // Cast check_data to const char + const unsigned char* check_ip = reinterpret_cast(check_data); + sockaddr_storage socket_info; + if (!Ssl::is_ip_address(check_ip, &socket_info)) { + // We couldn't convert check_ip into either IPv4 or IPv6 + debugs(83, 4, "Check data was not an IP address, not comparing.."); + return -1; + } + if (socket_info.ss_family == AF_INET) { + // IPv4 conversion was successful, use it for comparison + debugs(83, 4, "IPV4 address found, checking.."); + sockaddr_in* addr4 = reinterpret_cast(&socket_info); + if (altname_ip->length == sizeof(addr4->sin_addr) && + std::memcmp(altname_ip->data, &(addr4->sin_addr), sizeof(addr4->sin_addr)) == 0) { + debugs(83, 4, "IPV4 comparison successful!"); + return 0; + } + } else if (socket_info.ss_family == AF_INET6) { + debugs(83, 4, "IPV6 address found, checking.."); + sockaddr_in6* addr6 = reinterpret_cast(&socket_info); + if (altname_ip->length == sizeof(addr6->sin6_addr) && + std::memcmp(altname_ip->data, &(addr6->sin6_addr), sizeof(addr6->sin6_addr)) == 0) { + return 0; + } + } + return -1; +} + int Ssl::matchX509CommonNames(X509 *peer_cert, void *check_data, int (*check_func)(void *check_data, ASN1_STRING *cn_data)) { assert(peer_cert); @@ -213,14 +244,24 @@ int Ssl::matchX509CommonNames(X509 *peer_cert, void *check_data, int (*check_fun int numalts = sk_GENERAL_NAME_num(altnames); for (int i = 0; i < numalts; ++i) { const GENERAL_NAME *check = sk_GENERAL_NAME_value(altnames, i); - if (check->type != GEN_DNS) { - continue; - } - ASN1_STRING *cn_data = check->d.dNSName; - - if ( (*check_func)(check_data, cn_data) == 0) { - sk_GENERAL_NAME_pop_free(altnames, GENERAL_NAME_free); - return 1; + switch(check->type) { + case GEN_DNS: + // If the type is GEN_DNS, call check_func with the dNSName data + if ( (*check_func)(check_data, check->d.dNSName) == 0) { + sk_GENERAL_NAME_pop_free(altnames, GENERAL_NAME_free); + return 1; + } + break; + case GEN_IPADD: + debugs(83, 4, "Check type is GEN_IPADD, verifying..."); + // If it's an IP address, attempt to compare it with the check_data + if (compare_ip_addresses(check_data, check->d.iPAddress) == 0) { + sk_GENERAL_NAME_pop_free(altnames, GENERAL_NAME_free); + return 1; + } + break; + default: + continue; } } sk_GENERAL_NAME_pop_free(altnames, GENERAL_NAME_free); From 7b2541e2196475abab5d0b8f53c9163ae3454318 Mon Sep 17 00:00:00 2001 From: Tony Walker Date: Sun, 28 Apr 2024 15:30:50 +0100 Subject: [PATCH 03/38] Declare compare_ip_addresses as static --- src/ssl/support.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 2167c2cdee1..d81d05d258f 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -194,7 +194,7 @@ int Ssl::asn1timeToString(ASN1_TIME *tm, char *buf, int len) } // Compare 2 IP addresses -int compare_ip_addresses(void *check_data, ASN1_OCTET_STRING *altname_ip) { +static int compare_ip_addresses(void *check_data, ASN1_OCTET_STRING *altname_ip) { // Cast check_data to const char const unsigned char* check_ip = reinterpret_cast(check_data); sockaddr_storage socket_info; From d3875700db5651ee99ab12fff598f1c918da1fc8 Mon Sep 17 00:00:00 2001 From: Tony Walker Date: Thu, 9 May 2024 08:27:48 +0000 Subject: [PATCH 04/38] Switch to Ip::Address for IP verification --- src/security/cert_generators/file/Makefile.am | 1 + src/ssl/gadgets.cc | 33 ++++--------------- src/ssl/gadgets.h | 4 --- 3 files changed, 7 insertions(+), 31 deletions(-) diff --git a/src/security/cert_generators/file/Makefile.am b/src/security/cert_generators/file/Makefile.am index 070dc95e608..1005b28a877 100644 --- a/src/security/cert_generators/file/Makefile.am +++ b/src/security/cert_generators/file/Makefile.am @@ -24,6 +24,7 @@ security_file_certgen_SOURCES = \ security_file_certgen_LDADD = \ $(top_builddir)/src/ssl/libsslutil.la \ + $(top_builddir)/src/ip/libip.la \ $(top_builddir)/src/sbuf/libsbuf.la \ $(top_builddir)/src/debug/libdebug.la \ $(top_builddir)/src/error/liberror.la \ diff --git a/src/ssl/gadgets.cc b/src/ssl/gadgets.cc index c4823854364..4510433db55 100644 --- a/src/ssl/gadgets.cc +++ b/src/ssl/gadgets.cc @@ -11,6 +11,7 @@ #include "squid.h" #include "base/IoManip.h" #include "error/SysErrorDetail.h" +#include "ip/Address.h" #include "sbuf/Stream.h" #include "security/Io.h" #include "ssl/gadgets.h" @@ -469,25 +470,6 @@ mimicExtensions(Security::CertPointer & cert, Security::CertPointer const &mimic return added; } -/// Checks to see whether the address is an IP address and configure -/// the passed sockaddr_storage appropriately -int Ssl::is_ip_address(const unsigned char* ip, struct sockaddr_storage* ss) { - ss->ss_family = AF_UNSPEC; - - // Try and convert as IPv4 - if (inet_pton(AF_INET, (char*)ip, &((struct sockaddr_in *)ss)->sin_addr) == 1) { - ss->ss_family = AF_INET; - return 1; - } - // Try and convert as IPv6 - if (inet_pton(AF_INET6, (char*)ip, &((struct sockaddr_in6 *)ss)->sin6_addr) == 1) { - ss->ss_family = AF_INET6; - return 1; - } - // Otherwise, it's not an IP - return 0; -} - /// Adds a new subjectAltName extension contining Subject CN or returns false /// expects the caller to check for the existing subjectAltName extension static bool @@ -505,14 +487,11 @@ addAltNameWithSubjectCn(Security::CertPointer &cert) if (!cn_data) return false; - sockaddr_storage socket_info; - const char* altname_type; - const int is_ip = Ssl::is_ip_address(cn_data->data, &socket_info); - if (is_ip == 1) { - altname_type = "IP"; - } else { - altname_type = "DNS"; - } + Ip::Address ipAddress; + // Extract the CN string from cn_data and use it to determine its type + // using Ip::Address::operator "=" + const char* cn_cstr = (const char*) ASN1_STRING_get0_data(cn_data); + const char* altname_type = (ipAddress = cn_cstr) ? "IP" : "DNS"; char dnsName[1024]; // DNS names are limited to 256 characters const int res = snprintf(dnsName, sizeof(dnsName), "%s:%.*s", altname_type, cn_data->length, cn_data->data); if (res <= 0 || res >= static_cast(sizeof(dnsName))) diff --git a/src/ssl/gadgets.h b/src/ssl/gadgets.h index a4a3029e0ff..8e36df34cbb 100644 --- a/src/ssl/gadgets.h +++ b/src/ssl/gadgets.h @@ -292,10 +292,6 @@ bool CertificatesCmp(const Security::CertPointer &cert1, const Security::CertPoi /// wrapper for OpenSSL X509_get0_signature() which takes care of /// portability issues with older OpenSSL versions const ASN1_BIT_STRING *X509_get_signature(const Security::CertPointer &); - -/// Checks to see whether the address is an IP address and configure -/// the passed sockaddr_storage appropriately -int is_ip_address(const unsigned char* ip, struct sockaddr_storage* ss); } // namespace Ssl #endif // USE_OPENSSL From e1d60a9c314b0b8670444e30167539ad035c3ebf Mon Sep 17 00:00:00 2001 From: Tony Walker Date: Thu, 9 May 2024 08:30:20 +0000 Subject: [PATCH 05/38] Add VerifyAddress class for matching IP/Domainname --- src/ssl/support.cc | 130 +++++++++++++++++++++++++++------------------ src/ssl/support.h | 16 ++++++ 2 files changed, 95 insertions(+), 51 deletions(-) diff --git a/src/ssl/support.cc b/src/ssl/support.cc index d81d05d258f..fa74c57e1d4 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -19,10 +19,12 @@ #include "acl/FilledChecklist.h" #include "anyp/PortCfg.h" #include "anyp/Uri.h" +#include #include "fatal.h" #include "fd.h" #include "fde.h" #include "globals.h" +#include "ip/Address.h" #include "ipc/MemMap.h" #include "security/CertError.h" #include "security/Certificate.h" @@ -56,6 +58,79 @@ std::vector Ssl::BumpModeStr = { /*,"err"*/ }; +// Methods for the VerifyAddress class +// +// The public match method can be called with an ASN1_STRING which +// could either be an X509_NAME or GENERAL_NAME +int VerifyAddress::match(ASN1_STRING *asn1_data) { + // declare an empty server var which will be updated later + const char *server = nullptr; + if (ASN1_STRING_type(asn1_data) == V_ASN1_OCTET_STRING) { + // We've been passed an IP address from a GENERAL_NAME struct + // Attempt to safely convert this to a string + const unsigned char *ip = ASN1_STRING_get0_data(asn1_data); + char buffer[INET6_ADDRSTRLEN]; + if (ASN1_STRING_length(asn1_data) == 4) { + // IPv4 + inet_ntop(AF_INET, ip, buffer, sizeof(buffer)); + } else if (ASN1_STRING_length(asn1_data) == 16) { + // IPv6 + inet_ntop(AF_INET6, ip, buffer, sizeof(buffer)); + } else { + debugs(83, 4, "Tried to get an IP from ASN1_OCTET_STRING but failed"); + return 1; + } + server = strdup(buffer); + } else { + // Otherwise, assume we have the cn_data from a typical ASN1_STRING + // This could be a domainname or IP address + server = (const char *)asn1_data->data; + } + // Attempt to convert server into an IP and use matchIP if it's valid + Ip::Address ipAddress; + if (ipAddress = server) { + debugs(83, 4, "Verifying server IP " << private_check_data << " to certificate name/subjectAltName " << server); + return matchIp(ipAddress); + } + // Otherwise, match the original address as a domain name + return matchDomainNameData(asn1_data); +} + +int VerifyAddress::matchDomainNameData(ASN1_STRING *cn_data) { + char cn[1024]; + + 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) { + if (*s == '\0') + return 1; // always a domain mismatch. contains 0x00 + *d = *s; + } + cn[cn_data->length] = '\0'; + debugs(83, 4, "Verifying server domain " << private_check_data << " to certificate name/subjectAltName " << cn); + return matchDomainName(private_check_data, (cn[0] == '*' ? cn + 1 : cn), mdnRejectSubsubDomains); +} + +int VerifyAddress::matchIp(Ip::Address &iaddr) { + // Attempt to convert private_check_data to an IP address + // and compare it to iaddr + Ip::Address check_ip; + if ((check_ip = private_check_data) && (iaddr == check_ip)) { + return 0; + } + return 1; +} + +const char* VerifyAddress::processCheckData(void *check_data) { + return reinterpret_cast(check_data); +} + /** \defgroup ServerProtocolSSLInternal Server-Side SSL Internals \ingroup ServerProtocolSSLAPI @@ -193,36 +268,6 @@ int Ssl::asn1timeToString(ASN1_TIME *tm, char *buf, int len) return write; } -// Compare 2 IP addresses -static int compare_ip_addresses(void *check_data, ASN1_OCTET_STRING *altname_ip) { - // Cast check_data to const char - const unsigned char* check_ip = reinterpret_cast(check_data); - sockaddr_storage socket_info; - if (!Ssl::is_ip_address(check_ip, &socket_info)) { - // We couldn't convert check_ip into either IPv4 or IPv6 - debugs(83, 4, "Check data was not an IP address, not comparing.."); - return -1; - } - if (socket_info.ss_family == AF_INET) { - // IPv4 conversion was successful, use it for comparison - debugs(83, 4, "IPV4 address found, checking.."); - sockaddr_in* addr4 = reinterpret_cast(&socket_info); - if (altname_ip->length == sizeof(addr4->sin_addr) && - std::memcmp(altname_ip->data, &(addr4->sin_addr), sizeof(addr4->sin_addr)) == 0) { - debugs(83, 4, "IPV4 comparison successful!"); - return 0; - } - } else if (socket_info.ss_family == AF_INET6) { - debugs(83, 4, "IPV6 address found, checking.."); - sockaddr_in6* addr6 = reinterpret_cast(&socket_info); - if (altname_ip->length == sizeof(addr6->sin6_addr) && - std::memcmp(altname_ip->data, &(addr6->sin6_addr), sizeof(addr6->sin6_addr)) == 0) { - return 0; - } - } - return -1; -} - int Ssl::matchX509CommonNames(X509 *peer_cert, void *check_data, int (*check_func)(void *check_data, ASN1_STRING *cn_data)) { assert(peer_cert); @@ -254,8 +299,8 @@ int Ssl::matchX509CommonNames(X509 *peer_cert, void *check_data, int (*check_fun break; case GEN_IPADD: debugs(83, 4, "Check type is GEN_IPADD, verifying..."); - // If it's an IP address, attempt to compare it with the check_data - if (compare_ip_addresses(check_data, check->d.iPAddress) == 0) { + // If it's an IP address, call check_func with the iPAddress data + if ( (*check_func)(check_data, check->d.iPAddress) == 0) { sk_GENERAL_NAME_pop_free(altnames, GENERAL_NAME_free); return 1; } @@ -271,25 +316,8 @@ int Ssl::matchX509CommonNames(X509 *peer_cert, void *check_data, int (*check_fun 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 == 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) { - if (*s == '\0') - return 1; // always a domain mismatch. contains 0x00 - *d = *s; - } - 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), mdnRejectSubsubDomains); + VerifyAddress verify(check_data); + return verify.match(cn_data); } bool Ssl::checkX509ServerValidity(X509 *cert, const char *server) diff --git a/src/ssl/support.h b/src/ssl/support.h index 53e64c102fd..cb65a0872ea 100644 --- a/src/ssl/support.h +++ b/src/ssl/support.h @@ -16,6 +16,7 @@ #include "base/CbDataList.h" #include "comm/forward.h" #include "compat/openssl.h" +#include "ip/Address.h" #include "sbuf/SBuf.h" #include "security/Session.h" #include "ssl/gadgets.h" @@ -366,6 +367,21 @@ class VerifyCallbackParameters { } //namespace Ssl +class VerifyAddress { +public: + // Constructor + VerifyAddress(void *check_data) + : private_check_data(processCheckData(check_data)) + {} + int match(ASN1_STRING *cn_data); +private: + int matchDomainNameData(ASN1_STRING *cn_data); + int matchIp(Ip::Address &iaddr); + + const char* processCheckData(void *check_data); + const char* private_check_data; +}; + #if _SQUID_WINDOWS_ #if defined(__cplusplus) From fd1b782dd5b288b9ff0045639ada55cb42a2afb6 Mon Sep 17 00:00:00 2001 From: Tony Walker Date: Wed, 15 May 2024 13:55:53 +0000 Subject: [PATCH 06/38] Pass explicit address types to VerifyAddress --- src/acl/ServerName.cc | 5 +- src/security/ErrorDetail.cc | 7 ++- src/ssl/support.cc | 90 ++++++++++++++++++----------------- src/ssl/support.h | 14 ++++-- src/tests/stub_libsslsquid.cc | 2 +- 5 files changed, 66 insertions(+), 52 deletions(-) diff --git a/src/acl/ServerName.cc b/src/acl/ServerName.cc index f48629dddca..55594cac538 100644 --- a/src/acl/ServerName.cc +++ b/src/acl/ServerName.cc @@ -50,8 +50,11 @@ ACLServerNameData::match(const char *host) /// \retval 1 when the name does not match template int -check_cert_domain( void *check_data, ASN1_STRING *cn_data) +check_cert_domain( void *check_data, ASN1_STRING *cn_data, Ssl::AddressType addr_type) { + // addr_type is only declared here to ensure the signature type matches + // for matchX509CommonNames. Void it here to avoid compiler warnings. + (void)addr_type; char cn[1024]; ACLData * data = (ACLData *)check_data; diff --git a/src/security/ErrorDetail.cc b/src/security/ErrorDetail.cc index 66fbb5b4d1e..7aa7c63fca2 100644 --- a/src/security/ErrorDetail.cc +++ b/src/security/ErrorDetail.cc @@ -581,7 +581,7 @@ class CommonNamesPrinter explicit CommonNamesPrinter(std::ostream &os): os_(os) {} /// Ssl::matchX509CommonNames() visitor that reports the given name (if any) - static int PrintName(void *, ASN1_STRING *); + static int PrintName(void *, ASN1_STRING *, Ssl::AddressType addr_type); /// whether any names have been printed so far bool printed = false; @@ -593,8 +593,11 @@ class CommonNamesPrinter }; int -CommonNamesPrinter::PrintName(void * const printer, ASN1_STRING * const name) +CommonNamesPrinter::PrintName(void * const printer, ASN1_STRING * const name, Ssl::AddressType addr_type) { + // addr_type is only declared here to ensure the signature type matches + // for matchX509CommonNames. Void it here to avoid compiler warnings. + (void)addr_type; assert(printer); static_cast(printer)->printName(name); return 1; diff --git a/src/ssl/support.cc b/src/ssl/support.cc index fa74c57e1d4..5c6c98cd1bf 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -59,44 +59,46 @@ std::vector Ssl::BumpModeStr = { }; // Methods for the VerifyAddress class -// -// The public match method can be called with an ASN1_STRING which -// could either be an X509_NAME or GENERAL_NAME -int VerifyAddress::match(ASN1_STRING *asn1_data) { - // declare an empty server var which will be updated later - const char *server = nullptr; - if (ASN1_STRING_type(asn1_data) == V_ASN1_OCTET_STRING) { - // We've been passed an IP address from a GENERAL_NAME struct - // Attempt to safely convert this to a string - const unsigned char *ip = ASN1_STRING_get0_data(asn1_data); - char buffer[INET6_ADDRSTRLEN]; - if (ASN1_STRING_length(asn1_data) == 4) { - // IPv4 - inet_ntop(AF_INET, ip, buffer, sizeof(buffer)); - } else if (ASN1_STRING_length(asn1_data) == 16) { - // IPv6 - inet_ntop(AF_INET6, ip, buffer, sizeof(buffer)); - } else { - debugs(83, 4, "Tried to get an IP from ASN1_OCTET_STRING but failed"); - return 1; - } - server = strdup(buffer); - } else { - // Otherwise, assume we have the cn_data from a typical ASN1_STRING - // This could be a domainname or IP address - server = (const char *)asn1_data->data; - } - // Attempt to convert server into an IP and use matchIP if it's valid - Ip::Address ipAddress; - if (ipAddress = server) { - debugs(83, 4, "Verifying server IP " << private_check_data << " to certificate name/subjectAltName " << server); - return matchIp(ipAddress); +int Ssl::VerifyAddress::match(ASN1_STRING *asn1_data, Ssl::AddressType addr_type) { + switch (addr_type) { + // For IP addresses, create an in[6]_addr struct from the ASN1 data, + // create an Ip::Address from it and then use that to attempt a match. + case Ssl::AddressType::IP: + { + const unsigned char *address = ASN1_STRING_get0_data(asn1_data); + Ip::Address iaddr; + // Declare a buffer that can store either an IPV4 or IPV6 address + char theip[40]; + switch(ASN1_STRING_length(asn1_data)) { + case 4: + struct in_addr addr; + memcpy(&addr.s_addr, address, 4); + iaddr = addr; + iaddr.toStr(theip, 40); + debugs(83, 4, "Verifying server IP " << private_check_data << " IPV4: " << theip); + return matchIp(iaddr); + case 16: + struct in6_addr addr6; + memcpy(&addr6.s6_addr, address, 16); + iaddr = addr6; + iaddr.toStr(theip, 40); + debugs(83, 4, "Verifying server IP " << private_check_data << " IPV6: " << theip); + return matchIp(iaddr); + default: + // This doesn't look like an IP despite what the type is set to. Fail the match. + debugs(83, 4, "Unexpected ASN1 string length for an IP address"); + return 1; + } + } + case Ssl::AddressType::DNS: + debugs(83, 4, "Performing DNS domain match"); + return matchDomainNameData(asn1_data); } - // Otherwise, match the original address as a domain name - return matchDomainNameData(asn1_data); + // If we got here, fail + return 1; } -int VerifyAddress::matchDomainNameData(ASN1_STRING *cn_data) { +int Ssl::VerifyAddress::matchDomainNameData(ASN1_STRING *cn_data) { char cn[1024]; if (cn_data->length == 0) @@ -117,7 +119,7 @@ int VerifyAddress::matchDomainNameData(ASN1_STRING *cn_data) { return matchDomainName(private_check_data, (cn[0] == '*' ? cn + 1 : cn), mdnRejectSubsubDomains); } -int VerifyAddress::matchIp(Ip::Address &iaddr) { +int Ssl::VerifyAddress::matchIp(Ip::Address &iaddr) { // Attempt to convert private_check_data to an IP address // and compare it to iaddr Ip::Address check_ip; @@ -127,7 +129,7 @@ int VerifyAddress::matchIp(Ip::Address &iaddr) { return 1; } -const char* VerifyAddress::processCheckData(void *check_data) { +const char* Ssl::VerifyAddress::processCheckData(void *check_data) { return reinterpret_cast(check_data); } @@ -268,7 +270,7 @@ int Ssl::asn1timeToString(ASN1_TIME *tm, char *buf, int len) return write; } -int Ssl::matchX509CommonNames(X509 *peer_cert, void *check_data, int (*check_func)(void *check_data, ASN1_STRING *cn_data)) +int Ssl::matchX509CommonNames(X509 *peer_cert, void *check_data, int (*check_func)(void *check_data, ASN1_STRING *cn_data, Ssl::AddressType addr_type)) { assert(peer_cert); @@ -278,7 +280,7 @@ int Ssl::matchX509CommonNames(X509 *peer_cert, void *check_data, int (*check_fun ASN1_STRING *cn_data = X509_NAME_ENTRY_get_data(X509_NAME_get_entry(name, i)); - if ( (*check_func)(check_data, cn_data) == 0) + if ( (*check_func)(check_data, cn_data, Ssl::AddressType::DNS) == 0) return 1; } @@ -292,7 +294,7 @@ int Ssl::matchX509CommonNames(X509 *peer_cert, void *check_data, int (*check_fun switch(check->type) { case GEN_DNS: // If the type is GEN_DNS, call check_func with the dNSName data - if ( (*check_func)(check_data, check->d.dNSName) == 0) { + if ( (*check_func)(check_data, check->d.dNSName, Ssl::AddressType::DNS) == 0) { sk_GENERAL_NAME_pop_free(altnames, GENERAL_NAME_free); return 1; } @@ -300,7 +302,7 @@ int Ssl::matchX509CommonNames(X509 *peer_cert, void *check_data, int (*check_fun case GEN_IPADD: debugs(83, 4, "Check type is GEN_IPADD, verifying..."); // If it's an IP address, call check_func with the iPAddress data - if ( (*check_func)(check_data, check->d.iPAddress) == 0) { + if ( (*check_func)(check_data, check->d.iPAddress, Ssl::AddressType::IP) == 0) { sk_GENERAL_NAME_pop_free(altnames, GENERAL_NAME_free); return 1; } @@ -314,10 +316,10 @@ int Ssl::matchX509CommonNames(X509 *peer_cert, void *check_data, int (*check_fun return 0; } -static int check_domain( void *check_data, ASN1_STRING *cn_data) +static int check_domain( void *check_data, ASN1_STRING *cn_data, Ssl::AddressType addr_type) { - VerifyAddress verify(check_data); - return verify.match(cn_data); + Ssl::VerifyAddress verify(check_data); + return verify.match(cn_data, addr_type); } bool Ssl::checkX509ServerValidity(X509 *cert, const char *server) diff --git a/src/ssl/support.h b/src/ssl/support.h index cb65a0872ea..35c8a3a09d2 100644 --- a/src/ssl/support.h +++ b/src/ssl/support.h @@ -144,6 +144,12 @@ inline const char *bumpMode(int bm) /// certificates indexed by issuer name typedef std::multimap CertsIndexedList; +/// enum for use when checking domain matches +enum class AddressType { + IP, + DNS, +}; + /** * Load PEM-encoded certificates from the given file. */ @@ -283,7 +289,7 @@ void useSquidUntrusted(SSL_CTX *sslContext); \param check_func The function used to match X509 CNs. The CN data passed as ASN1_STRING data \return 1 if any of the certificate CN matches, 0 if none matches. */ -int matchX509CommonNames(X509 *peer_cert, void *check_data, int (*check_func)(void *check_data, ASN1_STRING *cn_data)); +int matchX509CommonNames(X509 *peer_cert, void *check_data, int (*check_func)(void *check_data, ASN1_STRING *cn_data, AddressType addr_type)); /** \ingroup ServerProtocolSSLAPI @@ -365,15 +371,13 @@ class VerifyCallbackParameters { bool hidMissingIssuer = false; }; -} //namespace Ssl - class VerifyAddress { public: // Constructor VerifyAddress(void *check_data) : private_check_data(processCheckData(check_data)) {} - int match(ASN1_STRING *cn_data); + int match(ASN1_STRING *cn_data, AddressType addr_type); private: int matchDomainNameData(ASN1_STRING *cn_data); int matchIp(Ip::Address &iaddr); @@ -381,6 +385,8 @@ class VerifyAddress { const char* processCheckData(void *check_data); const char* private_check_data; }; +} //namespace Ssl + #if _SQUID_WINDOWS_ diff --git a/src/tests/stub_libsslsquid.cc b/src/tests/stub_libsslsquid.cc index 6253bbced52..67b752baa2f 100644 --- a/src/tests/stub_libsslsquid.cc +++ b/src/tests/stub_libsslsquid.cc @@ -69,7 +69,7 @@ bool generateUntrustedCert(Security::CertPointer &, Security::PrivateKeyPointer Security::ContextPointer GenerateSslContext(CertificateProperties const &, Security::ServerOptions &, bool) STUB_RETVAL(Security::ContextPointer()) bool verifySslCertificate(const Security::ContextPointer &, CertificateProperties const &) STUB_RETVAL(false) Security::ContextPointer GenerateSslContextUsingPkeyAndCertFromMemory(const char *, Security::ServerOptions &, bool) STUB_RETVAL(Security::ContextPointer()) -int matchX509CommonNames(X509 *, void *, int (*)(void *, ASN1_STRING *)) STUB_RETVAL(0) +int matchX509CommonNames(X509 *, void *, int (*)(void *, ASN1_STRING *, Ssl::AddressType *)) STUB_RETVAL(0) bool checkX509ServerValidity(X509 *, const char *) STUB_RETVAL(false) int asn1timeToString(ASN1_TIME *, char *, int) STUB_RETVAL(0) void setClientSNI(SSL *, const char *) STUB From 5d7b0c01307d13ad50ec8cd193245af9c0f777f2 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 26 Jul 2024 10:40:54 -0400 Subject: [PATCH 07/38] fixup: Undo unwanted out-of-scope formatting change --- src/ssl/gadgets.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/ssl/gadgets.h b/src/ssl/gadgets.h index 8e36df34cbb..72da1781d68 100644 --- a/src/ssl/gadgets.h +++ b/src/ssl/gadgets.h @@ -292,6 +292,7 @@ bool CertificatesCmp(const Security::CertPointer &cert1, const Security::CertPoi /// wrapper for OpenSSL X509_get0_signature() which takes care of /// portability issues with older OpenSSL versions const ASN1_BIT_STRING *X509_get_signature(const Security::CertPointer &); + } // namespace Ssl #endif // USE_OPENSSL From d3377ecce8ceba5a0e0f88f03506edeef9b7dbea Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 26 Jul 2024 16:17:08 -0400 Subject: [PATCH 08/38] Migrate from (ASN1_STRING, often-ignored type) pairs to GeneralName ASN1_STRING may hold "binary" data (e.g., four IP address octets). In this context, it is pretty much equivalent to SBuf -- an opaque buffer with a known size. Ignoring the associated data type leads to wrong casts and, hence, comparison of incomparable objects or similar bugs. XXX: This unpolished implementation duplicates the equivalent of switch(type) statements in matches() implementations. It is also too general for the foreseeable Squid needs. --- src/acl/ServerName.cc | 84 ++++++------ src/client_side.cc | 2 +- src/security/ErrorDetail.cc | 62 +++++---- src/ssl/support.cc | 246 +++++++++++++++++++--------------- src/ssl/support.h | 105 ++++++++++----- src/tests/stub_libsslsquid.cc | 4 +- 6 files changed, 288 insertions(+), 215 deletions(-) diff --git a/src/acl/ServerName.cc b/src/acl/ServerName.cc index 55594cac538..da7eb5b252c 100644 --- a/src/acl/ServerName.cc +++ b/src/acl/ServerName.cc @@ -45,34 +45,38 @@ 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 -int -check_cert_domain( void *check_data, ASN1_STRING *cn_data, Ssl::AddressType addr_type) +namespace Acl { + +class ServerNameMatcher: public Ssl::GeneralNameMatcher { - // addr_type is only declared here to ensure the signature type matches - // for matchX509CommonNames. Void it here to avoid compiler warnings. - (void)addr_type; - char cn[1024]; - ACLData * data = (ACLData *)check_data; - - if (cn_data->length > (int)sizeof(cn) - 1) - return 1; // ignore data that does not fit our buffer - - char *s = reinterpret_cast(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; +public: + explicit ServerNameMatcher(ServerNameCheck::Parameters &p): parameters(p) {} + + bool match(const Ssl::GeneralName &) const override; + +private: + // TODO: Make ServerNameCheck::Parameters::match() and this reference constant. + ServerNameCheck::Parameters ¶meters; ///< configured ACL parameters +}; + +} // namespace Acl + +bool +Acl::ServerNameMatcher::match(const Ssl::GeneralName &name) const +{ + if (const auto domain = name.domainName()) + return parameters.match(SBuf(*domain).c_str()); // TODO: Upgrade string-matching ACLs to SBuf + + if (const auto ip = name.ip()) { + // This IP address conversion to c-string brackets IPv6 addresses. + // TODO: Document that fact in ssl::server_name. + char hostStr[MAX_IPSTRLEN] = ""; + (void)ip->toHostStr(hostStr, sizeof(hostStr)); + return parameters.match(hostStr); } - cn[cn_data->length] = '\0'; - debugs(28, 4, "Verifying certificate name/subjectAltName " << cn); - if (data->match(cn)) - return 0; - return 1; + + debugs(83, 7, "assume an unsupported name variant " << name.index() << " does not match"); + return false; } int @@ -82,37 +86,35 @@ 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 serverNameFromConn; if (ConnStateData *conn = checklist->conn()) { - const char *clientRequestedServerName = nullptr; - clientSniKeeper = conn->tlsClientSni(); - if (clientSniKeeper.isEmpty()) { + std::optional clientRequestedServerName; + const auto &clientSni = conn->tlsClientSni(); + if (clientSni.isEmpty()) { const char *host = checklist->request->url.host(); if (host && *host) // paranoid first condition: host() is never nil - clientRequestedServerName = host; + clientRequestedServerName = host; // TODO: Use Uri::hostOrIp() instead } else - clientRequestedServerName = clientSniKeeper.c_str(); + clientRequestedServerName = 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::findSubjectName(*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); + return Ssl::matchX509CommonNames(*peer_cert, ServerNameMatcher(*data)); if (!useServerProvided) - serverName = clientRequestedServerName; + serverNameFromConn = clientRequestedServerName; } } - if (!serverName) - serverName = "none"; - + const auto serverName = serverNameFromConn ? serverNameFromConn->c_str() : "none"; return data->match(serverName); } diff --git a/src/client_side.cc b/src/client_side.cc index 1ec0a8f4a50..c02ea98412c 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -1484,7 +1484,7 @@ bool ConnStateData::serveDelayedError(Http::Stream *context) // when we can extract the intended name from the bumped HTTP request. if (const Security::CertPointer &srvCert = sslServerBump->serverCert) { HttpRequest *request = http->request; - if (!Ssl::checkX509ServerValidity(srvCert.get(), request->url.host())) { + if (!Ssl::findSubjectName(*srvCert, SBuf(request->url.host()))) { // TODO: Use url.hostOrIp() debugs(33, 2, "SQUID_X509_V_ERR_DOMAIN_MISMATCH: Certificate " << "does not match domainname " << request->url.host()); diff --git a/src/security/ErrorDetail.cc b/src/security/ErrorDetail.cc index 7aa7c63fca2..85439004945 100644 --- a/src/security/ErrorDetail.cc +++ b/src/security/ErrorDetail.cc @@ -575,50 +575,56 @@ Security::ErrorDetail::printSubject(std::ostream &os) const #if USE_OPENSSL /// a helper class to print CNs extracted using Ssl::matchX509CommonNames() -class CommonNamesPrinter +class CommonNamesPrinter: public Ssl::GeneralNameMatcher { public: explicit CommonNamesPrinter(std::ostream &os): os_(os) {} - /// Ssl::matchX509CommonNames() visitor that reports the given name (if any) - static int PrintName(void *, ASN1_STRING *, Ssl::AddressType addr_type); + /* Ssl::GeneralNameMatcher API */ + bool match(const Ssl::GeneralName &) const override; - /// whether any names have been printed so far - bool printed = false; + /// the number of names printed so far + mutable size_t printed = 0; private: - void printName(const ASN1_STRING *); + std::ostream &itemStream() const; std::ostream &os_; ///< destination for printed names }; -int -CommonNamesPrinter::PrintName(void * const printer, ASN1_STRING * const name, Ssl::AddressType addr_type) -{ - // addr_type is only declared here to ensure the signature type matches - // for matchX509CommonNames. Void it here to avoid compiler warnings. - (void)addr_type; - assert(printer); - static_cast(printer)->printName(name); - return 1; -} - -/// prints an HTML-quoted version of the given common name (as a part of the +/// prints an HTML-quoted version of the given name (as a part of the /// printed names list) -void -CommonNamesPrinter::printName(const ASN1_STRING * const name) +bool +CommonNamesPrinter::match(const Ssl::GeneralName &name) const { - if (name && name->length) { - if (printed) - os_ << ", "; - + if (const auto domain = name.domainName()) { // TODO: Convert html_quote() into an std::ostream manipulator accepting (buf, n). - SBuf buf(reinterpret_cast(name->data), name->length); - os_ << html_quote(buf.c_str()); + itemStream() << html_quote(SBuf(*domain).c_str()); + return false; // keep going + } - printed = true; + if (const auto ip = name.ip()) { + char hostStr[MAX_IPSTRLEN] = ""; + const auto length = ip->toHostStr(hostStr, sizeof(hostStr)); // no html_quote() is needed + itemStream().write(hostStr, length); + return false; // keep going } + + debugs(83, 7, "cannot print an unsupported name variant " << name.index()); + return false; // keep going +} + +/// prints commas in front of each item except for the very first one +/// \returns a stream for printing the name +std::ostream & +CommonNamesPrinter::itemStream() const +{ + if (printed++) + os_ << ", "; + return os_; } + + #endif // USE_OPENSSL /// a list of the broken certificates CN and alternate names @@ -628,7 +634,7 @@ Security::ErrorDetail::printCommonName(std::ostream &os) const #if USE_OPENSSL if (broken_cert.get()) { CommonNamesPrinter printer(os); - Ssl::matchX509CommonNames(broken_cert.get(), &printer, printer.PrintName); + (void)Ssl::matchX509CommonNames(*broken_cert, printer); if (printer.printed) return; } diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 5c6c98cd1bf..0314b75f7c7 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -58,79 +58,56 @@ std::vector Ssl::BumpModeStr = { /*,"err"*/ }; -// Methods for the VerifyAddress class -int Ssl::VerifyAddress::match(ASN1_STRING *asn1_data, Ssl::AddressType addr_type) { - switch (addr_type) { - // For IP addresses, create an in[6]_addr struct from the ASN1 data, - // create an Ip::Address from it and then use that to attempt a match. - case Ssl::AddressType::IP: - { - const unsigned char *address = ASN1_STRING_get0_data(asn1_data); - Ip::Address iaddr; - // Declare a buffer that can store either an IPV4 or IPV6 address - char theip[40]; - switch(ASN1_STRING_length(asn1_data)) { - case 4: - struct in_addr addr; - memcpy(&addr.s_addr, address, 4); - iaddr = addr; - iaddr.toStr(theip, 40); - debugs(83, 4, "Verifying server IP " << private_check_data << " IPV4: " << theip); - return matchIp(iaddr); - case 16: - struct in6_addr addr6; - memcpy(&addr6.s6_addr, address, 16); - iaddr = addr6; - iaddr.toStr(theip, 40); - debugs(83, 4, "Verifying server IP " << private_check_data << " IPV6: " << theip); - return matchIp(iaddr); - default: - // This doesn't look like an IP despite what the type is set to. Fail the match. - debugs(83, 4, "Unexpected ASN1 string length for an IP address"); - return 1; - } - } - case Ssl::AddressType::DNS: - debugs(83, 4, "Performing DNS domain match"); - return matchDomainNameData(asn1_data); - } - // If we got here, fail - return 1; -} +namespace Ssl { -int Ssl::VerifyAddress::matchDomainNameData(ASN1_STRING *cn_data) { - char cn[1024]; +class OneNameMatcher: public GeneralNameMatcher +{ +public: + explicit OneNameMatcher(const SBuf &needle): needleStorage_(needle), needle_(needleStorage_.c_str()) {} - if (cn_data->length == 0) - return 1; // zero length cn, ignore + bool match(const GeneralName &) const override; - if (cn_data->length > (int)sizeof(cn) - 1) - return 1; //if does not fit our buffer just ignore +private: + bool matchDomainName(const SBuf &) const; + bool matchIp(const Ip::Address &) const; - char *s = reinterpret_cast(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(83, 4, "Verifying server domain " << private_check_data << " to certificate name/subjectAltName " << cn); - return matchDomainName(private_check_data, (cn[0] == '*' ? cn + 1 : cn), mdnRejectSubsubDomains); + SBuf needleStorage_; ///< a name we are looking for + const char * const needle_; ///< needleStorage_ using matchDomainName()-compatible type +}; + +} // namespace Ssl + +bool +Ssl::OneNameMatcher::match(const GeneralName &name) const +{ + if (const auto domain = name.domainName()) + return matchDomainName(*domain); + if (const auto ip = name.ip()) + return matchIp(*ip); + debugs(83, 7, "assume an unsupported name variant " << name.index() << " does not match: " << needle_); + return false; } -int Ssl::VerifyAddress::matchIp(Ip::Address &iaddr) { - // Attempt to convert private_check_data to an IP address - // and compare it to iaddr - Ip::Address check_ip; - if ((check_ip = private_check_data) && (iaddr == check_ip)) { - return 0; - } - return 1; +bool +Ssl::OneNameMatcher::matchDomainName(const SBuf &rawName) const { + debugs(83, 5, "needle=" << needle_ << " domain=" << rawName); // XXX: rawName may contain non-printable characters + auto name = rawName; + if (name.length() > 0 && name[0] == '*') + name.consume(1); + return ::matchDomainName(needle_, name.c_str(), mdnRejectSubsubDomains) == 0; } -const char* Ssl::VerifyAddress::processCheckData(void *check_data) { - return reinterpret_cast(check_data); +bool +Ssl::OneNameMatcher::matchIp(const Ip::Address &ip) const { + debugs(83, 5, "needle=" << needle_ << " ip=" << ip); + // if we cannot convert our needle to an IP, then it does not match any IP + Ip::Address needleIp; + if (!(needleIp = needle_)) { + // TODO: Consider caching conversion result in anticipation of matchIp() calls + debugs(83, 7, "needle is not an IP, cannot match"); + return false; + } + return (needleIp == ip); } /** @@ -270,61 +247,116 @@ int Ssl::asn1timeToString(ASN1_TIME *tm, char *buf, int len) return write; } -int Ssl::matchX509CommonNames(X509 *peer_cert, void *check_data, int (*check_func)(void *check_data, ASN1_STRING *cn_data, Ssl::AddressType addr_type)) +std::optional +Ssl::GeneralName::FromCommonName(const ASN1_STRING &buffer) { - assert(peer_cert); + // Unlike OpenSSL GENERAL_NAME, ASN1_STRING cannot tell us what kind of + // information this CN buffer stores. The rest of Squid code treats CN as a + // domain name, so we parse/validate the given buffer as such. - X509_NAME *name = X509_get_subject_name(peer_cert); + const SBuf raw(reinterpret_cast(buffer.data), buffer.length); + + if (raw.isEmpty()) { + debugs(83, 5, "rejecting empty CN"); + return std::nullopt; + } + + if (raw.find('\0') != SBuf::npos) { + debugs(83, 5, "rejecting CN with an ASCII NUL character: " << raw); // XXX: Contains non-printable characters! + return std::nullopt; + } + + debugs(83, 5, "parsed CN: " << raw); // XXX: May contain non-printable characters + const GeneralNameStorage storage(std::in_place_index<(size_t)GeneralNameKind::dNSName>, raw); // XXX: Remove C cast. + return GeneralName(storage); +} +std::optional +Ssl::GeneralName::FromSubjectAltName(const GENERAL_NAME &san) +{ + switch(san.type) { + case GEN_DNS: { + // san.d.dNSName is ASN1_IA5STRING + Assure(san.d.dNSName); + const SBuf raw(reinterpret_cast(san.d.dNSName->data), san.d.dNSName->length); + debugs(83, 5, "parsed DNS name: " << raw); // XXX: May contain non-printable characters + const GeneralNameStorage storage(std::in_place_index<(size_t)GeneralNameKind::dNSName>, raw); // XXX: Remove C cast. + return GeneralName(storage); + } + + case GEN_IPADD: { + // san.d.iPAddress is ASN1_OCTET_STRING + Assure(san.d.iPAddress); + if (san.d.iPAddress->length == 4) { + struct in_addr addr; + static_assert(sizeof(addr.s_addr) == 4); + memcpy(&addr.s_addr, san.d.iPAddress->data, sizeof(addr.s_addr)); + const Ip::Address ip(addr); + debugs(83, 5, "parsed IPv4: " << ip); + const GeneralNameStorage storage(std::in_place_index<(size_t)GeneralNameKind::iPAddress>, ip); // XXX: Remove C cast. + return GeneralName(storage); + } + + if (san.d.iPAddress->length == 16) { + struct in6_addr addr; + static_assert(sizeof(addr.s6_addr) == 16); + memcpy(&addr.s6_addr, san.d.iPAddress->data, sizeof(addr.s6_addr)); + const Ip::Address ip(addr); + debugs(83, 5, "parsed IPv6: " << ip); + const GeneralNameStorage storage(std::in_place_index<(size_t)GeneralNameKind::iPAddress>, ip); // XXX: Remove C cast. + return GeneralName(storage); + } + + debugs(83, 4, "unexpected length of an IP address SAN: " << san.d.iPAddress->length); + return std::nullopt; + } + + default: + debugs(83, 4, "unsupported SAN kind: " << san.type); + return std::nullopt; + } +} + +bool +Ssl::matchX509CommonNames(X509 &cert, const GeneralNameMatcher &matcher) +{ + const auto name = X509_get_subject_name(&cert); for (int i = X509_NAME_get_index_by_NID(name, NID_commonName, -1); i >= 0; i = X509_NAME_get_index_by_NID(name, NID_commonName, i)) { ASN1_STRING *cn_data = X509_NAME_ENTRY_get_data(X509_NAME_get_entry(name, i)); + if (!cn_data) { + debugs(83, 5, "failed to extract CN value: " << Ssl::ReportAndForgetErrors); + continue; + } + + if (const auto cn = GeneralName::FromCommonName(*cn_data)) { + if (matcher.match(*cn)) + return true; + } + } - if ( (*check_func)(check_data, cn_data, Ssl::AddressType::DNS) == 0) - return 1; - } - - STACK_OF(GENERAL_NAME) * altnames; - altnames = (STACK_OF(GENERAL_NAME)*)X509_get_ext_d2i(peer_cert, NID_subject_alt_name, nullptr, nullptr); - - if (altnames) { - int numalts = sk_GENERAL_NAME_num(altnames); - for (int i = 0; i < numalts; ++i) { - const GENERAL_NAME *check = sk_GENERAL_NAME_value(altnames, i); - switch(check->type) { - case GEN_DNS: - // If the type is GEN_DNS, call check_func with the dNSName data - if ( (*check_func)(check_data, check->d.dNSName, Ssl::AddressType::DNS) == 0) { - sk_GENERAL_NAME_pop_free(altnames, GENERAL_NAME_free); - return 1; - } - break; - case GEN_IPADD: - debugs(83, 4, "Check type is GEN_IPADD, verifying..."); - // If it's an IP address, call check_func with the iPAddress data - if ( (*check_func)(check_data, check->d.iPAddress, Ssl::AddressType::IP) == 0) { - sk_GENERAL_NAME_pop_free(altnames, GENERAL_NAME_free); - return 1; - } - break; - default: - continue; + const Ssl::GENERAL_NAME_STACK_Pointer sans(static_cast( + X509_get_ext_d2i(&cert, NID_subject_alt_name, nullptr, nullptr))); + if (sans) { + const auto sanCount = sk_GENERAL_NAME_num(sans.get()); + for (int i = 0; i < sanCount; ++i) { + const auto rawSan = sk_GENERAL_NAME_value(sans.get(), i); + Assure(rawSan); + if (const auto san = GeneralName::FromSubjectAltName(*rawSan)) { + if (matcher.match(*san)) + return true; } } - sk_GENERAL_NAME_pop_free(altnames, GENERAL_NAME_free); } - return 0; -} -static int check_domain( void *check_data, ASN1_STRING *cn_data, Ssl::AddressType addr_type) -{ - Ssl::VerifyAddress verify(check_data); - return verify.match(cn_data, addr_type); + debugs(83, 7, "no matches"); + return false; } -bool Ssl::checkX509ServerValidity(X509 *cert, const char *server) +bool +Ssl::findSubjectName(X509 &cert, const SBuf &name) { - return matchX509CommonNames(cert, (void *)server, check_domain); + return matchX509CommonNames(cert, OneNameMatcher(name)); } /// adjusts OpenSSL validation results for each verified certificate in ctx @@ -366,7 +398,7 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) // Check for domain mismatch only if the current certificate is the peer certificate. if (!dont_verify_domain && server && peer_cert.get() == X509_STORE_CTX_get_current_cert(ctx)) { - if (!Ssl::checkX509ServerValidity(peer_cert.get(), server->c_str())) { + if (!Ssl::findSubjectName(*peer_cert, *server)) { debugs(83, 2, "SQUID_X509_V_ERR_DOMAIN_MISMATCH: Certificate " << *peer_cert << " does not match domainname " << server); ok = 0; error_no = SQUID_X509_V_ERR_DOMAIN_MISMATCH; diff --git a/src/ssl/support.h b/src/ssl/support.h index 35c8a3a09d2..d3edd5bce1b 100644 --- a/src/ssl/support.h +++ b/src/ssl/support.h @@ -14,6 +14,7 @@ #if USE_OPENSSL #include "base/CbDataList.h" +#include "base/TypeTraits.h" #include "comm/forward.h" #include "compat/openssl.h" #include "ip/Address.h" @@ -32,6 +33,8 @@ #endif #include #include +#include +#include /** \defgroup ServerProtocolSSLAPI Server-Side SSL API @@ -144,10 +147,60 @@ inline const char *bumpMode(int bm) /// certificates indexed by issuer name typedef std::multimap CertsIndexedList; -/// enum for use when checking domain matches -enum class AddressType { - IP, - DNS, +/// Indexes of RFC 5280 GeneralName variants (and the corresponding types). We +/// do not yet support most of these variants, but we list all of them, so that +/// our index values match those in X.509 certificates/RFC 5280. +enum class GeneralNameKind: size_t { + otherName = 0, // OtherName + rfc822Name = 1, // IA5String + dNSName = 2, // IA5String + x400Address = 3, // ORAddress + directoryName = 4, // Name + ediPartyName = 5, // EDIPartyName + uniformResourceIdentifier = 6, // IA5String + iPAddress = 7, // OCTET STRING + registeredID = 8 // OBJECT IDENTIFIER +}; + +// TODO: Move/generalize if others need this. +/// Marks GeneralName variant that we do not yet support. Including this empty +/// class in GeneralName is necessary for GeneralNameKind values to match +/// indexes in X.509 certificates. +class UnsupportedVariant {}; + +/// a helper class to store successfully extracted/parsed GeneralName (RFC 5280) +using GeneralNameStorage = std::variant< + UnsupportedVariant, // OtherName (index is otherName) + UnsupportedVariant, // IA5String (index is rfc822Name) + SBuf, // IA5String (index is dNSName) + UnsupportedVariant, // ORAddress (index is x400Address) + UnsupportedVariant, // Name (index is directoryName) + UnsupportedVariant, // EDIPartyName (index is ediPartyName) + UnsupportedVariant, // IA5String (index is uniformResourceIdentifier) + Ip::Address, // OCTET STRING (index is iPAddress) + UnsupportedVariant // OBJECT IDENTIFIER (index is registeredID) +>; + +/// successfully extracted/parsed GeneralName (RFC 5280) +/// (a.k.a. OpenSSL GENERAL_NAME) +class GeneralName +{ +public: + static std::optional FromCommonName(const ASN1_STRING &); + static std::optional FromSubjectAltName( const GENERAL_NAME &); + + // XXX: remove these C casts + auto ip() const { return std::get_if<(size_t)GeneralNameKind::iPAddress>(&raw_); } + auto domainName() const { return std::get_if<(size_t)GeneralNameKind::dNSName>(&raw_); } + + /// XXX: Document all methods + auto index() const { return raw_.index(); } + +private: + // use a From*() function to create these objects + GeneralName(const GeneralNameStorage &raw): raw_(raw) {} + + GeneralNameStorage raw_; /// information we are providing access to }; /** @@ -280,25 +333,19 @@ bool configureSSLUsingPkeyAndCertFromMemory(SSL *ssl, const char *data, AnyP::Po */ void useSquidUntrusted(SSL_CTX *sslContext); -/** - \ingroup ServerProtocolSSLAPI - * Iterates over the X509 common and alternate names and to see if matches with given data - * using the check_func. - \param peer_cert The X509 cert to check - \param check_data The data with which the X509 CNs compared - \param check_func The function used to match X509 CNs. The CN data passed as ASN1_STRING data - \return 1 if any of the certificate CN matches, 0 if none matches. - */ -int matchX509CommonNames(X509 *peer_cert, void *check_data, int (*check_func)(void *check_data, ASN1_STRING *cn_data, AddressType addr_type)); +/// an algorithm for checking/testing/comparing X.509 certificate names +class GeneralNameMatcher: public Interface +{ +public: + /// XXX: Document + virtual bool match(const Ssl::GeneralName &) const = 0; +}; -/** - \ingroup ServerProtocolSSLAPI - * Check if the certificate is valid for a server - \param cert The X509 cert to check. - \param server The server name. - \return true if the certificate is valid for the server or false otherwise. - */ -bool checkX509ServerValidity(X509 *cert, const char *server); +/// determines whether at least one common or alternate certificate names matches +bool matchX509CommonNames(X509 &, const GeneralNameMatcher &); + +/// whether at least one common or alternate subject name matches the given one +bool findSubjectName(X509 &, const SBuf &name); /** \ingroup ServerProtocolSSLAPI @@ -371,20 +418,6 @@ class VerifyCallbackParameters { bool hidMissingIssuer = false; }; -class VerifyAddress { -public: - // Constructor - VerifyAddress(void *check_data) - : private_check_data(processCheckData(check_data)) - {} - int match(ASN1_STRING *cn_data, AddressType addr_type); -private: - int matchDomainNameData(ASN1_STRING *cn_data); - int matchIp(Ip::Address &iaddr); - - const char* processCheckData(void *check_data); - const char* private_check_data; -}; } //namespace Ssl diff --git a/src/tests/stub_libsslsquid.cc b/src/tests/stub_libsslsquid.cc index 67b752baa2f..1126c54d75b 100644 --- a/src/tests/stub_libsslsquid.cc +++ b/src/tests/stub_libsslsquid.cc @@ -69,8 +69,8 @@ bool generateUntrustedCert(Security::CertPointer &, Security::PrivateKeyPointer Security::ContextPointer GenerateSslContext(CertificateProperties const &, Security::ServerOptions &, bool) STUB_RETVAL(Security::ContextPointer()) bool verifySslCertificate(const Security::ContextPointer &, CertificateProperties const &) STUB_RETVAL(false) Security::ContextPointer GenerateSslContextUsingPkeyAndCertFromMemory(const char *, Security::ServerOptions &, bool) STUB_RETVAL(Security::ContextPointer()) -int matchX509CommonNames(X509 *, void *, int (*)(void *, ASN1_STRING *, Ssl::AddressType *)) STUB_RETVAL(0) -bool checkX509ServerValidity(X509 *, const char *) STUB_RETVAL(false) +bool matchX509CommonNames(X509 &, const GeneralNameMatcher &) STUB_RETVAL(false) +bool findSubjectName(X509 &, const SBuf &) STUB_RETVAL(false) int asn1timeToString(ASN1_TIME *, char *, int) STUB_RETVAL(0) void setClientSNI(SSL *, const char *) STUB SBuf GetX509PEM(X509 *) STUB_RETVAL(SBuf()) From f97b58e906e1e76997680e11d2ab9af6b8245ca8 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 29 Jul 2024 16:20:49 -0400 Subject: [PATCH 09/38] Simplified GeneralName by dropping support for UnsupportedVariant XXX: Still need to address duplication flagged in the previous branch commit (along with freshly added XXXs regarding index reporting). --- src/acl/ServerName.cc | 2 +- src/security/ErrorDetail.cc | 2 +- src/ssl/support.cc | 30 +++++++-------- src/ssl/support.h | 77 +++++++++++++++---------------------- 4 files changed, 49 insertions(+), 62 deletions(-) diff --git a/src/acl/ServerName.cc b/src/acl/ServerName.cc index da7eb5b252c..e0458e599cc 100644 --- a/src/acl/ServerName.cc +++ b/src/acl/ServerName.cc @@ -75,7 +75,7 @@ Acl::ServerNameMatcher::match(const Ssl::GeneralName &name) const return parameters.match(hostStr); } - debugs(83, 7, "assume an unsupported name variant " << name.index() << " does not match"); + debugs(83, 7, "assume an unsupported name variant (XXX:name.index()) does not match"); return false; } diff --git a/src/security/ErrorDetail.cc b/src/security/ErrorDetail.cc index 85439004945..045322a0f36 100644 --- a/src/security/ErrorDetail.cc +++ b/src/security/ErrorDetail.cc @@ -610,7 +610,7 @@ CommonNamesPrinter::match(const Ssl::GeneralName &name) const return false; // keep going } - debugs(83, 7, "cannot print an unsupported name variant " << name.index()); + debugs(83, 7, "cannot print an unsupported name variant (XXX:name.index())"); return false; // keep going } diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 0314b75f7c7..176ec8af929 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -84,7 +84,7 @@ Ssl::OneNameMatcher::match(const GeneralName &name) const return matchDomainName(*domain); if (const auto ip = name.ip()) return matchIp(*ip); - debugs(83, 7, "assume an unsupported name variant " << name.index() << " does not match: " << needle_); + debugs(83, 7, "assume an unsupported name variant (XXX: name.index()) does not match: " << needle_); return false; } @@ -257,18 +257,17 @@ Ssl::GeneralName::FromCommonName(const ASN1_STRING &buffer) const SBuf raw(reinterpret_cast(buffer.data), buffer.length); if (raw.isEmpty()) { - debugs(83, 5, "rejecting empty CN"); + debugs(83, 3, "rejecting empty CN"); return std::nullopt; } if (raw.find('\0') != SBuf::npos) { - debugs(83, 5, "rejecting CN with an ASCII NUL character: " << raw); // XXX: Contains non-printable characters! + debugs(83, 3, "rejecting CN with an ASCII NUL character: " << raw); // XXX: Contains non-printable characters! return std::nullopt; } debugs(83, 5, "parsed CN: " << raw); // XXX: May contain non-printable characters - const GeneralNameStorage storage(std::in_place_index<(size_t)GeneralNameKind::dNSName>, raw); // XXX: Remove C cast. - return GeneralName(storage); + return GeneralName(raw); } std::optional @@ -276,25 +275,27 @@ Ssl::GeneralName::FromSubjectAltName(const GENERAL_NAME &san) { switch(san.type) { case GEN_DNS: { - // san.d.dNSName is ASN1_IA5STRING + // san.d.dNSName is OpenSSL ASN1_IA5STRING Assure(san.d.dNSName); const SBuf raw(reinterpret_cast(san.d.dNSName->data), san.d.dNSName->length); + // XXX: Apply FromCommonName() checks! debugs(83, 5, "parsed DNS name: " << raw); // XXX: May contain non-printable characters - const GeneralNameStorage storage(std::in_place_index<(size_t)GeneralNameKind::dNSName>, raw); // XXX: Remove C cast. - return GeneralName(storage); + return GeneralName(raw); } case GEN_IPADD: { - // san.d.iPAddress is ASN1_OCTET_STRING + // san.d.iPAddress is OpenSSL ASN1_OCTET_STRING Assure(san.d.iPAddress); + + // RFC 5280 section 4.2.1.6 signals IPv4/IPv6 address family using data length + if (san.d.iPAddress->length == 4) { struct in_addr addr; static_assert(sizeof(addr.s_addr) == 4); memcpy(&addr.s_addr, san.d.iPAddress->data, sizeof(addr.s_addr)); const Ip::Address ip(addr); debugs(83, 5, "parsed IPv4: " << ip); - const GeneralNameStorage storage(std::in_place_index<(size_t)GeneralNameKind::iPAddress>, ip); // XXX: Remove C cast. - return GeneralName(storage); + return GeneralName(ip); } if (san.d.iPAddress->length == 16) { @@ -303,16 +304,15 @@ Ssl::GeneralName::FromSubjectAltName(const GENERAL_NAME &san) memcpy(&addr.s6_addr, san.d.iPAddress->data, sizeof(addr.s6_addr)); const Ip::Address ip(addr); debugs(83, 5, "parsed IPv6: " << ip); - const GeneralNameStorage storage(std::in_place_index<(size_t)GeneralNameKind::iPAddress>, ip); // XXX: Remove C cast. - return GeneralName(storage); + return GeneralName(ip); } - debugs(83, 4, "unexpected length of an IP address SAN: " << san.d.iPAddress->length); + debugs(83, 3, "unexpected length of an IP address SAN: " << san.d.iPAddress->length); return std::nullopt; } default: - debugs(83, 4, "unsupported SAN kind: " << san.type); + debugs(83, 3, "unsupported SAN kind: " << san.type); return std::nullopt; } } diff --git a/src/ssl/support.h b/src/ssl/support.h index d3edd5bce1b..41f7b0c6f77 100644 --- a/src/ssl/support.h +++ b/src/ssl/support.h @@ -147,60 +147,47 @@ inline const char *bumpMode(int bm) /// certificates indexed by issuer name typedef std::multimap CertsIndexedList; -/// Indexes of RFC 5280 GeneralName variants (and the corresponding types). We -/// do not yet support most of these variants, but we list all of them, so that -/// our index values match those in X.509 certificates/RFC 5280. -enum class GeneralNameKind: size_t { - otherName = 0, // OtherName - rfc822Name = 1, // IA5String - dNSName = 2, // IA5String - x400Address = 3, // ORAddress - directoryName = 4, // Name - ediPartyName = 5, // EDIPartyName - uniformResourceIdentifier = 6, // IA5String - iPAddress = 7, // OCTET STRING - registeredID = 8 // OBJECT IDENTIFIER -}; - -// TODO: Move/generalize if others need this. -/// Marks GeneralName variant that we do not yet support. Including this empty -/// class in GeneralName is necessary for GeneralNameKind values to match -/// indexes in X.509 certificates. -class UnsupportedVariant {}; - -/// a helper class to store successfully extracted/parsed GeneralName (RFC 5280) -using GeneralNameStorage = std::variant< - UnsupportedVariant, // OtherName (index is otherName) - UnsupportedVariant, // IA5String (index is rfc822Name) - SBuf, // IA5String (index is dNSName) - UnsupportedVariant, // ORAddress (index is x400Address) - UnsupportedVariant, // Name (index is directoryName) - UnsupportedVariant, // EDIPartyName (index is ediPartyName) - UnsupportedVariant, // IA5String (index is uniformResourceIdentifier) - Ip::Address, // OCTET STRING (index is iPAddress) - UnsupportedVariant // OBJECT IDENTIFIER (index is registeredID) ->; - -/// successfully extracted/parsed GeneralName (RFC 5280) -/// (a.k.a. OpenSSL GENERAL_NAME) +/// A successfully extracted/parsed certificate "name" field. See RFC 5280 +/// GeneralName and X520CommonName types for examples of information sources. class GeneralName { public: static std::optional FromCommonName(const ASN1_STRING &); static std::optional FromSubjectAltName( const GENERAL_NAME &); - // XXX: remove these C casts - auto ip() const { return std::get_if<(size_t)GeneralNameKind::iPAddress>(&raw_); } - auto domainName() const { return std::get_if<(size_t)GeneralNameKind::dNSName>(&raw_); } + // accessor methods below are mutually exclusive with domainName() - /// XXX: Document all methods - auto index() const { return raw_.index(); } + /// stored IP address (if any) + auto ip() const { return std::get_if(&raw_); } -private: - // use a From*() function to create these objects - GeneralName(const GeneralNameStorage &raw): raw_(raw) {} + /// stored domain name or domain name mask (if any) + auto domainName() const { return std::get_if(&raw_); } - GeneralNameStorage raw_; /// information we are providing access to +private: + using Storage = std::variant< + /// A domain name or domain name mask (e.g., *.example.com). This info + /// comes (with very little validation) from a source like these two: + /// * RFC 5280 "dNSName" variant of a subjectAltName extension + /// (GeneralName index is 2, underlying value type is IA5String); + /// * RFC 5280 X520CommonName component of a Subject distinguished name + /// field (underlying value type is DirectoryName). + /// + /// This string is never empty and never contains NUL characters, but it + /// may contain any other character, including non-printable ones. + SBuf, + + /// An IPv4 or an IPv6 address. This info comes (with very little + /// validation) from RFC 5280 "iPAddress" variant of a subjectAltName + /// extension (GeneralName index=7, value type=OCTET STRING). + /// + /// Ip::Address::isNoAddr() may be true for this address. + /// Ip::Address::isAnyAddr() may be true for this address. + Ip::Address>; + + // use a From*() function to create GeneralName objects + GeneralName(const Storage &raw): raw_(raw) {} + + Storage raw_; /// information we are providing access to }; /** From 59600674ea708d52e750bdb70e1bcefcf64e5e65 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 29 Jul 2024 16:49:05 -0400 Subject: [PATCH 10/38] fixup: Addressed duplication flagged in previous branch commits All duplicated `if` statements have been moved to GeneralNameMatcher::match(). --- src/acl/ServerName.cc | 29 +++++++++++++++-------------- src/security/ErrorDetail.cc | 35 ++++++++++++++++------------------- src/ssl/support.cc | 13 ++++++------- src/ssl/support.h | 15 ++++++++++++--- 4 files changed, 49 insertions(+), 43 deletions(-) diff --git a/src/acl/ServerName.cc b/src/acl/ServerName.cc index e0458e599cc..8b611526053 100644 --- a/src/acl/ServerName.cc +++ b/src/acl/ServerName.cc @@ -52,7 +52,10 @@ class ServerNameMatcher: public Ssl::GeneralNameMatcher public: explicit ServerNameMatcher(ServerNameCheck::Parameters &p): parameters(p) {} - bool match(const Ssl::GeneralName &) const override; +protected: + /* GeneralNameMatcher API */ + bool matchDomainName(const SBuf &) const override; + bool matchIp(const Ip::Address &) const override; private: // TODO: Make ServerNameCheck::Parameters::match() and this reference constant. @@ -62,21 +65,19 @@ class ServerNameMatcher: public Ssl::GeneralNameMatcher } // namespace Acl bool -Acl::ServerNameMatcher::match(const Ssl::GeneralName &name) const +Acl::ServerNameMatcher::matchDomainName(const SBuf &domain) const { - if (const auto domain = name.domainName()) - return parameters.match(SBuf(*domain).c_str()); // TODO: Upgrade string-matching ACLs to SBuf - - if (const auto ip = name.ip()) { - // This IP address conversion to c-string brackets IPv6 addresses. - // TODO: Document that fact in ssl::server_name. - char hostStr[MAX_IPSTRLEN] = ""; - (void)ip->toHostStr(hostStr, sizeof(hostStr)); - return parameters.match(hostStr); - } + return parameters.match(SBuf(domain).c_str()); // TODO: Upgrade string-matching ACLs to SBuf +} - debugs(83, 7, "assume an unsupported name variant (XXX:name.index()) does not match"); - return false; +bool +Acl::ServerNameMatcher::matchIp(const Ip::Address &ip) const +{ + // This IP address conversion to c-string brackets IPv6 addresses. + // TODO: Document that fact in ssl::server_name. + char hostStr[MAX_IPSTRLEN] = ""; + (void)ip.toHostStr(hostStr, sizeof(hostStr)); + return parameters.match(hostStr); } int diff --git a/src/security/ErrorDetail.cc b/src/security/ErrorDetail.cc index 045322a0f36..86b506dd053 100644 --- a/src/security/ErrorDetail.cc +++ b/src/security/ErrorDetail.cc @@ -580,37 +580,34 @@ class CommonNamesPrinter: public Ssl::GeneralNameMatcher public: explicit CommonNamesPrinter(std::ostream &os): os_(os) {} - /* Ssl::GeneralNameMatcher API */ - bool match(const Ssl::GeneralName &) const override; - /// the number of names printed so far mutable size_t printed = 0; +protected: + /* GeneralNameMatcher API */ + bool matchDomainName(const SBuf &) const override; + bool matchIp(const Ip::Address &) const override; + private: std::ostream &itemStream() const; std::ostream &os_; ///< destination for printed names }; -/// prints an HTML-quoted version of the given name (as a part of the -/// printed names list) bool -CommonNamesPrinter::match(const Ssl::GeneralName &name) const +CommonNamesPrinter::matchDomainName(const SBuf &domain) const { - if (const auto domain = name.domainName()) { - // TODO: Convert html_quote() into an std::ostream manipulator accepting (buf, n). - itemStream() << html_quote(SBuf(*domain).c_str()); - return false; // keep going - } - - if (const auto ip = name.ip()) { - char hostStr[MAX_IPSTRLEN] = ""; - const auto length = ip->toHostStr(hostStr, sizeof(hostStr)); // no html_quote() is needed - itemStream().write(hostStr, length); - return false; // keep going - } + // TODO: Convert html_quote() into an std::ostream manipulator accepting (buf, n). + itemStream() << html_quote(SBuf(domain).c_str()); + return false; // keep going +} - debugs(83, 7, "cannot print an unsupported name variant (XXX:name.index())"); +bool +CommonNamesPrinter::matchIp(const Ip::Address &ip) const +{ + char hostStr[MAX_IPSTRLEN] = ""; + const auto length = ip.toHostStr(hostStr, sizeof(hostStr)); // no html_quote() is needed + itemStream().write(hostStr, length); return false; // keep going } diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 176ec8af929..d9487826467 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -65,11 +65,10 @@ class OneNameMatcher: public GeneralNameMatcher public: explicit OneNameMatcher(const SBuf &needle): needleStorage_(needle), needle_(needleStorage_.c_str()) {} - bool match(const GeneralName &) const override; - -private: - bool matchDomainName(const SBuf &) const; - bool matchIp(const Ip::Address &) const; +protected: + /* GeneralNameMatcher API */ + bool matchDomainName(const SBuf &) const override; + bool matchIp(const Ip::Address &) const override; SBuf needleStorage_; ///< a name we are looking for const char * const needle_; ///< needleStorage_ using matchDomainName()-compatible type @@ -78,13 +77,13 @@ class OneNameMatcher: public GeneralNameMatcher } // namespace Ssl bool -Ssl::OneNameMatcher::match(const GeneralName &name) const +Ssl::GeneralNameMatcher::match(const GeneralName &name) const { if (const auto domain = name.domainName()) return matchDomainName(*domain); if (const auto ip = name.ip()) return matchIp(*ip); - debugs(83, 7, "assume an unsupported name variant (XXX: name.index()) does not match: " << needle_); + Assure(!"unreachable code: the above `if` statements must cover all name variants"); return false; } diff --git a/src/ssl/support.h b/src/ssl/support.h index 41f7b0c6f77..da4eba54d7e 100644 --- a/src/ssl/support.h +++ b/src/ssl/support.h @@ -155,7 +155,8 @@ class GeneralName static std::optional FromCommonName(const ASN1_STRING &); static std::optional FromSubjectAltName( const GENERAL_NAME &); - // accessor methods below are mutually exclusive with domainName() + // Accessor methods below are mutually exclusive: Exactly one method is + // guaranteed to return a result other than std::nullopt. /// stored IP address (if any) auto ip() const { return std::get_if(&raw_); } @@ -324,8 +325,16 @@ void useSquidUntrusted(SSL_CTX *sslContext); class GeneralNameMatcher: public Interface { public: - /// XXX: Document - virtual bool match(const Ssl::GeneralName &) const = 0; + /// whether the given name satisfies algorithm conditions + bool match(const Ssl::GeneralName &) const; + +protected: + // The methods below implement public match() API for each of the + // GeneralName variants. For each public match() method call, exactly one of + // these methods is called. + + virtual bool matchDomainName(const SBuf &) const = 0; + virtual bool matchIp(const Ip::Address &) const = 0; }; /// determines whether at least one common or alternate certificate names matches From ae203d24902606b34c4b7039586c7dc637788f3a Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 29 Jul 2024 16:49:51 -0400 Subject: [PATCH 11/38] fixup: Documented matchX509CommonNames() short-circuit effect --- src/ssl/support.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ssl/support.h b/src/ssl/support.h index da4eba54d7e..08d06dd1a39 100644 --- a/src/ssl/support.h +++ b/src/ssl/support.h @@ -337,7 +337,8 @@ class GeneralNameMatcher: public Interface virtual bool matchIp(const Ip::Address &) const = 0; }; -/// determines whether at least one common or alternate certificate names matches +/// Determines whether at least one common or alternate subject names matches. +/// The first match (if any) terminates the search. bool matchX509CommonNames(X509 &, const GeneralNameMatcher &); /// whether at least one common or alternate subject name matches the given one From aadf22720d6bfaa79a17581030f911d7a13ed7ec Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 29 Jul 2024 17:07:02 -0400 Subject: [PATCH 12/38] fixup: Fixed matchX509CommonNames() name to match the new scope We have to change all callers anyway... --- src/acl/ServerName.cc | 2 +- src/security/ErrorDetail.cc | 4 ++-- src/ssl/support.cc | 4 ++-- src/ssl/support.h | 2 +- src/tests/stub_libsslsquid.cc | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/acl/ServerName.cc b/src/acl/ServerName.cc index 8b611526053..2a4e3ae5c5f 100644 --- a/src/acl/ServerName.cc +++ b/src/acl/ServerName.cc @@ -109,7 +109,7 @@ Acl::ServerNameCheck::match(ACLChecklist * const ch) serverNameFromConn = clientRequestedServerName; else { // either no options or useServerProvided if (X509 *peer_cert = (conn->serverBump() ? conn->serverBump()->serverCert.get() : nullptr)) - return Ssl::matchX509CommonNames(*peer_cert, ServerNameMatcher(*data)); + return Ssl::findMatchingSubjectName(*peer_cert, ServerNameMatcher(*data)); if (!useServerProvided) serverNameFromConn = clientRequestedServerName; } diff --git a/src/security/ErrorDetail.cc b/src/security/ErrorDetail.cc index 86b506dd053..5dec89222c6 100644 --- a/src/security/ErrorDetail.cc +++ b/src/security/ErrorDetail.cc @@ -574,7 +574,7 @@ Security::ErrorDetail::printSubject(std::ostream &os) const } #if USE_OPENSSL -/// a helper class to print CNs extracted using Ssl::matchX509CommonNames() +/// a helper class to print CNs extracted using Ssl::findMatchingSubjectName() class CommonNamesPrinter: public Ssl::GeneralNameMatcher { public: @@ -631,7 +631,7 @@ Security::ErrorDetail::printCommonName(std::ostream &os) const #if USE_OPENSSL if (broken_cert.get()) { CommonNamesPrinter printer(os); - (void)Ssl::matchX509CommonNames(*broken_cert, printer); + (void)Ssl::findMatchingSubjectName(*broken_cert, printer); if (printer.printed) return; } diff --git a/src/ssl/support.cc b/src/ssl/support.cc index d9487826467..6d9655037f1 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -317,7 +317,7 @@ Ssl::GeneralName::FromSubjectAltName(const GENERAL_NAME &san) } bool -Ssl::matchX509CommonNames(X509 &cert, const GeneralNameMatcher &matcher) +Ssl::findMatchingSubjectName(X509 &cert, const GeneralNameMatcher &matcher) { const auto name = X509_get_subject_name(&cert); for (int i = X509_NAME_get_index_by_NID(name, NID_commonName, -1); i >= 0; i = X509_NAME_get_index_by_NID(name, NID_commonName, i)) { @@ -355,7 +355,7 @@ Ssl::matchX509CommonNames(X509 &cert, const GeneralNameMatcher &matcher) bool Ssl::findSubjectName(X509 &cert, const SBuf &name) { - return matchX509CommonNames(cert, OneNameMatcher(name)); + return findMatchingSubjectName(cert, OneNameMatcher(name)); } /// adjusts OpenSSL validation results for each verified certificate in ctx diff --git a/src/ssl/support.h b/src/ssl/support.h index 08d06dd1a39..dc436bf9e37 100644 --- a/src/ssl/support.h +++ b/src/ssl/support.h @@ -339,7 +339,7 @@ class GeneralNameMatcher: public Interface /// Determines whether at least one common or alternate subject names matches. /// The first match (if any) terminates the search. -bool matchX509CommonNames(X509 &, const GeneralNameMatcher &); +bool findMatchingSubjectName(X509 &, const GeneralNameMatcher &); /// whether at least one common or alternate subject name matches the given one bool findSubjectName(X509 &, const SBuf &name); diff --git a/src/tests/stub_libsslsquid.cc b/src/tests/stub_libsslsquid.cc index 1126c54d75b..03a65411996 100644 --- a/src/tests/stub_libsslsquid.cc +++ b/src/tests/stub_libsslsquid.cc @@ -69,7 +69,7 @@ bool generateUntrustedCert(Security::CertPointer &, Security::PrivateKeyPointer Security::ContextPointer GenerateSslContext(CertificateProperties const &, Security::ServerOptions &, bool) STUB_RETVAL(Security::ContextPointer()) bool verifySslCertificate(const Security::ContextPointer &, CertificateProperties const &) STUB_RETVAL(false) Security::ContextPointer GenerateSslContextUsingPkeyAndCertFromMemory(const char *, Security::ServerOptions &, bool) STUB_RETVAL(Security::ContextPointer()) -bool matchX509CommonNames(X509 &, const GeneralNameMatcher &) STUB_RETVAL(false) +bool findMatchingSubjectName(X509 &, const GeneralNameMatcher &) STUB_RETVAL(false) bool findSubjectName(X509 &, const SBuf &) STUB_RETVAL(false) int asn1timeToString(ASN1_TIME *, char *, int) STUB_RETVAL(0) void setClientSNI(SSL *, const char *) STUB From 79766df2c3a9542e53acc649e803909fb74ac2f6 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 29 Jul 2024 17:39:59 -0400 Subject: [PATCH 13/38] fixup: Addressed recently added branch XXX XXX: Apply FromCommonName() checks! --- src/ssl/support.cc | 29 +++++++++++++++++------------ src/ssl/support.h | 2 ++ 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 6d9655037f1..b3693beb5a4 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -246,29 +246,37 @@ int Ssl::asn1timeToString(ASN1_TIME *tm, char *buf, int len) return write; } +/// Interprets the given raw buffer as a domain name, rejecting names that +/// increase risks for legacy Squid code while being unlikely to be used in +/// valid certificates that Squid admins want to identify by (rejected) names. std::optional -Ssl::GeneralName::FromCommonName(const ASN1_STRING &buffer) +Ssl::GeneralName::ParseAsDomainName(const char * const description, const ASN1_STRING &buffer) { - // Unlike OpenSSL GENERAL_NAME, ASN1_STRING cannot tell us what kind of - // information this CN buffer stores. The rest of Squid code treats CN as a - // domain name, so we parse/validate the given buffer as such. - const SBuf raw(reinterpret_cast(buffer.data), buffer.length); if (raw.isEmpty()) { - debugs(83, 3, "rejecting empty CN"); + debugs(83, 3, "rejects empty " << description); return std::nullopt; } if (raw.find('\0') != SBuf::npos) { - debugs(83, 3, "rejecting CN with an ASCII NUL character: " << raw); // XXX: Contains non-printable characters! + debugs(83, 3, "rejects " << description << " with an ASCII NUL character: " << raw); // XXX: Contains non-printable characters! return std::nullopt; } - debugs(83, 5, "parsed CN: " << raw); // XXX: May contain non-printable characters + debugs(83, 5, "parsed " << description << ": " << raw); // XXX: May contain non-printable characters return GeneralName(raw); } +std::optional +Ssl::GeneralName::FromCommonName(const ASN1_STRING &buffer) +{ + // Unlike FromSubjectAltName() GENERAL_NAME, our ASN1_STRING cannot tell + // what kind of info this CN buffer stores. The rest of Squid code treats CN + // as a domain name, so we parse/validate the given buffer as such. + return ParseAsDomainName("CN", buffer); +} + std::optional Ssl::GeneralName::FromSubjectAltName(const GENERAL_NAME &san) { @@ -276,10 +284,7 @@ Ssl::GeneralName::FromSubjectAltName(const GENERAL_NAME &san) case GEN_DNS: { // san.d.dNSName is OpenSSL ASN1_IA5STRING Assure(san.d.dNSName); - const SBuf raw(reinterpret_cast(san.d.dNSName->data), san.d.dNSName->length); - // XXX: Apply FromCommonName() checks! - debugs(83, 5, "parsed DNS name: " << raw); // XXX: May contain non-printable characters - return GeneralName(raw); + return ParseAsDomainName("DNS name", *san.d.dNSName); } case GEN_IPADD: { diff --git a/src/ssl/support.h b/src/ssl/support.h index dc436bf9e37..a62ce054e88 100644 --- a/src/ssl/support.h +++ b/src/ssl/support.h @@ -185,6 +185,8 @@ class GeneralName /// Ip::Address::isAnyAddr() may be true for this address. Ip::Address>; + static std::optional ParseAsDomainName(const char *, const ASN1_STRING &); + // use a From*() function to create GeneralName objects GeneralName(const Storage &raw): raw_(raw) {} From 45861c08d80f849d3a5a67d0c4099d32e6b42ff5 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 30 Jul 2024 08:56:41 -0400 Subject: [PATCH 14/38] fixup: Formatted branch-modified sources --- src/acl/ServerName.cc | 2 +- src/security/ErrorDetail.cc | 1 - src/ssl/support.cc | 2 +- src/ssl/support.h | 37 ++++++++++++++++++------------------- 4 files changed, 20 insertions(+), 22 deletions(-) diff --git a/src/acl/ServerName.cc b/src/acl/ServerName.cc index 2a4e3ae5c5f..287e07b74ee 100644 --- a/src/acl/ServerName.cc +++ b/src/acl/ServerName.cc @@ -103,7 +103,7 @@ Acl::ServerNameCheck::match(ACLChecklist * const ch) // use the client requested name if it matches the server // certificate or if the certificate is not available if (!peer_cert || !clientRequestedServerName || - Ssl::findSubjectName(*peer_cert, *clientRequestedServerName)) + Ssl::findSubjectName(*peer_cert, *clientRequestedServerName)) serverNameFromConn = clientRequestedServerName; } else if (useClientRequested) serverNameFromConn = clientRequestedServerName; diff --git a/src/security/ErrorDetail.cc b/src/security/ErrorDetail.cc index 5dec89222c6..886fe39d240 100644 --- a/src/security/ErrorDetail.cc +++ b/src/security/ErrorDetail.cc @@ -621,7 +621,6 @@ CommonNamesPrinter::itemStream() const return os_; } - #endif // USE_OPENSSL /// a list of the broken certificates CN and alternate names diff --git a/src/ssl/support.cc b/src/ssl/support.cc index b3693beb5a4..9221d091085 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -340,7 +340,7 @@ Ssl::findMatchingSubjectName(X509 &cert, const GeneralNameMatcher &matcher) } const Ssl::GENERAL_NAME_STACK_Pointer sans(static_cast( - X509_get_ext_d2i(&cert, NID_subject_alt_name, nullptr, nullptr))); + X509_get_ext_d2i(&cert, NID_subject_alt_name, nullptr, nullptr))); if (sans) { const auto sanCount = sk_GENERAL_NAME_num(sans.get()); for (int i = 0; i < sanCount; ++i) { diff --git a/src/ssl/support.h b/src/ssl/support.h index a62ce054e88..27441123f76 100644 --- a/src/ssl/support.h +++ b/src/ssl/support.h @@ -166,24 +166,24 @@ class GeneralName private: using Storage = std::variant< - /// A domain name or domain name mask (e.g., *.example.com). This info - /// comes (with very little validation) from a source like these two: - /// * RFC 5280 "dNSName" variant of a subjectAltName extension - /// (GeneralName index is 2, underlying value type is IA5String); - /// * RFC 5280 X520CommonName component of a Subject distinguished name - /// field (underlying value type is DirectoryName). - /// - /// This string is never empty and never contains NUL characters, but it - /// may contain any other character, including non-printable ones. - SBuf, - - /// An IPv4 or an IPv6 address. This info comes (with very little - /// validation) from RFC 5280 "iPAddress" variant of a subjectAltName - /// extension (GeneralName index=7, value type=OCTET STRING). - /// - /// Ip::Address::isNoAddr() may be true for this address. - /// Ip::Address::isAnyAddr() may be true for this address. - Ip::Address>; + /// A domain name or domain name mask (e.g., *.example.com). This info + /// comes (with very little validation) from a source like these two: + /// * RFC 5280 "dNSName" variant of a subjectAltName extension + /// (GeneralName index is 2, underlying value type is IA5String); + /// * RFC 5280 X520CommonName component of a Subject distinguished name + /// field (underlying value type is DirectoryName). + /// + /// This string is never empty and never contains NUL characters, but it + /// may contain any other character, including non-printable ones. + SBuf, + + /// An IPv4 or an IPv6 address. This info comes (with very little + /// validation) from RFC 5280 "iPAddress" variant of a subjectAltName + /// extension (GeneralName index=7, value type=OCTET STRING). + /// + /// Ip::Address::isNoAddr() may be true for this address. + /// Ip::Address::isAnyAddr() may be true for this address. + Ip::Address>; static std::optional ParseAsDomainName(const char *, const ASN1_STRING &); @@ -419,7 +419,6 @@ class VerifyCallbackParameters { } //namespace Ssl - #if _SQUID_WINDOWS_ #if defined(__cplusplus) From 6a04acc9d8d6df6eddb06122d9a536c0cd4d1c85 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 30 Jul 2024 09:02:44 -0400 Subject: [PATCH 15/38] fixup: Fix #include problems detected by source-maintenance.sh ERROR: src/ssl/gadgets.cc does not include squid.h first! ERROR: src/ssl/support.cc includes openssl headers without including "compat/openssl.h" --- src/ssl/gadgets.cc | 4 +--- src/ssl/support.cc | 2 -- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/src/ssl/gadgets.cc b/src/ssl/gadgets.cc index 4510433db55..c56d06ed024 100644 --- a/src/ssl/gadgets.cc +++ b/src/ssl/gadgets.cc @@ -5,9 +5,7 @@ * contributions from numerous individuals and organizations. * Please see the COPYING and CONTRIBUTORS files for details. */ -#include -#include -#include + #include "squid.h" #include "base/IoManip.h" #include "error/SysErrorDetail.h" diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 9221d091085..2ff21592073 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -15,11 +15,9 @@ */ #if USE_OPENSSL -#include #include "acl/FilledChecklist.h" #include "anyp/PortCfg.h" #include "anyp/Uri.h" -#include #include "fatal.h" #include "fd.h" #include "fde.h" From bb979a2dbdf5936d6e00591b635840ab1fcb7271 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 30 Jul 2024 09:14:06 -0400 Subject: [PATCH 16/38] fixup: Detailed raw input reporting problems ... and merged similar out-of-scope XXXs into one TODO. --- src/ssl/support.cc | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 2ff21592073..153b5cbb9df 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -87,7 +87,10 @@ Ssl::GeneralNameMatcher::match(const GeneralName &name) const bool Ssl::OneNameMatcher::matchDomainName(const SBuf &rawName) const { - debugs(83, 5, "needle=" << needle_ << " domain=" << rawName); // XXX: rawName may contain non-printable characters + // TODO: Add debugs() stream manipulator to safely (i.e. without breaking + // cache.log message framing) dump raw input that may contain new lines. Use + // here and in similar contexts where we report such raw input. + debugs(83, 5, "needle=" << needle_ << " domain=" << rawName); auto name = rawName; if (name.length() > 0 && name[0] == '*') name.consume(1); @@ -258,11 +261,11 @@ Ssl::GeneralName::ParseAsDomainName(const char * const description, const ASN1_S } if (raw.find('\0') != SBuf::npos) { - debugs(83, 3, "rejects " << description << " with an ASCII NUL character: " << raw); // XXX: Contains non-printable characters! + debugs(83, 3, "rejects " << description << " with an ASCII NUL character: " << raw); return std::nullopt; } - debugs(83, 5, "parsed " << description << ": " << raw); // XXX: May contain non-printable characters + debugs(83, 5, "parsed " << description << ": " << raw); return GeneralName(raw); } From c0cc4684bd3a66a7475e91bd93f2fb4e7607d238 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 30 Jul 2024 11:53:16 -0400 Subject: [PATCH 17/38] fixup: Addressed critical documentation TODO --- src/acl/ServerName.cc | 16 ++++++++++++++-- src/cf.data.pre | 7 +++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/acl/ServerName.cc b/src/acl/ServerName.cc index 287e07b74ee..ea93b89087d 100644 --- a/src/acl/ServerName.cc +++ b/src/acl/ServerName.cc @@ -73,8 +73,20 @@ Acl::ServerNameMatcher::matchDomainName(const SBuf &domain) const bool Acl::ServerNameMatcher::matchIp(const Ip::Address &ip) const { - // This IP address conversion to c-string brackets IPv6 addresses. - // TODO: Document that fact in ssl::server_name. + // We are given an Ip::Address, but our ACL parameters use case-sensitive + // string equality 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 are bracketed, use lowercase hex digits (in + // known modern environments; see inet_ntop link below), and use "::" + // notation (where applicable). + // \sa https://cygwin.com/pipermail/cygwin/2009-March/174197.html + // + // 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.toHostStr(hostStr, sizeof(hostStr)); return parameters.match(hostStr); diff --git a/src/cf.data.pre b/src/cf.data.pre index 62130996f07..83eb55f3586 100644 --- a/src/cf.data.pre +++ b/src/cf.data.pre @@ -1596,6 +1596,13 @@ IF USE_OPENSSL # # Unlike dstdomain, this ACL does not perform DNS lookups. # + # A server name may be an IP address. For example, some real server + # certificate subject names (a.k.a. SANs) include IPv4 and IPv6 entries. + # Internally, Squid adds brackets and uses inet_ntop(3) to prep IPv6 + # names for matching. To match an IPv6 name, surround it with square + # brackets then use lowercase hex digits (if any) and "::" notation (if + # applicable). For example: [1080::8:800:200c:417a]. + # # An ACL option below may be used to restrict what information # sources are used to extract the server names from: # From d051cff06d312fa6282afb6129bf9b4cd4d1a276 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 30 Jul 2024 12:18:26 -0400 Subject: [PATCH 18/38] fixup: Documented another IPv6 handling bug --- src/ssl/support.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 153b5cbb9df..7189b26a0cd 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -101,6 +101,8 @@ bool Ssl::OneNameMatcher::matchIp(const Ip::Address &ip) const { debugs(83, 5, "needle=" << needle_ << " ip=" << ip); // if we cannot convert our needle to an IP, then it does not match any IP + // XXX: Ip::Address::operator "=" cannot parse bracketed IPv6 addresses + // (that our callers get from Uri::host() and other sources)! Ip::Address needleIp; if (!(needleIp = needle_)) { // TODO: Consider caching conversion result in anticipation of matchIp() calls From d77c89f9a35396dc0e63094f3dbd380c8935f588 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 30 Jul 2024 12:43:52 -0400 Subject: [PATCH 19/38] fixup: matchDomainName() is case-insensitive The now-removed branch-added documentation was based on an incorrect assumption about ServerNameCheck Parameters type: For equality matching, we are using case-insensitive matchDomainName() rather than strcmp(). ssl::server_name_regex is still affected, but it is best to recommend -i there instead of complicate admin life with inet_ntop(3) details. --- src/acl/ServerName.cc | 6 ++---- src/cf.data.pre | 8 ++++++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/acl/ServerName.cc b/src/acl/ServerName.cc index ea93b89087d..15229cc1a8f 100644 --- a/src/acl/ServerName.cc +++ b/src/acl/ServerName.cc @@ -78,10 +78,8 @@ Acl::ServerNameMatcher::matchIp(const Ip::Address &ip) const // 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 are bracketed, use lowercase hex digits (in - // known modern environments; see inet_ntop link below), and use "::" - // notation (where applicable). - // \sa https://cygwin.com/pipermail/cygwin/2009-March/174197.html + // produces): IPv6 addresses are bracketed and use "::" notation (where + // applicable). // // Similar problems affect dstdomain ACLs. TODO: Instead of relying on users // reading docs and following their inet_ntop(3) implementation to match diff --git a/src/cf.data.pre b/src/cf.data.pre index 83eb55f3586..8615ccfc0e0 100644 --- a/src/cf.data.pre +++ b/src/cf.data.pre @@ -1600,8 +1600,8 @@ IF USE_OPENSSL # certificate subject names (a.k.a. SANs) include IPv4 and IPv6 entries. # Internally, Squid adds brackets and uses inet_ntop(3) to prep IPv6 # names for matching. To match an IPv6 name, surround it with square - # brackets then use lowercase hex digits (if any) and "::" notation (if - # applicable). For example: [1080::8:800:200c:417a]. + # brackets and use "::" notation (if applicable). For example: + # [1080::8:800:200c:417a]. # # An ACL option below may be used to restrict what information # sources are used to extract the server names from: @@ -1626,6 +1626,10 @@ IF USE_OPENSSL acl aclname ssl::server_name_regex [-i] \.foo\.com ... # regex matches server name obtained from various sources [fast] + # + # See ssl::server_name for details, including IPv6 address formatting + # caveats. Use case-insensitive matching (i.e. -i option) to reduce + # dependency on how Squid formats or sanitizes server names. acl aclname connections_encrypted # matches transactions with all HTTP messages received over TLS From f5f87172b6fd100d6b8ff5d028438b9ae700abd7 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 30 Jul 2024 13:11:23 -0400 Subject: [PATCH 20/38] fixup: Polished branch-added comments --- src/acl/ServerName.cc | 1 + src/security/ErrorDetail.cc | 4 ++-- src/ssl/gadgets.cc | 2 -- src/ssl/support.cc | 1 + src/ssl/support.h | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/acl/ServerName.cc b/src/acl/ServerName.cc index 15229cc1a8f..350d12cf11c 100644 --- a/src/acl/ServerName.cc +++ b/src/acl/ServerName.cc @@ -47,6 +47,7 @@ ACLServerNameData::match(const char *host) namespace Acl { +/// GeneralNameMatcher for matching configured ACL parameters class ServerNameMatcher: public Ssl::GeneralNameMatcher { public: diff --git a/src/security/ErrorDetail.cc b/src/security/ErrorDetail.cc index 886fe39d240..e2184da5d09 100644 --- a/src/security/ErrorDetail.cc +++ b/src/security/ErrorDetail.cc @@ -574,7 +574,7 @@ Security::ErrorDetail::printSubject(std::ostream &os) const } #if USE_OPENSSL -/// a helper class to print CNs extracted using Ssl::findMatchingSubjectName() +/// prints X.509 names extracted using Ssl::findMatchingSubjectName() class CommonNamesPrinter: public Ssl::GeneralNameMatcher { public: @@ -611,7 +611,7 @@ CommonNamesPrinter::matchIp(const Ip::Address &ip) const return false; // keep going } -/// prints commas in front of each item except for the very first one +/// prints a comma in front of each item except for the very first one /// \returns a stream for printing the name std::ostream & CommonNamesPrinter::itemStream() const diff --git a/src/ssl/gadgets.cc b/src/ssl/gadgets.cc index c56d06ed024..526014b158d 100644 --- a/src/ssl/gadgets.cc +++ b/src/ssl/gadgets.cc @@ -486,8 +486,6 @@ addAltNameWithSubjectCn(Security::CertPointer &cert) return false; Ip::Address ipAddress; - // Extract the CN string from cn_data and use it to determine its type - // using Ip::Address::operator "=" const char* cn_cstr = (const char*) ASN1_STRING_get0_data(cn_data); const char* altname_type = (ipAddress = cn_cstr) ? "IP" : "DNS"; char dnsName[1024]; // DNS names are limited to 256 characters diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 7189b26a0cd..ea966b12704 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -58,6 +58,7 @@ std::vector Ssl::BumpModeStr = { namespace Ssl { +/// GeneralNameMatcher for matching a single X.509 name given at construction time class OneNameMatcher: public GeneralNameMatcher { public: diff --git a/src/ssl/support.h b/src/ssl/support.h index 27441123f76..df2c989abb6 100644 --- a/src/ssl/support.h +++ b/src/ssl/support.h @@ -190,7 +190,7 @@ class GeneralName // use a From*() function to create GeneralName objects GeneralName(const Storage &raw): raw_(raw) {} - Storage raw_; /// information we are providing access to + Storage raw_; ///< the name we are providing access to }; /** From 1a5e766fc6aa9ce73a9975df0ebe755b17c50539 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 30 Jul 2024 16:41:04 -0400 Subject: [PATCH 21/38] Added Ip::Address::Parse() to reduce IP parsing problems Using assignment operator as a parsing/testing function is a very bad idea: It goes against developer expectations about assignment operator behavior (i.e. that the assignment assigns and returns the updates object). It forces callers to create dummy Ip::Address objects. It also complicates calling code comprehension (and discovery). --- src/ip/Address.cc | 10 ++++++++++ src/ip/Address.h | 15 +++++++++++++++ src/ssl/gadgets.cc | 3 +-- src/ssl/support.cc | 17 +++++++---------- src/tests/stub_libip.cc | 1 + 5 files changed, 34 insertions(+), 12 deletions(-) diff --git a/src/ip/Address.cc b/src/ip/Address.cc index 62a07cfb9c2..37570298e0f 100644 --- a/src/ip/Address.cc +++ b/src/ip/Address.cc @@ -40,6 +40,16 @@ } printf("\n"); assert(b); \ } +std::optional +Ip::Address::Parse(const char * const raw) +{ + Address tmp; + // TODO: Merge with lookupHostIP() after removing DNS lookups from Ip. + if (tmp.lookupHostIP(raw, false)) + return tmp; + return std::nullopt; +} + int Ip::Address::cidr() const { diff --git a/src/ip/Address.h b/src/ip/Address.h index 49021a77d2a..4486dc50de0 100644 --- a/src/ip/Address.h +++ b/src/ip/Address.h @@ -31,6 +31,8 @@ #include #endif +#include + namespace Ip { @@ -41,6 +43,14 @@ class Address { public: + /// Creates an IP address object by parsing a given c-string. Accepts all + /// three forms of IPv6 addresses from RFC 4291 section 2.2. Examples of + /// valid input: 0, 1.0, 1.2.3.4, FF01::101, and ::FFFF:129.144.52.38. + /// Fails if input contains characters before or after a valid IP address. + /// For example, fails if given a bracketed IPv6 address (e.g., [::1]). + /// \returns std::nullopt if parsing fails + static std::optional
Parse(const char *); + /** @name Constructors */ /*@{*/ Address() { setEmpty(); } @@ -62,6 +72,11 @@ class Address Address& operator =(struct sockaddr_in6 const &s); bool operator =(const struct hostent &s); bool operator =(const struct addrinfo &s); + + /// Interprets the given c-string as an IP address and, upon success, + /// assigns that address. Does nothing if that interpretation fails. + /// \returns whether the assignment was performed + /// \deprecated Use Parse() instead. bool operator =(const char *s); /*@}*/ diff --git a/src/ssl/gadgets.cc b/src/ssl/gadgets.cc index 526014b158d..1cfca8eda40 100644 --- a/src/ssl/gadgets.cc +++ b/src/ssl/gadgets.cc @@ -485,9 +485,8 @@ addAltNameWithSubjectCn(Security::CertPointer &cert) if (!cn_data) return false; - Ip::Address ipAddress; const char* cn_cstr = (const char*) ASN1_STRING_get0_data(cn_data); - const char* altname_type = (ipAddress = cn_cstr) ? "IP" : "DNS"; + const auto altname_type = Ip::Address::Parse(cn_cstr) ? "IP" : "DNS"; char dnsName[1024]; // DNS names are limited to 256 characters const int res = snprintf(dnsName, sizeof(dnsName), "%s:%.*s", altname_type, cn_data->length, cn_data->data); if (res <= 0 || res >= static_cast(sizeof(dnsName))) diff --git a/src/ssl/support.cc b/src/ssl/support.cc index ea966b12704..63e4931f23b 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -101,16 +101,13 @@ Ssl::OneNameMatcher::matchDomainName(const SBuf &rawName) const { bool Ssl::OneNameMatcher::matchIp(const Ip::Address &ip) const { debugs(83, 5, "needle=" << needle_ << " ip=" << ip); - // if we cannot convert our needle to an IP, then it does not match any IP - // XXX: Ip::Address::operator "=" cannot parse bracketed IPv6 addresses - // (that our callers get from Uri::host() and other sources)! - Ip::Address needleIp; - if (!(needleIp = needle_)) { - // TODO: Consider caching conversion result in anticipation of matchIp() calls - debugs(83, 7, "needle is not an IP, cannot match"); - return false; - } - return (needleIp == ip); + // TODO: Consider caching conversion result, anticipating matchIp() calls. + // XXX: Ip::Address cannot parse bracketed IPv6 addresses (that + // Ssl::findSubjectName() callers get from Uri::host() and other sources)! + if (const auto needleIp = Ip::Address::Parse(needle_)) + return (*needleIp == ip); + debugs(83, 7, "needle is not an IP, cannot match"); + return false; } /** diff --git a/src/tests/stub_libip.cc b/src/tests/stub_libip.cc index b46ba075fc2..109dce317b7 100644 --- a/src/tests/stub_libip.cc +++ b/src/tests/stub_libip.cc @@ -13,6 +13,7 @@ #include "tests/STUB.h" #include "ip/Address.h" +std::optional Ip::Address::Parse(const char *) STUB_RETVAL(std::nullopt) Ip::Address::Address(const struct in_addr &) STUB Ip::Address::Address(const struct sockaddr_in &) STUB Ip::Address::Address(const struct in6_addr &) STUB From d47612e5d6968e7d0b56baa1c44f42c965554aa3 Mon Sep 17 00:00:00 2001 From: Tony Walker Date: Wed, 31 Jul 2024 13:41:24 +0100 Subject: [PATCH 22/38] Add my work address to CONTRIBUTORS --- CONTRIBUTORS | 1 + 1 file changed, 1 insertion(+) diff --git a/CONTRIBUTORS b/CONTRIBUTORS index cc104be4e85..be5a2f03440 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -519,6 +519,7 @@ Thank you! Tomas Hozza tomofumi-yoshida <51390036+tomofumi-yoshida@users.noreply.github.com> Tony Lorimer + Tony Walker trapexit Trever Adams Tsantilas Christos From ff58c410bffbec870f52550ba0843f3fe674d253 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 6 Aug 2024 08:59:21 -0400 Subject: [PATCH 23/38] Add AnyP::Host to encapsulate domain-vs-IP URI authority info This commit addresses Ssl::OneNameMatcher::matchIp() XXX. TODO: Refactor to reduce AnyP::Host, Ssl::GeneralName duplication. Also fixed Ip::Address::fromHost() description. --- src/acl/ServerName.cc | 23 +++++++---- src/anyp/Uri.cc | 73 +++++++++++++++++++++++++++++++++++ src/anyp/Uri.h | 69 +++++++++++++++++++++++++++++++++ src/anyp/forward.h | 2 + src/client_side.cc | 8 +++- src/ip/Address.h | 3 +- src/ssl/support.cc | 44 ++++++++++++++------- src/ssl/support.h | 3 +- src/tests/stub_libsslsquid.cc | 2 +- 9 files changed, 199 insertions(+), 28 deletions(-) diff --git a/src/acl/ServerName.cc b/src/acl/ServerName.cc index 350d12cf11c..89c689a310a 100644 --- a/src/acl/ServerName.cc +++ b/src/acl/ServerName.cc @@ -14,6 +14,7 @@ #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" @@ -98,16 +99,19 @@ Acl::ServerNameCheck::match(ACLChecklist * const ch) assert(checklist != nullptr && checklist->request != nullptr); - std::optional serverNameFromConn; + std::optional serverNameFromConn; if (ConnStateData *conn = checklist->conn()) { - std::optional clientRequestedServerName; + std::optional clientRequestedServerName; const auto &clientSni = conn->tlsClientSni(); if (clientSni.isEmpty()) { - const char *host = checklist->request->url.host(); - if (host && *host) // paranoid first condition: host() is never nil - clientRequestedServerName = host; // TODO: Use Uri::hostOrIp() instead - } else - clientRequestedServerName = clientSni; + 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::FromDecodedDomain(clientSni); + } if (useConsensus) { X509 *peer_cert = conn->serverBump() ? conn->serverBump()->serverCert.get() : nullptr; @@ -126,7 +130,10 @@ Acl::ServerNameCheck::match(ACLChecklist * const ch) } } - const auto serverName = serverNameFromConn ? serverNameFromConn->c_str() : "none"; + std::optional printedServerName; + if (serverNameFromConn) + printedServerName = ToSBuf(*serverNameFromConn); // no brackets + const auto serverName = printedServerName ? printedServerName->c_str() : "none"; return data->match(serverName); } diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index 590074491dd..bb8111b345a 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -49,6 +49,72 @@ UserInfoChars() return userInfoValid; } +/* AnyP::Host */ + +std::optional +AnyP::Host::FromIp(const Ip::Address &ip) +{ + // any IP value is acceptable + return Host(ip); +} + +std::optional +AnyP::Host::FromDecodedDomain(const SBuf &rawName) +{ + if (rawName.isEmpty()) { + debugs(23, 3, "rejecting empty name"); + return std::nullopt; + } + return Host(rawName); +} + +std::optional +AnyP::Host::FromUriRegName(const SBuf ®Name) +{ + if (regName.find('%') != SBuf::npos) { + debugs(23, 3, "rejecting percent-encoded reg-name: " << regName); + return std::nullopt; // TODO: Decode() instead + } + return FromDecodedDomain(regName); +} + +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; +} + +/* AnyP::Uri */ + /** * Governed by RFC 3986 section 2.1 */ @@ -133,6 +199,7 @@ AnyP::Uri::host(const char *src) touch(); } +// TODO: Replace with ToSBuf(parsedHost()) or similar. SBuf AnyP::Uri::hostOrIp() const { @@ -144,6 +211,12 @@ AnyP::Uri::hostOrIp() const return SBuf(host()); } +std::optional +AnyP::Uri::parsedHost() const +{ + return hostIsNumeric() ? Host::FromIp(hostIP()) : Host::FromUriRegName(SBuf(host())); +} + const SBuf & AnyP::Uri::path() const { diff --git a/src/anyp/Uri.h b/src/anyp/Uri.h index 81090e63cd8..0fb59c5f98c 100644 --- a/src/anyp/Uri.h +++ b/src/anyp/Uri.h @@ -15,12 +15,77 @@ #include "sbuf/SBuf.h" #include +#include +#include class HttpRequestMethod; namespace AnyP { +/// A non-empty host subcomponent of URI authority (RFC 3986 Section 3.2.2). +class Host +{ +public: + /// converts an already parsed IP address to a Host object + static std::optional FromIp(const Ip::Address &); + + /// parses host as a URI reg-name (which may be percent-encoded UTF-8) + static std::optional FromUriRegName(const SBuf &); + + /// parses host as a literal ASCII domain name (A-labels OK; see RFC 5890) + static std::optional FromDecodedDomain(const SBuf &); + + // Accessor methods below are mutually exclusive: Exactly one method is + // guaranteed to return a result other than std::nullopt. + + /// stored IP address (if any) + auto ip() const { return std::get_if(&raw_); } + + /// stored reg-name (if any) + auto domainName() const { return std::get_if(&raw_); } + +private: + using Storage = std::variant< + /// A domain name (or some other "registered name") after + /// percent-decoding. This string is never empty, but it may + /// contain any character, including ASCII NUL. Currently, + /// Host object creators reject percent-encoded host values + /// instead of decoding them. Host object users should treat + /// domain names as if they are UTF-8 percent-decoded values + /// so that they continue to work correctly when creators + /// start percent-decoding instead of rejecting. + SBuf, + + /// An IPv4 or an IPv6 address. + /// + /// Ip::Address::isNoAddr() may be true for this address. + /// Ip::Address::isAnyAddr() may be true for this address. + Ip::Address>; + + // use a From*() 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 &); + /** * Represents a Uniform Resource Identifier. * Can store both URL or URN representations. @@ -76,6 +141,10 @@ class Uri int hostIsNumeric(void) const {return hostIsNumeric_;} Ip::Address const & hostIP(void) const {return hostAddr_;} + /// Successfully interpreted non-empty host subcomponent of the authority + /// component (if any). XXX: Remove hostOrIp() and print Host instead. + std::optional parsedHost() const; + /// \returns the host subcomponent of the authority component /// If the host is an IPv6 address, returns that IP address with /// [brackets]. See RFC 3986 Section 3.2.2. diff --git a/src/anyp/forward.h b/src/anyp/forward.h index b7f03e6f075..8c81bd71552 100644 --- a/src/anyp/forward.h +++ b/src/anyp/forward.h @@ -17,6 +17,8 @@ namespace AnyP class PortCfg; typedef RefCount PortCfgPointer; +class Bracketed; +class Host; class Uri; class UriScheme; diff --git a/src/client_side.cc b/src/client_side.cc index c02ea98412c..8b0612d347b 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -1484,9 +1484,13 @@ bool ConnStateData::serveDelayedError(Http::Stream *context) // when we can extract the intended name from the bumped HTTP request. if (const Security::CertPointer &srvCert = sslServerBump->serverCert) { HttpRequest *request = http->request; - if (!Ssl::findSubjectName(*srvCert, SBuf(request->url.host()))) { // TODO: Use url.hostOrIp() + const auto host = request->url.parsedHost(); + if (host && Ssl::findSubjectName(*srvCert, *host)) { + debugs(33, 5, "certificate matches requested host: " << *host); + return false; + } else { debugs(33, 2, "SQUID_X509_V_ERR_DOMAIN_MISMATCH: Certificate " << - "does not match domainname " << request->url.host()); + "does not match request target " << RawPointer(host)); bool allowDomainMismatch = false; if (Config.ssl_client.cert_error) { diff --git a/src/ip/Address.h b/src/ip/Address.h index 4486dc50de0..937f3616b53 100644 --- a/src/ip/Address.h +++ b/src/ip/Address.h @@ -248,8 +248,7 @@ class Address unsigned int toHostStr(char *buf, const unsigned int len) const; /// Empties the address and then slowly imports the IP from a possibly - /// [bracketed] portless host. For the semi-reverse operation, see - /// toHostStr() which does export the port. + /// [bracketed] portless host. For the reverse operation, see toHostStr(). /// \returns whether the conversion was successful bool fromHost(const char *hostWithoutPort); diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 63e4931f23b..26b503ba4f8 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -58,19 +58,18 @@ std::vector Ssl::BumpModeStr = { namespace Ssl { -/// GeneralNameMatcher for matching a single X.509 name given at construction time +/// GeneralNameMatcher for matching a single AnyP::Host given at construction time class OneNameMatcher: public GeneralNameMatcher { public: - explicit OneNameMatcher(const SBuf &needle): needleStorage_(needle), needle_(needleStorage_.c_str()) {} + explicit OneNameMatcher(const AnyP::Host &needle): needle_(needle) {} protected: /* GeneralNameMatcher API */ bool matchDomainName(const SBuf &) const override; bool matchIp(const Ip::Address &) const override; - SBuf needleStorage_; ///< a name we are looking for - const char * const needle_; ///< needleStorage_ using matchDomainName()-compatible type + AnyP::Host needle_; ///< a name we are looking for }; } // namespace Ssl @@ -92,21 +91,28 @@ Ssl::OneNameMatcher::matchDomainName(const SBuf &rawName) const { // cache.log message framing) dump raw input that may contain new lines. Use // here and in similar contexts where we report such raw input. debugs(83, 5, "needle=" << needle_ << " domain=" << rawName); + if (needle_.ip()) { + // for example, a 127.0.0.1 IP needle does not match DNS:127.0.0.1 SAN + debugs(83, 7, "needle is an IP; mismatch"); + return false; + } + + Assure(needle_.domainName()); + auto domainNeedle = *needle_.domainName(); + auto name = rawName; if (name.length() > 0 && name[0] == '*') name.consume(1); - return ::matchDomainName(needle_, name.c_str(), mdnRejectSubsubDomains) == 0; + + return ::matchDomainName(domainNeedle.c_str(), name.c_str(), mdnRejectSubsubDomains) == 0; } bool Ssl::OneNameMatcher::matchIp(const Ip::Address &ip) const { debugs(83, 5, "needle=" << needle_ << " ip=" << ip); - // TODO: Consider caching conversion result, anticipating matchIp() calls. - // XXX: Ip::Address cannot parse bracketed IPv6 addresses (that - // Ssl::findSubjectName() callers get from Uri::host() and other sources)! - if (const auto needleIp = Ip::Address::Parse(needle_)) + if (const auto needleIp = needle_.ip()) return (*needleIp == ip); - debugs(83, 7, "needle is not an IP, cannot match"); + debugs(83, 7, "needle is not an IP; mismatch"); return false; } @@ -359,9 +365,9 @@ Ssl::findMatchingSubjectName(X509 &cert, const GeneralNameMatcher &matcher) } bool -Ssl::findSubjectName(X509 &cert, const SBuf &name) +Ssl::findSubjectName(X509 &cert, const AnyP::Host &host) { - return findMatchingSubjectName(cert, OneNameMatcher(name)); + return findMatchingSubjectName(cert, OneNameMatcher(host)); } /// adjusts OpenSSL validation results for each verified certificate in ctx @@ -403,8 +409,18 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) // Check for domain mismatch only if the current certificate is the peer certificate. if (!dont_verify_domain && server && peer_cert.get() == X509_STORE_CTX_get_current_cert(ctx)) { - if (!Ssl::findSubjectName(*peer_cert, *server)) { - debugs(83, 2, "SQUID_X509_V_ERR_DOMAIN_MISMATCH: Certificate " << *peer_cert << " does not match domainname " << server); + // XXX: This code does not know where the server name came from. The + // name may be valid but not compatible with requirements assumed + // and enforced by the AnyP::Host::FromDecodedDomain() call below. + // TODO: Store AnyP::Host (or equivalent) in ssl_ex_index_server. + if (const auto host = AnyP::Host::FromDecodedDomain(*server)) { + if (!Ssl::findSubjectName(*peer_cert, *host)) { + debugs(83, 2, "SQUID_X509_V_ERR_DOMAIN_MISMATCH: Certificate " << *peer_cert << " does not match domainname " << *host); + ok = 0; + error_no = SQUID_X509_V_ERR_DOMAIN_MISMATCH; + } + } else { + debugs(83, 2, "SQUID_X509_V_ERR_DOMAIN_MISMATCH: Cannot check whether certificate " << *peer_cert << " matches malformed domainname " << *server); ok = 0; error_no = SQUID_X509_V_ERR_DOMAIN_MISMATCH; } diff --git a/src/ssl/support.h b/src/ssl/support.h index df2c989abb6..56d02518110 100644 --- a/src/ssl/support.h +++ b/src/ssl/support.h @@ -13,6 +13,7 @@ #if USE_OPENSSL +#include "anyp/forward.h" #include "base/CbDataList.h" #include "base/TypeTraits.h" #include "comm/forward.h" @@ -344,7 +345,7 @@ class GeneralNameMatcher: public Interface bool findMatchingSubjectName(X509 &, const GeneralNameMatcher &); /// whether at least one common or alternate subject name matches the given one -bool findSubjectName(X509 &, const SBuf &name); +bool findSubjectName(X509 &, const AnyP::Host &); /** \ingroup ServerProtocolSSLAPI diff --git a/src/tests/stub_libsslsquid.cc b/src/tests/stub_libsslsquid.cc index 03a65411996..b646b1af586 100644 --- a/src/tests/stub_libsslsquid.cc +++ b/src/tests/stub_libsslsquid.cc @@ -70,7 +70,7 @@ Security::ContextPointer GenerateSslContext(CertificateProperties const &, Secur bool verifySslCertificate(const Security::ContextPointer &, CertificateProperties const &) STUB_RETVAL(false) Security::ContextPointer GenerateSslContextUsingPkeyAndCertFromMemory(const char *, Security::ServerOptions &, bool) STUB_RETVAL(Security::ContextPointer()) bool findMatchingSubjectName(X509 &, const GeneralNameMatcher &) STUB_RETVAL(false) -bool findSubjectName(X509 &, const SBuf &) STUB_RETVAL(false) +bool findSubjectName(X509 &, const AnyP::Host &) STUB_RETVAL(false) int asn1timeToString(ASN1_TIME *, char *, int) STUB_RETVAL(0) void setClientSNI(SSL *, const char *) STUB SBuf GetX509PEM(X509 *) STUB_RETVAL(SBuf()) From 21d5539001f2f868a7cbdb18f0cf14697ea24820 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 6 Aug 2024 12:16:58 -0400 Subject: [PATCH 24/38] fixup: Disassociated Anyp::Host from URI ... as the first step towards reducing AnyP::Host/Ssl::GeneralName code duplication. Also named Anyp::Host "domain name" variant type instead of using generic SBuf. Eventually, DomainName may even become a class, but even this small step already allows us to better document its type requirements and APIs. Also placed Anyp::Host:: variant first because it is simpler and because that variant order matches ip(),domainName() method order. Also moved Anyp::Host into dedicated files. --- src/acl/ServerName.cc | 3 +- src/anyp/Host.cc | 98 +++++++++++++++++++++++++++++++++++++++++++ src/anyp/Host.h | 96 ++++++++++++++++++++++++++++++++++++++++++ src/anyp/Makefile.am | 2 + src/anyp/Uri.cc | 82 +++++++----------------------------- src/anyp/Uri.h | 66 +---------------------------- src/client_side.cc | 1 + src/ssl/support.cc | 7 ++-- 8 files changed, 219 insertions(+), 136 deletions(-) create mode 100644 src/anyp/Host.cc create mode 100644 src/anyp/Host.h diff --git a/src/acl/ServerName.cc b/src/acl/ServerName.cc index 89c689a310a..6ccfe09776c 100644 --- a/src/acl/ServerName.cc +++ b/src/acl/ServerName.cc @@ -11,6 +11,7 @@ #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" @@ -110,7 +111,7 @@ Acl::ServerNameCheck::match(ACLChecklist * const ch) // 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::FromDecodedDomain(clientSni); + clientRequestedServerName = AnyP::Host::FromSimpleDomainName(clientSni); } if (useConsensus) { diff --git a/src/anyp/Host.cc b/src/anyp/Host.cc new file mode 100644 index 00000000000..0e01ac2a85a --- /dev/null +++ b/src/anyp/Host.cc @@ -0,0 +1,98 @@ +/* + * 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 + +std::optional +AnyP::Host::FromIp(const Ip::Address &ip) +{ + // any IP value is acceptable + return Host(ip); +} + +/// common parts of FromSimpleDomain() and FromWildDomain() +std::optional +AnyP::Host::FromDomainName(const SBuf &rawName) +{ + if (rawName.isEmpty()) { + debugs(23, 3, "rejecting empty name"); + return std::nullopt; + } + return Host(rawName); +} + +std::optional +AnyP::Host::FromSimpleDomainName(const SBuf &rawName) +{ + if (rawName.find('*') != SBuf::npos) { + debugs(23, 3, "rejecting wildcard: " << rawName); + return std::nullopt; + } + return FromDomainName(rawName); +} + +std::optional +AnyP::Host::FromWildDomainName(const SBuf &rawName) +{ + const static SBuf wildcardLabel("*."); + if (rawName.startsWith(wildcardLabel)) { + if (rawName.find('*', 2) != SBuf::npos) { + debugs(23, 3, "rejecting excessive wildcards: " << 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: " << rawName); + return std::nullopt; + } + // else: fall through to final checks + } + return FromDomainName(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; +} + + diff --git a/src/anyp/Host.h b/src/anyp/Host.h new file mode 100644 index 00000000000..857d3a1033b --- /dev/null +++ b/src/anyp/Host.h @@ -0,0 +1,96 @@ +/* + * 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 "ip/Address.h" +#include "sbuf/SBuf.h" + +#include +#include +#include + +namespace AnyP +{ + +/// A DNS domain name as described in RFC 1034 and RFC 1035. +/// +/// The object creator is responsible for removing any encodings (e.g., URI +/// percent-encoding) other than ASCII Compatible Encoding (ACE; RFC 5890) prior +/// to creating a DomainName object. Domain names are stored as dot-separated +/// ASCII substrings, with each substring representing a domain name label. +/// DomainName strings are suitable for creating DNS queries and byte-by-byte +/// case-insensitive comparison with configured dstdomain ACL parameters. +/// +/// Even though an empty domain name is valid in DNS, DomainName objects are +/// never empty. +/// +/// The first label of a DomainName object may be a "*" wildcard (RFC 9525 +/// Section 6.3) if and only if the object creator explicitly allows wildcards. +using DomainName = SBuf; + +/// 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 FromIp(const Ip::Address &); + + /// Parses input as a literal ASCII domain name (A-labels OK; see RFC 5890). + /// Does not allow wildcards; \sa FromWildDomainName(). + static std::optional FromSimpleDomainName(const SBuf &); + + /// Same as FromSimpleDomainName() but allows the first label to be a + /// wildcard (RFC 9525 Section 6.3). + static std::optional FromWildDomainName(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(&raw_); } + + /// stored domain name (if any) + auto domainName() const { return std::get_if(&raw_); } + +private: + using Storage = std::variant; + + static std::optional FromDomainName(const SBuf &); + + // use a From*() 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 */ + diff --git a/src/anyp/Makefile.am b/src/anyp/Makefile.am index 62c720e2d5e..14363e01958 100644 --- a/src/anyp/Makefile.am +++ b/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 \ diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index bb8111b345a..c9aca1ac513 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -9,6 +9,7 @@ /* DEBUG: section 23 URL Parsing */ #include "squid.h" +#include "anyp/Host.h" #include "anyp/Uri.h" #include "base/Raw.h" #include "globals.h" @@ -49,72 +50,6 @@ UserInfoChars() return userInfoValid; } -/* AnyP::Host */ - -std::optional -AnyP::Host::FromIp(const Ip::Address &ip) -{ - // any IP value is acceptable - return Host(ip); -} - -std::optional -AnyP::Host::FromDecodedDomain(const SBuf &rawName) -{ - if (rawName.isEmpty()) { - debugs(23, 3, "rejecting empty name"); - return std::nullopt; - } - return Host(rawName); -} - -std::optional -AnyP::Host::FromUriRegName(const SBuf ®Name) -{ - if (regName.find('%') != SBuf::npos) { - debugs(23, 3, "rejecting percent-encoded reg-name: " << regName); - return std::nullopt; // TODO: Decode() instead - } - return FromDecodedDomain(regName); -} - -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; -} - -/* AnyP::Uri */ - /** * Governed by RFC 3986 section 2.1 */ @@ -214,7 +149,20 @@ AnyP::Uri::hostOrIp() const std::optional AnyP::Uri::parsedHost() const { - return hostIsNumeric() ? Host::FromIp(hostIP()) : Host::FromUriRegName(SBuf(host())); + if (hostIsNumeric()) + return Host::FromIp(hostIP()); + + // XXX: Interpret host subcomponent as reg-name representing a DNS name. It + // may actually be, for example, a URN namespace ID (NID; see RFC 8141), but + // current Squid APIs do not support adequate representation of those cases. + const SBuf regName(host()); + + if (regName.find('%') != SBuf::npos) { + debugs(23, 3, "rejecting percent-encoded reg-name: " << regName); + return std::nullopt; // TODO: Decode() instead + } + + return Host::FromSimpleDomainName(regName); } const SBuf & diff --git a/src/anyp/Uri.h b/src/anyp/Uri.h index 0fb59c5f98c..863c9db374b 100644 --- a/src/anyp/Uri.h +++ b/src/anyp/Uri.h @@ -9,83 +9,19 @@ #ifndef SQUID_SRC_ANYP_URI_H #define SQUID_SRC_ANYP_URI_H +#include "anyp/forward.h" #include "anyp/UriScheme.h" #include "ip/Address.h" #include "rfc2181.h" #include "sbuf/SBuf.h" #include -#include -#include class HttpRequestMethod; namespace AnyP { -/// A non-empty host subcomponent of URI authority (RFC 3986 Section 3.2.2). -class Host -{ -public: - /// converts an already parsed IP address to a Host object - static std::optional FromIp(const Ip::Address &); - - /// parses host as a URI reg-name (which may be percent-encoded UTF-8) - static std::optional FromUriRegName(const SBuf &); - - /// parses host as a literal ASCII domain name (A-labels OK; see RFC 5890) - static std::optional FromDecodedDomain(const SBuf &); - - // Accessor methods below are mutually exclusive: Exactly one method is - // guaranteed to return a result other than std::nullopt. - - /// stored IP address (if any) - auto ip() const { return std::get_if(&raw_); } - - /// stored reg-name (if any) - auto domainName() const { return std::get_if(&raw_); } - -private: - using Storage = std::variant< - /// A domain name (or some other "registered name") after - /// percent-decoding. This string is never empty, but it may - /// contain any character, including ASCII NUL. Currently, - /// Host object creators reject percent-encoded host values - /// instead of decoding them. Host object users should treat - /// domain names as if they are UTF-8 percent-decoded values - /// so that they continue to work correctly when creators - /// start percent-decoding instead of rejecting. - SBuf, - - /// An IPv4 or an IPv6 address. - /// - /// Ip::Address::isNoAddr() may be true for this address. - /// Ip::Address::isAnyAddr() may be true for this address. - Ip::Address>; - - // use a From*() 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 &); - /** * Represents a Uniform Resource Identifier. * Can store both URL or URN representations. diff --git a/src/client_side.cc b/src/client_side.cc index 8b0612d347b..7c3b7f318ad 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -59,6 +59,7 @@ #include "squid.h" #include "acl/FilledChecklist.h" +#include "anyp/Host.h" #include "anyp/PortCfg.h" #include "base/AsyncCallbacks.h" #include "base/Subscription.h" diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 26b503ba4f8..1e8b118ee42 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -16,6 +16,7 @@ #if USE_OPENSSL #include "acl/FilledChecklist.h" +#include "anyp/Host.h" #include "anyp/PortCfg.h" #include "anyp/Uri.h" #include "fatal.h" @@ -410,10 +411,10 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) // Check for domain mismatch only if the current certificate is the peer certificate. if (!dont_verify_domain && server && peer_cert.get() == X509_STORE_CTX_get_current_cert(ctx)) { // XXX: This code does not know where the server name came from. The - // name may be valid but not compatible with requirements assumed - // and enforced by the AnyP::Host::FromDecodedDomain() call below. + // name may be valid but not compatible with requirements assumed or + // enforced by the AnyP::Host::FromSimpleDomainName() call below. // TODO: Store AnyP::Host (or equivalent) in ssl_ex_index_server. - if (const auto host = AnyP::Host::FromDecodedDomain(*server)) { + if (const auto host = AnyP::Host::FromSimpleDomainName(*server)) { if (!Ssl::findSubjectName(*peer_cert, *host)) { debugs(83, 2, "SQUID_X509_V_ERR_DOMAIN_MISMATCH: Certificate " << *peer_cert << " does not match domainname " << *host); ok = 0; From d6cfbefd4f372087447c80a959abd3adea3edb65 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 6 Aug 2024 15:25:40 -0400 Subject: [PATCH 25/38] Reuse AnyP::Host for Ssl::GeneralName, addressing earlier TODO --- src/acl/ServerName.cc | 4 +-- src/anyp/forward.h | 3 ++ src/security/ErrorDetail.cc | 4 +-- src/ssl/support.cc | 30 ++++++++++--------- src/ssl/support.h | 57 +++++++++---------------------------- 5 files changed, 36 insertions(+), 62 deletions(-) diff --git a/src/acl/ServerName.cc b/src/acl/ServerName.cc index 6ccfe09776c..7f653fc3a4b 100644 --- a/src/acl/ServerName.cc +++ b/src/acl/ServerName.cc @@ -57,7 +57,7 @@ class ServerNameMatcher: public Ssl::GeneralNameMatcher protected: /* GeneralNameMatcher API */ - bool matchDomainName(const SBuf &) const override; + bool matchDomainName(const AnyP::DomainName &) const override; bool matchIp(const Ip::Address &) const override; private: @@ -68,7 +68,7 @@ class ServerNameMatcher: public Ssl::GeneralNameMatcher } // namespace Acl bool -Acl::ServerNameMatcher::matchDomainName(const SBuf &domain) const +Acl::ServerNameMatcher::matchDomainName(const AnyP::DomainName &domain) const { return parameters.match(SBuf(domain).c_str()); // TODO: Upgrade string-matching ACLs to SBuf } diff --git a/src/anyp/forward.h b/src/anyp/forward.h index 8c81bd71552..b6bbe9d31a4 100644 --- a/src/anyp/forward.h +++ b/src/anyp/forward.h @@ -10,6 +10,7 @@ #define SQUID_SRC_ANYP_FORWARD_H #include "base/RefCount.h" +#include "sbuf/forward.h" namespace AnyP { @@ -22,6 +23,8 @@ class Host; class Uri; class UriScheme; +using DomainName = SBuf; + } // namespace AnyP #endif /* SQUID_SRC_ANYP_FORWARD_H */ diff --git a/src/security/ErrorDetail.cc b/src/security/ErrorDetail.cc index e2184da5d09..427099faf92 100644 --- a/src/security/ErrorDetail.cc +++ b/src/security/ErrorDetail.cc @@ -585,7 +585,7 @@ class CommonNamesPrinter: public Ssl::GeneralNameMatcher protected: /* GeneralNameMatcher API */ - bool matchDomainName(const SBuf &) const override; + bool matchDomainName(const AnyP::DomainName &) const override; bool matchIp(const Ip::Address &) const override; private: @@ -595,7 +595,7 @@ class CommonNamesPrinter: public Ssl::GeneralNameMatcher }; bool -CommonNamesPrinter::matchDomainName(const SBuf &domain) const +CommonNamesPrinter::matchDomainName(const AnyP::DomainName &domain) const { // TODO: Convert html_quote() into an std::ostream manipulator accepting (buf, n). itemStream() << html_quote(SBuf(domain).c_str()); diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 1e8b118ee42..a29c40a240d 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -67,7 +67,7 @@ class OneNameMatcher: public GeneralNameMatcher protected: /* GeneralNameMatcher API */ - bool matchDomainName(const SBuf &) const override; + bool matchDomainName(const AnyP::DomainName &) const override; bool matchIp(const Ip::Address &) const override; AnyP::Host needle_; ///< a name we are looking for @@ -87,7 +87,7 @@ Ssl::GeneralNameMatcher::match(const GeneralName &name) const } bool -Ssl::OneNameMatcher::matchDomainName(const SBuf &rawName) const { +Ssl::OneNameMatcher::matchDomainName(const AnyP::DomainName &rawName) const { // TODO: Add debugs() stream manipulator to safely (i.e. without breaking // cache.log message framing) dump raw input that may contain new lines. Use // here and in similar contexts where we report such raw input. @@ -254,11 +254,13 @@ int Ssl::asn1timeToString(ASN1_TIME *tm, char *buf, int len) return write; } +/// Domain name logic shared by ParseCommonName() and ParseSubjectAltName(): /// Interprets the given raw buffer as a domain name, rejecting names that /// increase risks for legacy Squid code while being unlikely to be used in /// valid certificates that Squid admins want to identify by (rejected) names. -std::optional -Ssl::GeneralName::ParseAsDomainName(const char * const description, const ASN1_STRING &buffer) +/// Supports wildcard domains. +static std::optional +ParseAsDomainName(const char * const description, const ASN1_STRING &buffer) { const SBuf raw(reinterpret_cast(buffer.data), buffer.length); @@ -273,20 +275,20 @@ Ssl::GeneralName::ParseAsDomainName(const char * const description, const ASN1_S } debugs(83, 5, "parsed " << description << ": " << raw); - return GeneralName(raw); + return AnyP::Host::FromWildDomainName(raw); } -std::optional -Ssl::GeneralName::FromCommonName(const ASN1_STRING &buffer) +static std::optional +ParseCommonName(const ASN1_STRING &buffer) { - // Unlike FromSubjectAltName() GENERAL_NAME, our ASN1_STRING cannot tell + // Unlike ParseSubjectAltName() GENERAL_NAME, our ASN1_STRING cannot tell // what kind of info this CN buffer stores. The rest of Squid code treats CN // as a domain name, so we parse/validate the given buffer as such. return ParseAsDomainName("CN", buffer); } -std::optional -Ssl::GeneralName::FromSubjectAltName(const GENERAL_NAME &san) +static std::optional +ParseSubjectAltName(const GENERAL_NAME &san) { switch(san.type) { case GEN_DNS: { @@ -307,7 +309,7 @@ Ssl::GeneralName::FromSubjectAltName(const GENERAL_NAME &san) memcpy(&addr.s_addr, san.d.iPAddress->data, sizeof(addr.s_addr)); const Ip::Address ip(addr); debugs(83, 5, "parsed IPv4: " << ip); - return GeneralName(ip); + return AnyP::Host::FromIp(ip); } if (san.d.iPAddress->length == 16) { @@ -316,7 +318,7 @@ Ssl::GeneralName::FromSubjectAltName(const GENERAL_NAME &san) memcpy(&addr.s6_addr, san.d.iPAddress->data, sizeof(addr.s6_addr)); const Ip::Address ip(addr); debugs(83, 5, "parsed IPv6: " << ip); - return GeneralName(ip); + return AnyP::Host::FromIp(ip); } debugs(83, 3, "unexpected length of an IP address SAN: " << san.d.iPAddress->length); @@ -341,7 +343,7 @@ Ssl::findMatchingSubjectName(X509 &cert, const GeneralNameMatcher &matcher) continue; } - if (const auto cn = GeneralName::FromCommonName(*cn_data)) { + if (const auto cn = ParseCommonName(*cn_data)) { if (matcher.match(*cn)) return true; } @@ -354,7 +356,7 @@ Ssl::findMatchingSubjectName(X509 &cert, const GeneralNameMatcher &matcher) for (int i = 0; i < sanCount; ++i) { const auto rawSan = sk_GENERAL_NAME_value(sans.get(), i); Assure(rawSan); - if (const auto san = GeneralName::FromSubjectAltName(*rawSan)) { + if (const auto san = ParseSubjectAltName(*rawSan)) { if (matcher.match(*san)) return true; } diff --git a/src/ssl/support.h b/src/ssl/support.h index 56d02518110..1be5fa8d842 100644 --- a/src/ssl/support.h +++ b/src/ssl/support.h @@ -150,49 +150,18 @@ typedef std::multimap CertsIndexedList; /// A successfully extracted/parsed certificate "name" field. See RFC 5280 /// GeneralName and X520CommonName types for examples of information sources. -class GeneralName -{ -public: - static std::optional FromCommonName(const ASN1_STRING &); - static std::optional FromSubjectAltName( const GENERAL_NAME &); - - // Accessor methods below are mutually exclusive: Exactly one method is - // guaranteed to return a result other than std::nullopt. - - /// stored IP address (if any) - auto ip() const { return std::get_if(&raw_); } - - /// stored domain name or domain name mask (if any) - auto domainName() const { return std::get_if(&raw_); } - -private: - using Storage = std::variant< - /// A domain name or domain name mask (e.g., *.example.com). This info - /// comes (with very little validation) from a source like these two: - /// * RFC 5280 "dNSName" variant of a subjectAltName extension - /// (GeneralName index is 2, underlying value type is IA5String); - /// * RFC 5280 X520CommonName component of a Subject distinguished name - /// field (underlying value type is DirectoryName). - /// - /// This string is never empty and never contains NUL characters, but it - /// may contain any other character, including non-printable ones. - SBuf, - - /// An IPv4 or an IPv6 address. This info comes (with very little - /// validation) from RFC 5280 "iPAddress" variant of a subjectAltName - /// extension (GeneralName index=7, value type=OCTET STRING). - /// - /// Ip::Address::isNoAddr() may be true for this address. - /// Ip::Address::isAnyAddr() may be true for this address. - Ip::Address>; - - static std::optional ParseAsDomainName(const char *, const ASN1_STRING &); - - // use a From*() function to create GeneralName objects - GeneralName(const Storage &raw): raw_(raw) {} - - Storage raw_; ///< the name we are providing access to -}; +/// For now, we only support the same two name variants as AnyP::Host: +/// +/// * An IPv4 or an IPv6 address. This info comes (with very little validation) +/// from RFC 5280 "iPAddress" variant of a subjectAltName +/// +/// * A domain name or domain name wildcard (e.g., *.example.com). This info +/// comes (with very little validation) from a source like these two: +/// - RFC 5280 "dNSName" variant of a subjectAltName extension (GeneralName +/// index is 2, underlying value type is IA5String); +/// - RFC 5280 X520CommonName component of a Subject distinguished name field +/// (underlying value type is DirectoryName). +using GeneralName = AnyP::Host; /** * Load PEM-encoded certificates from the given file. @@ -336,7 +305,7 @@ class GeneralNameMatcher: public Interface // GeneralName variants. For each public match() method call, exactly one of // these methods is called. - virtual bool matchDomainName(const SBuf &) const = 0; + virtual bool matchDomainName(const AnyP::DomainName &) const = 0; virtual bool matchIp(const Ip::Address &) const = 0; }; From 1cd2bcb32def524e16b366bf3ca3adc248272c77 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Wed, 7 Aug 2024 09:16:55 -0400 Subject: [PATCH 26/38] fixup: Polished comments and marked problems --- src/ip/Address.h | 2 +- src/ssl/gadgets.cc | 8 +++++--- src/ssl/support.cc | 2 ++ 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/ip/Address.h b/src/ip/Address.h index 937f3616b53..8f49bb29cf8 100644 --- a/src/ip/Address.h +++ b/src/ip/Address.h @@ -45,7 +45,7 @@ class Address public: /// Creates an IP address object by parsing a given c-string. Accepts all /// three forms of IPv6 addresses from RFC 4291 section 2.2. Examples of - /// valid input: 0, 1.0, 1.2.3.4, FF01::101, and ::FFFF:129.144.52.38. + /// valid input: 0, 1.0, 1.2.3.4, ff01::101, and ::FFFF:129.144.52.38. /// Fails if input contains characters before or after a valid IP address. /// For example, fails if given a bracketed IPv6 address (e.g., [::1]). /// \returns std::nullopt if parsing fails diff --git a/src/ssl/gadgets.cc b/src/ssl/gadgets.cc index 1cfca8eda40..8aac352ab69 100644 --- a/src/ssl/gadgets.cc +++ b/src/ssl/gadgets.cc @@ -485,10 +485,12 @@ addAltNameWithSubjectCn(Security::CertPointer &cert) if (!cn_data) return false; - const char* cn_cstr = (const char*) ASN1_STRING_get0_data(cn_data); - const auto altname_type = Ip::Address::Parse(cn_cstr) ? "IP" : "DNS"; + // XXX: This code creates an invalid "IP:" SAN. + // TODO: It must create a valid "IP:"" SAN instead. + const auto cn = reinterpret_cast(ASN1_STRING_get0_data(cn_data)); + const auto altNameKind = Ip::Address::Parse(cn) ? "IP" : "DNS"; char dnsName[1024]; // DNS names are limited to 256 characters - const int res = snprintf(dnsName, sizeof(dnsName), "%s:%.*s", altname_type, cn_data->length, cn_data->data); + const auto res = snprintf(dnsName, sizeof(dnsName), "%s:%s", altNameKind, cn); if (res <= 0 || res >= static_cast(sizeof(dnsName))) return false; diff --git a/src/ssl/support.cc b/src/ssl/support.cc index a29c40a240d..c6d99a25cfe 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -284,6 +284,8 @@ ParseCommonName(const ASN1_STRING &buffer) // Unlike ParseSubjectAltName() GENERAL_NAME, our ASN1_STRING cannot tell // what kind of info this CN buffer stores. The rest of Squid code treats CN // as a domain name, so we parse/validate the given buffer as such. + // XXX: "Squid code treats CN as a domain name" assertion is false because + // addAltNameWithSubjectCn() does not do that. TODO: Unify these two functions. return ParseAsDomainName("CN", buffer); } From 9fb154fdbfc6df0707dff6ca1ccedae3f81cfa76 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Wed, 7 Aug 2024 13:00:57 -0400 Subject: [PATCH 27/38] fixup: Addressed XXX re "treats CN as a domain name" ParseCommonNameAt() now uses the same logic as addAltNameWithSubjectCn() -- interpreting CN as an IP address if possible. Moved a few functions into ssl/gadgets.cc because certificate generation helper now needs them because it calls addAltNameWithSubjectCn() via Ssl::generateSslCertificate(). --- src/security/cert_generators/file/Makefile.am | 1 + src/ssl/gadgets.cc | 84 ++++++++++++++++--- src/ssl/gadgets.h | 10 +++ src/ssl/support.cc | 53 ++---------- 4 files changed, 93 insertions(+), 55 deletions(-) diff --git a/src/security/cert_generators/file/Makefile.am b/src/security/cert_generators/file/Makefile.am index 1005b28a877..a94b026dbaf 100644 --- a/src/security/cert_generators/file/Makefile.am +++ b/src/security/cert_generators/file/Makefile.am @@ -30,6 +30,7 @@ security_file_certgen_LDADD = \ $(top_builddir)/src/error/liberror.la \ $(top_builddir)/src/comm/libminimal.la \ $(top_builddir)/src/mem/libminimal.la \ + $(top_builddir)/src/anyp/libanyp.la \ $(top_builddir)/src/base/libbase.la \ $(top_builddir)/src/time/libtime.la \ $(SSLLIB) \ diff --git a/src/ssl/gadgets.cc b/src/ssl/gadgets.cc index 8aac352ab69..cfc5fb85c5e 100644 --- a/src/ssl/gadgets.cc +++ b/src/ssl/gadgets.cc @@ -7,6 +7,7 @@ */ #include "squid.h" +#include "anyp/Host.h" #include "base/IoManip.h" #include "error/SysErrorDetail.h" #include "ip/Address.h" @@ -468,6 +469,74 @@ mimicExtensions(Security::CertPointer & cert, Security::CertPointer const &mimic return added; } +SBuf +Ssl::AsnToSBuf(const ASN1_STRING &buffer) +{ + return SBuf(reinterpret_cast(buffer.data), buffer.length); +} + +std::optional +Ssl::ParseAsWildDomainName(const char * const description, const SBuf &raw) +{ + if (raw.isEmpty()) { + debugs(83, 3, "rejects empty " << description); + return std::nullopt; + } + + if (raw.find('\0') != SBuf::npos) { + debugs(83, 3, "rejects " << description << " with an ASCII NUL character: " << raw); + return std::nullopt; + } + + debugs(83, 5, "parsed " << description << ": " << raw); + return AnyP::Host::FromWildDomainName(raw); +} + +/// OpenSSL ASN1_STRING_to_UTF8() wrapper +static std::optional +ParseAsUtf8(const char * const description, const ASN1_STRING &asnBuffer) +{ + unsigned char *utfBuffer = nullptr; + const auto conversionResult = ASN1_STRING_to_UTF8(&utfBuffer, &asnBuffer); + if (conversionResult < 0) { + debugs(83, 3, description << " to UTF-8 conversion failure" << Ssl::ReportAndForgetErrors); + return std::nullopt; + } + Assure(utfBuffer); + const auto utfChars = reinterpret_cast(utfBuffer); + const auto utfLength = static_cast(conversionResult); + Ssl::UniqueCString bufferDestroyer(utfChars); + return SBuf(utfChars, utfLength); +} + +std::optional +Ssl::ParseCommonNameAt(X509_NAME &name, const int cnIndex) +{ + const auto cn = X509_NAME_ENTRY_get_data(X509_NAME_get_entry(&name, cnIndex)); + if (!cn) { + debugs(83, 7, "no CN at " << cnIndex); + return std::nullopt; + } + + // CN buffer usually contains an ASCII domain name, but X.509 and TLS allow + // other name encodings (e.g., UTF-16), and some CNs are not domain names + // (e.g., organization name or perhaps even a dotted IP address). We do our + // best to identify IP addresses and treat anything else as a domain name. + // TODO: Do not treat CNs with spaces or without periods as domain names. + + // OpenSSL does not offer ASN1_STRING_to_ASCII(), so we convert to UTF-8 + // that usually "works" for further parsing/validation/comparison purposes. + // TODO: Confirm that OpenSSL preserves UTF-8 when we add a "DNS:..." SAN. + + auto utf = ParseAsUtf8("CN", *cn); + if (!utf) + return std::nullopt; + + if (const auto ip = Ip::Address::Parse(utf->c_str())) + return AnyP::Host::FromIp(*ip); + return ParseAsWildDomainName("CN", *utf); +} + /// Adds a new subjectAltName extension contining Subject CN or returns false /// expects the caller to check for the existing subjectAltName extension static bool @@ -481,20 +550,15 @@ addAltNameWithSubjectCn(Security::CertPointer &cert) if (loc < 0) return false; - ASN1_STRING *cn_data = X509_NAME_ENTRY_get_data(X509_NAME_get_entry(name, loc)); - if (!cn_data) + const auto cn = Ssl::ParseCommonNameAt(*name, loc); + if (!cn) return false; // XXX: This code creates an invalid "IP:" SAN. // TODO: It must create a valid "IP:"" SAN instead. - const auto cn = reinterpret_cast(ASN1_STRING_get0_data(cn_data)); - const auto altNameKind = Ip::Address::Parse(cn) ? "IP" : "DNS"; - char dnsName[1024]; // DNS names are limited to 256 characters - const auto res = snprintf(dnsName, sizeof(dnsName), "%s:%s", altNameKind, cn); - if (res <= 0 || res >= static_cast(sizeof(dnsName))) - return false; - - X509_EXTENSION *ext = X509V3_EXT_conf_nid(nullptr, nullptr, NID_subject_alt_name, dnsName); + const auto altNameKind = cn->ip() ? "IP:" : "DNS:"; + auto altName = ToSBuf(altNameKind, *cn); + X509_EXTENSION *ext = X509V3_EXT_conf_nid(nullptr, nullptr, NID_subject_alt_name, altName.c_str()); if (!ext) return false; diff --git a/src/ssl/gadgets.h b/src/ssl/gadgets.h index 72da1781d68..8d83d23bb8d 100644 --- a/src/ssl/gadgets.h +++ b/src/ssl/gadgets.h @@ -11,6 +11,7 @@ #if USE_OPENSSL +#include "anyp/forward.h" #include "base/HardFun.h" #include "compat/openssl.h" #include "security/forward.h" @@ -278,6 +279,15 @@ bool certificateMatchesProperties(X509 *peer_cert, CertificateProperties const & */ const char *CommonHostName(X509 *x509); +/// converts ASN1_STRING to SBuf +SBuf AsnToSBuf(const ASN1_STRING &); + +/// interprets X.509 Subject or Issuer name entry (at the given position) as CN +std::optional ParseCommonNameAt(X509_NAME &, int); + +/// interprets the given X509-derived buffer as a DNS domain name (including wildcards) +std::optional ParseAsWildDomainName(const char *description, const SBuf &); + /** \ingroup ServerProtocolSSLAPI * Returns Organization from the certificate. diff --git a/src/ssl/support.cc b/src/ssl/support.cc index c6d99a25cfe..149f9562f81 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -254,49 +254,16 @@ int Ssl::asn1timeToString(ASN1_TIME *tm, char *buf, int len) return write; } -/// Domain name logic shared by ParseCommonName() and ParseSubjectAltName(): -/// Interprets the given raw buffer as a domain name, rejecting names that -/// increase risks for legacy Squid code while being unlikely to be used in -/// valid certificates that Squid admins want to identify by (rejected) names. -/// Supports wildcard domains. -static std::optional -ParseAsDomainName(const char * const description, const ASN1_STRING &buffer) -{ - const SBuf raw(reinterpret_cast(buffer.data), buffer.length); - - if (raw.isEmpty()) { - debugs(83, 3, "rejects empty " << description); - return std::nullopt; - } - - if (raw.find('\0') != SBuf::npos) { - debugs(83, 3, "rejects " << description << " with an ASCII NUL character: " << raw); - return std::nullopt; - } - - debugs(83, 5, "parsed " << description << ": " << raw); - return AnyP::Host::FromWildDomainName(raw); -} - -static std::optional -ParseCommonName(const ASN1_STRING &buffer) -{ - // Unlike ParseSubjectAltName() GENERAL_NAME, our ASN1_STRING cannot tell - // what kind of info this CN buffer stores. The rest of Squid code treats CN - // as a domain name, so we parse/validate the given buffer as such. - // XXX: "Squid code treats CN as a domain name" assertion is false because - // addAltNameWithSubjectCn() does not do that. TODO: Unify these two functions. - return ParseAsDomainName("CN", buffer); -} - static std::optional ParseSubjectAltName(const GENERAL_NAME &san) { switch(san.type) { case GEN_DNS: { - // san.d.dNSName is OpenSSL ASN1_IA5STRING Assure(san.d.dNSName); - return ParseAsDomainName("DNS name", *san.d.dNSName); + // GEN_DNS is an IA5STRING. IA5STRING is a subset of ASCII that does not + // need to be converted to UTF-8 (or some such) before we parse it. + const auto buffer = Ssl::AsnToSBuf(*san.d.dNSName); + return Ssl::ParseAsWildDomainName("SAN DNS name", buffer); } case GEN_IPADD: { @@ -338,16 +305,12 @@ Ssl::findMatchingSubjectName(X509 &cert, const GeneralNameMatcher &matcher) { const auto name = X509_get_subject_name(&cert); for (int i = X509_NAME_get_index_by_NID(name, NID_commonName, -1); i >= 0; i = X509_NAME_get_index_by_NID(name, NID_commonName, i)) { - - ASN1_STRING *cn_data = X509_NAME_ENTRY_get_data(X509_NAME_get_entry(name, i)); - if (!cn_data) { - debugs(83, 5, "failed to extract CN value: " << Ssl::ReportAndForgetErrors); - continue; - } - - if (const auto cn = ParseCommonName(*cn_data)) { + if (const auto cn = ParseCommonNameAt(*name, i)) { if (matcher.match(*cn)) return true; + } else { + debugs(83, 7, "ignoring unparsable CN at " << i); + continue; } } From 1d58a5bd02aaa0a39459bbe3dc6fa4494abe6a3f Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Wed, 7 Aug 2024 14:42:08 -0400 Subject: [PATCH 28/38] fixup: Fix "CONNECT :443 HTTP/1.1" handling ssl_verify_cb() cannot assume that ssl_ex_index_server stores a domain name. It does store (a textual representation of) an IP address when a request targets an IP address. --- src/ssl/gadgets.cc | 36 +++++++++++++++++++++++++++++------- src/ssl/gadgets.h | 4 ++++ src/ssl/support.cc | 2 +- 3 files changed, 34 insertions(+), 8 deletions(-) diff --git a/src/ssl/gadgets.cc b/src/ssl/gadgets.cc index cfc5fb85c5e..1f6a09c5695 100644 --- a/src/ssl/gadgets.cc +++ b/src/ssl/gadgets.cc @@ -492,6 +492,24 @@ Ssl::ParseAsWildDomainName(const char * const description, const SBuf &raw) return AnyP::Host::FromWildDomainName(raw); } +// XXX: Duplicates most of ParseAsWildDomainName(). TODO: Move both to AnyP::Host. +static std::optional +ParseAsSimpleDomainName(const char * const description, const SBuf &raw) +{ + if (raw.isEmpty()) { + debugs(83, 3, "rejects empty " << description); + return std::nullopt; + } + + if (raw.find('\0') != SBuf::npos) { + debugs(83, 3, "rejects " << description << " with an ASCII NUL character: " << raw); + return std::nullopt; + } + + debugs(83, 5, "parsed " << description << ": " << raw); + return AnyP::Host::FromSimpleDomainName(raw); +} + /// OpenSSL ASN1_STRING_to_UTF8() wrapper static std::optional ParseAsUtf8(const char * const description, const ASN1_STRING &asnBuffer) @@ -509,6 +527,14 @@ ParseAsUtf8(const char * const description, const ASN1_STRING &asnBuffer) return SBuf(utfChars, utfLength); } +std::optional +Ssl::ParseAsSimpleDomainNameOrIp(const char * const description, const SBuf &text) +{ + if (const auto ip = Ip::Address::Parse(SBuf(text).c_str())) + return AnyP::Host::FromIp(*ip); + return ParseAsSimpleDomainName(description, text); +} + std::optional Ssl::ParseCommonNameAt(X509_NAME &name, const int cnIndex) { @@ -528,13 +554,9 @@ Ssl::ParseCommonNameAt(X509_NAME &name, const int cnIndex) // that usually "works" for further parsing/validation/comparison purposes. // TODO: Confirm that OpenSSL preserves UTF-8 when we add a "DNS:..." SAN. - auto utf = ParseAsUtf8("CN", *cn); - if (!utf) - return std::nullopt; - - if (const auto ip = Ip::Address::Parse(utf->c_str())) - return AnyP::Host::FromIp(*ip); - return ParseAsWildDomainName("CN", *utf); + if (const auto utf = ParseAsUtf8("CN", *cn)) + return ParseAsSimpleDomainNameOrIp("CN", *utf); + return std::nullopt; } /// Adds a new subjectAltName extension contining Subject CN or returns false diff --git a/src/ssl/gadgets.h b/src/ssl/gadgets.h index 8d83d23bb8d..a50086c6378 100644 --- a/src/ssl/gadgets.h +++ b/src/ssl/gadgets.h @@ -288,6 +288,10 @@ std::optional ParseCommonNameAt(X509_NAME &, int); /// interprets the given X509-derived buffer as a DNS domain name (including wildcards) std::optional ParseAsWildDomainName(const char *description, const SBuf &); +/// interprets the given buffer as either a textual representation of an IP +/// address (if possible) or a domain name without wildcard support (otherwise) +std::optional ParseAsSimpleDomainNameOrIp(const char *description, const SBuf &); + /** \ingroup ServerProtocolSSLAPI * Returns Organization from the certificate. diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 149f9562f81..6ffd59b0223 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -381,7 +381,7 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) // name may be valid but not compatible with requirements assumed or // enforced by the AnyP::Host::FromSimpleDomainName() call below. // TODO: Store AnyP::Host (or equivalent) in ssl_ex_index_server. - if (const auto host = AnyP::Host::FromSimpleDomainName(*server)) { + if (const auto host = Ssl::ParseAsSimpleDomainNameOrIp("ssl_ex_index_server", *server)) { if (!Ssl::findSubjectName(*peer_cert, *host)) { debugs(83, 2, "SQUID_X509_V_ERR_DOMAIN_MISMATCH: Certificate " << *peer_cert << " does not match domainname " << *host); ok = 0; From a1af8975a06bc8bb0d8b02407a36eac8bc8a881e Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Wed, 7 Aug 2024 17:18:13 -0400 Subject: [PATCH 29/38] fixup: Addressed (invalid) branch-added XXX --- src/ssl/gadgets.cc | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/ssl/gadgets.cc b/src/ssl/gadgets.cc index 1f6a09c5695..a4956bce098 100644 --- a/src/ssl/gadgets.cc +++ b/src/ssl/gadgets.cc @@ -576,11 +576,13 @@ addAltNameWithSubjectCn(Security::CertPointer &cert) if (!cn) return false; - // XXX: This code creates an invalid "IP:" SAN. - // TODO: It must create a valid "IP:"" SAN instead. - const auto altNameKind = cn->ip() ? "IP:" : "DNS:"; - auto altName = ToSBuf(altNameKind, *cn); - X509_EXTENSION *ext = X509V3_EXT_conf_nid(nullptr, nullptr, NID_subject_alt_name, altName.c_str()); + // We create an "IP:address" or "DNS:name" text that X509V3_EXT_conf_nid() + // then parses and converts to OpenSSL GEN_IPADD or GEN_DNS GENERAL_NAME. + // TODO: Use X509_add1_ext_i2d() to add a GENERAL_NAME extension directly: + // https://github.com/openssl/openssl/issues/11706#issuecomment-633180151 + const auto altNamePrefix = cn->ip() ? "IP:" : "DNS:"; + auto altName = ToSBuf(altNamePrefix, *cn); + const auto ext = X509V3_EXT_conf_nid(nullptr, nullptr, NID_subject_alt_name, altName.c_str()); if (!ext) return false; From 432eba0df198c7f87c74f6848e8d2f7d7e65031b Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Thu, 8 Aug 2024 10:30:31 -0400 Subject: [PATCH 30/38] fixup: Addressed branch-added ParseAsWildDomainName() duplication XXX --- src/anyp/Host.cc | 17 +++++++++++++++++ src/ssl/gadgets.cc | 37 +------------------------------------ src/ssl/gadgets.h | 3 --- src/ssl/support.cc | 2 +- 4 files changed, 19 insertions(+), 40 deletions(-) diff --git a/src/anyp/Host.cc b/src/anyp/Host.cc index 0e01ac2a85a..e516377c244 100644 --- a/src/anyp/Host.cc +++ b/src/anyp/Host.cc @@ -15,6 +15,7 @@ std::optional AnyP::Host::FromIp(const Ip::Address &ip) { // any IP value is acceptable + debugs(23, 7, ip); return Host(ip); } @@ -26,6 +27,22 @@ AnyP::Host::FromDomainName(const SBuf &rawName) 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) { + const auto description = "XXX"; + debugs(83, 3, "rejects " << description << " with an ASCII NUL character: " << rawName); + return std::nullopt; + } + + // TODO: Consider rejecting names with isspace(3) bytes. + + debugs(23, 7, rawName); return Host(rawName); } diff --git a/src/ssl/gadgets.cc b/src/ssl/gadgets.cc index a4956bce098..6e8fc60b4dd 100644 --- a/src/ssl/gadgets.cc +++ b/src/ssl/gadgets.cc @@ -475,41 +475,6 @@ Ssl::AsnToSBuf(const ASN1_STRING &buffer) return SBuf(reinterpret_cast(buffer.data), buffer.length); } -std::optional -Ssl::ParseAsWildDomainName(const char * const description, const SBuf &raw) -{ - if (raw.isEmpty()) { - debugs(83, 3, "rejects empty " << description); - return std::nullopt; - } - - if (raw.find('\0') != SBuf::npos) { - debugs(83, 3, "rejects " << description << " with an ASCII NUL character: " << raw); - return std::nullopt; - } - - debugs(83, 5, "parsed " << description << ": " << raw); - return AnyP::Host::FromWildDomainName(raw); -} - -// XXX: Duplicates most of ParseAsWildDomainName(). TODO: Move both to AnyP::Host. -static std::optional -ParseAsSimpleDomainName(const char * const description, const SBuf &raw) -{ - if (raw.isEmpty()) { - debugs(83, 3, "rejects empty " << description); - return std::nullopt; - } - - if (raw.find('\0') != SBuf::npos) { - debugs(83, 3, "rejects " << description << " with an ASCII NUL character: " << raw); - return std::nullopt; - } - - debugs(83, 5, "parsed " << description << ": " << raw); - return AnyP::Host::FromSimpleDomainName(raw); -} - /// OpenSSL ASN1_STRING_to_UTF8() wrapper static std::optional ParseAsUtf8(const char * const description, const ASN1_STRING &asnBuffer) @@ -532,7 +497,7 @@ Ssl::ParseAsSimpleDomainNameOrIp(const char * const description, const SBuf &tex { if (const auto ip = Ip::Address::Parse(SBuf(text).c_str())) return AnyP::Host::FromIp(*ip); - return ParseAsSimpleDomainName(description, text); + return AnyP::Host::FromSimpleDomainName(/* description, */ text); } std::optional diff --git a/src/ssl/gadgets.h b/src/ssl/gadgets.h index a50086c6378..bde8bbc8b87 100644 --- a/src/ssl/gadgets.h +++ b/src/ssl/gadgets.h @@ -285,9 +285,6 @@ SBuf AsnToSBuf(const ASN1_STRING &); /// interprets X.509 Subject or Issuer name entry (at the given position) as CN std::optional ParseCommonNameAt(X509_NAME &, int); -/// interprets the given X509-derived buffer as a DNS domain name (including wildcards) -std::optional ParseAsWildDomainName(const char *description, const SBuf &); - /// interprets the given buffer as either a textual representation of an IP /// address (if possible) or a domain name without wildcard support (otherwise) std::optional ParseAsSimpleDomainNameOrIp(const char *description, const SBuf &); diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 6ffd59b0223..7eaaeb3ff01 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -263,7 +263,7 @@ ParseSubjectAltName(const GENERAL_NAME &san) // GEN_DNS is an IA5STRING. IA5STRING is a subset of ASCII that does not // need to be converted to UTF-8 (or some such) before we parse it. const auto buffer = Ssl::AsnToSBuf(*san.d.dNSName); - return Ssl::ParseAsWildDomainName("SAN DNS name", buffer); + return AnyP::Host::FromWildDomainName(/* XXX "SAN DNS name", */ buffer); } case GEN_IPADD: { From 7e4f16a7289ecdf0dd92ed15d9d486e9658a7127 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Thu, 8 Aug 2024 11:18:36 -0400 Subject: [PATCH 31/38] fixup: Clarified source code comment --- src/ssl/gadgets.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ssl/gadgets.cc b/src/ssl/gadgets.cc index 6e8fc60b4dd..e9cbcddc344 100644 --- a/src/ssl/gadgets.cc +++ b/src/ssl/gadgets.cc @@ -516,7 +516,8 @@ Ssl::ParseCommonNameAt(X509_NAME &name, const int cnIndex) // TODO: Do not treat CNs with spaces or without periods as domain names. // OpenSSL does not offer ASN1_STRING_to_ASCII(), so we convert to UTF-8 - // that usually "works" for further parsing/validation/comparison purposes. + // that usually "works" for further parsing/validation/comparison purposes + // even though Squid code will treat multi-byte characters as char bytes. // TODO: Confirm that OpenSSL preserves UTF-8 when we add a "DNS:..." SAN. if (const auto utf = ParseAsUtf8("CN", *cn)) From 7098ec4ec3d27701beeeee032a61dc91cf3269cd Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Thu, 8 Aug 2024 13:32:21 -0400 Subject: [PATCH 32/38] fixup: Polished parsing method names, API Do not pass high-level description to low-level parsing functions through mid-level parsing functions. This creates too much code noise while still not detailing the parsing context enough. It is better to just report non-obvious context switches and, inside the reported context, final success/failure outcomes instead. This situation is similar to but different from Tokenizer and BinaryTokenizer usage. (Binary)Tokenizer code does benefit from context state and field description parameters because that code does not know a humane-friendly "type" name for the being-parsed field; there is also no other good place to inject debugging in its frequent "parseA() && parseB()" call sequences. In (Binary)Tokenizer code, context/description debugging is essential for understanding what is being parsed. Here, it is more harmful than useful. --- src/acl/ServerName.cc | 2 +- src/anyp/Host.cc | 23 +++++++++++------------ src/anyp/Host.h | 14 +++++++------- src/anyp/Uri.cc | 4 ++-- src/ssl/gadgets.cc | 14 +++++++------- src/ssl/gadgets.h | 2 +- src/ssl/support.cc | 23 +++++++++++------------ 7 files changed, 40 insertions(+), 42 deletions(-) diff --git a/src/acl/ServerName.cc b/src/acl/ServerName.cc index 7f653fc3a4b..c8a17d10e24 100644 --- a/src/acl/ServerName.cc +++ b/src/acl/ServerName.cc @@ -111,7 +111,7 @@ Acl::ServerNameCheck::match(ACLChecklist * const ch) // 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::FromSimpleDomainName(clientSni); + clientRequestedServerName = AnyP::Host::ParseSimpleDomainName(clientSni); } if (useConsensus) { diff --git a/src/anyp/Host.cc b/src/anyp/Host.cc index e516377c244..6975fd71624 100644 --- a/src/anyp/Host.cc +++ b/src/anyp/Host.cc @@ -12,16 +12,16 @@ #include std::optional -AnyP::Host::FromIp(const Ip::Address &ip) +AnyP::Host::ParseIp(const Ip::Address &ip) { - // any IP value is acceptable + // any preparsed IP value is acceptable debugs(23, 7, ip); return Host(ip); } /// common parts of FromSimpleDomain() and FromWildDomain() std::optional -AnyP::Host::FromDomainName(const SBuf &rawName) +AnyP::Host::ParseDomainName(const SBuf &rawName) { if (rawName.isEmpty()) { debugs(23, 3, "rejecting empty name"); @@ -35,8 +35,7 @@ AnyP::Host::FromDomainName(const SBuf &rawName) // check works as an additional (albeit unreliable) layer of defense against // those (unsupported by Squid DNS code) encodings. if (rawName.find('\0') != SBuf::npos) { - const auto description = "XXX"; - debugs(83, 3, "rejects " << description << " with an ASCII NUL character: " << rawName); + debugs(83, 3, "rejecting ASCII NUL character in " << rawName); return std::nullopt; } @@ -47,34 +46,34 @@ AnyP::Host::FromDomainName(const SBuf &rawName) } std::optional -AnyP::Host::FromSimpleDomainName(const SBuf &rawName) +AnyP::Host::ParseSimpleDomainName(const SBuf &rawName) { if (rawName.find('*') != SBuf::npos) { - debugs(23, 3, "rejecting wildcard: " << rawName); + debugs(23, 3, "rejecting wildcard in " << rawName); return std::nullopt; } - return FromDomainName(rawName); + return ParseDomainName(rawName); } std::optional -AnyP::Host::FromWildDomainName(const SBuf &rawName) +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: " << rawName); + 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: " << rawName); + debugs(23, 3, "rejecting unsupported wildcard in " << rawName); return std::nullopt; } // else: fall through to final checks } - return FromDomainName(rawName); + return ParseDomainName(rawName); } std::ostream & diff --git a/src/anyp/Host.h b/src/anyp/Host.h index 857d3a1033b..c611d6b0da4 100644 --- a/src/anyp/Host.h +++ b/src/anyp/Host.h @@ -40,15 +40,15 @@ class Host { public: /// converts an already parsed IP address to a Host object - static std::optional FromIp(const Ip::Address &); + static std::optional ParseIp(const Ip::Address &); /// Parses input as a literal ASCII domain name (A-labels OK; see RFC 5890). - /// Does not allow wildcards; \sa FromWildDomainName(). - static std::optional FromSimpleDomainName(const SBuf &); + /// Does not allow wildcards; \sa ParseWildDomainName(). + static std::optional ParseSimpleDomainName(const SBuf &); - /// Same as FromSimpleDomainName() but allows the first label to be a + /// Same as ParseSimpleDomainName() but allows the first label to be a /// wildcard (RFC 9525 Section 6.3). - static std::optional FromWildDomainName(const SBuf &); + static std::optional ParseWildDomainName(const SBuf &); // Accessor methods below are mutually exclusive: Exactly one method is // guaranteed to return a result other than std::nullopt. @@ -65,9 +65,9 @@ class Host private: using Storage = std::variant; - static std::optional FromDomainName(const SBuf &); + static std::optional ParseDomainName(const SBuf &); - // use a From*() function to create Host objects + // use a Parse*() function to create Host objects Host(const Storage &raw): raw_(raw) {} Storage raw_; ///< the host we are providing access to diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc index c9aca1ac513..a5ab45cd03f 100644 --- a/src/anyp/Uri.cc +++ b/src/anyp/Uri.cc @@ -150,7 +150,7 @@ std::optional AnyP::Uri::parsedHost() const { if (hostIsNumeric()) - return Host::FromIp(hostIP()); + return Host::ParseIp(hostIP()); // XXX: Interpret host subcomponent as reg-name representing a DNS name. It // may actually be, for example, a URN namespace ID (NID; see RFC 8141), but @@ -162,7 +162,7 @@ AnyP::Uri::parsedHost() const return std::nullopt; // TODO: Decode() instead } - return Host::FromSimpleDomainName(regName); + return Host::ParseSimpleDomainName(regName); } const SBuf & diff --git a/src/ssl/gadgets.cc b/src/ssl/gadgets.cc index e9cbcddc344..a86629af4fb 100644 --- a/src/ssl/gadgets.cc +++ b/src/ssl/gadgets.cc @@ -477,12 +477,12 @@ Ssl::AsnToSBuf(const ASN1_STRING &buffer) /// OpenSSL ASN1_STRING_to_UTF8() wrapper static std::optional -ParseAsUtf8(const char * const description, const ASN1_STRING &asnBuffer) +ParseAsUtf8(const ASN1_STRING &asnBuffer) { unsigned char *utfBuffer = nullptr; const auto conversionResult = ASN1_STRING_to_UTF8(&utfBuffer, &asnBuffer); if (conversionResult < 0) { - debugs(83, 3, description << " to UTF-8 conversion failure" << Ssl::ReportAndForgetErrors); + debugs(83, 3, "failed" << Ssl::ReportAndForgetErrors); return std::nullopt; } Assure(utfBuffer); @@ -493,11 +493,11 @@ ParseAsUtf8(const char * const description, const ASN1_STRING &asnBuffer) } std::optional -Ssl::ParseAsSimpleDomainNameOrIp(const char * const description, const SBuf &text) +Ssl::ParseAsSimpleDomainNameOrIp(const SBuf &text) { if (const auto ip = Ip::Address::Parse(SBuf(text).c_str())) - return AnyP::Host::FromIp(*ip); - return AnyP::Host::FromSimpleDomainName(/* description, */ text); + return AnyP::Host::ParseIp(*ip); + return AnyP::Host::ParseSimpleDomainName(text); } std::optional @@ -520,8 +520,8 @@ Ssl::ParseCommonNameAt(X509_NAME &name, const int cnIndex) // even though Squid code will treat multi-byte characters as char bytes. // TODO: Confirm that OpenSSL preserves UTF-8 when we add a "DNS:..." SAN. - if (const auto utf = ParseAsUtf8("CN", *cn)) - return ParseAsSimpleDomainNameOrIp("CN", *utf); + if (const auto utf = ParseAsUtf8(*cn)) + return ParseAsSimpleDomainNameOrIp(*utf); return std::nullopt; } diff --git a/src/ssl/gadgets.h b/src/ssl/gadgets.h index bde8bbc8b87..6411ee11b19 100644 --- a/src/ssl/gadgets.h +++ b/src/ssl/gadgets.h @@ -287,7 +287,7 @@ std::optional ParseCommonNameAt(X509_NAME &, int); /// interprets the given buffer as either a textual representation of an IP /// address (if possible) or a domain name without wildcard support (otherwise) -std::optional ParseAsSimpleDomainNameOrIp(const char *description, const SBuf &); +std::optional ParseAsSimpleDomainNameOrIp(const SBuf &); /** \ingroup ServerProtocolSSLAPI diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 7eaaeb3ff01..3027fccd5f3 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -263,7 +263,7 @@ ParseSubjectAltName(const GENERAL_NAME &san) // GEN_DNS is an IA5STRING. IA5STRING is a subset of ASCII that does not // need to be converted to UTF-8 (or some such) before we parse it. const auto buffer = Ssl::AsnToSBuf(*san.d.dNSName); - return AnyP::Host::FromWildDomainName(/* XXX "SAN DNS name", */ buffer); + return AnyP::Host::ParseWildDomainName(buffer); } case GEN_IPADD: { @@ -277,8 +277,7 @@ ParseSubjectAltName(const GENERAL_NAME &san) static_assert(sizeof(addr.s_addr) == 4); memcpy(&addr.s_addr, san.d.iPAddress->data, sizeof(addr.s_addr)); const Ip::Address ip(addr); - debugs(83, 5, "parsed IPv4: " << ip); - return AnyP::Host::FromIp(ip); + return AnyP::Host::ParseIp(ip); } if (san.d.iPAddress->length == 16) { @@ -286,8 +285,7 @@ ParseSubjectAltName(const GENERAL_NAME &san) static_assert(sizeof(addr.s6_addr) == 16); memcpy(&addr.s6_addr, san.d.iPAddress->data, sizeof(addr.s6_addr)); const Ip::Address ip(addr); - debugs(83, 5, "parsed IPv6: " << ip); - return AnyP::Host::FromIp(ip); + return AnyP::Host::ParseIp(ip); } debugs(83, 3, "unexpected length of an IP address SAN: " << san.d.iPAddress->length); @@ -305,12 +303,10 @@ Ssl::findMatchingSubjectName(X509 &cert, const GeneralNameMatcher &matcher) { const auto name = X509_get_subject_name(&cert); for (int i = X509_NAME_get_index_by_NID(name, NID_commonName, -1); i >= 0; i = X509_NAME_get_index_by_NID(name, NID_commonName, i)) { + debugs(83, 7, "checking CN at " << i); if (const auto cn = ParseCommonNameAt(*name, i)) { if (matcher.match(*cn)) return true; - } else { - debugs(83, 7, "ignoring unparsable CN at " << i); - continue; } } @@ -319,6 +315,7 @@ Ssl::findMatchingSubjectName(X509 &cert, const GeneralNameMatcher &matcher) if (sans) { const auto sanCount = sk_GENERAL_NAME_num(sans.get()); for (int i = 0; i < sanCount; ++i) { + debugs(83, 7, "checking SAN at " << i); const auto rawSan = sk_GENERAL_NAME_value(sans.get(), i); Assure(rawSan); if (const auto san = ParseSubjectAltName(*rawSan)) { @@ -379,16 +376,18 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) if (!dont_verify_domain && server && peer_cert.get() == X509_STORE_CTX_get_current_cert(ctx)) { // XXX: This code does not know where the server name came from. The // name may be valid but not compatible with requirements assumed or - // enforced by the AnyP::Host::FromSimpleDomainName() call below. + // enforced by the AnyP::Host::ParseSimpleDomainName() call below. // TODO: Store AnyP::Host (or equivalent) in ssl_ex_index_server. - if (const auto host = Ssl::ParseAsSimpleDomainNameOrIp("ssl_ex_index_server", *server)) { - if (!Ssl::findSubjectName(*peer_cert, *host)) { + if (const auto host = Ssl::ParseAsSimpleDomainNameOrIp(*server)) { + if (Ssl::findSubjectName(*peer_cert, *host)) { + debugs(83, 5, "certificate subject matches " << *host); + } else { debugs(83, 2, "SQUID_X509_V_ERR_DOMAIN_MISMATCH: Certificate " << *peer_cert << " does not match domainname " << *host); ok = 0; error_no = SQUID_X509_V_ERR_DOMAIN_MISMATCH; } } else { - debugs(83, 2, "SQUID_X509_V_ERR_DOMAIN_MISMATCH: Cannot check whether certificate " << *peer_cert << " matches malformed domainname " << *server); + debugs(83, 2, "SQUID_X509_V_ERR_DOMAIN_MISMATCH: Cannot check whether certificate " << *peer_cert << " subject matches malformed domainname " << *server); ok = 0; error_no = SQUID_X509_V_ERR_DOMAIN_MISMATCH; } From bc9c6b9dc17bccbd285c23474fe492ca96467047 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Thu, 8 Aug 2024 14:55:16 -0400 Subject: [PATCH 33/38] Do not bracket IPv6 addresses when matching server_name parameters * Our existing src ACL examples do not use brackets. * Brackets are HTTP/authority-specific port-protecting delimiter. It is conceptually wrong to force admins to use them in other contexts where there is no need for these delimiters (and there is a need for other delimiters like spaces). Also fixed "ACL parameters use case-sensitive string equality" lie missed in branch commit d77c89f9. --- src/acl/ServerName.cc | 18 +++++++++--------- src/cf.data.pre | 11 +++++------ src/security/ErrorDetail.cc | 6 +++--- 3 files changed, 17 insertions(+), 18 deletions(-) diff --git a/src/acl/ServerName.cc b/src/acl/ServerName.cc index c8a17d10e24..65d899956a2 100644 --- a/src/acl/ServerName.cc +++ b/src/acl/ServerName.cc @@ -76,20 +76,20 @@ Acl::ServerNameMatcher::matchDomainName(const AnyP::DomainName &domain) const bool Acl::ServerNameMatcher::matchIp(const Ip::Address &ip) const { - // We are given an Ip::Address, but our ACL parameters use case-sensitive - // string equality 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 are bracketed and use "::" notation (where - // applicable). + // 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.toHostStr(hostStr, sizeof(hostStr)); + char hostStr[MAX_IPSTRLEN]; + (void)ip.toStr(hostStr, sizeof(hostStr)); // no brackets return parameters.match(hostStr); } diff --git a/src/cf.data.pre b/src/cf.data.pre index 8615ccfc0e0..4be911f03c4 100644 --- a/src/cf.data.pre +++ b/src/cf.data.pre @@ -1596,12 +1596,11 @@ IF USE_OPENSSL # # Unlike dstdomain, this ACL does not perform DNS lookups. # - # A server name may be an IP address. For example, some real server - # certificate subject names (a.k.a. SANs) include IPv4 and IPv6 entries. - # Internally, Squid adds brackets and uses inet_ntop(3) to prep IPv6 - # names for matching. To match an IPv6 name, surround it with square - # brackets and use "::" notation (if applicable). For example: - # [1080::8:800:200c:417a]. + # A server name may be an IP address. For example, subject alternative + # names (a.k.a. SANs) in some real server certificates include IPv4 and + # IPv6 entries. Internally, Squid uses inet_ntop(3) to prep IP names for + # matching. When using IPv6 names, use "::" notation (if applicable). + # Do not use brackets. For example: 1080::8:800:200c:417a. # # An ACL option below may be used to restrict what information # sources are used to extract the server names from: diff --git a/src/security/ErrorDetail.cc b/src/security/ErrorDetail.cc index 427099faf92..f7d938cf3ec 100644 --- a/src/security/ErrorDetail.cc +++ b/src/security/ErrorDetail.cc @@ -605,9 +605,9 @@ CommonNamesPrinter::matchDomainName(const AnyP::DomainName &domain) const bool CommonNamesPrinter::matchIp(const Ip::Address &ip) const { - char hostStr[MAX_IPSTRLEN] = ""; - const auto length = ip.toHostStr(hostStr, sizeof(hostStr)); // no html_quote() is needed - itemStream().write(hostStr, length); + char hostStr[MAX_IPSTRLEN]; + (void)ip.toStr(hostStr, sizeof(hostStr)); // no html_quote() is needed; no brackets + itemStream().write(hostStr, strlen(hostStr)); return false; // keep going } From cdc1ca38ec254487181cfd2a6b9d18b729325369 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Thu, 8 Aug 2024 23:31:52 -0400 Subject: [PATCH 34/38] fixup: Fixed DomainName namespace --- src/acl/ServerName.cc | 4 ++-- src/anyp/Host.h | 19 ++----------------- src/anyp/forward.h | 3 --- src/dns/forward.h | 17 +++++++++++++++++ src/security/ErrorDetail.cc | 4 ++-- src/ssl/support.cc | 4 ++-- src/ssl/support.h | 3 ++- 7 files changed, 27 insertions(+), 27 deletions(-) diff --git a/src/acl/ServerName.cc b/src/acl/ServerName.cc index 65d899956a2..ad315162db5 100644 --- a/src/acl/ServerName.cc +++ b/src/acl/ServerName.cc @@ -57,7 +57,7 @@ class ServerNameMatcher: public Ssl::GeneralNameMatcher protected: /* GeneralNameMatcher API */ - bool matchDomainName(const AnyP::DomainName &) const override; + bool matchDomainName(const Dns::DomainName &) const override; bool matchIp(const Ip::Address &) const override; private: @@ -68,7 +68,7 @@ class ServerNameMatcher: public Ssl::GeneralNameMatcher } // namespace Acl bool -Acl::ServerNameMatcher::matchDomainName(const AnyP::DomainName &domain) const +Acl::ServerNameMatcher::matchDomainName(const Dns::DomainName &domain) const { return parameters.match(SBuf(domain).c_str()); // TODO: Upgrade string-matching ACLs to SBuf } diff --git a/src/anyp/Host.h b/src/anyp/Host.h index c611d6b0da4..b3f5018db7d 100644 --- a/src/anyp/Host.h +++ b/src/anyp/Host.h @@ -9,6 +9,7 @@ #ifndef SQUID_SRC_ANYP_HOST_H #define SQUID_SRC_ANYP_HOST_H +#include "dns/forward.h" #include "ip/Address.h" #include "sbuf/SBuf.h" @@ -19,22 +20,6 @@ namespace AnyP { -/// A DNS domain name as described in RFC 1034 and RFC 1035. -/// -/// The object creator is responsible for removing any encodings (e.g., URI -/// percent-encoding) other than ASCII Compatible Encoding (ACE; RFC 5890) prior -/// to creating a DomainName object. Domain names are stored as dot-separated -/// ASCII substrings, with each substring representing a domain name label. -/// DomainName strings are suitable for creating DNS queries and byte-by-byte -/// case-insensitive comparison with configured dstdomain ACL parameters. -/// -/// Even though an empty domain name is valid in DNS, DomainName objects are -/// never empty. -/// -/// The first label of a DomainName object may be a "*" wildcard (RFC 9525 -/// Section 6.3) if and only if the object creator explicitly allows wildcards. -using DomainName = SBuf; - /// either a domain name (as defined in DNS RFC 1034) or an IP address class Host { @@ -63,7 +48,7 @@ class Host auto domainName() const { return std::get_if(&raw_); } private: - using Storage = std::variant; + using Storage = std::variant; static std::optional ParseDomainName(const SBuf &); diff --git a/src/anyp/forward.h b/src/anyp/forward.h index b6bbe9d31a4..8c81bd71552 100644 --- a/src/anyp/forward.h +++ b/src/anyp/forward.h @@ -10,7 +10,6 @@ #define SQUID_SRC_ANYP_FORWARD_H #include "base/RefCount.h" -#include "sbuf/forward.h" namespace AnyP { @@ -23,8 +22,6 @@ class Host; class Uri; class UriScheme; -using DomainName = SBuf; - } // namespace AnyP #endif /* SQUID_SRC_ANYP_FORWARD_H */ diff --git a/src/dns/forward.h b/src/dns/forward.h index 0a0f57f804c..1ebbb8a59c3 100644 --- a/src/dns/forward.h +++ b/src/dns/forward.h @@ -10,6 +10,7 @@ #define SQUID_SRC_DNS_FORWARD_H #include "ip/forward.h" +#include "sbuf/forward.h" class rfc1035_rr; @@ -23,6 +24,22 @@ class LookupDetails; void Init(void); +/// A DNS domain name as described in RFC 1034 and RFC 1035. +/// +/// The object creator is responsible for removing any encodings (e.g., URI +/// percent-encoding) other than ASCII Compatible Encoding (ACE; RFC 5890) prior +/// to creating a DomainName object. Domain names are stored as dot-separated +/// ASCII substrings, with each substring representing a domain name label. +/// DomainName strings are suitable for creating DNS queries and byte-by-byte +/// case-insensitive comparison with configured dstdomain ACL parameters. +/// +/// Even though an empty domain name is valid in DNS, DomainName objects are +/// never empty. +/// +/// The first label of a DomainName object may be a "*" wildcard (RFC 9525 +/// Section 6.3) if and only if the object creator explicitly allows wildcards. +using DomainName = SBuf; + } // namespace Dns // internal DNS client API diff --git a/src/security/ErrorDetail.cc b/src/security/ErrorDetail.cc index f7d938cf3ec..c444da908a6 100644 --- a/src/security/ErrorDetail.cc +++ b/src/security/ErrorDetail.cc @@ -585,7 +585,7 @@ class CommonNamesPrinter: public Ssl::GeneralNameMatcher protected: /* GeneralNameMatcher API */ - bool matchDomainName(const AnyP::DomainName &) const override; + bool matchDomainName(const Dns::DomainName &) const override; bool matchIp(const Ip::Address &) const override; private: @@ -595,7 +595,7 @@ class CommonNamesPrinter: public Ssl::GeneralNameMatcher }; bool -CommonNamesPrinter::matchDomainName(const AnyP::DomainName &domain) const +CommonNamesPrinter::matchDomainName(const Dns::DomainName &domain) const { // TODO: Convert html_quote() into an std::ostream manipulator accepting (buf, n). itemStream() << html_quote(SBuf(domain).c_str()); diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 3027fccd5f3..da939862266 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -67,7 +67,7 @@ class OneNameMatcher: public GeneralNameMatcher protected: /* GeneralNameMatcher API */ - bool matchDomainName(const AnyP::DomainName &) const override; + bool matchDomainName(const Dns::DomainName &) const override; bool matchIp(const Ip::Address &) const override; AnyP::Host needle_; ///< a name we are looking for @@ -87,7 +87,7 @@ Ssl::GeneralNameMatcher::match(const GeneralName &name) const } bool -Ssl::OneNameMatcher::matchDomainName(const AnyP::DomainName &rawName) const { +Ssl::OneNameMatcher::matchDomainName(const Dns::DomainName &rawName) const { // TODO: Add debugs() stream manipulator to safely (i.e. without breaking // cache.log message framing) dump raw input that may contain new lines. Use // here and in similar contexts where we report such raw input. diff --git a/src/ssl/support.h b/src/ssl/support.h index 1be5fa8d842..7c6ee7cbc51 100644 --- a/src/ssl/support.h +++ b/src/ssl/support.h @@ -18,6 +18,7 @@ #include "base/TypeTraits.h" #include "comm/forward.h" #include "compat/openssl.h" +#include "dns/forward.h" #include "ip/Address.h" #include "sbuf/SBuf.h" #include "security/Session.h" @@ -305,7 +306,7 @@ class GeneralNameMatcher: public Interface // GeneralName variants. For each public match() method call, exactly one of // these methods is called. - virtual bool matchDomainName(const AnyP::DomainName &) const = 0; + virtual bool matchDomainName(const Dns::DomainName &) const = 0; virtual bool matchIp(const Ip::Address &) const = 0; }; From 0d5e877838f925a7b0bc23a1f5d6d181bb9a31ef Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Thu, 8 Aug 2024 23:32:32 -0400 Subject: [PATCH 35/38] fixup: formatted modified sources --- src/anyp/Host.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/anyp/Host.cc b/src/anyp/Host.cc index 6975fd71624..ed1549981c5 100644 --- a/src/anyp/Host.cc +++ b/src/anyp/Host.cc @@ -111,4 +111,3 @@ AnyP::operator <<(std::ostream &os, const Bracketed &hostWrapper) return os; } - From 12234514795ccddc347bd8a4a539bdd2ab41dd5e Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 9 Aug 2024 14:06:15 -0400 Subject: [PATCH 36/38] fixup: Fix build on some platforms (missing header) error: `optional` in namespace `std` does not name a template type --- src/ssl/gadgets.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/ssl/gadgets.h b/src/ssl/gadgets.h index 6411ee11b19..06595040092 100644 --- a/src/ssl/gadgets.h +++ b/src/ssl/gadgets.h @@ -17,6 +17,7 @@ #include "security/forward.h" #include "ssl/crtd_message.h" +#include #include #if HAVE_OPENSSL_ASN1_H From 2ea18f7e99d8d0e96ea53a92dd423b48eb7ddcd4 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 9 Aug 2024 16:53:06 -0400 Subject: [PATCH 37/38] fixup: Fix "make distcheck" in CodeQL-tests (missing header) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit src/ssl/gadgets.h:291:61: error: ‘SBuf’ does not name a type --- src/ssl/gadgets.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/ssl/gadgets.h b/src/ssl/gadgets.h index 06595040092..db5e52d6d4c 100644 --- a/src/ssl/gadgets.h +++ b/src/ssl/gadgets.h @@ -14,6 +14,7 @@ #include "anyp/forward.h" #include "base/HardFun.h" #include "compat/openssl.h" +#include "sbuf/forward.h" #include "security/forward.h" #include "ssl/crtd_message.h" From a8e992a0eda1293d01fb82adbaf64b8512a19f19 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sat, 16 Nov 2024 16:49:43 -0500 Subject: [PATCH 38/38] fixup: Better names for new functions --- src/acl/ServerName.cc | 4 ++-- src/client_side.cc | 2 +- src/security/ErrorDetail.cc | 4 ++-- src/ssl/support.cc | 8 ++++---- src/ssl/support.h | 4 ++-- src/tests/stub_libsslsquid.cc | 4 ++-- 6 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/acl/ServerName.cc b/src/acl/ServerName.cc index ad315162db5..0659a096603 100644 --- a/src/acl/ServerName.cc +++ b/src/acl/ServerName.cc @@ -119,13 +119,13 @@ Acl::ServerNameCheck::match(ACLChecklist * const ch) // use the client requested name if it matches the server // certificate or if the certificate is not available if (!peer_cert || !clientRequestedServerName || - Ssl::findSubjectName(*peer_cert, *clientRequestedServerName)) + Ssl::HasSubjectName(*peer_cert, *clientRequestedServerName)) serverNameFromConn = clientRequestedServerName; } else if (useClientRequested) serverNameFromConn = clientRequestedServerName; else { // either no options or useServerProvided if (X509 *peer_cert = (conn->serverBump() ? conn->serverBump()->serverCert.get() : nullptr)) - return Ssl::findMatchingSubjectName(*peer_cert, ServerNameMatcher(*data)); + return Ssl::HasMatchingSubjectName(*peer_cert, ServerNameMatcher(*data)); if (!useServerProvided) serverNameFromConn = clientRequestedServerName; } diff --git a/src/client_side.cc b/src/client_side.cc index 7c3b7f318ad..0ff140fc03f 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -1486,7 +1486,7 @@ bool ConnStateData::serveDelayedError(Http::Stream *context) if (const Security::CertPointer &srvCert = sslServerBump->serverCert) { HttpRequest *request = http->request; const auto host = request->url.parsedHost(); - if (host && Ssl::findSubjectName(*srvCert, *host)) { + if (host && Ssl::HasSubjectName(*srvCert, *host)) { debugs(33, 5, "certificate matches requested host: " << *host); return false; } else { diff --git a/src/security/ErrorDetail.cc b/src/security/ErrorDetail.cc index c444da908a6..25c03e93488 100644 --- a/src/security/ErrorDetail.cc +++ b/src/security/ErrorDetail.cc @@ -574,7 +574,7 @@ Security::ErrorDetail::printSubject(std::ostream &os) const } #if USE_OPENSSL -/// prints X.509 names extracted using Ssl::findMatchingSubjectName() +/// prints X.509 names extracted using Ssl::HasMatchingSubjectName() class CommonNamesPrinter: public Ssl::GeneralNameMatcher { public: @@ -630,7 +630,7 @@ Security::ErrorDetail::printCommonName(std::ostream &os) const #if USE_OPENSSL if (broken_cert.get()) { CommonNamesPrinter printer(os); - (void)Ssl::findMatchingSubjectName(*broken_cert, printer); + (void)Ssl::HasMatchingSubjectName(*broken_cert, printer); if (printer.printed) return; } diff --git a/src/ssl/support.cc b/src/ssl/support.cc index da939862266..9ef0af8928a 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -299,7 +299,7 @@ ParseSubjectAltName(const GENERAL_NAME &san) } bool -Ssl::findMatchingSubjectName(X509 &cert, const GeneralNameMatcher &matcher) +Ssl::HasMatchingSubjectName(X509 &cert, const GeneralNameMatcher &matcher) { const auto name = X509_get_subject_name(&cert); for (int i = X509_NAME_get_index_by_NID(name, NID_commonName, -1); i >= 0; i = X509_NAME_get_index_by_NID(name, NID_commonName, i)) { @@ -330,9 +330,9 @@ Ssl::findMatchingSubjectName(X509 &cert, const GeneralNameMatcher &matcher) } bool -Ssl::findSubjectName(X509 &cert, const AnyP::Host &host) +Ssl::HasSubjectName(X509 &cert, const AnyP::Host &host) { - return findMatchingSubjectName(cert, OneNameMatcher(host)); + return HasMatchingSubjectName(cert, OneNameMatcher(host)); } /// adjusts OpenSSL validation results for each verified certificate in ctx @@ -379,7 +379,7 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) // enforced by the AnyP::Host::ParseSimpleDomainName() call below. // TODO: Store AnyP::Host (or equivalent) in ssl_ex_index_server. if (const auto host = Ssl::ParseAsSimpleDomainNameOrIp(*server)) { - if (Ssl::findSubjectName(*peer_cert, *host)) { + if (Ssl::HasSubjectName(*peer_cert, *host)) { debugs(83, 5, "certificate subject matches " << *host); } else { debugs(83, 2, "SQUID_X509_V_ERR_DOMAIN_MISMATCH: Certificate " << *peer_cert << " does not match domainname " << *host); diff --git a/src/ssl/support.h b/src/ssl/support.h index 7c6ee7cbc51..18d29ab241e 100644 --- a/src/ssl/support.h +++ b/src/ssl/support.h @@ -312,10 +312,10 @@ class GeneralNameMatcher: public Interface /// Determines whether at least one common or alternate subject names matches. /// The first match (if any) terminates the search. -bool findMatchingSubjectName(X509 &, const GeneralNameMatcher &); +bool HasMatchingSubjectName(X509 &, const GeneralNameMatcher &); /// whether at least one common or alternate subject name matches the given one -bool findSubjectName(X509 &, const AnyP::Host &); +bool HasSubjectName(X509 &, const AnyP::Host &); /** \ingroup ServerProtocolSSLAPI diff --git a/src/tests/stub_libsslsquid.cc b/src/tests/stub_libsslsquid.cc index b646b1af586..a3cd170e6b2 100644 --- a/src/tests/stub_libsslsquid.cc +++ b/src/tests/stub_libsslsquid.cc @@ -69,8 +69,8 @@ bool generateUntrustedCert(Security::CertPointer &, Security::PrivateKeyPointer Security::ContextPointer GenerateSslContext(CertificateProperties const &, Security::ServerOptions &, bool) STUB_RETVAL(Security::ContextPointer()) bool verifySslCertificate(const Security::ContextPointer &, CertificateProperties const &) STUB_RETVAL(false) Security::ContextPointer GenerateSslContextUsingPkeyAndCertFromMemory(const char *, Security::ServerOptions &, bool) STUB_RETVAL(Security::ContextPointer()) -bool findMatchingSubjectName(X509 &, const GeneralNameMatcher &) STUB_RETVAL(false) -bool findSubjectName(X509 &, const AnyP::Host &) STUB_RETVAL(false) +bool HasMatchingSubjectName(X509 &, const GeneralNameMatcher &) STUB_RETVAL(false) +bool HasSubjectName(X509 &, const AnyP::Host &) STUB_RETVAL(false) int asn1timeToString(ASN1_TIME *, char *, int) STUB_RETVAL(0) void setClientSNI(SSL *, const char *) STUB SBuf GetX509PEM(X509 *) STUB_RETVAL(SBuf())