From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Tue, 14 Jun 2022 15:24:28 +0000 (+0000)
Subject: Avoid reading tls-cert=bundle twice when loading certificates (#1073)
X-Git-Tag: SQUID_6_0_1~166
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=c25648fb4eca515f8a6a535cc93c2ac2eccc292b;p=thirdparty%2Fsquid.git

Avoid reading tls-cert=bundle twice when loading certificates (#1073)

After opening and reading the certificate bundle to load the required
traffic-signing certificate, Squid was opening and reading the same file
again to load the optional intermediate certificates. Loading the
traffic-signing certificate twice triggered bugs and further
complicating naturally tricky code.

No functionality changes expected except minor cache.log message changes
and certificate reporting improvements in "squid -k parse" mode.
---

diff --git a/src/security/KeyData.cc b/src/security/KeyData.cc
index 73dc5ab23d..de083e90e6 100644
--- a/src/security/KeyData.cc
+++ b/src/security/KeyData.cc
@@ -15,9 +15,10 @@
 #include "ssl/bio.h"
 #include "ssl/gadgets.h"
 
-/// load a signing certificate from certFile
+/// load the signing certificate and its chain, if any, from certFile
+/// \return true if the signing certificate was obtained
 bool
-Security::KeyData::loadX509CertFromFile()
+Security::KeyData::loadCertificates()
 {
     debugs(83, DBG_IMPORTANT, "Using certificate in " << certFile);
     cert.reset(); // paranoid: ensure cert is unset
@@ -33,7 +34,7 @@ Security::KeyData::loadX509CertFromFile()
 
     try {
         cert = Ssl::ReadCertificate(bio);
-        return true;
+        debugs(83, DBG_PARSE_NOTE(2), "Loaded signing certificate: " << *cert);
     }
     catch (...) {
         // TODO: Convert the rest of this method to throw on errors instead.
@@ -42,6 +43,40 @@ Security::KeyData::loadX509CertFromFile()
         return false;
     }
 
+    try {
+        // selected bundled certificates in sending order: wireCerts = cert + chain
+        CertList wireCerts(1, cert);
+
+        while (const auto bundledCert = Ssl::ReadOptionalCertificate(bio)) {
+            assert(!wireCerts.empty()); // this->cert is there (at least)
+
+            // We cannot chain any certificate after a self-signed certificate. This
+            // check also protects the IssuedBy() check below from adding duplicated
+            // (i.e. listed multiple times in the bundle) self-signed certificates.
+            if (SelfSigned(*wireCerts.back())) {
+                debugs(83, DBG_PARSE_NOTE(2), "WARNING: Ignoring certificate after a self-signed one: " << *bundledCert);
+                continue; // ... but keep going to report all ignored certificates
+            }
+
+            // add the bundled certificate if it extends the chain further
+            if (IssuedBy(*wireCerts.back(), *bundledCert)) {
+                debugs(83, DBG_PARSE_NOTE(3), "Adding issuer CA: " << *bundledCert);
+                // OpenSSL API requires that we order certificates such that the
+                // chain can be appended directly into the on-wire traffic.
+                chain.emplace_back(bundledCert);
+                wireCerts.emplace_back(bundledCert);
+                continue;
+            }
+
+            debugs(83, DBG_PARSE_NOTE(2), "WARNING: Ignoring certificate that does not extend the chain: " << *bundledCert);
+        }
+    }
+    catch (...) {
+        // TODO: Reject configs with malformed intermediate certs instead.
+        debugs(83, DBG_IMPORTANT, "ERROR: Failure while loading intermediate certificate(s) from '" << certFile << "':" <<
+               Debug::Extra << "problem: " << CurrentException);
+    }
+
 #elif USE_GNUTLS
     const char *certFilename = certFile.c_str();
     gnutls_datum_t data;
@@ -73,6 +108,9 @@ Security::KeyData::loadX509CertFromFile()
         });
     }
 
+    // XXX: implement chain loading
+    debugs(83, 2, "Loading certificate chain from PEM files not implemented in this Squid.");
+
 #else
     // do nothing.
 #endif
@@ -84,58 +122,6 @@ Security::KeyData::loadX509CertFromFile()
     return bool(cert);
 }
 
