From 987a0ab896b78c82bd2f31dda75864b68130f24b4c213cb71263aa322861ce83 Mon Sep 17 00:00:00 2001 From: Martin Pluskal Date: Wed, 6 Jun 2018 13:59:50 +0000 Subject: [PATCH] Accepting request 614571 from home:adamm:branches:server:proxy - a3f6783.patch: Fixes certificate handling with intermediates chains OBS-URL: https://build.opensuse.org/request/show/614571 OBS-URL: https://build.opensuse.org/package/show/server:proxy/squid?expand=0&rev=157 --- a3f6783.patch | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++ squid.changes | 6 ++++ squid.spec | 2 ++ 3 files changed, 95 insertions(+) create mode 100644 a3f6783.patch diff --git a/a3f6783.patch b/a3f6783.patch new file mode 100644 index 0000000..d3a09b0 --- /dev/null +++ b/a3f6783.patch @@ -0,0 +1,87 @@ +commit a3f6783b1cfe4b8067312fa65828fcd925757c38 +Author: Amos Jeffries +Date: Tue Jun 5 06:11:29 2018 +0000 + + Bug 4831: filter chain certificates for validity when loading (#187) + + 51e09c08a5e6c582e7d93af99a8f2cfcb14ea9e6 adding + GnuTLS support required splitting the way + certificate chains were loaded. This resulted in the + leaf certificate being added twice at the prefix of a + chain in the serverHello. + + It turns out that some recipients validate strictly that the + chain delivered by a serverHello does not contain extra + certificates and reject the handshake if they do. + + This patch implements the XXX about filtering certificates + for chain sequence order and self-sign properties, added + in the initial PR. Resolving the bug 4831 regression and also + reporting failures at startup/reconfigure for admins. + + Also, add debug display of certificate names for simpler + detection and administrative fix when loaded files fail + these tests. + +diff --git a/src/security/KeyData.cc b/src/security/KeyData.cc +index 23d123954..052c64ffd 100644 +--- a/src/security/KeyData.cc ++++ b/src/security/KeyData.cc +@@ -86,8 +86,6 @@ void + Security::KeyData::loadX509ChainFromFile() + { + #if USE_OPENSSL +- // XXX: This BIO loads the public cert as first chain cert, +- // so the code appending chains sends it twice in handshakes. + const char *certFilename = certFile.c_str(); + Ssl::BIO_Pointer bio(BIO_new(BIO_s_file())); + if (!bio || !BIO_read_filename(bio.get(), certFilename)) { +@@ -96,14 +94,41 @@ Security::KeyData::loadX509ChainFromFile() + return; + } + +- if (X509_check_issued(cert.get(), cert.get()) == X509_V_OK) +- debugs(83, 5, "Certificate is self-signed, will not be chained"); +- else { ++#if TLS_CHAIN_NO_SELFSIGNED // ignore self-signed certs in the chain ++ if (X509_check_issued(cert.get(), cert.get()) == X509_V_OK) { ++ char *nameStr = X509_NAME_oneline(X509_get_subject_name(cert.get()), nullptr, 0); ++ debugs(83, DBG_PARSE_NOTE(2), "Certificate is self-signed, will not be chained: " << nameStr); ++ OPENSSL_free(nameStr); ++ } else ++#endif ++ { ++ debugs(83, DBG_PARSE_NOTE(3), "Using certificate chain in " << certFile); + // and add to the chain any other certificate exist in the file +- while (X509 *ca = PEM_read_bio_X509(bio.get(), nullptr, nullptr, nullptr)) { +- // XXX: self-signed check should be applied to all certs loaded. +- // XXX: missing checks that the chained certs are actually part of a chain for validating cert. +- chain.emplace_front(Security::CertPointer(ca)); ++ CertPointer latestCert = cert; ++ ++ while (auto ca = PEM_read_bio_X509(bio.get(), nullptr, nullptr, nullptr)) { ++ // get Issuer name of the cert for debug display ++ char *nameStr = X509_NAME_oneline(X509_get_subject_name(ca), nullptr, 0); ++ ++#if TLS_CHAIN_NO_SELFSIGNED // ignore self-signed certs in the chain ++ // self-signed certificates are not valid in a sent chain ++ if (X509_check_issued(ca, ca) == X509_V_OK) { ++ debugs(83, DBG_PARSE_NOTE(2), "CA " << nameStr << " is self-signed, will not be chained: " << nameStr); ++ OPENSSL_free(nameStr); ++ continue; ++ } ++#endif ++ // checks that the chained certs are actually part of a chain for validating cert ++ if (X509_check_issued(ca, latestCert.get()) == X509_V_OK) { ++ debugs(83, DBG_PARSE_NOTE(3), "Adding issuer CA: " << nameStr); ++ // 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_front(latestCert); ++ } else { ++ debugs(83, DBG_PARSE_NOTE(2), "Ignoring non-issuer CA from " << certFile << ": " << nameStr); ++ } ++ OPENSSL_free(nameStr); + } + } + diff --git a/squid.changes b/squid.changes index e59beb1..2e106b1 100644 --- a/squid.changes +++ b/squid.changes @@ -1,3 +1,9 @@ +------------------------------------------------------------------- +Wed Jun 6 13:52:01 UTC 2018 - adam.majer@suse.de + +- a3f6783.patch: Fixes certificate handling with intermediates + chains + ------------------------------------------------------------------- Tue May 15 07:43:44 UTC 2018 - adam.majer@suse.de diff --git a/squid.spec b/squid.spec index b8ae115..db402fd 100644 --- a/squid.spec +++ b/squid.spec @@ -38,6 +38,7 @@ Source13: http://www.squid-cache.org/pgp.asc#/squid.keyring Source15: cache_dir.sed Source16: initialize_cache_if_needed.sh Patch1: missing_installs.patch +Patch2: a3f6783.patch BuildRequires: cppunit-devel BuildRequires: db-devel BuildRequires: ed @@ -86,6 +87,7 @@ cp %{SOURCE10} . # upstream patches after RELEASE perl -p -i -e 's|%{_prefix}/local/bin/perl|%{_bindir}/perl|' `find -name "*.pl"` %patch1 -p1 +%patch2 -p1 %build autoreconf -fi