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

diff --git a/src/acl/FilledChecklist.h b/src/acl/FilledChecklist.h
index 15bb05dc044176c3bf4d8bcbc2d82cdef69dc0e2..163328b7167d0fee542eab4caa2bc7d11f0ab662 100644 (file)
--- 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 9a0a5452e8ebb6255716235147091ec865edf7bf..493fb7cbeff8a71a11d2c74cca51cd014a3a7663 100644 (file)
--- 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 cfebd9a94d7a8de849ad9eb4f12dd3cc88f4a0ba..61eac7eb5e3c94b1eed8c6256eb45668818b3283 100644 (file)
--- 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 0313be286026bc8489a31eff4d407d81c61e3782..6ebd1a0c09909d92751c1d27fa7a3da3dce9270d 100644 (file)
--- 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 873b269d112186423ac8318cd1af8c7582d8ec02..a6f720353a0b4d8913576285175af37c0d11b78b 100644 (file)
--- 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