From 757a738c27144fec74ea66a52e17a9c7bd532cfd Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 17 May 2022 16:29:49 +0000 Subject: [PATCH] Bug 5211: support.cc:355: "!filledCheck->sslErrors" assertion (#1044) One master transaction may encounter several certificate validation-related errors because Squid may need to validate several server certificates for one connection and may even need to open several server connections. Since 2012 commit 4fb72cb, ssl_verify_cb() incorrectly assumed that no past errors were possible. The callback started to assert when recent code changes (commit e227da8?) improved delivery of past errors to ACL checks. While fixing this bug, we discovered that ServerBump::serverSession ignored new TLS connection when Squid connected to another destination after a failed TLS handshake. That stale value was then used to extract stale information about past TLS errors. In some cases, that would prevent Squid from hitting the now-fixed assertion. In other cases, it could result in wrong sslproxy_cert_sign and sslproxy_cert_adapt decisions. Now also fixed; see Ssl::ServerBump::attachServerSession(). A nil PeerConnector::check->sslErrors value may get stale because PeerConnector::check is filled just once: sslError will remain nil during the second validation pass (after PeerConnector fetches missing intermediate certificates and revalidates), despite validation errors discovered during the first pass. Today, ssl_verify_cb() does not really use FilledChecklist::sslErrors preset by its PeerConnector, so a stale value currently has no effect. Fixing this waiting-to-happen problem is not trivial (we tried). A proper fix deserves a dedicated PR. Added XXX. While working on these changes, we discovered that Squid implements two mutually exclusive ssl_error (FilledChecklist::sslErrors) definitions: * In sslproxy_cert_error context, ssl_error matches the last or current error being examined by the sslproxy_cert_error code. * In sslproxy_cert_sign and sslproxy_cert_adapt context, ssl_error matches any previously observed validation error. This change updates Squid documentation to reflect the above findings. Unfortunately, it is not feasible to remove this unwanted directive "sensitivity" without deprecating/replacing sslproxy_cert_error and ssl_error: Selecting any one of the two ssl_error definitions above may seriously (and silently) break existing configurations: # If all-errors semantics is adopted for sslproxy_cert_error, then # this configuration will start allowing certificates with _any_ # error as long as that certificate also has a minorError: acl minorError ssl_error ... sslproxy_cert_error allow minorError # If last-error semantics is adopted for sslproxy_cert_sign, then # this configuration will stop properly signing certificates that # mimic self-signed real certificates if the very last error during # that real certificate validation was _not_ certSelfSigned: sslproxy_cert_sign signSelf ssl::certSelfSigned --- src/acl/FilledChecklist.h | 5 ++++- src/cf.data.pre | 10 +++++++--- src/security/PeerConnector.cc | 3 +++ src/ssl/ServerBump.cc | 3 --- src/ssl/support.cc | 4 ++-- 5 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/acl/FilledChecklist.h b/src/acl/FilledChecklist.h index 15bb05dc04..163328b716 100644 --- a/src/acl/FilledChecklist.h +++ b/src/acl/FilledChecklist.h @@ -87,7 +87,10 @@ public: char *snmp_community; #endif - /// SSL [certificate validation] errors, in undefined order + /// TLS server [certificate validation] errors, in undefined order. + /// The errors are accumulated as Squid goes through validation steps + /// and server certificates. They are cleared on connection retries. + /// For sslproxy_cert_error checks, contains just the current/last error. const Security::CertErrors *sslErrors; /// Peer certificate being checked by ssl_verify_cb() and by diff --git a/src/cf.data.pre b/src/cf.data.pre index 9a0a5452e8..493fb7cbef 100644 --- a/src/cf.data.pre +++ b/src/cf.data.pre @@ -1486,6 +1486,13 @@ IF USE_OPENSSL acl aclname ssl_error errorname # match against SSL certificate validation error [fast] # + # When used with sslproxy_cert_error, this ACL tests a single + # certificate validation error currently being evaluated by that + # directive. When used with slproxy_cert_sign or sslproxy_cert_adapt, + # the ACL tests all past certificate validation errors associated with + # the current Squid-to-server connection (attempt). This ACL is not yet + # supported for use with other directives. + # # For valid error names see in @DEFAULT_ERROR_DIR@/templates/error-details.txt # template file. # @@ -1500,9 +1507,6 @@ IF USE_OPENSSL # The ssl::certHasExpired, ssl::certNotYetValid, ssl::certDomainMismatch, # ssl::certUntrusted, and ssl::certSelfSigned can also be used as # predefined ACLs, just like the 'all' ACL. - # - # NOTE: The ssl_error ACL is only supported with sslproxy_cert_error, - # sslproxy_cert_sign, and sslproxy_cert_adapt options. acl aclname server_cert_fingerprint [-sha1] fingerprint # match against server SSL certificate fingerprint [fast] diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc index cfebd9a94d..61eac7eb5e 100644 --- a/src/security/PeerConnector.cc +++ b/src/security/PeerConnector.cc @@ -163,6 +163,9 @@ Security::PeerConnector::initialize(Security::SessionPointer &serverSession) if (!Ssl::TheConfig.ssl_crt_validator) { // Create the ACL check list now, while we have access to more info. // The list is used in ssl_verify_cb() and is freed in ssl_free(). + // XXX: This info may change, especially if we fetch missing certs. + // TODO: Remove ACLFilledChecklist::sslErrors and other pre-computed + // state in favor of the ACLs accessing current/fresh info directly. if (acl_access *acl = ::Config.ssl_client.cert_error) { ACLFilledChecklist *check = new ACLFilledChecklist(acl, request.getRaw(), dash_str); fillChecklist(*check); diff --git a/src/ssl/ServerBump.cc b/src/ssl/ServerBump.cc index 0313be2860..6ebd1a0c09 100644 --- a/src/ssl/ServerBump.cc +++ b/src/ssl/ServerBump.cc @@ -59,9 +59,6 @@ Ssl::ServerBump::~ServerBump() void Ssl::ServerBump::attachServerSession(const Security::SessionPointer &s) { - if (serverSession) - return; - serverSession = s; } diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 873b269d11..a6f720353a 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -352,7 +352,7 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) if (error_no != SQUID_X509_V_ERR_INFINITE_VALIDATION) { if (check) { ACLFilledChecklist *filledCheck = Filled(check); - assert(!filledCheck->sslErrors); + const auto savedErrors = filledCheck->sslErrors; filledCheck->sslErrors = new Security::CertErrors(Security::CertError(error_no, broken_cert)); filledCheck->serverCert = peer_cert; if (check->fastCheck().allowed()) { @@ -362,7 +362,7 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) debugs(83, 5, "confirming SSL error " << error_no); } delete filledCheck->sslErrors; - filledCheck->sslErrors = NULL; + filledCheck->sslErrors = savedErrors; filledCheck->serverCert.reset(); } // If the certificate validator is used then we need to allow all errors and -- 2.47.2