Bug fix: broken intermediate certificate is mimicked instead of the origin server...

 In the case of SSL error we are trying to get the server ssl certificate from
the Ssl::ErrorDetail object using the Ssl::ErrorDetail::peerCert method.
But with the current implementation this method does not return the peer
certificate but instead the broken certificate which may be different than the
peer certificate.

This patch:
1) Stores both server and broken certificate in Ssl::ErrorDetail objects
2) Fix the Ssl::ErrorDetail::peerCert method to return always server certificate
3) Add a new method the Ssl::ErrorDetail::brokenCert which return the broken
   certificate

diff --git a/src/client_side.cc b/src/client_side.cc
index e3efdd0eeb4fb55e91d58ec2c963a1484b298ac1..4c61773d6fdbd3719d1477e8ea811ff553114786 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -2497,7 +2497,7 @@ bool ConnStateData::serveDelayedError(ClientSocketContext *context)
 #else
                 err->xerrno = EACCES;
 #endif
-                Ssl::ErrorDetail *errDetail = new Ssl::ErrorDetail( SQUID_X509_V_ERR_DOMAIN_MISMATCH, server_cert);
+                Ssl::ErrorDetail *errDetail = new Ssl::ErrorDetail( SQUID_X509_V_ERR_DOMAIN_MISMATCH, server_cert, NULL);
                 err->detail = errDetail;
                 repContext->setReplyToError(request->method, err);
                 assert(context->http->out.offset == 0);

diff --git a/src/forward.cc b/src/forward.cc
index 36c6c798b214c4e4c8462225572fcacd153faeea..6fe78c7a4980c9ffc78b300833f78145aa090e96 100644 (file)
--- a/src/forward.cc
+++ b/src/forward.cc
@@ -656,18 +656,17 @@ FwdState::negotiateSSL(int fd)
             } else {
                 // server_cert can be be NULL
                 X509 *server_cert = SSL_get_peer_certificate(ssl);
-                errDetails = new Ssl::ErrorDetail(SQUID_ERR_SSL_HANDSHAKE, server_cert);
+                errDetails = new Ssl::ErrorDetail(SQUID_ERR_SSL_HANDSHAKE, server_cert, NULL);
                 X509_free(server_cert);
             }
 
             if (ssl_lib_error != SSL_ERROR_NONE)
                 errDetails->setLibError(ssl_lib_error);
 
