From d2f0c10659152f0b99e4a21c1040dfc3d260f92e Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Thu, 11 Feb 2021 22:31:00 +0000 Subject: [PATCH] Detail certificate validation errors during TLS handshake (#770) Fix certificate validation error handling in Security::Connect/Accept(). The existing validation details were not propagated/copied to IoResult, requiring the caller to extract them via ssl_ex_index_ssl_error_detail. The clunky approach even required a special "ErrorDetail generations" API to figure out which error detail is "primary": the one received in IoResult or the just extracted one. That API is removed now. This change is used by the upcoming improvements that fetch missing TLS v1.3 server certificates, but it also has an immediate positive effect on the existing reporting of the client certificate validation errors. Currently, only a general TLS error is reported for those cases because Security::Accept() code forgot to check ssl_ex_index_ssl_error_detail. This is a Measurement Factory project. --- src/security/ErrorDetail.cc | 3 --- src/security/ErrorDetail.h | 9 --------- src/security/Io.cc | 11 ++++++++--- src/security/PeerConnector.cc | 25 +++++-------------------- src/ssl/PeekingPeerConnector.cc | 3 +++ 5 files changed, 16 insertions(+), 35 deletions(-) diff --git a/src/security/ErrorDetail.cc b/src/security/ErrorDetail.cc index 7bec4098d6..8803e2ca7b 100644 --- a/src/security/ErrorDetail.cc +++ b/src/security/ErrorDetail.cc @@ -438,11 +438,8 @@ Security::ErrorNameFromCode(const ErrorCode err, const bool prefixRawCode) /* Security::ErrorDetail */ -uint64_t Security::ErrorDetail::Generations = 0; - /// helper constructor implementing the logic shared by the two public ones Security::ErrorDetail::ErrorDetail(const ErrorCode err, const int aSysErrorNo): - generation(++Generations), error_no(err), // We could restrict errno(3) collection to cases where the TLS library // explicitly talks about the errno being set, but correctly detecting those diff --git a/src/security/ErrorDetail.h b/src/security/ErrorDetail.h index 89e3be9947..4bb780b540 100644 --- a/src/security/ErrorDetail.h +++ b/src/security/ErrorDetail.h @@ -55,12 +55,6 @@ public: ErrorDetail(ErrorCode anErrorCode, LibErrorCode aLibErrorNo, int aSysErrorNo); #endif - /// \returns whether we (rather than `them`) should detail ErrorState - bool takesPriorityOver(const ErrorDetail &them) const { - // to reduce pointless updates, return false if us is them - return this->generation < them.generation; - } - /* ErrorDetail API */ virtual SBuf brief() const; virtual SBuf verbose(const HttpRequestPointer &) const; @@ -97,9 +91,6 @@ private: const char *err_lib_error() const; size_t convert(const char *code, const char **value) const; - static uint64_t Generations; ///< the total number of ErrorDetails ever made - uint64_t generation; ///< the number of ErrorDetails made before us plus one - 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/Io.cc b/src/security/Io.cc index cc648e4df4..d6714049b3 100644 --- a/src/security/Io.cc +++ b/src/security/Io.cc @@ -84,9 +84,14 @@ Security::Handshake(Comm::Connection &transport, const ErrorCode topError, Fun i } // now we know that we are dealing with a real problem; detail it - const ErrorDetail::Pointer errorDetail = - new ErrorDetail(topError, ioError, xerrno); - + ErrorDetail::Pointer errorDetail; + if (const auto oldDetail = SSL_get_ex_data(connection, ssl_ex_index_ssl_error_detail)) { + errorDetail = *static_cast(oldDetail); + } else { + errorDetail = new ErrorDetail(topError, ioError, xerrno); + if (const auto serverCert = SSL_get_peer_certificate(connection)) + errorDetail->setPeerCertificate(CertPointer(serverCert)); + } IoResult ioResult(errorDetail); // collect debugging-related details diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc index 83986912fb..8a2a69a138 100644 --- a/src/security/PeerConnector.cc +++ b/src/security/PeerConnector.cc @@ -415,28 +415,13 @@ Security::PeerConnector::noteWantWrite() } void -Security::PeerConnector::noteNegotiationError(const Security::ErrorDetailPointer &callerDetail) +Security::PeerConnector::noteNegotiationError(const Security::ErrorDetailPointer &detail) { - auto primaryDetail = callerDetail; -#if USE_OPENSSL - const auto tlsConnection = fd_table[serverConnection()->fd].ssl.get(); - - // find the highest priority detail - if (const auto storedDetailRaw = SSL_get_ex_data(tlsConnection, ssl_ex_index_ssl_error_detail)) { - const auto &storedDetail = *static_cast(storedDetailRaw); - if (storedDetail->takesPriorityOver(*primaryDetail)) - primaryDetail = storedDetail; - } - - if (!primaryDetail->peerCert()) { - if (const auto serverCert = SSL_get_peer_certificate(tlsConnection)) - primaryDetail->setPeerCertificate(CertPointer(serverCert)); - } -#endif - const auto anErr = ErrorState::NewForwarding(ERR_SECURE_CONNECT_FAIL, request, al); - anErr->xerrno = primaryDetail->sysError(); - anErr->detailError(primaryDetail); + if (detail) { + anErr->xerrno = detail->sysError(); + anErr->detailError(detail); + } noteNegotiationDone(anErr); bail(anErr); } diff --git a/src/ssl/PeekingPeerConnector.cc b/src/ssl/PeekingPeerConnector.cc index 5ea4e4ba91..804148f245 100644 --- a/src/ssl/PeekingPeerConnector.cc +++ b/src/ssl/PeekingPeerConnector.cc @@ -338,6 +338,9 @@ Ssl::PeekingPeerConnector::noteNegotiationError(const Security::ErrorDetailPoint // thus hiding them. // Abort if no certificate found probably because of malformed or // unsupported server Hello message (TODO: make configurable). + // TODO: Add/use a positive "successfully validated server cert" signal + // instead of relying on the "![presumably_]validation_error && serverCert" + // signal combo. if (!SSL_get_ex_data(session.get(), ssl_ex_index_ssl_error_detail) && (srvBio->bumpMode() == Ssl::bumpPeek || srvBio->bumpMode() == Ssl::bumpStare) && srvBio->holdWrite()) { Security::CertPointer serverCert(SSL_get_peer_certificate(session.get())); -- 2.47.2