-/// load any intermediate certs that form the chain with the loaded signing cert
-void
-Security::KeyData::loadX509ChainFromFile()
-{
-#if USE_OPENSSL
-    const char *certFilename = certFile.c_str();
-    Ssl::BIO_Pointer bio(BIO_new(BIO_s_file()));
-    if (!bio || !BIO_read_filename(bio.get(), certFilename)) {
-        const auto x = ERR_get_error();
-        debugs(83, DBG_IMPORTANT, "ERROR: unable to load chain file '" << certFile << "': " << ErrorString(x));
-        return;
-    }
-
-    if (SelfSigned(*cert)) {
-        debugs(83, DBG_PARSE_NOTE(2), "Signing certificate is self-signed: " << *cert);
-        // TODO: Warn if there are other (unusable) certificates present.
-    } else {
-        debugs(83, DBG_PARSE_NOTE(3), "Using certificate chain in " << certFile);
-        // and add to the chain any other certificate exist in the file
-        CertPointer latestCert = cert;
-
-        while (const auto ca = Ssl::ReadOptionalCertificate(bio)) {
-            // checks that the chained certs are actually part of a chain for validating cert
-            if (IssuedBy(*latestCert, *ca)) {
-
-                if (SelfSigned(*latestCert)) { // TODO: Rename to lastChained
-                    Assure(SelfSigned(*ca));
-                    Assure(!SelfSigned(*cert)); // TODO: Rename to leafCert or signingCert
-                    debugs(83, DBG_PARSE_NOTE(2), "WARNING: Ignoring repeated Root CA: " << *ca);
-                    continue;
-                }
-
-                debugs(83, DBG_PARSE_NOTE(3), "Adding issuer CA: " << *ca);
-                // OpenSSL API requires that we order certificates such that the
-                // chain can be appended directly into the on-wire traffic.
-                latestCert = CertPointer(ca);
-                chain.emplace_back(latestCert);
-            } else {
-                debugs(83, DBG_PARSE_NOTE(2), certFile << ": Ignoring non-issuer CA " << *ca);
-            }
-        }
-    }
-
-#elif USE_GNUTLS
-    // XXX: implement chain loading
-    debugs(83, 2, "Loading certificate chain from PEM files not implemented in this Squid.");
-
-#else
-    // nothing to do.
-#endif
-}
-
 /**
  * Read X.509 private key from file.
  */
@@ -186,21 +172,11 @@ void
 Security::KeyData::loadFromFiles(const AnyP::PortCfg &port, const char *portType)
 {
     char buf[128];
-    if (!loadX509CertFromFile()) {
+    if (!loadCertificates()) {
         debugs(83, DBG_IMPORTANT, "WARNING: '" << portType << "_port " << port.s.toUrl(buf, sizeof(buf)) << "' missing certificate in '" << certFile << "'");
         return;
     }
 
-    // certificate chain in the PEM file is optional
-    try {
-        loadX509ChainFromFile();
-    }
-    catch (...) {
-        // XXX: Reject malformed configurations by letting exceptions propagate.
-        debugs(83, DBG_CRITICAL, "ERROR: '" << portType << "_port " << port.s.toUrl(buf, sizeof(buf)) << "' cannot load intermediate certificates from '" << certFile << "':" <<
-               Debug::Extra << "problem: " << CurrentException);
-    }
-
     // pkey is mandatory, not having it makes cert and chain pointless.
     if (!loadX509PrivateKeyFromFile()) {
         debugs(83, DBG_IMPORTANT, "WARNING: '" << portType << "_port " << port.s.toUrl(buf, sizeof(buf)) << "' missing private key in '" << privateKeyFile << "'");
diff --git a/src/security/KeyData.h b/src/security/KeyData.h
index 97bbc72c2a..4f9b48903e 100644
--- a/src/security/KeyData.h
+++ b/src/security/KeyData.h
@@ -35,8 +35,7 @@ public:
     Security::CertList chain;
 
 private:
-    bool loadX509CertFromFile();
-    void loadX509ChainFromFile();
+    bool loadCertificates();
     bool loadX509PrivateKeyFromFile();
 };