From: Christos Tsantilas Date: Tue, 17 Oct 2017 14:51:20 +0000 (+0300) Subject: Fixed reporting of validation errors for downloaded intermediate certs. (#72) X-Git-Tag: M-staged-PR71~44 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=8fe1a85a6896f7778ba60754ab6702007969f2d2;p=thirdparty%2Fsquid.git Fixed reporting of validation errors for downloaded intermediate certs. (#72) When Squid or its helper could not validate a downloaded intermediate certificate (or the root certificate), Squid error page contained '[Not available]' instead of the broken certificate details, and '-1' instead of depth of the broken certificate in logs. This is a Measurement Factory project. --- diff --git a/src/ssl/cert_validate_message.cc b/src/ssl/cert_validate_message.cc index 6680053b52..0ff77aac11 100644 --- a/src/ssl/cert_validate_message.cc +++ b/src/ssl/cert_validate_message.cc @@ -16,12 +16,28 @@ #include "ssl/support.h" #include "util.h" +/// Retrieves the certificates chain used to verify the peer. +/// This is the full chain built by OpenSSL while verifying the server +/// certificate or, if this is not available, the chain sent by server. +/// \return the certificates chain or nil +static STACK_OF(X509) * +PeerValidationCertificatesChain(const Security::SessionPointer &ssl) +{ + assert(ssl); + // The full chain built by openSSL while verifying the server cert, + // retrieved from verify callback: + if (const auto certs = static_cast(SSL_get_ex_data(ssl.get(), ssl_ex_index_ssl_cert_chain))) + return certs; + + /// Last resort: certificates chain sent by server + return SSL_get_peer_cert_chain(ssl.get()); // may be nil +} + void Ssl::CertValidationMsg::composeRequest(CertValidationRequest const &vcert) { body.clear(); body += Ssl::CertValidationMsg::param_host + "=" + vcert.domainName; - STACK_OF(X509) *peerCerts = static_cast(SSL_get_ex_data(vcert.ssl.get(), ssl_ex_index_ssl_cert_chain)); if (const char *sslVersion = SSL_get_version(vcert.ssl.get())) body += "\n" + Ssl::CertValidationMsg::param_proto_version + "=" + sslVersion; @@ -29,9 +45,7 @@ Ssl::CertValidationMsg::composeRequest(CertValidationRequest const &vcert) if (const char *cipherName = SSL_CIPHER_get_name(SSL_get_current_cipher(vcert.ssl.get()))) body += "\n" + Ssl::CertValidationMsg::param_cipher + "=" + cipherName; - if (!peerCerts) - peerCerts = SSL_get_peer_cert_chain(vcert.ssl.get()); - + STACK_OF(X509) *peerCerts = PeerValidationCertificatesChain(vcert.ssl); if (peerCerts) { Ssl::BIO_Pointer bio(BIO_new(BIO_s_mem())); for (int i = 0; i < sk_X509_num(peerCerts); ++i) { @@ -75,10 +89,12 @@ get_error_id(const char *label, size_t len) } bool -Ssl::CertValidationMsg::parseResponse(CertValidationResponse &resp, STACK_OF(X509) *peerCerts, std::string &error) +Ssl::CertValidationMsg::parseResponse(CertValidationResponse &resp, std::string &error) { std::vector certs; + const STACK_OF(X509) *peerCerts = PeerValidationCertificatesChain(resp.ssl); + const char *param = body.c_str(); while (*param) { while (xisspace(*param)) param++; diff --git a/src/ssl/cert_validate_message.h b/src/ssl/cert_validate_message.h index ad5b886ca7..1995e25ff0 100644 --- a/src/ssl/cert_validate_message.h +++ b/src/ssl/cert_validate_message.h @@ -59,12 +59,13 @@ public: }; typedef std::vector RecvdErrors; - + explicit CertValidationResponse(const Security::SessionPointer &aSession) : ssl(aSession) {} /// Search in errors list for the error item with id=errorId. /// If none found a new RecvdError item added with the given id; RecvdError &getError(int errorId); RecvdErrors errors; ///< The list of parsed errors Helper::ResultCode resultCode; ///< The helper result code + Security::SessionPointer ssl; }; /** @@ -99,7 +100,7 @@ public: void composeRequest(CertValidationRequest const &vcert); /// Parse a response message and fill the resp object with parsed informations - bool parseResponse(CertValidationResponse &resp, STACK_OF(X509) *peerCerts, std::string &error); + bool parseResponse(CertValidationResponse &resp, std::string &error); /// Search a CertItems list for the certificate with ID "name" X509 *getCertByName(std::vector const &, std::string const & name); diff --git a/src/ssl/helper.cc b/src/ssl/helper.cc index 9d9723b4ab..f625e4d2e9 100644 --- a/src/ssl/helper.cc +++ b/src/ssl/helper.cc @@ -264,11 +264,11 @@ static void sslCrtvdHandleReplyWrapper(void *data, const ::Helper::Reply &reply) { Ssl::CertValidationMsg replyMsg(Ssl::CrtdMessage::REPLY); - Ssl::CertValidationResponse::Pointer validationResponse = new Ssl::CertValidationResponse; std::string error; submitData *crtdvdData = static_cast(data); - STACK_OF(X509) *peerCerts = SSL_get_peer_cert_chain(crtdvdData->ssl.get()); + assert(crtdvdData->ssl.get()); + Ssl::CertValidationResponse::Pointer validationResponse = new Ssl::CertValidationResponse(crtdvdData->ssl); if (reply.result == ::Helper::BrokenHelper) { debugs(83, DBG_IMPORTANT, "\"ssl_crtvd\" helper error response: " << reply.other().content()); validationResponse->resultCode = ::Helper::BrokenHelper; @@ -276,7 +276,7 @@ sslCrtvdHandleReplyWrapper(void *data, const ::Helper::Reply &reply) debugs(83, DBG_IMPORTANT, "\"ssl_crtvd\" helper returned NULL response"); validationResponse->resultCode = ::Helper::BrokenHelper; } else if (replyMsg.parse(reply.other().content(), reply.other().contentSize()) != Ssl::CrtdMessage::OK || - !replyMsg.parseResponse(*validationResponse, peerCerts, error) ) { + !replyMsg.parseResponse(*validationResponse, error) ) { debugs(83, DBG_IMPORTANT, "WARNING: Reply from ssl_crtvd for " << " is incorrect"); debugs(83, DBG_IMPORTANT, "Certificate cannot be validated. ssl_crtvd response: " << replyMsg.getBody()); validationResponse->resultCode = ::Helper::BrokenHelper; @@ -326,7 +326,7 @@ void Ssl::CertValidationHelper::sslSubmit(Ssl::CertValidationRequest const &requ } if (!ssl_crt_validator->trySubmit(crtdvdData->query.c_str(), sslCrtvdHandleReplyWrapper, crtdvdData)) { - Ssl::CertValidationResponse::Pointer resp = new Ssl::CertValidationResponse;; + Ssl::CertValidationResponse::Pointer resp = new Ssl::CertValidationResponse(crtdvdData->ssl); resp->resultCode = ::Helper::BrokenHelper; Ssl::CertValidationHelper::CbDialer *dialer = dynamic_cast(callback->getDialer()); Must(dialer);