From: Christos Tsantilas Date: Thu, 6 Jun 2013 13:53:16 +0000 (+0300) Subject: Tying validation errors to certificates X-Git-Tag: SQUID_3_4_0_1~68 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=62a7607ee3d0a7ffa001fcb8ad53163f94c9b3da;p=thirdparty%2Fsquid.git Tying validation errors to certificates When Squid sends errors to the certificate validation daemon, the daemon cannot tell which certificate caused which error. This is especially bad because the validator has to return that same information in the response (the response format requires the validator to match the error to the certificate). This patch adjust the validation request format to provide that information using a set of the following key=value pairs: error_name_N=the name of the certificate error number N error_cert_N=the ID of the certificate which caused error_name_N where N is non-negative integer. N values start from zero and increase sequentially. This is a Measurement Factory project --- diff --git a/helpers/ssl/cert_valid.pl b/helpers/ssl/cert_valid.pl index e82af6711a..1992563a78 100755 --- a/helpers/ssl/cert_valid.pl +++ b/helpers/ssl/cert_valid.pl @@ -80,7 +80,7 @@ while (<>) { } else { my $readlen = length($body); my %certs = (); - my @errors = (); + my %errors = (); my @responseErrors = (); while($readlen < $bodylen) { @@ -93,12 +93,12 @@ while (<>) { print(STDERR logPrefix()."GOT ". "Code=".$code." $bodylen \n") if ($debug); #.$body; my $hostname; - parseRequest($body, \$hostname, \@errors, \%certs); + parseRequest($body, \$hostname, \%errors, \%certs); print(STDERR logPrefix()."Parse result: \n") if ($debug); print(STDERR logPrefix()."\tFOUND host:".$hostname."\n") if ($debug); print(STDERR logPrefix()."\tFOUND ERRORS:") if ($debug); - foreach my $err (@errors) { - print(STDERR logPrefix()."$err ,") if ($debug); + foreach my $err (keys %errors) { + print(STDERR logPrefix().$errors{$err}{"name"}."/".$errors{$err}{"cert"}." ,") if ($debug); } print(STDERR "\n") if ($debug); foreach my $key (keys %certs) { @@ -110,12 +110,12 @@ while (<>) { my $peerCertId = (keys %certs)[0]; # Echo back the errors: fill the responseErrors array with the errors we read. - foreach my $err (@errors) { + foreach my $err (keys %errors) { $haserror = 1; appendError (\@responseErrors, - $err, #The error name + $errors{$err}{"name"}, #The error name "Checked by Cert Validator", # An error reason - $peerCertId # The cert ID. We are always filling with the peer certificate. + $errors{$err}{"cert"} # The cert ID. We are always filling with the peer certificate. ); } @@ -177,19 +177,25 @@ sub parseRequest $$hostname = $host; $request =~ s/^host=.*\n//; } - if ($request =~ /^errors=/) { - my($vallen) = index($request, "\n"); - my $listerrors = substr($request, 7, $vallen - 7); - @$errors = split /,/, $listerrors; - $request =~ s/^errors=.*\n//; - } - elsif ($request =~ /^cert_(\d+)=/) { + if ($request =~ /^cert_(\d+)=/) { my $certId = "cert_".$1; my($vallen) = index($request, "-----END CERTIFICATE-----") + length("-----END CERTIFICATE-----"); my $x509 = Crypt::OpenSSL::X509->new_from_string(substr($request, index($request, "-----BEGIN"))); $certs->{$certId} = $x509; $request = substr($request, $vallen); } + elsif ($request =~ /^error_name_(\d+)=(.*)$/m) { + my $errorId = $1; + my $errorName = $2; + $request =~ s/^error_name_\d+=.*$//m; + $errors->{$errorId}{"name"} = $errorName; + } + elsif ($request =~ /^error_cert_(\d+)=(.*)$/m) { + my $errorId = $1; + my $certId = $2; + $request =~ s/^error_cert_\d+=.*$//m; + $errors->{$errorId}{"cert"} = $certId; + } else { print(STDERR logPrefix()."ParseError on \"".$request."\"\n") if ($debug); $request = "";# finish processing.... diff --git a/src/AclRegs.cc b/src/AclRegs.cc index 54656f96b8..cd7387497c 100644 --- a/src/AclRegs.cc +++ b/src/AclRegs.cc @@ -149,7 +149,7 @@ ACLStrategised ACLUrlPort::RegistryEntry_(new ACLIntRange, ACLUrlPortStrate #if USE_SSL ACL::Prototype ACLSslError::RegistryProtoype(&ACLSslError::RegistryEntry_, "ssl_error"); -ACLStrategised ACLSslError::RegistryEntry_(new ACLSslErrorData, ACLSslErrorStrategy::Instance(), "ssl_error"); +ACLStrategised ACLSslError::RegistryEntry_(new ACLSslErrorData, ACLSslErrorStrategy::Instance(), "ssl_error"); ACL::Prototype ACLCertificate::UserRegistryProtoype(&ACLCertificate::UserRegistryEntry_, "user_cert"); ACLStrategised ACLCertificate::UserRegistryEntry_(new ACLCertificateData (Ssl::GetX509UserAttribute, "*"), ACLCertificateStrategy::Instance(), "user_cert"); ACL::Prototype ACLCertificate::CARegistryProtoype(&ACLCertificate::CARegistryEntry_, "ca_cert"); diff --git a/src/acl/FilledChecklist.h b/src/acl/FilledChecklist.h index c54ad40d75..05131181ac 100644 --- a/src/acl/FilledChecklist.h +++ b/src/acl/FilledChecklist.h @@ -70,7 +70,7 @@ public: #if USE_SSL /// SSL [certificate validation] errors, in undefined order - Ssl::Errors *sslErrors; + Ssl::CertErrors *sslErrors; /// The peer certificate Ssl::X509_Pointer serverCert; #endif diff --git a/src/acl/SslError.h b/src/acl/SslError.h index c6b5b25649..ea71844062 100644 --- a/src/acl/SslError.h +++ b/src/acl/SslError.h @@ -4,7 +4,7 @@ #include "acl/Strategised.h" #include "ssl/support.h" -class ACLSslErrorStrategy : public ACLStrategy +class ACLSslErrorStrategy : public ACLStrategy { public: @@ -27,7 +27,7 @@ class ACLSslError private: static ACL::Prototype RegistryProtoype; - static ACLStrategised RegistryEntry_; + static ACLStrategised RegistryEntry_; }; #endif /* SQUID_ACLSSL_ERROR_H */ diff --git a/src/acl/SslErrorData.cc b/src/acl/SslErrorData.cc index d9f07200fe..2de5e2394f 100644 --- a/src/acl/SslErrorData.cc +++ b/src/acl/SslErrorData.cc @@ -52,10 +52,10 @@ ACLSslErrorData::~ACLSslErrorData() } bool -ACLSslErrorData::match(const Ssl::Errors *toFind) +ACLSslErrorData::match(const Ssl::CertErrors *toFind) { - for (const Ssl::Errors *err = toFind; err; err = err->next ) { - if (values->findAndTune(err->element)) + for (const Ssl::CertErrors *err = toFind; err; err = err->next ) { + if (values->findAndTune(err->element.code)) return true; } return false; diff --git a/src/acl/SslErrorData.h b/src/acl/SslErrorData.h index 2ee298d156..70f1c14223 100644 --- a/src/acl/SslErrorData.h +++ b/src/acl/SslErrorData.h @@ -7,7 +7,7 @@ #include "ssl/ErrorDetail.h" #include -class ACLSslErrorData : public ACLData +class ACLSslErrorData : public ACLData { public: @@ -17,7 +17,7 @@ public: ACLSslErrorData(ACLSslErrorData const &); ACLSslErrorData &operator= (ACLSslErrorData const &); virtual ~ACLSslErrorData(); - bool match(const Ssl::Errors *); + bool match(const Ssl::CertErrors *); wordlist *dump(); void parse(); bool empty() const; diff --git a/src/client_side.cc b/src/client_side.cc index 64cc98c978..c4e3106198 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -2606,16 +2606,16 @@ bool ConnStateData::serveDelayedError(ClientSocketContext *context) // In bump-server-first mode, we have not necessarily seen the intended // server name at certificate-peeking time. Check for domain mismatch now, // when we can extract the intended name from the bumped HTTP request. - if (sslServerBump->serverCert.get()) { + if (X509 *srvCert = sslServerBump->serverCert.get()) { HttpRequest *request = http->request; - if (!Ssl::checkX509ServerValidity(sslServerBump->serverCert.get(), request->GetHost())) { + if (!Ssl::checkX509ServerValidity(srvCert, request->GetHost())) { debugs(33, 2, "SQUID_X509_V_ERR_DOMAIN_MISMATCH: Certificate " << "does not match domainname " << request->GetHost()); bool allowDomainMismatch = false; if (Config.ssl_client.cert_error) { ACLFilledChecklist check(Config.ssl_client.cert_error, request, dash_str); - check.sslErrors = new Ssl::Errors(SQUID_X509_V_ERR_DOMAIN_MISMATCH); + check.sslErrors = new Ssl::CertErrors(Ssl::CertError(SQUID_X509_V_ERR_DOMAIN_MISMATCH, srvCert)); allowDomainMismatch = (check.fastCheck() == ACCESS_ALLOWED); delete check.sslErrors; check.sslErrors = NULL; @@ -2637,7 +2637,7 @@ bool ConnStateData::serveDelayedError(ClientSocketContext *context) err->src_addr = clientConnection->remote; Ssl::ErrorDetail *errDetail = new Ssl::ErrorDetail( SQUID_X509_V_ERR_DOMAIN_MISMATCH, - sslServerBump->serverCert.get(), NULL); + srvCert, NULL); err->detail = errDetail; // Save the original request for logging purposes. if (!context->http->al->request) { diff --git a/src/forward.cc b/src/forward.cc index c56d5f92c0..8f309573bf 100644 --- a/src/forward.cc +++ b/src/forward.cc @@ -710,7 +710,7 @@ FwdState::negotiateSSL(int fd) serverBump->serverCert.resetAndLock(errDetails->peerCert()); // remember validation errors, if any - if (Ssl::Errors *errs = static_cast(SSL_get_ex_data(ssl, ssl_ex_index_ssl_errors))) + if (Ssl::CertErrors *errs = static_cast(SSL_get_ex_data(ssl, ssl_ex_index_ssl_errors))) serverBump->sslErrors = cbdataReference(errs); } } @@ -748,7 +748,7 @@ FwdState::negotiateSSL(int fd) serverBump->serverCert.reset(SSL_get_peer_certificate(ssl)); // remember validation errors, if any - if (Ssl::Errors *errs = static_cast(SSL_get_ex_data(ssl, ssl_ex_index_ssl_errors))) + if (Ssl::CertErrors *errs = static_cast(SSL_get_ex_data(ssl, ssl_ex_index_ssl_errors))) serverBump->sslErrors = cbdataReference(errs); } } @@ -768,7 +768,7 @@ FwdState::negotiateSSL(int fd) // Ssl::CertValidationHelper::submit method. validationRequest.ssl = ssl; validationRequest.domainName = request->GetHost(); - if (Ssl::Errors *errs = static_cast(SSL_get_ex_data(ssl, ssl_ex_index_ssl_errors))) + if (Ssl::CertErrors *errs = static_cast(SSL_get_ex_data(ssl, ssl_ex_index_ssl_errors))) // validationRequest disappears on return so no need to cbdataReference validationRequest.errors = errs; else @@ -807,7 +807,7 @@ FwdState::sslCrtvdHandleReplyWrapper(void *data, Ssl::CertValidationResponse con void FwdState::sslCrtvdHandleReply(Ssl::CertValidationResponse const &validationResponse) { - Ssl::Errors *errs = NULL; + Ssl::CertErrors *errs = NULL; Ssl::ErrorDetail *errDetails = NULL; bool validatorFailed = false; if (!Comm::IsConnOpen(serverConnection())) { @@ -860,11 +860,11 @@ FwdState::sslCrtvdHandleReply(Ssl::CertValidationResponse const &validationRespo /// Checks errors in the cert. validator response against sslproxy_cert_error. /// The first honored error, if any, is returned via errDetails parameter. -/// The method returns all seen errors except SSL_ERROR_NONE as Ssl::Errors. -Ssl::Errors * +/// The method returns all seen errors except SSL_ERROR_NONE as Ssl::CertErrors. +Ssl::CertErrors * FwdState::sslCrtvdCheckForErrors(Ssl::CertValidationResponse const &resp, Ssl::ErrorDetail *& errDetails) { - Ssl::Errors *errs = NULL; + Ssl::CertErrors *errs = NULL; ACLFilledChecklist *check = NULL; if (acl_access *acl = Config.ssl_client.cert_error) @@ -880,7 +880,7 @@ FwdState::sslCrtvdCheckForErrors(Ssl::CertValidationResponse const &resp, Ssl::E if (!errDetails) { bool allowed = false; if (check) { - check->sslErrors = new Ssl::Errors(i->error_no); + check->sslErrors = new Ssl::CertErrors(Ssl::CertError(i->error_no, i->cert.get())); if (check->fastCheck() == ACCESS_ALLOWED) allowed = true; } @@ -903,9 +903,9 @@ FwdState::sslCrtvdCheckForErrors(Ssl::CertValidationResponse const &resp, Ssl::E } if (!errs) - errs = new Ssl::Errors(i->error_no); + errs = new Ssl::CertErrors(Ssl::CertError(i->error_no, i->cert.get())); else - errs->push_back_unique(i->error_no); + errs->push_back_unique(Ssl::CertError(i->error_no, i->cert.get())); } if (check) delete check; diff --git a/src/forward.h b/src/forward.h index 239888537c..a1b70f2a2c 100644 --- a/src/forward.h +++ b/src/forward.h @@ -90,7 +90,7 @@ public: /// Process response from cert validator helper void sslCrtvdHandleReply(Ssl::CertValidationResponse const &); /// Check SSL errors returned from cert validator against sslproxy_cert_error access list - Ssl::Errors *sslCrtvdCheckForErrors(Ssl::CertValidationResponse const &, Ssl::ErrorDetail *&); + Ssl::CertErrors *sslCrtvdCheckForErrors(Ssl::CertValidationResponse const &, Ssl::ErrorDetail *&); #endif private: // hidden for safer management of self; use static fwdStart diff --git a/src/ssl/ServerBump.h b/src/ssl/ServerBump.h index f214e726aa..c7955d13be 100644 --- a/src/ssl/ServerBump.h +++ b/src/ssl/ServerBump.h @@ -27,7 +27,7 @@ public: HttpRequest::Pointer request; StoreEntry *entry; ///< for receiving Squid-generated error messages Ssl::X509_Pointer serverCert; ///< HTTPS server certificate - Ssl::Errors *sslErrors; ///< SSL [certificate validation] errors + Ssl::CertErrors *sslErrors; ///< SSL [certificate validation] errors private: store_client *sc; ///< dummy client to prevent entry trimming diff --git a/src/ssl/cert_validate_message.cc b/src/ssl/cert_validate_message.cc index 77f7c6db3b..4559b0a0ea 100644 --- a/src/ssl/cert_validate_message.cc +++ b/src/ssl/cert_validate_message.cc @@ -10,36 +10,38 @@ Ssl::CertValidationMsg::composeRequest(CertValidationRequest const &vcert) { body.clear(); body += Ssl::CertValidationMsg::param_host + "=" + vcert.domainName; - if (vcert.errors) { - body += "\n" + Ssl::CertValidationMsg::param_error + "="; - bool comma = false; - for (const Ssl::Errors *err = vcert.errors; err; err = err->next ) { - if (comma) - body += ","; - body += GetErrorName(err->element); - comma = true; - } - } - STACK_OF(X509) *peerCerts = SSL_get_peer_cert_chain(vcert.ssl); if (peerCerts) { - body +="\n"; Ssl::BIO_Pointer bio(BIO_new(BIO_s_mem())); for (int i = 0; i < sk_X509_num(peerCerts); ++i) { X509 *cert = sk_X509_value(peerCerts, i); PEM_write_bio_X509(bio.get(), cert); - body = body + "cert_" + xitoa(i) + "="; + body = body + "\n" + param_cert + xitoa(i) + "="; char *ptr; long len = BIO_get_mem_data(bio.get(), &ptr); - body.append(ptr, len); - // Normally openssl toolkit terminates Certificate with a '\n'. - if (ptr[len-1] != '\n') - body +="\n"; + body.append(ptr, (ptr[len-1] == '\n' ? len - 1 : len)); if (!BIO_reset(bio.get())) { // print an error? } } } + + if (vcert.errors) { + int i = 0; + for (const Ssl::CertErrors *err = vcert.errors; err; err = err->next, ++i) { + body +="\n"; + body = body + param_error_name + xitoa(i) + "=" + GetErrorName(err->element.code) + "\n"; + int errorCertPos = -1; + if (err->element.cert.get()) + errorCertPos = sk_X509_find(peerCerts, err->element.cert.get()); + if (errorCertPos < 0) { + // assert this error ? + debugs(83, 4, "WARNING: wrong cert in cert validator request"); + } + body += param_error_cert + xitoa(i) + "="; + body += param_cert + xitoa((errorCertPos >= 0 ? errorCertPos : 0)); + } + } } static int @@ -212,7 +214,6 @@ Ssl::CertValidationMsg::CertItem::setCert(X509 *aCert) const std::string Ssl::CertValidationMsg::code_cert_validate("cert_validate"); const std::string Ssl::CertValidationMsg::param_domain("domain"); -const std::string Ssl::CertValidationMsg::param_error("errors"); const std::string Ssl::CertValidationMsg::param_cert("cert_"); const std::string Ssl::CertValidationMsg::param_error_name("error_name_"); const std::string Ssl::CertValidationMsg::param_error_reason("error_reason_"); diff --git a/src/ssl/cert_validate_message.h b/src/ssl/cert_validate_message.h index 26c5c3ede3..9f92aa991d 100644 --- a/src/ssl/cert_validate_message.h +++ b/src/ssl/cert_validate_message.h @@ -20,7 +20,7 @@ class CertValidationRequest { public: SSL *ssl; - Errors *errors; ///< The list of errors detected + CertErrors *errors; ///< The list of errors detected std::string domainName; ///< The server name CertValidationRequest() : ssl(NULL), errors(NULL) {} }; @@ -99,8 +99,6 @@ public: static const std::string code_cert_validate; /// Parameter name for passing intended domain name static const std::string param_domain; - /// Parameter name for passing SSL errors - static const std::string param_error; /// Parameter name for passing SSL certificates static const std::string param_cert; /// Parameter name for passing the major SSL error diff --git a/src/ssl/support.cc b/src/ssl/support.cc index 405cabd541..105d1e322e 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -261,16 +261,20 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) } if (!ok) { - Ssl::Errors *errs = static_cast(SSL_get_ex_data(ssl, ssl_ex_index_ssl_errors)); + X509 *broken_cert = X509_STORE_CTX_get_current_cert(ctx); + if (!broken_cert) + broken_cert = peer_cert; + + Ssl::CertErrors *errs = static_cast(SSL_get_ex_data(ssl, ssl_ex_index_ssl_errors)); if (!errs) { - errs = new Ssl::Errors(error_no); + errs = new Ssl::CertErrors(Ssl::CertError(error_no, broken_cert)); if (!SSL_set_ex_data(ssl, ssl_ex_index_ssl_errors, (void *)errs)) { debugs(83, 2, "Failed to set ssl error_no in ssl_verify_cb: Certificate " << buffer); delete errs; errs = NULL; } } else // remember another error number - errs->push_back_unique(error_no); + errs->push_back_unique(Ssl::CertError(error_no, broken_cert)); if (const char *err_descr = Ssl::GetErrorDescr(error_no)) debugs(83, 5, err_descr << ": " << buffer); @@ -280,7 +284,7 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) if (check) { ACLFilledChecklist *filledCheck = Filled(check); assert(!filledCheck->sslErrors); - filledCheck->sslErrors = new Ssl::Errors(error_no); + filledCheck->sslErrors = new Ssl::CertErrors(Ssl::CertError(error_no, broken_cert)); filledCheck->serverCert.resetAndLock(peer_cert); if (check->fastCheck() == ACCESS_ALLOWED) { debugs(83, 3, "bypassing SSL error " << error_no << " in " << buffer); @@ -635,7 +639,7 @@ static void ssl_free_SslErrors(void *, void *ptr, CRYPTO_EX_DATA *, int, long, void *) { - Ssl::Errors *errs = static_cast (ptr); + Ssl::CertErrors *errs = static_cast (ptr); delete errs; } @@ -1596,4 +1600,34 @@ bool Ssl::generateUntrustedCert(X509_Pointer &untrustedCert, EVP_PKEY_Pointer &u return Ssl::generateSslCertificate(untrustedCert, untrustedPkey, certProperties); } +Ssl::CertError::CertError(ssl_error_t anErr, X509 *aCert): code(anErr) +{ + cert.resetAndLock(aCert); +} + +Ssl::CertError::CertError(CertError const &err): code(err.code) +{ + cert.resetAndLock(err.cert.get()); +} + +Ssl::CertError & +Ssl::CertError::operator = (const CertError &old) +{ + code = old.code; + cert.resetAndLock(old.cert.get()); + return *this; +} + +bool +Ssl::CertError::operator == (const CertError &ce) const +{ + return code == ce.code && cert.get() == ce.cert.get(); +} + +bool +Ssl::CertError::operator != (const CertError &ce) const +{ + return code != ce.code || cert.get() != ce.cert.get(); +} + #endif /* USE_SSL */ diff --git a/src/ssl/support.h b/src/ssl/support.h index f0f24c6eb6..e2ea797c4b 100644 --- a/src/ssl/support.h +++ b/src/ssl/support.h @@ -74,6 +74,22 @@ typedef int ssl_error_t; typedef CbDataList Errors; +/// An SSL certificate-related error. +/// Pairs an error code with the certificate experiencing the error. +class CertError { +public: + ssl_error_t code; ///< certificate error code + X509_Pointer cert; ///< certificate with the above error code + CertError(ssl_error_t anErr, X509 *aCert); + CertError(CertError const &err); + CertError & operator = (const CertError &old); + bool operator == (const CertError &ce) const; + bool operator != (const CertError &ce) const; +}; + +/// Holds a list of certificate SSL errors +typedef CbDataList CertErrors; + } //namespace Ssl /// \ingroup ServerProtocolSSLAPI