Expand %x and %D after bumped SQUID_X509_V_ERR_DOMAIN_MISMATCH (#2373)

Squid detects SQUID_X509_V_ERR_DOMAIN_MISMATCH errors during various
processing stages, including when receiving an HTTP request on a
successfully bumped TLS connection. If that request targets a domain not
covered by the server certificate, and sslproxy_cert_error prohibits a
mismatch (it does by default), then Squid terminates the transaction
with an ERR_SECURE_CONNECT_FAIL response. That generated error response
body lacked %x and %D error details:

```diff
  The system returned:

- [No Error] (TLS code: [Unknown Error Code])
+ [No Error] (TLS code: SQUID_X509_V_ERR_DOMAIN_MISMATCH)

- [No Error Detail]
+ Certificate does not match domainname: /L=.../O=.../CN=example.com
```

The first `[No Error]` expansion of %E remains unchanged because this
particular error does not set `errno`.

ConnStateData::serveDelayedError() changes fix the above problem but %x
expansion in error pages and %err_detail in access log get a misleading
`+broken_cert` detail. To address that flaw, we changed the default for
broken certificate in Security::ErrorDetail constructor API from peer
certificate to nil. When broken certificate is nil, ErrorDetail now uses
valid certificate to expand %ssl_cn and similar certificate-inspecting
error page %codes.

All Security::ErrorDetail creators were checked and adjusted if needed:

* ConnStateData::serveDelayedError(): No caller changes. Using the new
  ErrorDetail creation API fixes this code by supplying nil broken
  certificate (because the certificate is _valid_ in this context).

* ssl_verify_cb(): No caller changes. We already use peer certificate as
  the default broken certificate because doing so is "reasonable" here.

* Security::PeerConnector::sslCrtvdCheckForErrors(): Adjusted to keep
  the original "if there was no error_cert_ID, then use peerCert"
  behavior while using new Security::ErrorDetail creation API.

Thus, the last two contexts are not affected by this error reporting API
change. The exceptional serveDelayedError() caller is affected, but
Squid did not report any certificate detail in that case until this
branch fixes, so this branch does not change one "reporting certificate"
to another; it only starts reporting (important) information when none
was available before.

This is a Measurement Factory project.

diff --git a/src/client_side.cc b/src/client_side.cc
index ae7a863a4864e978dd96ded8c890b1b7faa305e9..3c78c62e5c2804d0f53e36fb58b5a6cadad858d3 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -1503,6 +1503,7 @@ bool ConnStateData::serveDelayedError(Http::Stream *context)
                 const Security::ErrorDetail::Pointer errDetail = new Security::ErrorDetail(
                     SQUID_X509_V_ERR_DOMAIN_MISMATCH,
                     srvCert, nullptr);
+                err->detailError(errDetail);
                 updateError(ERR_SECURE_CONNECT_FAIL, errDetail);
                 repContext->setReplyToError(request->method, err);
                 assert(context->http->out.offset == 0);

diff --git a/src/security/ErrorDetail.cc b/src/security/ErrorDetail.cc
index 9b3de592a3db883f7821ac91fb2782c4a9524c6f..afe9fd2e0eb4febf479dadaeb5c2733e8b000f52 100644 (file)
--- a/src/security/ErrorDetail.cc
+++ b/src/security/ErrorDetail.cc
@@ -468,7 +468,7 @@ Security::ErrorDetail::ErrorDetail(const ErrorCode anErrorCode, const CertPointe
 {
     errReason = aReason;
     peer_cert = cert;
-    broken_cert = broken ? broken : cert;
+    broken_cert = broken;
 }
 
 #if USE_OPENSSL
@@ -564,8 +564,8 @@ Security::ErrorDetail::verbose(const HttpRequestPointer &request) const
 void
 Security::ErrorDetail::printSubject(std::ostream &os) const
 {
-    if (broken_cert) {
-        auto buf = SubjectName(*broken_cert);
+    if (const auto cert = certificateToReport()) {
+        auto buf = SubjectName(*cert);
         if (!buf.isEmpty()) {
             // TODO: Convert html_quote() into an std::ostream manipulator.
             // quote to avoid possible html code injection through
@@ -632,9 +632,9 @@ void
 Security::ErrorDetail::printCommonName(std::ostream &os) const
 {
 #if USE_OPENSSL
-    if (broken_cert.get()) {
+    if (const auto cert = certificateToReport()) {
         CommonNamesPrinter printer(os);
-        (void)Ssl::HasMatchingSubjectName(*broken_cert, printer);
+        (void)Ssl::HasMatchingSubjectName(*cert, printer);
         if (printer.printed)
             return;
     }
@@ -646,8 +646,8 @@ Security::ErrorDetail::printCommonName(std::ostream &os) const
 void
 Security::ErrorDetail::printCaName(std::ostream &os) const
 {
-    if (broken_cert) {
-        auto buf = IssuerName(*broken_cert);
+    if (const auto cert = certificateToReport()) {
+        auto buf = IssuerName(*cert);
         if (!buf.isEmpty()) {
             // quote to avoid possible html code injection through
             // certificate issuer subject
@@ -663,8 +663,8 @@ void
 Security::ErrorDetail::printNotBefore(std::ostream &os) const
 {
 #if USE_OPENSSL
-    if (broken_cert.get()) {
-        if (const auto tm = X509_getm_notBefore(broken_cert.get())) {
+    if (const auto cert = certificateToReport()) {
+        if (const auto tm = X509_getm_notBefore(cert)) {
             // TODO: Add and use an ASN1_TIME printing operator instead.
             static char tmpBuffer[256]; // A temporary buffer
             Ssl::asn1timeToString(tm, tmpBuffer, sizeof(tmpBuffer));
@@ -681,8 +681,8 @@ void
 Security::ErrorDetail::printNotAfter(std::ostream &os) const
 {
 #if USE_OPENSSL
-    if (broken_cert.get()) {
-        if (const auto tm = X509_getm_notAfter(broken_cert.get())) {
+    if (const auto cert = certificateToReport()) {
+        if (const auto tm = X509_getm_notAfter(cert)) {
             // XXX: Reduce code duplication.
             static char tmpBuffer[256]; // A temporary buffer
             Ssl::asn1timeToString(tm, tmpBuffer, sizeof(tmpBuffer));
@@ -747,7 +747,7 @@ Security::ErrorDetail::printErrorLibError(std::ostream &os) const
  * %ssl_error_descr: A short description of the SSL error
  * %ssl_lib_error: human-readable low-level error string by ErrorString()
  *
- * Certificate information extracted from broken (not necessarily peer!) cert
+ * Peer or intermediate certificate info extracted from certificateToReport()
  * %ssl_cn: The comma-separated list of common and alternate names
  * %ssl_subject: The certificate subject
  * %ssl_ca_name: The certificate issuer name

diff --git a/src/security/ErrorDetail.h b/src/security/ErrorDetail.h
index a7199f5ca75e5f5260afc60b32415aab40e5ef28..b027ca80b341d2a9c6938cd199afa6c9eb6152b5 100644 (file)
--- a/src/security/ErrorDetail.h
+++ b/src/security/ErrorDetail.h
@@ -43,8 +43,11 @@ class ErrorDetail: public ::ErrorDetail
 public:
     typedef ErrorDetailPointer Pointer;
 
-    /// Details a server-side certificate verification failure.
-    /// If `broken` is nil, then the broken certificate is the peer certificate.
+    /// Details an origin or cache_peer certificate verification failure or mismatch.
+    /// \param peer is an origin server or cache_peer certificate
+    /// \param broken is the problematic certificate (if verification failed) or nil (otherwise)
+    /// Two non-nil certificates may differ when, for example, an intermediate
+    /// (rather than leaf) certificate fails validation.
     ErrorDetail(ErrorCode err_no, const CertPointer &peer, const CertPointer &broken, const char *aReason = nullptr);
 
 #if USE_OPENSSL
@@ -95,6 +98,10 @@ private:
     void printErrorLibError(std::ostream &os) const;
     size_t convertErrorCodeToDescription(const char *code, std::ostream &os) const;
 
+    /// peer or intermediate certificate that should be used when expanding
+    /// various error page %codes like %ssl_subject
+    Certificate *certificateToReport() const { return broken_cert ? broken_cert.get() : peer_cert.get(); }
+
     CertPointer peer_cert; ///< A pointer to the peer certificate
     CertPointer broken_cert; ///< A pointer to the broken certificate (peer or intermediate)
 

diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc
index 6a896a3fee7657cdf26d3f086b1d087f08ece501..811d264d6d77254b2402c2824cf4d2dc7cc68f75 100644 (file)
--- a/src/security/PeerConnector.cc
+++ b/src/security/PeerConnector.cc
@@ -417,8 +417,14 @@ Security::PeerConnector::sslCrtvdCheckForErrors(Ssl::CertValidationResponse cons
                 debugs(83, 3, "bypassing SSL error " << i->error_no << " in " << "buffer");
             } else {
                 debugs(83, 5, "confirming SSL error " << i->error_no);
-                const auto &brokenCert = i->cert;
                 Security::CertPointer peerCert(SSL_get_peer_certificate(session.get()));
+
+                // Features/SslServerCertValidator docs do not specify whether
+                // error_cert_ID is an optional helper response field. For now,
+                // to preserve initial implementation behavior, we assume that
+                // it is optional and that it defaults to peerCert.
+                const auto &brokenCert = i->cert ? i->cert : peerCert;
+
                 const char *aReason = i->error_reason.empty() ? nullptr : i->error_reason.c_str();
                 errDetails = new ErrorDetail(i->error_no, peerCert, brokenCert, aReason);
             }