Test that OpenSSL mTLS integration is not vulnerable to certificate stuffing

This adds a test that our OpenSSL mTLS integration is not
vulnerable to CVE-2023-2422-style certificate credential
stuffing.

Spoiler alert: we're not, and never have been vulnerable.
But this test shall help to ensure we also never accidentally
will be in the future.

If a server is vulnerable to certificate stuffing, a sneaky
client may include both a valid certificate chain (containing
credential set A) as well as a self-signed peer certificate
(containing credential set B). The vulnerable server thinks
the latter cert has been verified, even though the mTLS
implementation only verifies the first (actual) client cert
as being signed by the CA. The server may then wrongfully
choose to include set B as the client's credentials.

We explicitly only consider certificates in the chain at
OpenSSL "error depth zero", which means the "end entity
certificate", i.e. the client peer.

1 files changed, 45 insertions, 2 deletions

diff --git a/vespalib/src/tests/net/tls/openssl_impl/openssl_impl_test.cpp b/vespalib/src/tests/net/tls/openssl_impl/openssl_impl_test.cpp
index a75c7dff150..6d5c5fa6308 100644
--- a/vespalib/src/tests/net/tls/openssl_impl/openssl_impl_test.cpp
+++ b/vespalib/src/tests/net/tls/openssl_impl/openssl_impl_test.cpp
@@ -455,8 +455,8 @@ struct CertFixture : Fixture {
     {}
     ~CertFixture();
 
-    CertKeyWrapper create_ca_issued_peer_cert(const std::vector<vespalib::string>& common_names,
-                                              const std::vector<vespalib::string>& sans) {
+    static X509Certificate::SubjectInfo make_subject_info(const std::vector<vespalib::string>& common_names,
+                                                          const std::vector<vespalib::string>& sans) {
         auto dn = X509Certificate::DistinguishedName()
                 .country("US").state("CA").locality("Sunnyvale")
                 .organization("Wile E. Coyote, Ltd.")
@@ -468,12 +468,27 @@ struct CertFixture : Fixture {
         for (auto& san : sans) {
             subject.add_subject_alt_name(san);
         }
+        return subject;
+    }
+
+    CertKeyWrapper create_ca_issued_peer_cert(const std::vector<vespalib::string>& common_names,
+                                              const std::vector<vespalib::string>& sans) const {
+        auto subject = make_subject_info(common_names, sans);
         auto key = PrivateKey::generate_p256_ec_key();
         auto params = X509Certificate::Params::issued_by(std::move(subject), key, root_ca.cert, root_ca.key);
         auto cert = X509Certificate::generate_from(std::move(params));
         return {std::move(cert), std::move(key)};
     }
 
+    CertKeyWrapper create_self_signed_peer_cert(const std::vector<vespalib::string>& common_names,
+                                                const std::vector<vespalib::string>& sans) const {
+        auto subject = make_subject_info(common_names, sans);
+        auto key = PrivateKey::generate_p256_ec_key();
+        auto params = X509Certificate::Params::self_signed(std::move(subject), key);
+        auto cert = X509Certificate::generate_from(std::move(params));
+        return {std::move(cert), std::move(key)};
+    }
+
     static std::unique_ptr<OpenSslCryptoCodecImpl> create_openssl_codec_with_authz_mode(
             const TransportSecurityOptions& opts,
             std::shared_ptr<CertificateVerificationCallback> cert_verify_callback,
@@ -663,6 +678,34 @@ TEST_F("Only DNS and URI SANs are enumerated", CertFixture) {
     EXPECT_EQUAL(0u, server_cb->creds.uri_sans.size());
 }
 
+// A server must only trust the actual verified peer certificate, not any other random
+// certificate that the client decides to include in its certificate chain. See CVE-2023-2422.
+// Note: this is a preemptive test; we are not--and have never been--vulnerable to this issue.
+TEST_F("Certificate credential extraction is not vulnerable to CVE-2023-2422", CertFixture) {
+    auto good_ck = f.create_ca_issued_peer_cert({}, {{"DNS:legit.example.com"}});
+    auto evil_ck = f.create_self_signed_peer_cert({"rudolf.example.com"}, {{"DNS:blodstrupmoen.example.com"}});
+
+    auto ts_params = TransportSecurityOptions::Params().
+            ca_certs_pem(f.root_ca.cert->to_pem()).
+            // Concatenate CA-signed good cert with self-signed cert with different credentials.
+            // We should only ever look at the good cert.
+            cert_chain_pem(good_ck.cert->to_pem() + evil_ck.cert->to_pem()).
+            private_key_pem(good_ck.key->private_to_pem() + evil_ck.key->private_to_pem()).
+            authorized_peers(AuthorizedPeers::allow_all_authenticated());
+
+    f.client = f.create_openssl_codec(TransportSecurityOptions(std::move(ts_params)),
+                                      std::make_shared<PrintingCertificateCallback>(),
+                                      CryptoCodec::Mode::Client);
+    auto server_cb = std::make_shared<MockCertificateCallback>();
+    f.reset_server_with_cert_opts(good_ck, server_cb);
+    ASSERT_TRUE(f.handshake());
+
+    auto& creds = server_cb->creds;
+    EXPECT_EQUAL("", creds.common_name);
+    ASSERT_EQUAL(1u, creds.dns_sans.size());
+    EXPECT_EQUAL("legit.example.com", creds.dns_sans[0]);
+}
+
 // We don't test too many combinations of peer policies here, only that
 // the wiring is set up. Verification logic is tested elsewhere.