-            X509 *srvX509 = errDetails->peerCert();
             if (request->clientConnectionManager.valid()) {
                 // Get the server certificate from ErrorDetail object and store it 
                 // to connection manager
-                request->clientConnectionManager->setBumpServerCert(X509_dup(srvX509));
+                request->clientConnectionManager->setBumpServerCert(X509_dup(errDetails->peerCert()));
 
                 // if there is a list of ssl errors, pass it to connection manager
                 if (Ssl::Errors *errNoList = static_cast<Ssl::Errors *>(SSL_get_ex_data(ssl, ssl_ex_index_ssl_error_sslerrno)))
@@ -676,7 +675,7 @@ FwdState::negotiateSSL(int fd)
 
             if (request->flags.sslPeek) {
                 // If possible, set host name to server certificate CN.
-                if (srvX509) {
+                if (X509 *srvX509 = errDetails->brokenCert()) {
                     if (const char *name = Ssl::CommonHostName(srvX509)) {
                         request->SetHost(name);
                         debugs(83, 3, HERE << "reset request host: " << name);

diff --git a/src/ssl/ErrorDetail.cc b/src/ssl/ErrorDetail.cc
index bb49b10815758884a57920c4c5cf7a51fc1bab99..74ce22e117480b296e3ce075c2884276a931d193 100644 (file)
--- a/src/ssl/ErrorDetail.cc
+++ b/src/ssl/ErrorDetail.cc
@@ -346,15 +346,15 @@ const String &Ssl::ErrorDetail::toString() const
     return errDetailStr;
 }
 
-/* We may do not want to use X509_dup but instead
-   internal SSL locking:
-   CRYPTO_add(&(cert->references),1,CRYPTO_LOCK_X509);
-   peer_cert.reset(cert);
-*/
-Ssl::ErrorDetail::ErrorDetail( Ssl::ssl_error_t err_no, X509 *cert): error_no (err_no), lib_error_no(SSL_ERROR_NONE)
+Ssl::ErrorDetail::ErrorDetail( Ssl::ssl_error_t err_no, X509 *cert, X509 *broken): error_no (err_no), lib_error_no(SSL_ERROR_NONE)
 {
     if (cert)
-        peer_cert.reset(X509_dup(cert));
+        peer_cert.resetAndLock(cert);
+
+    if (broken)
+        broken_cert.resetAndLock(broken);
+    else
+        broken_cert.resetAndLock(cert);
 
     detailEntry.error_no = SSL_ERROR_NONE;
 }
@@ -365,7 +365,11 @@ Ssl::ErrorDetail::ErrorDetail(Ssl::ErrorDetail const &anErrDetail)
     request = anErrDetail.request;
 
     if (anErrDetail.peer_cert.get()) {
-        peer_cert.reset(X509_dup(anErrDetail.peer_cert.get()));
+        peer_cert.resetAndLock(anErrDetail.peer_cert.get());
+    }
+
+    if (anErrDetail.broken_cert.get()) {
+        broken_cert.resetAndLock(anErrDetail.broken_cert.get());
     }
 
     detailEntry = anErrDetail.detailEntry;

diff --git a/src/ssl/ErrorDetail.h b/src/ssl/ErrorDetail.h
index 8938551d80a800eb776eda9172c4d2b6f6a79bed..186c006630f6eed6bcf58a45cefb68b38689c75b 100644 (file)
--- a/src/ssl/ErrorDetail.h
+++ b/src/ssl/ErrorDetail.h
@@ -46,7 +46,8 @@ const char *GetErrorDescr(ssl_error_t value);
 class ErrorDetail
 {
 public:
-    ErrorDetail(ssl_error_t err_no, X509 *cert);
+    // if broken certificate is nil, the peer certificate is broken
+    ErrorDetail(ssl_error_t err_no, X509 *peer, X509 *broken);
     ErrorDetail(ErrorDetail const &);
     const String &toString() const;  ///< An error detail string to embed in squid error pages
     void useRequest(HttpRequest *aRequest) { if (aRequest != NULL) request = aRequest;}
@@ -58,6 +59,8 @@ public:
     void setLibError(unsigned long lib_err_no) {lib_error_no = lib_err_no;}
     ///The peer certificate
     X509 *peerCert() { return peer_cert.get(); }
+    /// peer or intermediate certificate that failed validation
+    X509 *brokenCert() {return broken_cert.get(); }
 private:
     typedef const char * (ErrorDetail::*fmt_action_t)() const;
     /**
@@ -87,6 +90,7 @@ private:
     ssl_error_t error_no;   ///< The error code
     unsigned long lib_error_no; ///< low-level error returned by OpenSSL ERR_get_error(3SSL)
     X509_Pointer peer_cert; ///< A pointer to the peer certificate
+    X509_Pointer broken_cert; ///< A pointer to the broken certificate (peer or intermediate)
     mutable ErrorDetailEntry detailEntry;
     HttpRequest::Pointer request;
 };

diff --git a/src/ssl/support.cc b/src/ssl/support.cc
index a41615bff40f1e28cb5839f208e64eafb0367149..d28cb373097e3253256d0fad2206368459b0c2dd 100644 (file)
--- a/src/ssl/support.cc
+++ b/src/ssl/support.cc
@@ -295,7 +295,7 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx)
         }
 
         Ssl::ErrorDetail *errDetail =
-            new Ssl::ErrorDetail(error_no, broken_cert);
+            new Ssl::ErrorDetail(error_no, peer_cert, broken_cert);
 
         if (!SSL_set_ex_data(ssl, ssl_ex_index_ssl_error_detail,  errDetail)) {
             debugs(83, 2, "Failed to set Ssl::ErrorDetail in ssl_verify_cb: Certificate " << buffer);