CVE-2022-21654

tls allows re-use when some cert validation settings have changed Signed-off-by: Yan Avlasov <yavlasov@google.com>
envoyproxy · Feb 22, 2022 · e9f936d · e9f936d
1 parent 9371333
commit e9f936d
Show file tree

Hide file tree

Showing 4 changed files with 196 additions and 1 deletion.
diff --git 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
@@ -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
@@ -378,6 +378,35 @@ void DefaultCertValidator::updateDigestForSessionId(bssl::ScopedEVP_MD_CTX& md,
                               sizeof(std::remove_reference<decltype(hash)>::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
@@ -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) {