From e9f936d85dc1edc34fabd0a1725ec180f2316353 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Tue, 1 Feb 2022 19:57:15 +0000 Subject: [PATCH] CVE-2022-21654 tls allows re-use when some cert validation settings have changed Signed-off-by: Yan Avlasov --- .../certificate_validation_context_config.h | 5 + .../tls/cert_validator/cert_validator.h | 5 +- .../tls/cert_validator/default_validator.cc | 29 ++++ .../transport_sockets/tls/ssl_socket_test.cc | 158 ++++++++++++++++++ 4 files changed, 196 insertions(+), 1 deletion(-) diff --git a/envoy/ssl/certificate_validation_context_config.h b/envoy/ssl/certificate_validation_context_config.h index d331c45ce254..1d839be7e45a 100644 --- a/envoy/ssl/certificate_validation_context_config.h +++ b/envoy/ssl/certificate_validation_context_config.h @@ -15,6 +15,11 @@ namespace Envoy { namespace Ssl { +// SECURITY NOTE +// +// When adding or changing this interface, it is likely that a change is needed to +// `DefaultCertValidator::updateDigestForSessionId` in +// `source/extensions/transport_sockets/tls/cert_validator/default_validator.cc`. class CertificateValidationContextConfig { public: virtual ~CertificateValidationContextConfig() = default; diff --git a/source/extensions/transport_sockets/tls/cert_validator/cert_validator.h b/source/extensions/transport_sockets/tls/cert_validator/cert_validator.h index 5e0f589b4890..57d18c595210 100644 --- a/source/extensions/transport_sockets/tls/cert_validator/cert_validator.h +++ b/source/extensions/transport_sockets/tls/cert_validator/cert_validator.h @@ -62,7 +62,10 @@ class CertValidator { bool handshaker_provides_certificates) PURE; /** - * Called when calculation hash for session context ids + * Called when calculation hash for session context ids. This hash MUST include all + * configuration used to validate a peer certificate, so that if this configuration + * is changed, sessions cannot be re-used and must be re-negotiated and re-validated + * using the new settings. * * @param md the store context * @param hash_buffer the buffer used for digest calculation diff --git a/source/extensions/transport_sockets/tls/cert_validator/default_validator.cc b/source/extensions/transport_sockets/tls/cert_validator/default_validator.cc index 3b28200b73ab..da61d6115c02 100644 --- a/source/extensions/transport_sockets/tls/cert_validator/default_validator.cc +++ b/source/extensions/transport_sockets/tls/cert_validator/default_validator.cc @@ -378,6 +378,35 @@ void DefaultCertValidator::updateDigestForSessionId(bssl::ScopedEVP_MD_CTX& md, sizeof(std::remove_reference::type::value_type)); RELEASE_ASSERT(rc == 1, Utility::getLastCryptoError().value_or("")); } + + rc = EVP_DigestUpdate(md.get(), &verify_trusted_ca_, sizeof(verify_trusted_ca_)); + RELEASE_ASSERT(rc == 1, Utility::getLastCryptoError().value_or("")); + + if (config_ != nullptr) { + for (const auto& matcher : config_->subjectAltNameMatchers()) { + size_t hash = MessageUtil::hash(matcher); + rc = EVP_DigestUpdate(md.get(), &hash, sizeof(hash)); + RELEASE_ASSERT(rc == 1, Utility::getLastCryptoError().value_or("")); + } + + const std::string& crl = config_->certificateRevocationList(); + if (!crl.empty()) { + rc = EVP_DigestUpdate(md.get(), crl.data(), crl.length()); + RELEASE_ASSERT(rc == 1, Utility::getLastCryptoError().value_or("")); + } + + bool allow_expired = config_->allowExpiredCertificate(); + rc = EVP_DigestUpdate(md.get(), &allow_expired, sizeof(allow_expired)); + RELEASE_ASSERT(rc == 1, Utility::getLastCryptoError().value_or("")); + + auto trust_chain_verification = config_->trustChainVerification(); + rc = EVP_DigestUpdate(md.get(), &trust_chain_verification, sizeof(trust_chain_verification)); + RELEASE_ASSERT(rc == 1, Utility::getLastCryptoError().value_or("")); + + auto only_leaf_crl = config_->onlyVerifyLeafCertificateCrl(); + rc = EVP_DigestUpdate(md.get(), &only_leaf_crl, sizeof(only_leaf_crl)); + RELEASE_ASSERT(rc == 1, Utility::getLastCryptoError().value_or("")); + } } void DefaultCertValidator::addClientValidationContext(SSL_CTX* ctx, bool require_client_cert) { diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 5e9d7fa605b0..06b61503fe81 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -3373,6 +3373,164 @@ TEST_P(SslSocketTest, TicketSessionResumptionDifferentServerNames) { client_ctx_yaml, false, GetParam()); } +// Sessions cannot be resumed even though the server certificates are the same, +// because of the different `verify_certificate_hash` settings. +TEST_P(SslSocketTest, TicketSessionResumptionDifferentVerifyCertHash) { + const std::string server_ctx_yaml1 = absl::StrCat(R"EOF( + session_ticket_keys: + keys: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ticket_key_a" + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_key.pem" + validation_context: + trusted_ca: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" + verify_certificate_hash: + - ")EOF", + TEST_SAN_URI_CERT_256_HASH, "\""); + + const std::string server_ctx_yaml2 = absl::StrCat(R"EOF( + session_ticket_keys: + keys: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ticket_key_a" + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_key.pem" + validation_context: + trusted_ca: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" + verify_certificate_hash: + - "0000000000000000000000000000000000000000000000000000000000000000" + - ")EOF", + TEST_SAN_URI_CERT_256_HASH, "\""); + + const std::string client_ctx_yaml = R"EOF( + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_uri_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_uri_key.pem" +)EOF"; + + testTicketSessionResumption(server_ctx_yaml1, {}, server_ctx_yaml1, {}, client_ctx_yaml, true, + GetParam()); + testTicketSessionResumption(server_ctx_yaml1, {}, server_ctx_yaml2, {}, client_ctx_yaml, false, + GetParam()); +} + +// Sessions cannot be resumed even though the server certificates are the same, +// because of the different `verify_certificate_spki` settings. +TEST_P(SslSocketTest, TicketSessionResumptionDifferentVerifyCertSpki) { + const std::string server_ctx_yaml1 = absl::StrCat(R"EOF( + session_ticket_keys: + keys: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ticket_key_a" + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_key.pem" + validation_context: + trusted_ca: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" + verify_certificate_spki: + - ")EOF", + TEST_SAN_URI_CERT_SPKI, "\""); + + const std::string server_ctx_yaml2 = absl::StrCat(R"EOF( + session_ticket_keys: + keys: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ticket_key_a" + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_key.pem" + validation_context: + trusted_ca: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" + verify_certificate_spki: + - "NvqYIYSbgK2vCJpQhObf77vv+bQWtc5ek5RIOwPiC9A=" + - ")EOF", + TEST_SAN_URI_CERT_SPKI, "\""); + + const std::string client_ctx_yaml = R"EOF( + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_uri_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_uri_key.pem" +)EOF"; + + testTicketSessionResumption(server_ctx_yaml1, {}, server_ctx_yaml1, {}, client_ctx_yaml, true, + GetParam()); + testTicketSessionResumption(server_ctx_yaml1, {}, server_ctx_yaml2, {}, client_ctx_yaml, false, + GetParam()); +} + +// Sessions cannot be resumed even though the server certificates are the same, +// because of the different `match_subject_alt_names` settings. +TEST_P(SslSocketTest, TicketSessionResumptionDifferentMatchSAN) { + const std::string server_ctx_yaml1 = R"EOF( + session_ticket_keys: + keys: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ticket_key_a" + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_key.pem" + validation_context: + trusted_ca: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" + match_subject_alt_names: + - exact: "spiffe://lyft.com/test-team" +)EOF"; + + const std::string server_ctx_yaml2 = R"EOF( + session_ticket_keys: + keys: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ticket_key_a" + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_key.pem" + validation_context: + trusted_ca: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" + match_subject_alt_names: + - prefix: "spiffe://lyft.com/test-team" +")EOF"; + + const std::string client_ctx_yaml = R"EOF( + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_uri_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_uri_key.pem" +)EOF"; + + testTicketSessionResumption(server_ctx_yaml1, {}, server_ctx_yaml1, {}, client_ctx_yaml, true, + GetParam()); + testTicketSessionResumption(server_ctx_yaml1, {}, server_ctx_yaml2, {}, client_ctx_yaml, false, + GetParam()); +} + // Sessions can be resumed because the server certificates are different but the CN/SANs and // issuer are identical TEST_P(SslSocketTest, TicketSessionResumptionDifferentServerCert) {