Skip to content

Commit

Permalink
Bug 5363: Handle IP-based X.509 SANs better (squid-cache#1793)
Browse files Browse the repository at this point in the history
Most X.509 Subject Alternate Name extensions encountered by Squid are
based on DNS domain names. However, real-world servers (including
publicly available servers that use vanity IP addresses) also use
IP-based SANs. Squid mishandled IP-based SANs in several ways:

* When generating certificates for servers targeted by their IP
  addresses, addAltNameWithSubjectCn() used that target IP as a
  DNS-based SAN, resulting in a frankenstein DNS:[ip] SAN value that
  clients ignored when validating a Squid-generated certificate.

* When validating a received certificate, Squid was ignoring IP-based
  SANs. When Subject CN did not match the requested IP target, Squid
  only looked at DNS-based SANs, incorrectly failing validation.

* When checking certificate-related ACLs like ssl::server_name,
  matchX509CommonNames() ignored IP-based SANs, not matching
  certificates containing ACL-listed IP addresses.

Squid now recognizes and generates IP-based SANs.

Squid now attempts to match IP-based SANs with ACL-listed IP addresses,
but the success of that attempt depends on whether ACL IP parameters are
formatted the same way inet_ntop(3) formats those IP addresses: Matching
is still done using c-string/domain-based ::matchDomainName() (for
ssl::server_name) and string-based regexes (for ssl::server_name_regex).
Similar problems affect dstdomain and dstdomain_regex ACLs. A dedicated
fix is needed to stop treating IPs as domain names in those contexts.

This change introduces partial support for preserving IP-vs-domain
distinction in parsed/internal Squid state rather than converting both
to a string and then assuming that string is a DNS domain name.
  • Loading branch information
walkert authored and squid-anubis committed Nov 16, 2024
1 parent c8ea6a3 commit 22b2a7a
Show file tree
Hide file tree
Showing 21 changed files with 649 additions and 148 deletions.
1 change: 1 addition & 0 deletions CONTRIBUTORS
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,7 @@ Thank you!
Tomas Hozza <[email protected]>
tomofumi-yoshida <[email protected]>
Tony Lorimer <[email protected]>
Tony Walker <[email protected]>
trapexit <[email protected]>
Trever Adams <[email protected]>
Tsantilas Christos <[email protected]>
Expand Down
109 changes: 67 additions & 42 deletions src/acl/ServerName.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@
#include "squid.h"
#include "acl/FilledChecklist.h"
#include "acl/ServerName.h"
#include "anyp/Host.h"
#include "client_side.h"
#include "http/Stream.h"
#include "HttpRequest.h"
#include "sbuf/Stream.h"
#include "ssl/bio.h"
#include "ssl/ServerBump.h"
#include "ssl/support.h"
Expand Down Expand Up @@ -45,31 +47,50 @@ ACLServerNameData::match(const char *host)

}

/// A helper function to be used with Ssl::matchX509CommonNames().
/// \retval 0 when the name (cn or an alternate name) matches acl data
/// \retval 1 when the name does not match
template<class MatchType>
int
check_cert_domain( void *check_data, ASN1_STRING *cn_data)
namespace Acl {

/// GeneralNameMatcher for matching configured ACL parameters
class ServerNameMatcher: public Ssl::GeneralNameMatcher
{
char cn[1024];
ACLData<MatchType> * data = (ACLData<MatchType> *)check_data;

if (cn_data->length > (int)sizeof(cn) - 1)
return 1; // ignore data that does not fit our buffer

char *s = reinterpret_cast<char *>(cn_data->data);
char *d = cn;
for (int i = 0; i < cn_data->length; ++i, ++d, ++s) {
if (*s == '\0')
return 1; // always a domain mismatch. contains 0x00
*d = *s;
}
cn[cn_data->length] = '\0';
debugs(28, 4, "Verifying certificate name/subjectAltName " << cn);
if (data->match(cn))
return 0;
return 1;
public:
explicit ServerNameMatcher(ServerNameCheck::Parameters &p): parameters(p) {}

protected:
/* GeneralNameMatcher API */
bool matchDomainName(const Dns::DomainName &) const override;
bool matchIp(const Ip::Address &) const override;

private:
// TODO: Make ServerNameCheck::Parameters::match() and this reference constant.
ServerNameCheck::Parameters &parameters; ///< configured ACL parameters
};

} // namespace Acl

bool
Acl::ServerNameMatcher::matchDomainName(const Dns::DomainName &domain) const
{
return parameters.match(SBuf(domain).c_str()); // TODO: Upgrade string-matching ACLs to SBuf
}

