From: Alex Rousskov Date: Fri, 20 May 2022 18:33:42 +0000 (+0000) Subject: TLS library-agnostic X509 certificate interrogation functions (#1057) X-Git-Tag: SQUID_6_0_1~181 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=907831e6daa8d86d4b2e9cfd6e7336fcfdfbd591;p=thirdparty%2Fsquid.git TLS library-agnostic X509 certificate interrogation functions (#1057) Use added X509_check_issued() replacements. The only case left is in src/ssl/gadgets.cc which is used by certificate helpers that cannot be linked with libsecurity yet. Use added X509_NAME_oneline() replacements, where feasible. This change speeds up ssl_verify_cb() and other functions that used to extract and copy certificate name into a buffer even when that name was unused because debugging levels were not elevated enough, including by default. Also fixes memory leak when debugging section 83 at level 3+ of an OpenSSL-using Squid (missing name cleanup in clientNegotiateSSL()). Also fixes a (usually symptom-free) sslcrtd bug: C strings allocated by OpenSSL were freed by xfree() instead of OPENSSL_free(). Co-authored-by: Amos Jeffries --- diff --git a/src/client_side.cc b/src/client_side.cc index f85594fe51..5b61e0a710 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -107,6 +107,7 @@ #include "proxyp/Header.h" #include "proxyp/Parser.h" #include "sbuf/Stream.h" +#include "security/Certificate.h" #include "security/CommunicationSecrets.h" #include "security/Io.h" #include "security/KeyLog.h" @@ -2423,10 +2424,10 @@ clientNegotiateSSL(int fd, void *data) if (client_cert) { debugs(83, 3, "FD " << fd << " client certificate: subject: " << - X509_NAME_oneline(X509_get_subject_name(client_cert), 0, 0)); + Security::SubjectName(*client_cert)); debugs(83, 3, "FD " << fd << " client certificate: issuer: " << - X509_NAME_oneline(X509_get_issuer_name(client_cert), 0, 0)); + Security::IssuerName(*client_cert)); X509_free(client_cert); } else { diff --git a/src/format/Format.cc b/src/format/Format.cc index f0296569dc..1316eebd38 100644 --- a/src/format/Format.cc +++ b/src/format/Format.cc @@ -25,6 +25,7 @@ #include "sbuf/Stream.h" #include "sbuf/StringConvert.h" #include "security/CertError.h" +#include "security/Certificate.h" #include "security/NegotiationHistory.h" #include "Store.h" #include "tools.h" @@ -1253,20 +1254,16 @@ Format::Format::assemble(MemBuf &mb, const AccessLogEntry::Pointer &al, int logS break; case LFT_SSL_USER_CERT_SUBJECT: - if (X509 *cert = al->cache.sslClientCert.get()) { - if (X509_NAME *subject = X509_get_subject_name(cert)) { - X509_NAME_oneline(subject, tmp, sizeof(tmp)); - out = tmp; - } + if (const auto &cert = al->cache.sslClientCert) { + sb = Security::SubjectName(*cert); + out = sb.c_str(); } break; case LFT_SSL_USER_CERT_ISSUER: - if (X509 *cert = al->cache.sslClientCert.get()) { - if (X509_NAME *issuer = X509_get_issuer_name(cert)) { - X509_NAME_oneline(issuer, tmp, sizeof(tmp)); - out = tmp; - } + if (const auto &cert = al->cache.sslClientCert) { + sb = Security::IssuerName(*cert); + out = sb.c_str(); } break; diff --git a/src/security/Certificate.cc b/src/security/Certificate.cc new file mode 100644 index 0000000000..47c932ec0b --- /dev/null +++ b/src/security/Certificate.cc @@ -0,0 +1,141 @@ +/* + * Copyright (C) 1996-2022 The Squid Software Foundation and contributors + * + * Squid software is distributed under GPLv2+ license and includes + * contributions from numerous individuals and organizations. + * Please see the COPYING and CONTRIBUTORS files for details. + */ + +#include "squid.h" +#include "debug/Stream.h" +#include "sbuf/SBuf.h" +#include "security/Certificate.h" + +#if USE_OPENSSL +#include "ssl/gadgets.h" +#endif + +#include + +inline +const char * +MissingLibraryError() +{ + return "[need OpenSSL or GnuTLS]"; +} + +SBuf +Security::IssuerName(Certificate &cert) +{ + SBuf out; + +#if USE_OPENSSL + Ssl::ForgetErrors(); + const auto name = Ssl::OneLineSummary(*X509_get_issuer_name(&cert)); + if (!name) { + debugs(83, DBG_PARSE_NOTE(2), "WARNING: cannot get certificate Issuer:" << + Ssl::ReportAndForgetErrors); + return out; + } + out.append(name.get()); + +#elif USE_GNUTLS + gnutls_x509_dn_t issuer; + auto x = gnutls_x509_crt_get_issuer(&cert, &issuer); + if (x != GNUTLS_E_SUCCESS) { + debugs(83, DBG_PARSE_NOTE(2), "WARNING: cannot get certificate Issuer: " << ErrorString(x)); + return out; + } + + gnutls_datum_t name; + x = gnutls_x509_dn_get_str(issuer, &name); + if (x != GNUTLS_E_SUCCESS) { + debugs(83, DBG_PARSE_NOTE(2), "WARNING: cannot describe certificate Issuer: " << ErrorString(x)); + return out; + } + out.append(reinterpret_cast(name.data), name.size); + gnutls_free(name.data); + +#else + debugs(83, DBG_PARSE_NOTE(2), "WARNING: cannot get certificate Issuer: " << MissingLibraryError()); + (void)cert; +#endif + + return out; +} + +SBuf +Security::SubjectName(Certificate &cert) +{ + SBuf out; + +#if USE_OPENSSL + Ssl::ForgetErrors(); + const auto name = Ssl::OneLineSummary(*X509_get_subject_name(&cert)); + if (!name) { + debugs(83, DBG_PARSE_NOTE(2), "WARNING: cannot get certificate SubjectName:" << + Ssl::ReportAndForgetErrors); + return out; + } + out.append(name.get()); + +#elif USE_GNUTLS + gnutls_x509_dn_t subject; + auto x = gnutls_x509_crt_get_subject(&cert, &subject); + if (x != GNUTLS_E_SUCCESS) { + debugs(83, DBG_PARSE_NOTE(2), "WARNING: cannot get certificate SubjectName: " << ErrorString(x)); + return out; + } + + gnutls_datum_t name; + x = gnutls_x509_dn_get_str(subject, &name); + if (x != GNUTLS_E_SUCCESS) { + debugs(83, DBG_PARSE_NOTE(2), "WARNING: cannot describe certificate SubjectName: " << ErrorString(x)); + return out; + } + out.append(reinterpret_cast(name.data), name.size); + gnutls_free(name.data); + +#else + debugs(83, DBG_PARSE_NOTE(2), "WARNING: cannot get certificate SubjectName: " << MissingLibraryError()); + (void)cert; +#endif + + return out; +} + +bool +Security::IssuedBy(Certificate &cert, Certificate &issuer) +{ +#if USE_OPENSSL + Ssl::ForgetErrors(); + const auto result = X509_check_issued(&issuer, &cert); + if (result == X509_V_OK) + return true; + debugs(83, DBG_PARSE_NOTE(3), issuer << " did not sign " << cert << ":" << + Debug::Extra << "X509_check_issued() result: " << X509_verify_cert_error_string(result) << " (" << result << ")" << + Ssl::ReportAndForgetErrors); +#elif USE_GNUTLS + const auto result = gnutls_x509_crt_check_issuer(&cert, &issuer); + if (result == 1) + return true; + debugs(83, DBG_PARSE_NOTE(3), issuer << " did not sign " << cert); +#else + debugs(83, DBG_PARSE_NOTE(2), "WARNING: cannot determine certificates relationship: " << MissingLibraryError()); + (void)cert; + (void)issuer; +#endif + return false; +} + +std::ostream & +operator <<(std::ostream &os, Security::Certificate &cert) +{ + const auto name = Security::SubjectName(cert); + if (name.isEmpty()) + os << "[no subject name]"; + else + os << name; + return os; +} + diff --git a/src/security/Certificate.h b/src/security/Certificate.h new file mode 100644 index 0000000000..99d04f5441 --- /dev/null +++ b/src/security/Certificate.h @@ -0,0 +1,43 @@ +/* + * Copyright (C) 1996-2022 The Squid Software Foundation and contributors + * + * Squid software is distributed under GPLv2+ license and includes + * contributions from numerous individuals and organizations. + * Please see the COPYING and CONTRIBUTORS files for details. + */ + +#ifndef SQUID_SRC_SECURITY_CERTIFICATE_H +#define SQUID_SRC_SECURITY_CERTIFICATE_H + +#include "security/forward.h" + +// The accessing/testing functions below require a non-constant Certificate when +// it is modified by an underlying library implementation (e.g., GnuTLS). + +namespace Security +{ + +/// The SubjectName field of the given certificate (if found) or an empty SBuf. +SBuf SubjectName(Certificate &); + +/// The Issuer field of the given certificate (if found) or an empty SBuf. +SBuf IssuerName(Certificate &); + +/// Whether cert was (correctly) issued by the given issuer. +/// Due to complexity of the underlying checks, it is impossible to clearly +/// distinguish pure negative answers (e.g., two independent certificates) +/// from errors (e.g., the issuer certificate lacks the right CA extension). +bool IssuedBy(Certificate &cert, Certificate &issuer); + +/// Whether the given certificate is self-signed. +inline bool SelfSigned(Certificate &c) { return IssuedBy(c, c); } + +} // namespace Security + +// Declared outside Security because all underlying Security::Certificate types +// are declared inside global namespace. +/// reports a one-line gist of the Certificate Subject Name (for debugging) +std::ostream &operator <<(std::ostream &, Security::Certificate &); + +#endif /* SQUID_SRC_SECURITY_CERTIFICATE_H */ + diff --git a/src/security/ErrorDetail.cc b/src/security/ErrorDetail.cc index 129c4a41d0..de81e27dd9 100644 --- a/src/security/ErrorDetail.cc +++ b/src/security/ErrorDetail.cc @@ -12,6 +12,7 @@ #include "html_quote.h" #include "sbuf/SBuf.h" #include "sbuf/Stream.h" +#include "security/Certificate.h" #include "security/ErrorDetail.h" #include "security/forward.h" #include "security/Io.h" @@ -561,16 +562,14 @@ Security::ErrorDetail::verbose(const HttpRequestPointer &request) const const char * Security::ErrorDetail::subject() const { -#if USE_OPENSSL - if (broken_cert.get()) { - static char tmpBuffer[256]; // A temporary buffer - if (X509_NAME_oneline(X509_get_subject_name(broken_cert.get()), tmpBuffer, sizeof(tmpBuffer))) { + if (broken_cert) { + auto buf = SubjectName(*broken_cert); + if (!buf.isEmpty()) { // quote to avoid possible html code injection through // certificate subject - return html_quote(tmpBuffer); + return html_quote(buf.c_str()); } } -#endif // USE_OPENSSL return "[Not available]"; } @@ -614,16 +613,14 @@ Security::ErrorDetail::cn() const const char * Security::ErrorDetail::ca_name() const { -#if USE_OPENSSL - if (broken_cert.get()) { - static char tmpBuffer[256]; // A temporary buffer - if (X509_NAME_oneline(X509_get_issuer_name(broken_cert.get()), tmpBuffer, sizeof(tmpBuffer))) { + if (broken_cert) { + auto buf = IssuerName(*broken_cert); + if (!buf.isEmpty()) { // quote to avoid possible html code injection through // certificate issuer subject - return html_quote(tmpBuffer); + return html_quote(buf.c_str()); } } -#endif // USE_OPENSSL return "[Not available]"; } diff --git a/src/security/KeyData.cc b/src/security/KeyData.cc index 8e3b577acc..e12d0bbba8 100644 --- a/src/security/KeyData.cc +++ b/src/security/KeyData.cc @@ -9,6 +9,7 @@ #include "squid.h" #include "anyp/PortCfg.h" #include "fatal.h" +#include "security/Certificate.h" #include "security/KeyData.h" #include "SquidConfig.h" #include "ssl/bio.h" @@ -97,10 +98,8 @@ Security::KeyData::loadX509ChainFromFile() } #if TLS_CHAIN_NO_SELFSIGNED // ignore self-signed certs in the chain - if (X509_check_issued(cert.get(), cert.get()) == X509_V_OK) { - char *nameStr = X509_NAME_oneline(X509_get_subject_name(cert.get()), nullptr, 0); - debugs(83, DBG_PARSE_NOTE(2), "Certificate is self-signed, will not be chained: " << nameStr); - OPENSSL_free(nameStr); + if (SelfSigned(*cert)) { + debugs(83, DBG_PARSE_NOTE(2), "Certificate is self-signed, will not be chained: " << *cert); } else #endif { @@ -109,29 +108,24 @@ Security::KeyData::loadX509ChainFromFile() CertPointer latestCert = cert; while (const auto ca = Ssl::ReadOptionalCertificate(bio)) { - // get Issuer name of the cert for debug display - char *nameStr = X509_NAME_oneline(X509_get_subject_name(ca.get()), nullptr, 0); #if TLS_CHAIN_NO_SELFSIGNED // ignore self-signed certs in the chain // self-signed certificates are not valid in a sent chain - if (X509_check_issued(ca.get(), ca.get()) == X509_V_OK) { - debugs(83, DBG_PARSE_NOTE(2), "CA " << nameStr << " is self-signed, will not be chained: " << nameStr); - OPENSSL_free(nameStr); + if (SelfSigned(*ca)) { + debugs(83, DBG_PARSE_NOTE(2), "CA certificate is self-signed, will not be chained: " << *ca); continue; } #endif // checks that the chained certs are actually part of a chain for validating cert - const auto checkCode = X509_check_issued(ca.get(), latestCert.get()); - if (checkCode == X509_V_OK) { - debugs(83, DBG_PARSE_NOTE(3), "Adding issuer CA: " << nameStr); + if (IssuedBy(*latestCert, *ca)) { + debugs(83, DBG_PARSE_NOTE(3), "Adding issuer CA: " << *ca); // OpenSSL API requires that we order certificates such that the // chain can be appended directly into the on-wire traffic. latestCert = CertPointer(ca); chain.emplace_back(latestCert); } else { - debugs(83, DBG_PARSE_NOTE(2), certFile << ": Ignoring non-issuer CA " << nameStr << ": " << X509_verify_cert_error_string(checkCode) << " (" << checkCode << ")"); + debugs(83, DBG_PARSE_NOTE(2), certFile << ": Ignoring non-issuer CA " << *ca); } - OPENSSL_free(nameStr); } } diff --git a/src/security/LockingPointer.h b/src/security/LockingPointer.h index b6f6e8a53a..93c973099b 100644 --- a/src/security/LockingPointer.h +++ b/src/security/LockingPointer.h @@ -9,6 +9,7 @@ #ifndef SQUID_SRC_SECURITY_LOCKINGPOINTER_H #define SQUID_SRC_SECURITY_LOCKINGPOINTER_H +#include "base/Assure.h" #include "base/HardFun.h" #if USE_OPENSSL @@ -100,6 +101,7 @@ public: bool operator ==(const SelfType &o) const { return (o.get() == raw); } bool operator !=(const SelfType &o) const { return (o.get() != raw); } + T &operator *() const { Assure(raw); return *raw; } T *operator ->() const { return raw; } /// Returns raw and possibly nullptr pointer diff --git a/src/security/Makefile.am b/src/security/Makefile.am index e483bd55f7..fb365ac686 100644 --- a/src/security/Makefile.am +++ b/src/security/Makefile.am @@ -16,6 +16,8 @@ libsecurity_la_SOURCES = \ BlindPeerConnector.cc \ BlindPeerConnector.h \ CertError.h \ + Certificate.cc \ + Certificate.h \ CommunicationSecrets.cc \ CommunicationSecrets.h \ Context.h \ diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc index 27c6ceedb8..73ac79d3ee 100644 --- a/src/security/PeerConnector.cc +++ b/src/security/PeerConnector.cc @@ -22,6 +22,7 @@ #include "neighbors.h" #include "pconn.h" #include "security/Io.h" +#include "security/Certificate.h" #include "security/NegotiationHistory.h" #include "security/PeerConnector.h" #include "SquidConfig.h" @@ -669,8 +670,7 @@ Security::PeerConnector::certDownloadingDone(SBuf &obj, int downloadStatus) // TODO: support collection of certificates const unsigned char *raw = (const unsigned char*)obj.rawContent(); if (X509 *cert = d2i_X509(NULL, &raw, obj.length())) { - char buffer[1024]; - debugs(81, 5, "Retrieved certificate: " << X509_NAME_oneline(X509_get_subject_name(cert), buffer, 1024)); + debugs(81, 5, "Retrieved certificate: " << *cert); if (!downloadedCerts) downloadedCerts.reset(sk_X509_new_null()); @@ -680,14 +680,12 @@ Security::PeerConnector::certDownloadingDone(SBuf &obj, int downloadStatus) const auto certsList = SSL_get_peer_cert_chain(&sconn); if (!Ssl::findIssuerCertificate(cert, certsList, ctx)) { if (const auto issuerUri = Ssl::findIssuerUri(cert)) { - debugs(81, 5, "certificate " << - X509_NAME_oneline(X509_get_subject_name(cert), buffer, sizeof(buffer)) << + debugs(81, 5, "certificate " << *cert << " points to its missing issuer certificate at " << issuerUri); urlsOfMissingCerts.push(SBuf(issuerUri)); } else { debugs(81, 3, "found a certificate with no IAI, " << - "signed by a missing issuer certificate: " << - X509_NAME_oneline(X509_get_subject_name(cert), buffer, sizeof(buffer))); + "signed by a missing issuer certificate: " << *cert); // We could short-circuit here, proceeding to chain validation // that is likely to fail. Instead, we keep going because we // hope that if we find at least one certificate to fetch, it diff --git a/src/security/cert_generators/file/certificate_db.cc b/src/security/cert_generators/file/certificate_db.cc index 01e0cde27a..e97d324d3b 100644 --- a/src/security/cert_generators/file/certificate_db.cc +++ b/src/security/cert_generators/file/certificate_db.cc @@ -292,15 +292,12 @@ Ssl::CertificateDb::addCertAndPrivateKey(std::string const &useKey, const Securi if(useKey.empty()) return false; - // Functor to wrap xfree() for std::unique_ptr - typedef HardFun CharDeleter; - Row row; ASN1_INTEGER * ai = X509_get_serialNumber(cert.get()); std::string serial_string; Ssl::BIGNUM_Pointer serial(ASN1_INTEGER_to_BN(ai, NULL)); { - std::unique_ptr hex_bn(BN_bn2hex(serial.get())); + const UniqueCString hex_bn(BN_bn2hex(serial.get())); serial_string = std::string(hex_bn.get()); } row.setValue(cnlSerial, serial_string.c_str()); @@ -339,7 +336,7 @@ Ssl::CertificateDb::addCertAndPrivateKey(std::string const &useKey, const Securi const auto tm = X509_getm_notAfter(cert.get()); row.setValue(cnlExp_date, std::string(reinterpret_cast(tm->data), tm->length).c_str()); - std::unique_ptr subject(X509_NAME_oneline(X509_get_subject_name(cert.get()), nullptr, 0)); + const auto subject = OneLineSummary(*X509_get_subject_name(cert.get())); row.setValue(cnlName, subject.get()); row.setValue(cnlKey, useKey.c_str()); diff --git a/src/security/forward.h b/src/security/forward.h index efb840871f..d0d745efb0 100644 --- a/src/security/forward.h +++ b/src/security/forward.h @@ -53,6 +53,14 @@ #define SSL_FLAG_VERIFY_CRL_ALL (1<<6) #define SSL_FLAG_CONDITIONAL_AUTH (1<<7) +#if !USE_OPENSSL && !USE_GNUTLS +/// A helper type to keep all three possible underlying types of the +/// Security::Certificate typedef below inside global namespace, so that +/// argument-dependent lookup for operator "<<" (Certificate) works inside +/// functions declared in Security and global namespaces. +struct notls_x509 {}; +#endif + /// Network/connection security abstraction layer namespace Security { @@ -66,7 +74,7 @@ typedef X509 Certificate; #elif USE_GNUTLS typedef struct gnutls_x509_crt_int Certificate; #else -typedef class {} Certificate; +typedef struct notls_x509 Certificate; #endif #if USE_OPENSSL diff --git a/src/ssl/gadgets.cc b/src/ssl/gadgets.cc index eb3ca3b546..762e2ab4f3 100644 --- a/src/ssl/gadgets.cc +++ b/src/ssl/gadgets.cc @@ -825,6 +825,12 @@ Ssl::WritePrivateKey(Ssl::BIO_Pointer &bio, const Security::PrivateKeyPointer &p return true; } +Ssl::UniqueCString +Ssl::OneLineSummary(X509_NAME &name) +{ + return Ssl::UniqueCString(X509_NAME_oneline(&name, nullptr, 0)); +} + bool Ssl::sslDateIsInTheFuture(char const * date) { ASN1_UTCTIME tm; diff --git a/src/ssl/gadgets.h b/src/ssl/gadgets.h index 1785e72bbc..3c95e5fdbc 100644 --- a/src/ssl/gadgets.h +++ b/src/ssl/gadgets.h @@ -73,6 +73,10 @@ typedef std::unique_ptr> X509_STORE_CTX_Pointer; +// not using CtoCpp1() here because OpenSSL_free() takes void* rather than char* +inline void OPENSSL_free_for_c_strings(char * const string) { OPENSSL_free(string); } +using UniqueCString = std::unique_ptr >; + /// Clear any errors accumulated by OpenSSL in its global storage. void ForgetErrors(); @@ -155,6 +159,9 @@ bool WriteX509Certificate(BIO_Pointer &bio, const Security::CertPointer & cert); */ bool WritePrivateKey(BIO_Pointer &bio, const Security::PrivateKeyPointer &pkey); +/// a RAII wrapper for the memory-allocating flavor of X509_NAME_oneline() +UniqueCString OneLineSummary(X509_NAME &); + /** \ingroup SslCrtdSslAPI * Supported certificate signing algorithms diff --git a/src/ssl/stub_libsslutil.cc b/src/ssl/stub_libsslutil.cc index ffd4fbb1b6..1bcc5a99e0 100644 --- a/src/ssl/stub_libsslutil.cc +++ b/src/ssl/stub_libsslutil.cc @@ -35,6 +35,7 @@ X509 * Ssl::signRequest(X509_REQ_Pointer const &, Security::CertPointer const &, bool Ssl::generateSslCertificateAndPrivateKey(char const *, Security::CertPointer const &, Security::PrivateKeyPointer const &, Security::CertPointer &, Security::PrivateKeyPointer &, BIGNUM const *) STUB_RETVAL(false) void Ssl::readCertAndPrivateKeyFromFiles(Security::CertPointer &, Security::PrivateKeyPointer &, char const *, char const *) STUB bool Ssl::sslDateIsInTheFuture(char const *) STUB_RETVAL(false) +UniqueCString Ssl::OneLineSummary(X509_NAME &) STUB_RETVAL({}) #include "ssl/helper.h" void Ssl::Helper::Init() STUB diff --git a/src/ssl/support.cc b/src/ssl/support.cc index a6f720353a..be0360cf26 100644 --- a/src/ssl/support.cc +++ b/src/ssl/support.cc @@ -24,6 +24,7 @@ #include "globals.h" #include "ipc/MemMap.h" #include "security/CertError.h" +#include "security/Certificate.h" #include "security/ErrorDetail.h" #include "security/Session.h" #include "SquidConfig.h" @@ -263,7 +264,6 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) // preserve original ctx->error before SSL_ calls can overwrite it Security::ErrorCode error_no = ok ? SSL_ERROR_NONE : X509_STORE_CTX_get_error(ctx); - char buffer[256] = ""; SSL *ssl = (SSL *)X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx()); SSL_CTX *sslctx = SSL_get_SSL_CTX(ssl); SBuf *server = (SBuf *)SSL_get_ex_data(ssl, ssl_ex_index_server); @@ -273,8 +273,6 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) Security::CertPointer peer_cert; peer_cert.resetAndLock(X509_STORE_CTX_get0_cert(ctx)); - X509_NAME_oneline(X509_get_subject_name(peer_cert.get()), buffer, sizeof(buffer)); - // detect infinite loops uint32_t *validationCounter = static_cast(SSL_get_ex_data(ssl, ssl_ex_index_ssl_validation_counter)); if (!validationCounter) { @@ -289,16 +287,16 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) ok = 0; // or the validation loop will never stop error_no = SQUID_X509_V_ERR_INFINITE_VALIDATION; debugs(83, 2, "SQUID_X509_V_ERR_INFINITE_VALIDATION: " << - *validationCounter << " iterations while checking " << buffer); + *validationCounter << " iterations while checking " << *peer_cert); } if (ok) { - debugs(83, 5, "SSL Certificate signature OK: " << buffer); + debugs(83, 5, "SSL Certificate signature OK: " << *peer_cert); // Check for domain mismatch only if the current certificate is the peer certificate. if (!dont_verify_domain && server && peer_cert.get() == X509_STORE_CTX_get_current_cert(ctx)) { if (!Ssl::checkX509ServerValidity(peer_cert.get(), server->c_str())) { - debugs(83, 2, "SQUID_X509_V_ERR_DOMAIN_MISMATCH: Certificate " << buffer << " does not match domainname " << server); + debugs(83, 2, "SQUID_X509_V_ERR_DOMAIN_MISMATCH: Certificate " << *peer_cert << " does not match domainname " << server); ok = 0; error_no = SQUID_X509_V_ERR_DOMAIN_MISMATCH; } @@ -308,7 +306,7 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) if (ok && peeked_cert) { // Check whether the already peeked certificate matches the new one. if (X509_cmp(peer_cert.get(), peeked_cert) != 0) { - debugs(83, 2, "SQUID_X509_V_ERR_CERT_CHANGE: Certificate " << buffer << " does not match peeked certificate"); + debugs(83, 2, "SQUID_X509_V_ERR_CERT_CHANGE: Certificate " << *peer_cert << " does not match peeked certificate"); ok = 0; error_no = SQUID_X509_V_ERR_CERT_CHANGE; } @@ -335,7 +333,7 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) if (!errs) { errs = new Security::CertErrors(Security::CertError(error_no, broken_cert, depth)); 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); + debugs(83, 2, "Failed to set ssl error_no in ssl_verify_cb: Certificate " << *peer_cert); delete errs; errs = NULL; } @@ -343,9 +341,9 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) errs->push_back_unique(Security::CertError(error_no, broken_cert, depth)); if (const char *err_descr = Ssl::GetErrorDescr(error_no)) - debugs(83, 5, err_descr << ": " << buffer); + debugs(83, 5, err_descr << ": " << *peer_cert); else - debugs(83, DBG_IMPORTANT, "ERROR: SSL unknown certificate error " << error_no << " in " << buffer); + debugs(83, DBG_IMPORTANT, "ERROR: SSL unknown certificate error " << error_no << " in " << *peer_cert); // Check if the certificate error can be bypassed. // Infinity validation loop errors can not bypassed. @@ -356,7 +354,7 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) filledCheck->sslErrors = new Security::CertErrors(Security::CertError(error_no, broken_cert)); filledCheck->serverCert = peer_cert; if (check->fastCheck().allowed()) { - debugs(83, 3, "bypassing SSL error " << error_no << " in " << buffer); + debugs(83, 3, "bypassing SSL error " << error_no << " in " << *peer_cert); ok = 1; } else { debugs(83, 5, "confirming SSL error " << error_no); @@ -397,7 +395,7 @@ ssl_verify_cb(int ok, X509_STORE_CTX * ctx) if (SSL_set_ex_data(ssl, ssl_ex_index_ssl_error_detail, edp.get())) edp.release(); else - debugs(83, 2, "failed to store a " << buffer << " error detail: " << *edp); + debugs(83, 2, "failed to store a " << *peer_cert << " error detail: " << *edp); } return ok; @@ -1117,9 +1115,8 @@ Ssl::loadCerts(const char *certsFile, Ssl::CertsIndexedList &list) } while (auto aCert = ReadOptionalCertificate(in)) { - static char buffer[2048]; - X509_NAME_oneline(X509_get_subject_name(aCert.get()), buffer, sizeof(buffer)); - list.insert(std::pair(SBuf(buffer), aCert.release())); + const auto name = Security::SubjectName(*aCert); + list.insert(std::pair(name, aCert.release())); } debugs(83, 4, "Loaded " << list.size() << " certificates from file: '" << certsFile << "'"); return true; @@ -1130,17 +1127,14 @@ Ssl::loadCerts(const char *certsFile, Ssl::CertsIndexedList &list) static X509 * findCertIssuerFast(Ssl::CertsIndexedList &list, X509 *cert) { - static char buffer[2048]; - - if (X509_NAME *issuerName = X509_get_issuer_name(cert)) - X509_NAME_oneline(issuerName, buffer, sizeof(buffer)); - else + const auto name = Security::IssuerName(*cert); + if (name.isEmpty()) return NULL; - const auto ret = list.equal_range(SBuf(buffer)); + const auto ret = list.equal_range(name); for (Ssl::CertsIndexedList::iterator it = ret.first; it != ret.second; ++it) { X509 *issuer = it->second; - if (X509_check_issued(issuer, cert) == X509_V_OK) { + if (Security::IssuedBy(*cert, *issuer)) { return issuer; } } @@ -1157,7 +1151,7 @@ sk_x509_findIssuer(const STACK_OF(X509) *sk, X509 *cert) const auto certCount = sk_X509_num(sk); for (int i = 0; i < certCount; ++i) { const auto issuer = sk_X509_value(sk, i); - if (X509_check_issued(issuer, cert) == X509_V_OK) + if (Security::IssuedBy(*cert, *issuer)) return issuer; } return nullptr; @@ -1183,8 +1177,7 @@ findIssuerInCaDb(X509 *cert, const Security::ContextPointer &connContext) const auto ret = X509_STORE_CTX_get1_issuer(&issuer, storeCtx, cert); if (ret > 0) { assert(issuer); - char buffer[256]; - debugs(83, 5, "found " << X509_NAME_oneline(X509_get_subject_name(issuer), buffer, sizeof(buffer))); + debugs(83, 5, "found " << *issuer); } else { debugs(83, ret < 0 ? 2 : 3, "not found or failure: " << ret); assert(!issuer); @@ -1237,9 +1230,8 @@ Ssl::missingChainCertificatesUrls(std::queue &URIs, const STACK_OF(X509) & if (const auto issuerUri = findIssuerUri(cert)) { URIs.push(SBuf(issuerUri)); } else { - static char name[2048]; debugs(83, 3, "Issuer certificate for " << - X509_NAME_oneline(X509_get_subject_name(cert), name, sizeof(name)) << + *cert << " is missing and its URI is not provided"); } } @@ -1262,7 +1254,7 @@ completeIssuers(X509_STORE_CTX *ctx, STACK_OF(X509) &untrustedCerts) current.resetAndLock(X509_STORE_CTX_get0_cert(ctx)); int i = 0; for (i = 0; current && (i < depth); ++i) { - if (X509_check_issued(current.get(), current.get()) == X509_V_OK) { + if (Security::SelfSigned(*current)) { // either ctx->cert is itself self-signed or untrustedCerts // already contain the self-signed current certificate break; diff --git a/src/tests/stub_libsecurity.cc b/src/tests/stub_libsecurity.cc index b0a1ae109b..c006ae9efe 100644 --- a/src/tests/stub_libsecurity.cc +++ b/src/tests/stub_libsecurity.cc @@ -28,6 +28,12 @@ void BlindPeerConnector::noteNegotiationDone(ErrorState *) STUB Security::EncryptorAnswer::~EncryptorAnswer() {} std::ostream &Security::operator <<(std::ostream &os, const Security::EncryptorAnswer &) STUB_RETVAL(os) +#include "security/Certificate.h" +SBuf Security::SubjectName(Certificate &) STUB_RETVAL(SBuf()) +SBuf Security::IssuerName(Certificate &) STUB_RETVAL(SBuf()) +bool Security::IssuedBy(Certificate &, Certificate &) STUB_RETVAL(false) +std::ostream &operator <<(std::ostream &os, Security::Certificate &) STUB_RETVAL(os) + #include "security/Handshake.h" Security::HandshakeParser::HandshakeParser(MessageSource) STUB bool Security::HandshakeParser::parseHello(const SBuf &) STUB_RETVAL(false)