From 5963e7f036409b2ab4cda43598237f25599fcc76 Mon Sep 17 00:00:00 2001 From: Matt Braithwaite Date: Wed, 1 Apr 2020 14:11:00 -0700 Subject: [PATCH 01/12] tls: make X.509 certificate parsing and verification explicit. BoringSSL would like to deprecate OpenSSL's X.509 certificate parsing and verification code. Although the replacements are not yet ready, we can take a step in this direction by making certificate parsing and certificate chain validation explicit. This changes Envoy to use TLS_with_buffers_method(), which removes the OpenSSL behavior of parsing certificates to an |X509| object implicitly, as well as the behavior of calling X509_verify_cert() implicitly. Instead, Envoy now does these things explicitly, as required. Usefully, operations that require parsing (such as extracting a certificate's serial number) are now distinguished from operations that do not require parsing (such as returning the SHA-256 hash of a certificate's DER encoding). This makes it easier to take the next step of using a better parser, when it is available. Signed-off-by: Matt Braithwaite Signed-off-by: Piotr Sikora --- .../transport_sockets/tls/context_impl.cc | 291 +++++++++++++----- .../transport_sockets/tls/context_impl.h | 16 +- .../transport_sockets/tls/ssl_socket.cc | 119 ++++--- .../transport_sockets/tls/ssl_socket.h | 8 + .../transport_sockets/tls/ssl_socket_test.cc | 4 +- 5 files changed, 313 insertions(+), 125 deletions(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index 6e68385d73e4..d52e5c884dad 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -23,8 +23,10 @@ #include "extensions/transport_sockets/tls/utility.h" #include "absl/strings/str_join.h" +#include "openssl/bytestring.h" #include "openssl/evp.h" #include "openssl/hmac.h" +#include "openssl/pool.h" #include "openssl/rand.h" namespace Envoy { @@ -48,6 +50,92 @@ bool cbsContainsU16(CBS& cbs, uint16_t n) { return false; } +int sslTlsContextIndex() { + CONSTRUCT_ON_FIRST_USE(int, []() -> int { + int ssl_context_index = SSL_CTX_get_ex_new_index(0, nullptr, nullptr, nullptr, nullptr); + RELEASE_ASSERT(ssl_context_index >= 0, ""); + return ssl_context_index; + }()); +} + +static bool SeekToSubject(CRYPTO_BUFFER* cert, CBS* tbs_certificate) { + CBS der; + CRYPTO_BUFFER_init_CBS(cert, &der); + CBS certificate, opt_version, serial, signature, issuer, validity; + // From RFC 5280, section 4.1 + // + // Certificate ::= SEQUENCE { + // tbsCertificate TBSCertificate, + // signatureAlgorithm AlgorithmIdentifier, + // signatureValue BIT STRING } + // + // TBSCertificate ::= SEQUENCE { + // version [0] EXPLICIT Version DEFAULT v1, + // serialNumber CertificateSerialNumber, + // signature AlgorithmIdentifier, + // issuer Name, + // validity Validity, + // subject Name, + // subjectPublicKeyInfo SubjectPublicKeyInfo, + // ... + if (!CBS_get_asn1(&der, &certificate, CBS_ASN1_SEQUENCE) || + CBS_len(&der) != 0 || // We don't allow junk after the certificate. + !CBS_get_asn1(&certificate, tbs_certificate, CBS_ASN1_SEQUENCE) || + !CBS_get_optional_asn1(tbs_certificate, &opt_version, nullptr, + CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0) || + !CBS_get_asn1(tbs_certificate, &serial, CBS_ASN1_INTEGER) || + !CBS_get_asn1(tbs_certificate, &signature, CBS_ASN1_SEQUENCE) || + !CBS_get_asn1(tbs_certificate, &issuer, CBS_ASN1_SEQUENCE) || + !CBS_get_asn1(tbs_certificate, &validity, CBS_ASN1_SEQUENCE)) { + return false; + } + + return true; +} + +bool ExtractSPKIFromDERCert(CRYPTO_BUFFER* cert, CBS* spki) { + CBS tbs_certificate; + if (!SeekToSubject(cert, &tbs_certificate)) { + return false; + } + + CBS subject; + if (!CBS_get_asn1(&tbs_certificate, &subject, CBS_ASN1_SEQUENCE) || + !CBS_get_asn1_element(&tbs_certificate, spki, CBS_ASN1_SEQUENCE)) { + return false; + } + return true; +} + +bool ExtractSubjectNameFromDERCert(CRYPTO_BUFFER* cert, absl::string_view* subject_out) { + CBS tbs_certificate; + if (!SeekToSubject(cert, &tbs_certificate)) { + return false; + } + + CBS subject; + if (!CBS_get_asn1_element(&tbs_certificate, &subject, CBS_ASN1_SEQUENCE)) { + return false; + } + *subject_out = + absl::string_view(reinterpret_cast(CBS_data(&subject)), CBS_len(&subject)); + return true; +} + +static void PushBufferIfUnique(STACK_OF(CRYPTO_BUFFER) * stack, absl::string_view value) { + for (auto buf : stack) { + if (CRYPTO_BUFFER_len(buf) == value.size() && + memcmp(CRYPTO_BUFFER_data(buf), value.data(), value.size()) == 0) { + return; + } + } + + bssl::UniquePtr buf( + CRYPTO_BUFFER_new(reinterpret_cast(value.data()), value.size(), nullptr)); + bool rc = bssl::PushToStack(stack, std::move(buf)); + RELEASE_ASSERT(rc == true, Utility::getLastCryptoError().value_or("")); +} + } // namespace int ContextImpl::sslExtendedSocketInfoIndex() { @@ -75,11 +163,16 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c tls_contexts_.resize(std::max(static_cast(1), tls_certificates.size())); for (auto& ctx : tls_contexts_) { - ctx.ssl_ctx_.reset(SSL_CTX_new(TLS_method())); + ctx.ssl_ctx_.reset(SSL_CTX_new(TLS_with_buffers_method())); + ctx.x509_store_.reset(X509_STORE_new()); + RELEASE_ASSERT(ctx.x509_store_ != nullptr, Utility::getLastCryptoError().value_or("")); int rc = SSL_CTX_set_app_data(ctx.ssl_ctx_.get(), this); RELEASE_ASSERT(rc == 1, Utility::getLastCryptoError().value_or("")); + rc = SSL_CTX_set_ex_data(ctx.ssl_ctx_.get(), sslTlsContextIndex(), &ctx); + RELEASE_ASSERT(rc == 1, Utility::getLastCryptoError().value_or("")); + rc = SSL_CTX_set_min_proto_version(ctx.ssl_ctx_.get(), config.minProtocolVersion()); RELEASE_ASSERT(rc == 1, Utility::getLastCryptoError().value_or("")); @@ -137,7 +230,7 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c } for (auto& ctx : tls_contexts_) { - X509_STORE* store = SSL_CTX_get_cert_store(ctx.ssl_ctx_.get()); + X509_STORE* store = ctx.x509_store_.get(); bool has_crl = false; for (const X509_INFO* item : list.get()) { if (item->x509) { @@ -162,10 +255,10 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c verify_mode = SSL_VERIFY_PEER; verify_trusted_ca_ = true; - // NOTE: We're using SSL_CTX_set_cert_verify_callback() instead of X509_verify_cert() - // directly. However, our new callback is still calling X509_verify_cert() under - // the hood. Therefore, to ignore cert expiration, we need to set the callback - // for X509_verify_cert to ignore that error. + // NOTE: We're using SSL_CTX_set_custom_verify_cb() instead of X509_verify_cert() directly. + // However, our new callback is still calling X509_verify_cert() under the hood. Therefore, to + // ignore cert expiration, we need to set the callback for X509_verify_cert() to ignore that + // error. if (config.certificateValidationContext()->allowExpiredCertificate()) { X509_STORE_set_verify_cb(store, ContextImpl::ignoreCertificateExpirationCallback); } @@ -190,7 +283,7 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c } for (auto& ctx : tls_contexts_) { - X509_STORE* store = SSL_CTX_get_cert_store(ctx.ssl_ctx_.get()); + X509_STORE* store = ctx.x509_store_.get(); for (const X509_INFO* item : list.get()) { if (item->crl) { X509_STORE_add_crl(store, item->crl); @@ -246,9 +339,12 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c } for (auto& ctx : tls_contexts_) { - if (verify_mode != SSL_VERIFY_NONE) { - SSL_CTX_set_verify(ctx.ssl_ctx_.get(), verify_mode, nullptr); - SSL_CTX_set_cert_verify_callback(ctx.ssl_ctx_.get(), ContextImpl::verifyCallback, this); + if (verify_mode == SSL_VERIFY_NONE) { + SSL_CTX_set_custom_verify( + ctx.ssl_ctx_.get(), SSL_VERIFY_NONE, + [](SSL*, uint8_t*) -> ssl_verify_result_t { return ssl_verify_ok; } /* always succeed */); + } else { + SSL_CTX_set_custom_verify(ctx.ssl_ctx_.get(), verify_mode, ContextImpl::verifyCallback); } } @@ -262,29 +358,24 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c BIO_new_mem_buf(const_cast(tls_certificate.certificateChain().data()), tls_certificate.certificateChain().size())); RELEASE_ASSERT(bio != nullptr, ""); - ctx.cert_chain_.reset(PEM_read_bio_X509_AUX(bio.get(), nullptr, nullptr, nullptr)); - if (ctx.cert_chain_ == nullptr || - !SSL_CTX_use_certificate(ctx.ssl_ctx_.get(), ctx.cert_chain_.get())) { - while (uint64_t err = ERR_get_error()) { - ENVOY_LOG_MISC(debug, "SSL error: {}:{}:{}:{}", err, ERR_lib_error_string(err), - ERR_func_error_string(err), ERR_GET_REASON(err), - ERR_reason_error_string(err)); - } + uint8_t* data = nullptr; + long len; + if (!PEM_bytes_read_bio(&data, &len, nullptr, PEM_STRING_X509, bio.get(), nullptr, nullptr) || + !data) { throw EnvoyException( absl::StrCat("Failed to load certificate chain from ", ctx.cert_chain_file_path_)); } + bssl::UniquePtr der(data); + ctx.cert_.reset(CRYPTO_BUFFER_new(data, len, nullptr)); + // Read rest of the certificate chain. + std::vector> chain; while (true) { - bssl::UniquePtr cert(PEM_read_bio_X509(bio.get(), nullptr, nullptr, nullptr)); - if (cert == nullptr) { + if (!PEM_bytes_read_bio(&data, &len, nullptr, PEM_STRING_X509, bio.get(), nullptr, nullptr)) { break; } - if (!SSL_CTX_add_extra_chain_cert(ctx.ssl_ctx_.get(), cert.get())) { - throw EnvoyException( - absl::StrCat("Failed to load certificate chain from ", ctx.cert_chain_file_path_)); - } - // SSL_CTX_add_extra_chain_cert() takes ownership. - cert.release(); + der.reset(data); + chain.push_back(bssl::UniquePtr(CRYPTO_BUFFER_new(data, len, nullptr))); } // Check for EOF. const uint32_t err = ERR_peek_last_error(); @@ -295,7 +386,17 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c absl::StrCat("Failed to load certificate chain from ", ctx.cert_chain_file_path_)); } - bssl::UniquePtr public_key(X509_get_pubkey(ctx.cert_chain_.get())); + std::vector certs(chain.size() + 1); + certs[0] = ctx.cert_.get(); + for (size_t i = 0; i < chain.size(); ++i) { + certs[i + 1] = chain[i].get(); + } + + CBS spki; + if (!ExtractSPKIFromDERCert(ctx.cert_.get(), &spki)) { + throw EnvoyException(absl::StrCat("Failed to extract SPKI from ", ctx.cert_chain_file_path_)); + } + bssl::UniquePtr public_key(EVP_parse_public_key(&spki)); const int pkey_id = EVP_PKEY_id(public_key.get()); if (!cert_pkey_ids.insert(pkey_id).second) { throw EnvoyException(fmt::format("Failed to load certificate chain from {}, at most one " @@ -364,7 +465,11 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c fmt::format("Private key method doesn't support FIPS mode with current parameters")); } #endif - SSL_CTX_set_private_key_method(ctx.ssl_ctx_.get(), private_key_method.get()); + if (!SSL_CTX_set_chain_and_key(ctx.ssl_ctx_.get(), certs.data(), certs.size(), nullptr, + private_key_method.get())) { + throw EnvoyException( + absl::StrCat("Failed to set certificate chain from ", ctx.cert_chain_file_path_)); + } } else { // Load private key. bio.reset(BIO_new_mem_buf(const_cast(tls_certificate.privateKey().data()), @@ -375,10 +480,16 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c !tls_certificate.password().empty() ? const_cast(tls_certificate.password().c_str()) : nullptr)); - if (pkey == nullptr || !SSL_CTX_use_PrivateKey(ctx.ssl_ctx_.get(), pkey.get())) { + if (pkey == nullptr) { throw EnvoyException( absl::StrCat("Failed to load private key from ", tls_certificate.privateKeyPath())); } + if (!SSL_CTX_set_chain_and_key(ctx.ssl_ctx_.get(), certs.data(), certs.size(), pkey.get(), + nullptr)) { + throw EnvoyException(absl::StrCat( + "Failed to set certificate chain from ", tls_certificate.certificateChainPath(), + " and/or private key from ", tls_certificate.privateKeyPath())); + } #ifdef BORINGSSL_FIPS // Verify that private keys are passing FIPS pairwise consistency tests. @@ -506,16 +617,39 @@ int ContextImpl::ignoreCertificateExpirationCallback(int ok, X509_STORE_CTX* ctx return ok; } -int ContextImpl::verifyCallback(X509_STORE_CTX* store_ctx, void* arg) { - ContextImpl* impl = reinterpret_cast(arg); - SSL* ssl = reinterpret_cast( - X509_STORE_CTX_get_ex_data(store_ctx, SSL_get_ex_data_X509_STORE_CTX_idx())); +ssl_verify_result_t ContextImpl::verifyCallback(SSL* ssl, uint8_t* out_alert) { + ContextImpl* impl = reinterpret_cast(SSL_CTX_get_app_data(SSL_get_SSL_CTX(ssl))); Envoy::Ssl::SslExtendedSocketInfo* sslExtendedInfo = reinterpret_cast( SSL_get_ex_data(ssl, ContextImpl::sslExtendedSocketInfoIndex())); + const STACK_OF(CRYPTO_BUFFER)* buffers = SSL_get0_peer_certificates(ssl); + bssl::UniquePtr cert = nullptr; + bssl::UniquePtr chain(sk_X509_new(nullptr)); + for (size_t i = 0; i < sk_CRYPTO_BUFFER_num(buffers); ++i) { + bssl::UniquePtr x509(X509_parse_from_buffer(sk_CRYPTO_BUFFER_value(buffers, i))); + if (!x509) { + continue; + } + if (!cert) { + cert = std::move(x509); + } else { + bool rc = bssl::PushToStack(chain.get(), std::move(x509)); + RELEASE_ASSERT(rc == true, Utility::getLastCryptoError().value_or("")); + } + } + if (impl->verify_trusted_ca_) { - int ret = X509_verify_cert(store_ctx); + bssl::ScopedX509_STORE_CTX store_ctx; + if (!X509_STORE_CTX_init(store_ctx.get(), impl->tls_contexts_[0].x509_store_.get(), cert.get(), + chain.get()) || + !X509_STORE_CTX_set_default(store_ctx.get(), + SSL_is_server(ssl) ? "ssl_client" : "ssl_server")) { + return ssl_verify_invalid; + } + + int ret = X509_verify_cert(store_ctx.get()); + *out_alert = SSL_alert_from_verify_result(store_ctx->error); if (sslExtendedInfo) { sslExtendedInfo->setCertificateValidationStatus( ret == 1 ? Envoy::Ssl::ClientValidationStatus::Validated @@ -524,12 +658,10 @@ int ContextImpl::verifyCallback(X509_STORE_CTX* store_ctx, void* arg) { if (ret <= 0) { impl->stats_.fail_verify_error_.inc(); - return impl->allow_untrusted_certificate_ ? 1 : ret; + return impl->allow_untrusted_certificate_ ? ssl_verify_ok : ssl_verify_invalid; } } - bssl::UniquePtr cert(SSL_get_peer_certificate(ssl)); - const Network::TransportSocketOptions* transport_socket_options = static_cast(SSL_get_app_data(ssl)); @@ -550,9 +682,12 @@ int ContextImpl::verifyCallback(X509_STORE_CTX* store_ctx, void* arg) { } } - return impl->allow_untrusted_certificate_ - ? 1 - : (validated != Envoy::Ssl::ClientValidationStatus::Failed); + if (impl->allow_untrusted_certificate_ || + (validated != Envoy::Ssl::ClientValidationStatus::Failed)) { + return ssl_verify_ok; + } else { + return ssl_verify_invalid; + } } Envoy::Ssl::ClientValidationStatus ContextImpl::verifyCertificate( @@ -626,8 +761,8 @@ void ContextImpl::logHandshake(SSL* ssl) const { incCounter(ssl_sigalgs_, sigalg, unknown_ssl_algorithm_); } - bssl::UniquePtr cert(SSL_get_peer_certificate(ssl)); - if (!cert.get()) { + const STACK_OF(CRYPTO_BUFFER) * chain(SSL_get0_peer_certificates(ssl)); + if (!chain || sk_CRYPTO_BUFFER_num(chain) == 0) { stats_.no_certificate_.inc(); } } @@ -703,6 +838,12 @@ bool ContextImpl::dnsNameMatch(const std::string& dns_name, const char* pattern) return false; } +CRYPTO_BUFFER* ContextImpl::leafCertificate(SSL* ssl) { + auto tls_context = reinterpret_cast( + SSL_CTX_get_ex_data(SSL_get_SSL_CTX(ssl), sslTlsContextIndex())); + return tls_context->cert_.get(); +} + bool ContextImpl::verifyCertificateHashList( X509* cert, const std::vector>& expected_hashes) { std::vector computed_hash(SHA256_DIGEST_LENGTH); @@ -751,8 +892,16 @@ SslStats ContextImpl::generateStats(Stats::Scope& store) { size_t ContextImpl::daysUntilFirstCertExpires() const { int daysUntilExpiration = Utility::getDaysUntilExpiration(ca_cert_.get(), time_source_); for (auto& ctx : tls_contexts_) { - daysUntilExpiration = std::min( - Utility::getDaysUntilExpiration(ctx.cert_chain_.get(), time_source_), daysUntilExpiration); + if (ctx.cert_ == nullptr) { + return 0; + } + auto* start = CRYPTO_BUFFER_data(ctx.cert_.get()); + bssl::UniquePtr x509(d2i_X509(nullptr, &start, CRYPTO_BUFFER_len(ctx.cert_.get()))); + if (!x509) { + return 0; + } + daysUntilExpiration = std::min(Utility::getDaysUntilExpiration(x509.get(), time_source_), + daysUntilExpiration); } if (daysUntilExpiration < 0) { // Ensure that the return value is unsigned return 0; @@ -770,11 +919,15 @@ Envoy::Ssl::CertificateDetailsPtr ContextImpl::getCaCertInformation() const { std::vector ContextImpl::getCertChainInformation() const { std::vector cert_details; for (const auto& ctx : tls_contexts_) { - if (ctx.cert_chain_ == nullptr) { + if (ctx.cert_ == nullptr) { + continue; + } + auto* start = CRYPTO_BUFFER_data(ctx.cert_.get()); + bssl::UniquePtr x509(d2i_X509(nullptr, &start, CRYPTO_BUFFER_len(ctx.cert_.get()))); + if (!x509) { continue; } - cert_details.emplace_back( - certificateDetails(ctx.cert_chain_.get(), ctx.getCertChainFileName())); + cert_details.emplace_back(certificateDetails(x509.get(), ctx.getCertChainFileName())); } return cert_details; } @@ -865,7 +1018,8 @@ bssl::UniquePtr ClientContextImpl::newSsl(const Network::TransportSocketOpt if (options && !options->verifySubjectAltNameListOverride().empty()) { SSL_set_app_data(ssl_con.get(), options); - SSL_set_verify(ssl_con.get(), SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, nullptr); + SSL_set_custom_verify(ssl_con.get(), SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, + ContextImpl::verifyCallback); } if (options && !options->applicationProtocolListOverride().empty()) { @@ -1021,9 +1175,11 @@ ServerContextImpl::generateHashForSessionContextId(const std::vector cert(d2i_X509(nullptr, &start, CRYPTO_BUFFER_len(ctx.cert_.get()))); RELEASE_ASSERT(cert != nullptr, "TLS context should have an active certificate"); - X509_NAME* cert_subject = X509_get_subject_name(cert); + X509_NAME* cert_subject = X509_get_subject_name(cert.get()); RELEASE_ASSERT(cert_subject != nullptr, "TLS certificate should have a subject"); const int cn_index = X509_NAME_get_index_by_NID(cert_subject, NID_commonName, -1); @@ -1042,7 +1198,7 @@ ServerContextImpl::generateHashForSessionContextId(const std::vector san_names(static_cast( - X509_get_ext_d2i(cert, NID_subject_alt_name, nullptr, nullptr))); + X509_get_ext_d2i(cert.get(), NID_subject_alt_name, nullptr, nullptr))); if (san_names != nullptr) { for (const GENERAL_NAME* san : san_names.get()) { @@ -1074,7 +1230,8 @@ ServerContextImpl::generateHashForSessionContextId(const std::vector bio( BIO_new_mem_buf(const_cast(config.caCert().data()), config.caCert().size())); RELEASE_ASSERT(bio != nullptr, ""); - // Based on BoringSSL's SSL_add_file_cert_subjects_to_stack(). - bssl::UniquePtr list(sk_X509_NAME_new( - [](const X509_NAME** a, const X509_NAME** b) -> int { return X509_NAME_cmp(*a, *b); })); + bssl::UniquePtr list(sk_CRYPTO_BUFFER_new_null()); RELEASE_ASSERT(list != nullptr, ""); for (;;) { - bssl::UniquePtr cert(PEM_read_bio_X509(bio.get(), nullptr, nullptr, nullptr)); - if (cert == nullptr) { + uint8_t* data = nullptr; + long len; + if (!PEM_bytes_read_bio(&data, &len, nullptr, PEM_STRING_X509, bio.get(), nullptr, nullptr)) { break; } - X509_NAME* name = X509_get_subject_name(cert.get()); - if (name == nullptr) { - throw EnvoyException( - absl::StrCat("Failed to load trusted client CA certificates from ", config.caCertPath())); - } - // Check for duplicates. - if (sk_X509_NAME_find(list.get(), nullptr, name)) { - continue; - } - bssl::UniquePtr name_dup(X509_NAME_dup(name)); - if (name_dup == nullptr || !sk_X509_NAME_push(list.get(), name_dup.release())) { + bssl::UniquePtr der(data); + bssl::UniquePtr cert(CRYPTO_BUFFER_new(data, len, nullptr)); + absl::string_view name; + if (!ExtractSubjectNameFromDERCert(cert.get(), &name)) { throw EnvoyException( absl::StrCat("Failed to load trusted client CA certificates from ", config.caCertPath())); } + PushBufferIfUnique(list.get(), name); } // Check for EOF. const uint32_t err = ERR_peek_last_error(); @@ -1322,11 +1472,12 @@ void ServerContextImpl::TlsContext::addClientValidationContext( throw EnvoyException( absl::StrCat("Failed to load trusted client CA certificates from ", config.caCertPath())); } - SSL_CTX_set_client_CA_list(ssl_ctx_.get(), list.release()); + SSL_CTX_set0_client_CAs(ssl_ctx_.get(), list.release()); // SSL_VERIFY_PEER or stronger mode was already set in ContextImpl::ContextImpl(). if (require_client_cert) { - SSL_CTX_set_verify(ssl_ctx_.get(), SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, nullptr); + SSL_CTX_set_custom_verify(ssl_ctx_.get(), SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, + ContextImpl::verifyCallback); } } diff --git a/source/extensions/transport_sockets/tls/context_impl.h b/source/extensions/transport_sockets/tls/context_impl.h index b72168337d72..f2c1ba0908e9 100644 --- a/source/extensions/transport_sockets/tls/context_impl.h +++ b/source/extensions/transport_sockets/tls/context_impl.h @@ -86,6 +86,13 @@ class ContextImpl : public virtual Envoy::Ssl::Context { */ static bool dnsNameMatch(const std::string& dns_name, const char* pattern); + /** + * Returns the leaf certificate used by the supplied connection. + * @param ssl a connected SSL connection + * @return the DER-encoded leaf certificate that was presented during the SSL handshake. + */ + static CRYPTO_BUFFER* leafCertificate(SSL* ssl); + SslStats& stats() { return stats_; } /** @@ -114,8 +121,8 @@ class ContextImpl : public virtual Envoy::Ssl::Context { // A X509_STORE_CTX_verify_cb callback for ignoring cert expiration in X509_verify_cert(). static int ignoreCertificateExpirationCallback(int ok, X509_STORE_CTX* store_ctx); - // A SSL_CTX_set_cert_verify_callback for custom cert validation. - static int verifyCallback(X509_STORE_CTX* store_ctx, void* arg); + // A SSL_CTX_set_custom_verify_cb for custom cert validation. + static ssl_verify_result_t verifyCallback(SSL* ssl, uint8_t* out_alert); Envoy::Ssl::ClientValidationStatus verifyCertificate(X509* cert, const std::vector& verify_san_list, @@ -158,7 +165,10 @@ class ContextImpl : public virtual Envoy::Ssl::Context { // safely substituted via SSL_set_SSL_CTX() during the // SSL_CTX_set_select_certificate_cb() callback following ClientHello. bssl::UniquePtr ssl_ctx_; - bssl::UniquePtr cert_chain_; + // x509_store_ configures X509_verify_cert(). + bssl::UniquePtr x509_store_; + // cert_ is the leaf certificate. (We don't retain the rest of the chain.) + bssl::UniquePtr cert_; std::string cert_chain_file_path_; bool is_ecdsa_{}; Ssl::PrivateKeyMethodProviderSharedPtr private_key_method_provider_{}; diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index 1e3082f80653..baff03291ecf 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -316,8 +316,8 @@ SslSocketInfo::SslSocketInfo(bssl::UniquePtr ssl, ContextImplSharedPtr ctx) } bool SslSocketInfo::peerCertificatePresented() const { - bssl::UniquePtr cert(SSL_get_peer_certificate(ssl_.get())); - return cert != nullptr; + const STACK_OF(CRYPTO_BUFFER)* chain = SSL_get0_peer_certificates(ssl_.get()); + return chain != nullptr && sk_CRYPTO_BUFFER_num(chain) > 0; } bool SslSocketInfo::peerCertificateValidated() const { @@ -330,13 +330,11 @@ absl::Span SslSocketInfo::uriSanLocalCertificate() const { return cached_uri_san_local_certificate_; } - // The cert object is not owned. - X509* cert = SSL_get_certificate(ssl_.get()); - if (!cert) { + if (!getX509Certificate()) { ASSERT(cached_uri_san_local_certificate_.empty()); return cached_uri_san_local_certificate_; } - cached_uri_san_local_certificate_ = Utility::getSubjectAltNames(*cert, GEN_URI); + cached_uri_san_local_certificate_ = Utility::getSubjectAltNames(*getX509Certificate(), GEN_URI); return cached_uri_san_local_certificate_; } @@ -345,12 +343,11 @@ absl::Span SslSocketInfo::dnsSansLocalCertificate() const { return cached_dns_san_local_certificate_; } - X509* cert = SSL_get_certificate(ssl_.get()); - if (!cert) { + if (!getX509Certificate()) { ASSERT(cached_dns_san_local_certificate_.empty()); return cached_dns_san_local_certificate_; } - cached_dns_san_local_certificate_ = Utility::getSubjectAltNames(*cert, GEN_DNS); + cached_dns_san_local_certificate_ = Utility::getSubjectAltNames(*getX509Certificate(), GEN_DNS); return cached_dns_san_local_certificate_; } @@ -358,16 +355,15 @@ const std::string& SslSocketInfo::sha256PeerCertificateDigest() const { if (!cached_sha_256_peer_certificate_digest_.empty()) { return cached_sha_256_peer_certificate_digest_; } - bssl::UniquePtr cert(SSL_get_peer_certificate(ssl_.get())); - if (!cert) { + const STACK_OF(CRYPTO_BUFFER)* chain = SSL_get0_peer_certificates(ssl_.get()); + if (!chain || sk_CRYPTO_BUFFER_num(chain) == 0) { ASSERT(cached_sha_256_peer_certificate_digest_.empty()); return cached_sha_256_peer_certificate_digest_; } + CRYPTO_BUFFER* leaf = sk_CRYPTO_BUFFER_value(chain, 0); std::vector computed_hash(SHA256_DIGEST_LENGTH); - unsigned int n; - X509_digest(cert.get(), EVP_sha256(), computed_hash.data(), &n); - RELEASE_ASSERT(n == computed_hash.size(), ""); + SHA256(CRYPTO_BUFFER_data(leaf), CRYPTO_BUFFER_len(leaf), computed_hash.data()); cached_sha_256_peer_certificate_digest_ = Hex::encode(computed_hash); return cached_sha_256_peer_certificate_digest_; } @@ -376,15 +372,18 @@ const std::string& SslSocketInfo::urlEncodedPemEncodedPeerCertificate() const { if (!cached_url_encoded_pem_encoded_peer_certificate_.empty()) { return cached_url_encoded_pem_encoded_peer_certificate_; } - bssl::UniquePtr cert(SSL_get_peer_certificate(ssl_.get())); - if (!cert) { + const STACK_OF(CRYPTO_BUFFER)* chain = SSL_get0_peer_certificates(ssl_.get()); + if (!chain || sk_CRYPTO_BUFFER_num(chain) == 0) { ASSERT(cached_url_encoded_pem_encoded_peer_certificate_.empty()); return cached_url_encoded_pem_encoded_peer_certificate_; } + CRYPTO_BUFFER* leaf = sk_CRYPTO_BUFFER_value(chain, 0); bssl::UniquePtr buf(BIO_new(BIO_s_mem())); RELEASE_ASSERT(buf != nullptr, ""); - RELEASE_ASSERT(PEM_write_bio_X509(buf.get(), cert.get()) == 1, ""); + RELEASE_ASSERT(PEM_write_bio(buf.get(), PEM_STRING_X509, /* header */ "", + CRYPTO_BUFFER_data(leaf), CRYPTO_BUFFER_len(leaf)) != 0, + ""); const uint8_t* output; size_t length; RELEASE_ASSERT(BIO_mem_contents(buf.get(), &output, &length) == 1, ""); @@ -399,18 +398,19 @@ const std::string& SslSocketInfo::urlEncodedPemEncodedPeerCertificateChain() con return cached_url_encoded_pem_encoded_peer_cert_chain_; } - STACK_OF(X509)* cert_chain = SSL_get_peer_full_cert_chain(ssl_.get()); - if (cert_chain == nullptr) { + const STACK_OF(CRYPTO_BUFFER)* chain = SSL_get0_peer_certificates(ssl_.get()); + if (!chain) { ASSERT(cached_url_encoded_pem_encoded_peer_cert_chain_.empty()); return cached_url_encoded_pem_encoded_peer_cert_chain_; } - for (uint64_t i = 0; i < sk_X509_num(cert_chain); i++) { - X509* cert = sk_X509_value(cert_chain, i); - + for (uint64_t i = 0; i < sk_CRYPTO_BUFFER_num(chain); i++) { + CRYPTO_BUFFER* cert = sk_CRYPTO_BUFFER_value(chain, i); bssl::UniquePtr buf(BIO_new(BIO_s_mem())); RELEASE_ASSERT(buf != nullptr, ""); - RELEASE_ASSERT(PEM_write_bio_X509(buf.get(), cert) == 1, ""); + RELEASE_ASSERT(PEM_write_bio(buf.get(), PEM_STRING_X509, /* header */ "", + CRYPTO_BUFFER_data(cert), CRYPTO_BUFFER_len(cert)) != 0, + ""); const uint8_t* output; size_t length; RELEASE_ASSERT(BIO_mem_contents(buf.get(), &output, &length) == 1, ""); @@ -429,12 +429,12 @@ absl::Span SslSocketInfo::uriSanPeerCertificate() const { return cached_uri_san_peer_certificate_; } - bssl::UniquePtr cert(SSL_get_peer_certificate(ssl_.get())); - if (!cert) { + if (!getX509PeerCertificate()) { ASSERT(cached_uri_san_peer_certificate_.empty()); return cached_uri_san_peer_certificate_; } - cached_uri_san_peer_certificate_ = Utility::getSubjectAltNames(*cert, GEN_URI); + cached_uri_san_peer_certificate_ = + Utility::getSubjectAltNames(*getX509PeerCertificate(), GEN_URI); return cached_uri_san_peer_certificate_; } @@ -443,12 +443,12 @@ absl::Span SslSocketInfo::dnsSansPeerCertificate() const { return cached_dns_san_peer_certificate_; } - bssl::UniquePtr cert(SSL_get_peer_certificate(ssl_.get())); - if (!cert) { + if (!getX509PeerCertificate()) { ASSERT(cached_dns_san_peer_certificate_.empty()); return cached_dns_san_peer_certificate_; } - cached_dns_san_peer_certificate_ = Utility::getSubjectAltNames(*cert, GEN_DNS); + cached_dns_san_peer_certificate_ = + Utility::getSubjectAltNames(*getX509PeerCertificate(), GEN_DNS); return cached_dns_san_peer_certificate_; } @@ -503,11 +503,35 @@ const std::string& SslSocketInfo::tlsVersion() const { } absl::optional SslSocketInfo::x509Extension(absl::string_view extension_name) const { - bssl::UniquePtr cert(SSL_get_peer_certificate(ssl_.get())); - if (!cert) { + if (!getX509PeerCertificate()) { return absl::nullopt; } - return Utility::getX509ExtensionValue(*cert, extension_name); + return Utility::getX509ExtensionValue(*getX509PeerCertificate(), extension_name); +} + +X509* SslSocketInfo::getX509Certificate() const { + if (cached_certificate_) { + return cached_certificate_.get(); + } + CRYPTO_BUFFER* leaf = ContextImpl::leafCertificate(ssl_.get()); + if (leaf) { + auto* start = CRYPTO_BUFFER_data(leaf); + cached_certificate_.reset(d2i_X509(nullptr, &start, CRYPTO_BUFFER_len(leaf))); + } + return cached_certificate_.get(); +} + +X509* SslSocketInfo::getX509PeerCertificate() const { + if (cached_peer_certificate_) { + return cached_peer_certificate_.get(); + } + const STACK_OF(CRYPTO_BUFFER)* chain = SSL_get0_peer_certificates(ssl_.get()); + if (chain && sk_CRYPTO_BUFFER_num(chain) > 0) { + CRYPTO_BUFFER* leaf = sk_CRYPTO_BUFFER_value(chain, 0); + auto* start = CRYPTO_BUFFER_data(leaf); + cached_peer_certificate_.reset(d2i_X509(nullptr, &start, CRYPTO_BUFFER_len(leaf))); + } + return cached_peer_certificate_.get(); } absl::string_view SslSocket::failureReason() const { return failure_reason_; } @@ -516,12 +540,12 @@ const std::string& SslSocketInfo::serialNumberPeerCertificate() const { if (!cached_serial_number_peer_certificate_.empty()) { return cached_serial_number_peer_certificate_; } - bssl::UniquePtr cert(SSL_get_peer_certificate(ssl_.get())); - if (!cert) { + if (!getX509PeerCertificate()) { ASSERT(cached_serial_number_peer_certificate_.empty()); return cached_serial_number_peer_certificate_; } - cached_serial_number_peer_certificate_ = Utility::getSerialNumberFromCertificate(*cert.get()); + cached_serial_number_peer_certificate_ = + Utility::getSerialNumberFromCertificate(*getX509PeerCertificate()); return cached_serial_number_peer_certificate_; } @@ -529,12 +553,11 @@ const std::string& SslSocketInfo::issuerPeerCertificate() const { if (!cached_issuer_peer_certificate_.empty()) { return cached_issuer_peer_certificate_; } - bssl::UniquePtr cert(SSL_get_peer_certificate(ssl_.get())); - if (!cert) { + if (!getX509PeerCertificate()) { ASSERT(cached_issuer_peer_certificate_.empty()); return cached_issuer_peer_certificate_; } - cached_issuer_peer_certificate_ = Utility::getIssuerFromCertificate(*cert); + cached_issuer_peer_certificate_ = Utility::getIssuerFromCertificate(*getX509PeerCertificate()); return cached_issuer_peer_certificate_; } @@ -542,12 +565,11 @@ const std::string& SslSocketInfo::subjectPeerCertificate() const { if (!cached_subject_peer_certificate_.empty()) { return cached_subject_peer_certificate_; } - bssl::UniquePtr cert(SSL_get_peer_certificate(ssl_.get())); - if (!cert) { + if (!getX509PeerCertificate()) { ASSERT(cached_subject_peer_certificate_.empty()); return cached_subject_peer_certificate_; } - cached_subject_peer_certificate_ = Utility::getSubjectFromCertificate(*cert); + cached_subject_peer_certificate_ = Utility::getSubjectFromCertificate(*getX509PeerCertificate()); return cached_subject_peer_certificate_; } @@ -555,29 +577,26 @@ const std::string& SslSocketInfo::subjectLocalCertificate() const { if (!cached_subject_local_certificate_.empty()) { return cached_subject_local_certificate_; } - X509* cert = SSL_get_certificate(ssl_.get()); - if (!cert) { + if (!getX509Certificate()) { ASSERT(cached_subject_local_certificate_.empty()); return cached_subject_local_certificate_; } - cached_subject_local_certificate_ = Utility::getSubjectFromCertificate(*cert); + cached_subject_local_certificate_ = Utility::getSubjectFromCertificate(*getX509Certificate()); return cached_subject_local_certificate_; } absl::optional SslSocketInfo::validFromPeerCertificate() const { - bssl::UniquePtr cert(SSL_get_peer_certificate(ssl_.get())); - if (!cert) { + if (!getX509PeerCertificate()) { return absl::nullopt; } - return Utility::getValidFrom(*cert); + return Utility::getValidFrom(*getX509PeerCertificate()); } absl::optional SslSocketInfo::expirationPeerCertificate() const { - bssl::UniquePtr cert(SSL_get_peer_certificate(ssl_.get())); - if (!cert) { + if (!getX509PeerCertificate()) { return absl::nullopt; } - return Utility::getExpirationTime(*cert); + return Utility::getExpirationTime(*getX509PeerCertificate()); } const std::string& SslSocketInfo::sessionId() const { diff --git a/source/extensions/transport_sockets/tls/ssl_socket.h b/source/extensions/transport_sockets/tls/ssl_socket.h index 43ee5efdfceb..6edad9ce6164 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.h +++ b/source/extensions/transport_sockets/tls/ssl_socket.h @@ -82,6 +82,14 @@ class SslSocketInfo : public Envoy::Ssl::ConnectionInfo { bssl::UniquePtr ssl_; private: + // getX509Certificate parses our leaf certificate, and caches and returns the result. + X509* getX509Certificate() const; + // getX509PeerCertificate parses the peer's leaf certificate (if any), and caches and returns the + // result. + X509* getX509PeerCertificate() const; + + mutable bssl::UniquePtr cached_certificate_; + mutable bssl::UniquePtr cached_peer_certificate_; mutable std::vector cached_uri_san_local_certificate_; mutable std::string cached_sha_256_peer_certificate_digest_; mutable std::string cached_serial_number_peer_certificate_; diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index a77c0eeb7ef1..0e75821f7a42 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -2512,9 +2512,9 @@ TEST_P(SslSocketTest, ClientAuthMultipleCAs) { SSL_set_cert_cb( ssl_socket->rawSslForTest(), [](SSL* ssl, void*) -> int { - STACK_OF(X509_NAME)* list = SSL_get_client_CA_list(ssl); + const STACK_OF(CRYPTO_BUFFER)* list = SSL_get0_server_requested_CAs(ssl); EXPECT_NE(nullptr, list); - EXPECT_EQ(2U, sk_X509_NAME_num(list)); + EXPECT_EQ(2U, sk_CRYPTO_BUFFER_num(list)); return 1; }, nullptr); From 4c386f577e755e0811344b897a079430659052bd Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Thu, 2 Apr 2020 01:27:41 +0000 Subject: [PATCH 02/12] review: fix casing of function names. Signed-off-by: Piotr Sikora --- .../transport_sockets/tls/context_impl.cc | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index d52e5c884dad..2396f9f95515 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -58,7 +58,7 @@ int sslTlsContextIndex() { }()); } -static bool SeekToSubject(CRYPTO_BUFFER* cert, CBS* tbs_certificate) { +static bool seekToSubject(CRYPTO_BUFFER* cert, CBS* tbs_certificate) { CBS der; CRYPTO_BUFFER_init_CBS(cert, &der); CBS certificate, opt_version, serial, signature, issuer, validity; @@ -93,9 +93,9 @@ static bool SeekToSubject(CRYPTO_BUFFER* cert, CBS* tbs_certificate) { return true; } -bool ExtractSPKIFromDERCert(CRYPTO_BUFFER* cert, CBS* spki) { +bool extractSPKIFromDERCert(CRYPTO_BUFFER* cert, CBS* spki) { CBS tbs_certificate; - if (!SeekToSubject(cert, &tbs_certificate)) { + if (!seekToSubject(cert, &tbs_certificate)) { return false; } @@ -107,9 +107,9 @@ bool ExtractSPKIFromDERCert(CRYPTO_BUFFER* cert, CBS* spki) { return true; } -bool ExtractSubjectNameFromDERCert(CRYPTO_BUFFER* cert, absl::string_view* subject_out) { +bool extractSubjectNameFromDERCert(CRYPTO_BUFFER* cert, absl::string_view* subject_out) { CBS tbs_certificate; - if (!SeekToSubject(cert, &tbs_certificate)) { + if (!seekToSubject(cert, &tbs_certificate)) { return false; } @@ -122,7 +122,7 @@ bool ExtractSubjectNameFromDERCert(CRYPTO_BUFFER* cert, absl::string_view* subje return true; } -static void PushBufferIfUnique(STACK_OF(CRYPTO_BUFFER) * stack, absl::string_view value) { +static void pushBufferIfUnique(STACK_OF(CRYPTO_BUFFER) * stack, absl::string_view value) { for (auto buf : stack) { if (CRYPTO_BUFFER_len(buf) == value.size() && memcmp(CRYPTO_BUFFER_data(buf), value.data(), value.size()) == 0) { @@ -393,7 +393,7 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c } CBS spki; - if (!ExtractSPKIFromDERCert(ctx.cert_.get(), &spki)) { + if (!extractSPKIFromDERCert(ctx.cert_.get(), &spki)) { throw EnvoyException(absl::StrCat("Failed to extract SPKI from ", ctx.cert_chain_file_path_)); } bssl::UniquePtr public_key(EVP_parse_public_key(&spki)); @@ -1458,11 +1458,11 @@ void ServerContextImpl::TlsContext::addClientValidationContext( bssl::UniquePtr der(data); bssl::UniquePtr cert(CRYPTO_BUFFER_new(data, len, nullptr)); absl::string_view name; - if (!ExtractSubjectNameFromDERCert(cert.get(), &name)) { + if (!extractSubjectNameFromDERCert(cert.get(), &name)) { throw EnvoyException( absl::StrCat("Failed to load trusted client CA certificates from ", config.caCertPath())); } - PushBufferIfUnique(list.get(), name); + pushBufferIfUnique(list.get(), name); } // Check for EOF. const uint32_t err = ERR_peek_last_error(); From 5744ac8797c6a255263e79715c98089b4da8fff9 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Sat, 4 Apr 2020 00:38:32 +0000 Subject: [PATCH 03/12] review: drop static. Signed-off-by: Piotr Sikora --- source/extensions/transport_sockets/tls/context_impl.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index 2396f9f95515..0fb6cdcd7bed 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -58,7 +58,7 @@ int sslTlsContextIndex() { }()); } -static bool seekToSubject(CRYPTO_BUFFER* cert, CBS* tbs_certificate) { +bool seekToSubject(CRYPTO_BUFFER* cert, CBS* tbs_certificate) { CBS der; CRYPTO_BUFFER_init_CBS(cert, &der); CBS certificate, opt_version, serial, signature, issuer, validity; @@ -122,7 +122,7 @@ bool extractSubjectNameFromDERCert(CRYPTO_BUFFER* cert, absl::string_view* subje return true; } -static void pushBufferIfUnique(STACK_OF(CRYPTO_BUFFER) * stack, absl::string_view value) { +void pushBufferIfUnique(STACK_OF(CRYPTO_BUFFER) * stack, absl::string_view value) { for (auto buf : stack) { if (CRYPTO_BUFFER_len(buf) == value.size() && memcmp(CRYPTO_BUFFER_data(buf), value.data(), value.size()) == 0) { From b7fa99a7b707326ba16c09aee169f6e1c0f99605 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Sat, 4 Apr 2020 00:39:07 +0000 Subject: [PATCH 04/12] review: use absl::optional. Signed-off-by: Piotr Sikora --- .../transport_sockets/tls/context_impl.cc | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index 0fb6cdcd7bed..79666aa9e033 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -104,22 +104,22 @@ bool extractSPKIFromDERCert(CRYPTO_BUFFER* cert, CBS* spki) { !CBS_get_asn1_element(&tbs_certificate, spki, CBS_ASN1_SEQUENCE)) { return false; } + return true; } -bool extractSubjectNameFromDERCert(CRYPTO_BUFFER* cert, absl::string_view* subject_out) { +absl::optional extractSubjectNameFromDERCert(CRYPTO_BUFFER* cert) { CBS tbs_certificate; if (!seekToSubject(cert, &tbs_certificate)) { - return false; + return absl::nullopt; } CBS subject; if (!CBS_get_asn1_element(&tbs_certificate, &subject, CBS_ASN1_SEQUENCE)) { - return false; + return absl::nullopt; } - *subject_out = - absl::string_view(reinterpret_cast(CBS_data(&subject)), CBS_len(&subject)); - return true; + + return absl::string_view(reinterpret_cast(CBS_data(&subject)), CBS_len(&subject)); } void pushBufferIfUnique(STACK_OF(CRYPTO_BUFFER) * stack, absl::string_view value) { @@ -1457,12 +1457,12 @@ void ServerContextImpl::TlsContext::addClientValidationContext( } bssl::UniquePtr der(data); bssl::UniquePtr cert(CRYPTO_BUFFER_new(data, len, nullptr)); - absl::string_view name; - if (!extractSubjectNameFromDERCert(cert.get(), &name)) { + auto name = extractSubjectNameFromDERCert(cert.get()); + if (!name) { throw EnvoyException( absl::StrCat("Failed to load trusted client CA certificates from ", config.caCertPath())); } - pushBufferIfUnique(list.get(), name); + pushBufferIfUnique(list.get(), name.value()); } // Check for EOF. const uint32_t err = ERR_peek_last_error(); From e23fc68778aca403bbd4245d46886ff8e018fff4 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Sat, 4 Apr 2020 00:39:28 +0000 Subject: [PATCH 05/12] review: remove unnecessary check. Signed-off-by: Piotr Sikora --- source/extensions/transport_sockets/tls/context_impl.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index 79666aa9e033..0bdeef7e5784 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -360,8 +360,7 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c RELEASE_ASSERT(bio != nullptr, ""); uint8_t* data = nullptr; long len; - if (!PEM_bytes_read_bio(&data, &len, nullptr, PEM_STRING_X509, bio.get(), nullptr, nullptr) || - !data) { + if (!PEM_bytes_read_bio(&data, &len, nullptr, PEM_STRING_X509, bio.get(), nullptr, nullptr)) { throw EnvoyException( absl::StrCat("Failed to load certificate chain from ", ctx.cert_chain_file_path_)); } From 66db64415bee75d095c8cba43ab1f9ee649208ea Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Mon, 10 Aug 2020 04:01:50 +0000 Subject: [PATCH 06/12] review: rename leafCertificate() to localLeafCertificate(). Signed-off-by: Piotr Sikora --- source/extensions/transport_sockets/tls/context_impl.cc | 2 +- source/extensions/transport_sockets/tls/context_impl.h | 2 +- source/extensions/transport_sockets/tls/ssl_socket.cc | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index 30be5e3b9ea8..eea861dd5b1d 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -875,7 +875,7 @@ bool ContextImpl::dnsNameMatch(const absl::string_view dns_name, const absl::str return false; } -CRYPTO_BUFFER* ContextImpl::leafCertificate(SSL* ssl) { +CRYPTO_BUFFER* ContextImpl::localLeafCertificate(SSL* ssl) { auto tls_context = reinterpret_cast( SSL_CTX_get_ex_data(SSL_get_SSL_CTX(ssl), sslTlsContextIndex())); return tls_context->cert_.get(); diff --git a/source/extensions/transport_sockets/tls/context_impl.h b/source/extensions/transport_sockets/tls/context_impl.h index f0035aa52eeb..60d5dfb5809a 100644 --- a/source/extensions/transport_sockets/tls/context_impl.h +++ b/source/extensions/transport_sockets/tls/context_impl.h @@ -91,7 +91,7 @@ class ContextImpl : public virtual Envoy::Ssl::Context { * @param ssl a connected SSL connection * @return the DER-encoded leaf certificate that was presented during the SSL handshake. */ - static CRYPTO_BUFFER* leafCertificate(SSL* ssl); + static CRYPTO_BUFFER* localLeafCertificate(SSL* ssl); SslStats& stats() { return stats_; } diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index 949630b33931..d9bea54a7e75 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -529,7 +529,7 @@ X509* SslSocketInfo::getX509Certificate() const { if (cached_certificate_) { return cached_certificate_.get(); } - CRYPTO_BUFFER* leaf = ContextImpl::leafCertificate(ssl_.get()); + CRYPTO_BUFFER* leaf = ContextImpl::localLeafCertificate(ssl_.get()); if (leaf) { auto* start = CRYPTO_BUFFER_data(leaf); cached_certificate_.reset(d2i_X509(nullptr, &start, CRYPTO_BUFFER_len(leaf))); From 91ecf00d51acd1a71757a74026a8be7220fc65bf Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Mon, 10 Aug 2020 04:10:53 +0000 Subject: [PATCH 07/12] review: use X509_parse_from_buffer(). Signed-off-by: Piotr Sikora --- source/extensions/transport_sockets/tls/context_impl.cc | 9 +++------ source/extensions/transport_sockets/tls/ssl_socket.cc | 6 ++---- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index eea861dd5b1d..39388340cc60 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -932,8 +932,7 @@ size_t ContextImpl::daysUntilFirstCertExpires() const { if (ctx.cert_ == nullptr) { return 0; } - auto* start = CRYPTO_BUFFER_data(ctx.cert_.get()); - bssl::UniquePtr x509(d2i_X509(nullptr, &start, CRYPTO_BUFFER_len(ctx.cert_.get()))); + bssl::UniquePtr x509(X509_parse_from_buffer(ctx.cert_.get())); if (!x509) { return 0; } @@ -959,8 +958,7 @@ std::vector ContextImpl::getCertChainInformat if (ctx.cert_ == nullptr) { continue; } - auto* start = CRYPTO_BUFFER_data(ctx.cert_.get()); - bssl::UniquePtr x509(d2i_X509(nullptr, &start, CRYPTO_BUFFER_len(ctx.cert_.get()))); + bssl::UniquePtr x509(X509_parse_from_buffer(ctx.cert_.get())); if (!x509) { continue; } @@ -1236,8 +1234,7 @@ ServerContextImpl::generateHashForSessionContextId(const std::vector cert(d2i_X509(nullptr, &start, CRYPTO_BUFFER_len(ctx.cert_.get()))); + bssl::UniquePtr cert(X509_parse_from_buffer(ctx.cert_.get())); RELEASE_ASSERT(cert != nullptr, "TLS context should have an active certificate"); X509_NAME* cert_subject = X509_get_subject_name(cert.get()); RELEASE_ASSERT(cert_subject != nullptr, "TLS certificate should have a subject"); diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index d9bea54a7e75..03b476fbef8e 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -531,8 +531,7 @@ X509* SslSocketInfo::getX509Certificate() const { } CRYPTO_BUFFER* leaf = ContextImpl::localLeafCertificate(ssl_.get()); if (leaf) { - auto* start = CRYPTO_BUFFER_data(leaf); - cached_certificate_.reset(d2i_X509(nullptr, &start, CRYPTO_BUFFER_len(leaf))); + cached_certificate_.reset(X509_parse_from_buffer(leaf)); } return cached_certificate_.get(); } @@ -544,8 +543,7 @@ X509* SslSocketInfo::getX509PeerCertificate() const { const STACK_OF(CRYPTO_BUFFER)* chain = SSL_get0_peer_certificates(ssl_.get()); if (chain && sk_CRYPTO_BUFFER_num(chain) > 0) { CRYPTO_BUFFER* leaf = sk_CRYPTO_BUFFER_value(chain, 0); - auto* start = CRYPTO_BUFFER_data(leaf); - cached_peer_certificate_.reset(d2i_X509(nullptr, &start, CRYPTO_BUFFER_len(leaf))); + cached_peer_certificate_.reset(X509_parse_from_buffer(leaf)); } return cached_peer_certificate_.get(); } From 46a37d35e129df761724b5e31c65f184670a6924 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Mon, 10 Aug 2020 04:18:25 +0000 Subject: [PATCH 08/12] review: reject corrupted peer certificates. Signed-off-by: Piotr Sikora --- source/extensions/transport_sockets/tls/context_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index 39388340cc60..ff314786ffe1 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -640,7 +640,7 @@ ssl_verify_result_t ContextImpl::verifyCallback(SSL* ssl, uint8_t* out_alert) { for (size_t i = 0; i < sk_CRYPTO_BUFFER_num(buffers); ++i) { bssl::UniquePtr x509(X509_parse_from_buffer(sk_CRYPTO_BUFFER_value(buffers, i))); if (!x509) { - continue; + return ssl_verify_invalid; } if (!cert) { cert = std::move(x509); From c1685bf77e47139a624d78a50ae2b0809dc62b46 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Mon, 10 Aug 2020 06:46:22 +0000 Subject: [PATCH 09/12] review: guard against cases that cannot happen. Signed-off-by: Piotr Sikora --- .../extensions/transport_sockets/tls/context_impl.cc | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index ff314786ffe1..c52bfe06f70e 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -930,12 +930,10 @@ size_t ContextImpl::daysUntilFirstCertExpires() const { int daysUntilExpiration = Utility::getDaysUntilExpiration(ca_cert_.get(), time_source_); for (auto& ctx : tls_contexts_) { if (ctx.cert_ == nullptr) { - return 0; + continue; } bssl::UniquePtr x509(X509_parse_from_buffer(ctx.cert_.get())); - if (!x509) { - return 0; - } + RELEASE_ASSERT(x509 != nullptr, "TLS context must have a valid certificate"); daysUntilExpiration = std::min(Utility::getDaysUntilExpiration(x509.get(), time_source_), daysUntilExpiration); } @@ -959,9 +957,7 @@ std::vector ContextImpl::getCertChainInformat continue; } bssl::UniquePtr x509(X509_parse_from_buffer(ctx.cert_.get())); - if (!x509) { - continue; - } + RELEASE_ASSERT(x509 != nullptr, "TLS context must have a valid certificate"); cert_details.emplace_back(certificateDetails(x509.get(), ctx.getCertChainFileName())); } return cert_details; From 13af8c04753606c49e7338733ba10a1b9281e2e0 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Mon, 10 Aug 2020 06:49:50 +0000 Subject: [PATCH 10/12] review: address some of Greg's comments. Signed-off-by: Piotr Sikora --- source/extensions/transport_sockets/tls/context_impl.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index c52bfe06f70e..86c97dacf653 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -381,16 +381,17 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c throw EnvoyException( absl::StrCat("Failed to load certificate chain from ", ctx.cert_chain_file_path_)); } - bssl::UniquePtr der(data); - ctx.cert_.reset(CRYPTO_BUFFER_new(data, len, nullptr)); - + { + bssl::UniquePtr der(data); + ctx.cert_.reset(CRYPTO_BUFFER_new(data, len, nullptr)); + } // Read rest of the certificate chain. std::vector> chain; while (true) { if (!PEM_bytes_read_bio(&data, &len, nullptr, PEM_STRING_X509, bio.get(), nullptr, nullptr)) { break; } - der.reset(data); + bssl::UniquePtr der(data); chain.push_back(bssl::UniquePtr(CRYPTO_BUFFER_new(data, len, nullptr))); } // Check for EOF. From 9611e50cf4ba4e7df3f77b5d284a8c5bc776d9d6 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Mon, 10 Aug 2020 07:18:05 +0000 Subject: [PATCH 11/12] review: use TLS_with_buffers_method(). Signed-off-by: Piotr Sikora --- test/extensions/transport_sockets/tls/context_impl_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/extensions/transport_sockets/tls/context_impl_test.cc b/test/extensions/transport_sockets/tls/context_impl_test.cc index 60cec6e1fe17..4bb3d4a970a2 100644 --- a/test/extensions/transport_sockets/tls/context_impl_test.cc +++ b/test/extensions/transport_sockets/tls/context_impl_test.cc @@ -75,7 +75,7 @@ INSTANTIATE_TEST_SUITE_P(CipherSuites, SslLibraryCipherSuiteSupport, // Tests for whether new cipher suites are added. When they are, they must be added to // knownCipherSuites() so that this test can detect if they are removed in the future. TEST_F(SslLibraryCipherSuiteSupport, CipherSuitesNotAdded) { - bssl::UniquePtr ctx(SSL_CTX_new(TLS_method())); + bssl::UniquePtr ctx(SSL_CTX_new(TLS_with_buffers_method())); EXPECT_NE(0, SSL_CTX_set_strict_cipher_list(ctx.get(), "ALL")); std::vector present_cipher_suites; @@ -89,7 +89,7 @@ TEST_F(SslLibraryCipherSuiteSupport, CipherSuitesNotAdded) { // suite is removed, it must be added to the release notes as an incompatible change, because it can // cause previously loadable configurations to no longer load if they reference the cipher suite. TEST_P(SslLibraryCipherSuiteSupport, CipherSuitesNotRemoved) { - bssl::UniquePtr ctx(SSL_CTX_new(TLS_method())); + bssl::UniquePtr ctx(SSL_CTX_new(TLS_with_buffers_method())); EXPECT_NE(0, SSL_CTX_set_strict_cipher_list(ctx.get(), GetParam().c_str())); } From 577c26fd29969b84bfe21d6e00070fe885ed96f6 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Mon, 10 Aug 2020 07:20:22 +0000 Subject: [PATCH 12/12] review: fix comment. Signed-off-by: Piotr Sikora --- source/extensions/transport_sockets/tls/context_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index 86c97dacf653..6f136859fa18 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -272,7 +272,7 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c verify_mode = SSL_VERIFY_PEER; verify_trusted_ca_ = true; - // NOTE: We're using SSL_CTX_set_custom_verify_cb() instead of X509_verify_cert() directly. + // NOTE: We're using SSL_CTX_set_custom_verify() instead of X509_verify_cert() directly. // However, our new callback is still calling X509_verify_cert() under the hood. Therefore, to // ignore cert expiration, we need to set the callback for X509_verify_cert() to ignore that // error.