bool
Acl::ServerNameMatcher::matchIp(const Ip::Address &ip) const
{
// We are given an Ip::Address, but our ACL parameters use case-insensitive
// string equality (::matchDomainName) or regex string matches. There are
// many ways to convert an IPv6 address to a string, but only one format can
// correctly match certain configured parameters. Our ssl::server_name docs
// request the following ACL parameter formatting (that this to-string
// conversion code produces): IPv6 addresses use "::" notation (where
// applicable) and are not bracketed.
//
// Similar problems affect dstdomain ACLs. TODO: Instead of relying on users
// reading docs and following their inet_ntop(3) implementation to match
// IPv6 addresses handled by matchDomainName(), enhance matchDomainName()
// code and ACL parameter storage to support Ip::Address objects.
char hostStr[MAX_IPSTRLEN];
(void)ip.toStr(hostStr, sizeof(hostStr)); // no brackets
return parameters.match(hostStr);
}

int
Expand All @@ -79,37 +100,41 @@ Acl::ServerNameCheck::match(ACLChecklist * const ch)

assert(checklist != nullptr && checklist->request != nullptr);

const char *serverName = nullptr;
SBuf clientSniKeeper; // because c_str() is not constant
std::optional<AnyP::Host> serverNameFromConn;
if (ConnStateData *conn = checklist->conn()) {
const char *clientRequestedServerName = nullptr;
clientSniKeeper = conn->tlsClientSni();
if (clientSniKeeper.isEmpty()) {
const char *host = checklist->request->url.host();
if (host && *host) // paranoid first condition: host() is never nil
clientRequestedServerName = host;
} else
clientRequestedServerName = clientSniKeeper.c_str();
std::optional<AnyP::Host> clientRequestedServerName;
const auto &clientSni = conn->tlsClientSni();
if (clientSni.isEmpty()) {
clientRequestedServerName = checklist->request->url.parsedHost();
} else {
// RFC 6066: "The hostname is represented as a byte string using
// ASCII encoding"; "Literal IPv4 and IPv6 addresses are not
// permitted". TODO: Store TlsDetails::serverName and similar
// domains using a new domain-only type instead of SBuf.
clientRequestedServerName = AnyP::Host::ParseSimpleDomainName(clientSni);
}

if (useConsensus) {
X509 *peer_cert = conn->serverBump() ? conn->serverBump()->serverCert.get() : nullptr;
// use the client requested name if it matches the server
// certificate or if the certificate is not available
if (!peer_cert || Ssl::checkX509ServerValidity(peer_cert, clientRequestedServerName))
serverName = clientRequestedServerName;
if (!peer_cert || !clientRequestedServerName ||
Ssl::HasSubjectName(*peer_cert, *clientRequestedServerName))
serverNameFromConn = clientRequestedServerName;
} else if (useClientRequested)
serverName = clientRequestedServerName;
serverNameFromConn = clientRequestedServerName;
else { // either no options or useServerProvided
if (X509 *peer_cert = (conn->serverBump() ? conn->serverBump()->serverCert.get() : nullptr))
return Ssl::matchX509CommonNames(peer_cert, data.get(), check_cert_domain<const char*>);
return Ssl::HasMatchingSubjectName(*peer_cert, ServerNameMatcher(*data));
if (!useServerProvided)
serverName = clientRequestedServerName;
serverNameFromConn = clientRequestedServerName;
}
}

if (!serverName)
serverName = "none";

std::optional<SBuf> printedServerName;
if (serverNameFromConn)
printedServerName = ToSBuf(*serverNameFromConn); // no brackets
const auto serverName = printedServerName ? printedServerName->c_str() : "none";
return data->match(serverName);
}

Expand Down
113 changes: 113 additions & 0 deletions src/anyp/Host.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
/*
* Copyright (C) 1996-2023 The Squid Software Foundation and contributors
*
* Squid software is distributed under GPLv2+ license and includes
* contributions from numerous individuals and organizations.
* Please see the COPYING and CONTRIBUTORS files for details.
*/

#include "squid.h"
#include "anyp/Host.h"

#include <iostream>

std::optional<AnyP::Host>
AnyP::Host::ParseIp(const Ip::Address &ip)
{
// any preparsed IP value is acceptable
debugs(23, 7, ip);
return Host(ip);
}

/// common parts of FromSimpleDomain() and FromWildDomain()
std::optional<AnyP::Host>
AnyP::Host::ParseDomainName(const SBuf &rawName)
{
if (rawName.isEmpty()) {
debugs(23, 3, "rejecting empty name");
return std::nullopt;
}

// Reject bytes incompatible with rfc1035NamePack() and ::matchDomainName()
// implementations (at least). Such bytes can come from percent-encoded HTTP
// URIs or length-based X.509 fields, for example. Higher-level parsers must
// reject or convert domain name encodings like UTF-16, but this low-level
// check works as an additional (albeit unreliable) layer of defense against
// those (unsupported by Squid DNS code) encodings.
if (rawName.find('\0') != SBuf::npos) {
debugs(83, 3, "rejecting ASCII NUL character in " << rawName);
return std::nullopt;
}

// TODO: Consider rejecting names with isspace(3) bytes.

debugs(23, 7, rawName);
return Host(rawName);
}

