CVE-2022-21654: Improper Certificate Validation (CWE-295)
value: Envoy is an open source edge and service proxy, designed for cloud-native applications. Envoy's tls allows re-use when some cert validation settings have changed from their default configuration. The only workaround for this issue is to ensure that default tls settings are used. Users are advised to upgrade.
CWE description: The product does not validate, or incorrectly validates, a certificate.
CVSS v3 base score: 9.8
Published: 2022-02-22T23:15Z
Repository: https://github.com/envoyproxy/envoy
Language: C++
Files changed: envoy/ssl/certificate_validation_context_config.h, source/extensions/transport_sockets/tls/cert_validator/cert_validator.h, source/extensions/transport_sockets/tls/cert_validator/default_validator.cc, test/extensions/transport_sockets/tls/ssl_socket_test.cc
Commit date: 2022-02-22 17:58:22 +0000
Commit message: CVE-2022-21654

tls allows re-use when some cert validation settings have changed

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Diff: diff --git a/envoy/ssl/certificate_validation_context_config.h b/envoy/ssl/certificate_validation_context_config.h
index d331c45ce2..1d839be7e4 100644
--- a/envoy/ssl/certificate_validation_context_config.h
+++ b/envoy/ssl/certificate_validation_context_config.h
@@ -13,10 +13,15 @@
 #include "absl/types/optional.h"
 
 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 5e0f589b48..57d18c5952 100644
--- a/source/extensions/transport_sockets/tls/cert_validator/cert_validator.h
+++ b/source/extensions/transport_sockets/tls/cert_validator/cert_validator.h
@@ -60,11 +60,14 @@ public:
    */
   virtual int initializeSslContexts(std::vector<SSL_CTX*> contexts,
                                     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
    * @param hash_length the expected length of hash
    */
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 3b28200b73..da61d6115c 100644
--- a/source/extensions/transport_sockets/tls/cert_validator/default_validator.cc
+++ b/source/extensions/transport_sockets/tls/cert_validator/default_validator.cc
@@ -376,10 +376,39 @@ void DefaultCertValidator::updateDigestForSessionId(bssl::ScopedEVP_MD_CTX& md,
     rc = EVP_DigestUpdate(md.get(), hash.data(),
                           hash.size() *
                               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) {
   if (config_ == nullptr || config_->caCert().empty()) {
     return;
diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc
index 5e9d7fa605..06b61503fe 100644
--- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc
+++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc
@@ -3371,10 +3371,168 @@ TEST_P(SslSocketTest, TicketSessionResumptionDifferentServerNames) {
 
   testTicketSessionResumption(server_ctx_yaml1, server_names1, 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_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) {
   const std::string server_ctx_yaml1 = R"EOF(
   common_tls_context:

Vulnerable code:    * Called when calculation hash for session context ids
Fixed code: // 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`.
   * 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.

  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(""));
  }
// 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());
}