From: Ricardo Ferreira Ribeiro Date: Wed, 11 Feb 2026 03:03:25 +0000 (+0000) Subject: Expand %x and %D after bumped SQUID_X509_V_ERR_DOMAIN_MISMATCH (#2373) X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=611cda11fe494d2e1f6353f4864cd0d874b7eb54;p=thirdparty%2Fsquid.git 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 ae7a863a48..3c78c62e5c 100644 --- 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 9b3de592a3..afe9fd2e0e 100644 --- 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 a7199f5ca7..b027ca80b3 100644 --- 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 6a896a3fee..811d264d6d 100644 --- 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); }