std::optional<AnyP::Host>
AnyP::Host::ParseSimpleDomainName(const SBuf &rawName)
{
if (rawName.find('*') != SBuf::npos) {
debugs(23, 3, "rejecting wildcard in " << rawName);
return std::nullopt;
}
return ParseDomainName(rawName);
}

std::optional<AnyP::Host>
AnyP::Host::ParseWildDomainName(const SBuf &rawName)
{
const static SBuf wildcardLabel("*.");
if (rawName.startsWith(wildcardLabel)) {
if (rawName.find('*', 2) != SBuf::npos) {
debugs(23, 3, "rejecting excessive wildcards in " << rawName);
return std::nullopt;
}
// else: fall through to final checks
} else {
if (rawName.find('*', 0) != SBuf::npos) {
// this case includes "*" and "example.*" input
debugs(23, 3, "rejecting unsupported wildcard in " << rawName);
return std::nullopt;
}
// else: fall through to final checks
}
return ParseDomainName(rawName);
}

std::ostream &
AnyP::operator <<(std::ostream &os, const Host &host)
{
if (const auto ip = host.ip()) {
char buf[MAX_IPSTRLEN];
(void)ip->toStr(buf, sizeof(buf)); // no brackets
os << buf;
} else {
// If Host object creators start applying Uri::Decode() to reg-names,
// then we must start applying Uri::Encode() here, but only to names
// that require it. See "The reg-name syntax allows percent-encoded
// octets" paragraph in RFC 3986.
const auto domainName = host.domainName();
Assure(domainName);
os << *domainName;
}
return os;
}

std::ostream &
AnyP::operator <<(std::ostream &os, const Bracketed &hostWrapper)
{
bool addBrackets = false;
if (const auto ip = hostWrapper.host.ip())
addBrackets = ip->isIPv6();

if (addBrackets)
os << '[';
os << hostWrapper.host;
if (addBrackets)
os << ']';

return os;
}

81 changes: 81 additions & 0 deletions src/anyp/Host.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/*
* Copyright (C) 1996-2023 The Squid Software Foundation and contributors
*
* Squid software is distributed under GPLv2+ license and includes
* contributions from numerous individuals and organizations.
* Please see the COPYING and CONTRIBUTORS files for details.
*/

#ifndef SQUID_SRC_ANYP_HOST_H
#define SQUID_SRC_ANYP_HOST_H

#include "dns/forward.h"
#include "ip/Address.h"
#include "sbuf/SBuf.h"

#include <iosfwd>
#include <optional>
#include <variant>

namespace AnyP
{

/// either a domain name (as defined in DNS RFC 1034) or an IP address
class Host
{
public:
/// converts an already parsed IP address to a Host object
static std::optional<Host> ParseIp(const Ip::Address &);

/// Parses input as a literal ASCII domain name (A-labels OK; see RFC 5890).
/// Does not allow wildcards; \sa ParseWildDomainName().
static std::optional<Host> ParseSimpleDomainName(const SBuf &);

/// Same as ParseSimpleDomainName() but allows the first label to be a
/// wildcard (RFC 9525 Section 6.3).
static std::optional<Host> ParseWildDomainName(const SBuf &);

// Accessor methods below are mutually exclusive: Exactly one method is
// guaranteed to return a result other than std::nullopt.

/// stored IPv or IPv6 address (if any)
///
/// Ip::Address::isNoAddr() may be true for the returned address.
/// Ip::Address::isAnyAddr() may be true for the returned address.
auto ip() const { return std::get_if<Ip::Address>(&raw_); }

/// stored domain name (if any)
auto domainName() const { return std::get_if<SBuf>(&raw_); }

private:
using Storage = std::variant<Ip::Address, Dns::DomainName>;

static std::optional<Host> ParseDomainName(const SBuf &);

// use a Parse*() function to create Host objects
Host(const Storage &raw): raw_(raw) {}

Storage raw_; ///< the host we are providing access to
};

/// helps print Host value in RFC 3986 Section 3.2.2 format, with square
/// brackets around an IPv6 address (if the Host value is an IPv6 address)
class Bracketed
{
public:
explicit Bracketed(const Host &aHost): host(aHost) {}
const Host &host;
};

/// prints Host value _without_ square brackets around an IPv6 address (even
/// when the Host value is an IPv6 address); \sa Bracketed
std::ostream &operator <<(std::ostream &, const Host &);

/// prints Host value _without_ square brackets around an IPv6 address (even
/// when the Host value is an IPv6 address); \sa Bracketed
std::ostream &operator <<(std::ostream &, const Bracketed &);

} // namespace Anyp

#endif /* SQUID_SRC_ANYP_HOST_H */

2 changes: 2 additions & 0 deletions src/anyp/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand Down
Loading

0 comments on commit 22b2a7a

Please sign in to comment.