From 3aa32cc5f624141bb2a373f95d380b106f19b890 Mon Sep 17 00:00:00 2001 From: Samuel Chiang Date: Wed, 13 Nov 2024 12:58:44 -0800 Subject: [PATCH] Account for cipher auth with multiple cert slots (#1956) Ruby has a dependency on the multiple certificate slot mechanisms that OpenSSL allows for. We've already done the work to support this, but another Ruby 3.1 test exposed a gap in our support. We were only looking at the negotiated signature algorithms to retrieve the right certificate for the server to send back, but the cipher authentication scheme was also checked in OpenSSL as well. Ruby's tests happen to only depend on configuring the authentication scheme which brought this to light. We already happen to do this when checking private keys for TLS 1.0/1.1, this also fixes it to check the cipher against the initial public key. This change also introduces all of the mentioned cipher authenticated check behavior for TLS 1.2. ### Testing: New unit test that allows all possible sigalgs, with the cipher authentication suite being the only restriction. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license. --- ssl/extensions.cc | 2 +- ssl/internal.h | 5 +++ ssl/ssl_privkey.cc | 60 ++++++++++++++++++++++++++---------- ssl/ssl_test.cc | 77 ++++++++++++++++++++++++++++++++++------------ ssl/ssl_x509.cc | 2 +- 5 files changed, 109 insertions(+), 37 deletions(-) diff --git a/ssl/extensions.cc b/ssl/extensions.cc index 45b9e8d3c9..a44bb87095 100644 --- a/ssl/extensions.cc +++ b/ssl/extensions.cc @@ -4139,7 +4139,7 @@ bool tls1_choose_signature_algorithm(SSL_HANDSHAKE *hs, uint16_t *out) { // Before TLS 1.2, the signature algorithm isn't negotiated as part of the // handshake. if (ssl_protocol_version(ssl) < TLS1_2_VERSION) { - if (tls1_get_legacy_signature_algorithm(out, hs->local_pubkey.get()) || + if (ssl_public_key_supports_legacy_signature_algorithm(out, hs) || ssl_cert_private_keys_supports_legacy_signature_algorithm(out, hs)) { return true; } diff --git a/ssl/internal.h b/ssl/internal.h index 494a466801..bd4a51087b 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -1140,6 +1140,11 @@ enum ssl_private_key_result_t ssl_private_key_decrypt(SSL_HANDSHAKE *hs, bool ssl_public_key_supports_signature_algorithm(SSL_HANDSHAKE *hs, uint16_t sigalg); +// ssl_public_key_supports_legacy_signature_algorithm is the tls1.0/1.1 +// version of |ssl_public_key_supports_signature_algorithm|. +bool ssl_public_key_supports_legacy_signature_algorithm(uint16_t *out, + SSL_HANDSHAKE *hs); + // ssl_cert_private_keys_supports_signature_algorithm returns whether any of // |hs|'s available private keys supports |sigalg|. If one does, we switch to // using that private key and the corresponding certificate for the rest of the diff --git a/ssl/ssl_privkey.cc b/ssl/ssl_privkey.cc index 2a6ec0a95c..6806e54a3c 100644 --- a/ssl/ssl_privkey.cc +++ b/ssl/ssl_privkey.cc @@ -383,10 +383,25 @@ static bool ssl_public_key_rsa_pss_check(EVP_PKEY *pubkey, uint16_t sigalg) { return true; } +static bool tls12_pkey_supports_cipher_auth(SSL_HANDSHAKE *hs, + const EVP_PKEY *key) { + SSL *const ssl = hs->ssl; + // We may have a private key that supports the signature algorithm, but we + // need to verify that the negotiated cipher allows it. This behavior is only + // done in OpenSSL servers with TLS version 1.2 and below since TLS 1.3 does + // not have cipher-based authentication configuration. Since authentication is + // configured outside the ciphersuite in TLS 1.3, we use the |SSL_aGENERIC| + // flag defined for all TLS 1.3 ciphers to indicate support. + return !ssl->server || (hs->new_cipher->algorithm_auth & + (ssl_cipher_auth_mask_for_key(key) | SSL_aGENERIC)); +} + bool ssl_public_key_supports_signature_algorithm(SSL_HANDSHAKE *hs, uint16_t sigalg) { SSL *const ssl = hs->ssl; - if (!pkey_supports_algorithm(ssl, hs->local_pubkey.get(), sigalg)) { + assert(ssl_protocol_version(ssl) >= TLS1_2_VERSION); + if (!tls12_pkey_supports_cipher_auth(hs, hs->local_pubkey.get()) || + !pkey_supports_algorithm(ssl, hs->local_pubkey.get(), sigalg)) { return false; } @@ -397,8 +412,7 @@ bool ssl_public_key_supports_signature_algorithm(SSL_HANDSHAKE *hs, return true; } -UniquePtr ssl_cert_parse_leaf_pubkey( - STACK_OF(CRYPTO_BUFFER) *chain) { +UniquePtr ssl_cert_parse_leaf_pubkey(STACK_OF(CRYPTO_BUFFER) *chain) { const CRYPTO_BUFFER *buf = sk_CRYPTO_BUFFER_value(chain, 0); if (buf == nullptr) { return nullptr; @@ -408,6 +422,17 @@ UniquePtr ssl_cert_parse_leaf_pubkey( return ssl_cert_parse_pubkey(&leaf); } +bool ssl_public_key_supports_legacy_signature_algorithm(uint16_t *out, + SSL_HANDSHAKE *hs) { + SSL *const ssl = hs->ssl; + assert(ssl_protocol_version(ssl) < TLS1_2_VERSION); + const uint32_t auth_allowed = + !ssl->server || (hs->new_cipher->algorithm_auth & + ssl_cipher_auth_mask_for_key(hs->local_pubkey.get())); + return tls1_get_legacy_signature_algorithm(out, hs->local_pubkey.get()) && + auth_allowed; +} + bool ssl_cert_private_keys_supports_legacy_signature_algorithm( uint16_t *out, SSL_HANDSHAKE *hs) { SSL *const ssl = hs->ssl; @@ -447,6 +472,7 @@ bool ssl_cert_private_keys_supports_legacy_signature_algorithm( bool ssl_cert_private_keys_supports_signature_algorithm(SSL_HANDSHAKE *hs, uint16_t sigalg) { SSL *const ssl = hs->ssl; + assert(ssl_protocol_version(ssl) >= TLS1_2_VERSION); CERT *cert = hs->config->cert.get(); // Only the server without delegated credentials has support for multiple // certificate slots. @@ -457,20 +483,22 @@ bool ssl_cert_private_keys_supports_signature_algorithm(SSL_HANDSHAKE *hs, for (size_t i = 0; i < cert->cert_private_keys.size(); i++) { EVP_PKEY *private_key = cert->cert_private_keys[i].privatekey.get(); UniquePtr public_key = - ssl_cert_parse_leaf_pubkey(cert->cert_private_keys[i].chain.get()); - if (private_key != nullptr && public_key != nullptr && - pkey_supports_algorithm(ssl, private_key, sigalg)) { - if (!ssl_public_key_rsa_pss_check(public_key.get(), sigalg)) { - return false; - } + ssl_cert_parse_leaf_pubkey(cert->cert_private_keys[i].chain.get()); + if (private_key != nullptr && public_key != nullptr) { + if (tls12_pkey_supports_cipher_auth(hs, private_key) && + pkey_supports_algorithm(ssl, private_key, sigalg)) { + if (!ssl_public_key_rsa_pss_check(public_key.get(), sigalg)) { + return false; + } - // Update certificate slot index if all checks have passed. - // - // If the server has a valid private key available to use, we switch to - // using that certificate for the rest of the connection. - cert->cert_private_key_idx = (int)i; - hs->local_pubkey = std::move(public_key); - return true; + // Update certificate slot index if all checks have passed. + // + // If the server has a valid private key available to use, we switch to + // using that certificate for the rest of the connection. + cert->cert_private_key_idx = (int)i; + hs->local_pubkey = std::move(public_key); + return true; + } } } return false; diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 12808f1dea..f85ecc0654 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -6484,7 +6484,7 @@ class MultipleCertificateSlotTest SSL_CTX *server_ctx, std::vector sigalgs, int last_cert_type_set, - int should_fail) { + int should_connect) { EXPECT_TRUE(SSL_CTX_set_signing_algorithm_prefs(client_ctx, sigalgs.data(), sigalgs.size())); EXPECT_TRUE(SSL_CTX_set_verify_algorithm_prefs(client_ctx, sigalgs.data(), @@ -6500,8 +6500,8 @@ class MultipleCertificateSlotTest EXPECT_EQ(ConnectClientAndServer(&client, &server, client_ctx, server_ctx, config, false), - should_fail); - if (!should_fail) { + should_connect); + if (!should_connect) { return; } @@ -6526,10 +6526,9 @@ INSTANTIATE_TEST_SUITE_P( // Sets up the |SSL_CTX| with |SSL_CTX_use_certificate| & |SSL_use_PrivateKey|. TEST_P(MultipleCertificateSlotTest, CertificateSlotIndex) { - if ((version == TLS1_1_VERSION || version == TLS1_VERSION) && - slot_index == SSL_PKEY_ED25519) { + if (version < TLS1_2_VERSION && slot_index == SSL_PKEY_ED25519) { // ED25519 is not supported in versions prior to TLS1.2. - return; + GTEST_SKIP(); } bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_method())); bssl::UniquePtr server_ctx(CreateContextWithCertificate( @@ -6545,10 +6544,9 @@ TEST_P(MultipleCertificateSlotTest, CertificateSlotIndex) { // Sets up the |SSL_CTX| with |SSL_CTX_set_chain_and_key|. TEST_P(MultipleCertificateSlotTest, SetChainAndKeyIndex) { - if ((version == TLS1_1_VERSION || version == TLS1_VERSION) && - slot_index == SSL_PKEY_ED25519) { + if (version < TLS1_2_VERSION && slot_index == SSL_PKEY_ED25519) { // ED25519 is not supported in versions prior to TLS1.2. - return; + GTEST_SKIP(); } bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_method())); bssl::UniquePtr server_ctx(SSL_CTX_new(TLS_method())); @@ -6573,11 +6571,10 @@ TEST_P(MultipleCertificateSlotTest, SetChainAndKeyIndex) { slot_index, true); } -TEST_P(MultipleCertificateSlotTest, AutomaticSelection) { - if ((version == TLS1_1_VERSION || version == TLS1_VERSION) && - slot_index == SSL_PKEY_ED25519) { +TEST_P(MultipleCertificateSlotTest, AutomaticSelectionSigAlgs) { + if (version < TLS1_2_VERSION && slot_index == SSL_PKEY_ED25519) { // ED25519 is not supported in versions prior to TLS1.2. - return; + GTEST_SKIP(); } bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_method())); @@ -6608,11 +6605,54 @@ TEST_P(MultipleCertificateSlotTest, AutomaticSelection) { {certificate_key_param().corresponding_sigalg}, SSL_PKEY_ED25519, true); } +TEST_P(MultipleCertificateSlotTest, AutomaticSelectionCipherAuth) { + if ((version < TLS1_2_VERSION && slot_index == SSL_PKEY_ED25519) || + version >= TLS1_3_VERSION) { + // ED25519 is not supported in versions prior to TLS1.2. + // TLS 1.3 not have cipher-based authentication configuration. + GTEST_SKIP(); + } + + bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_method())); + bssl::UniquePtr server_ctx(SSL_CTX_new(TLS_method())); + + ASSERT_TRUE( + SSL_CTX_use_certificate(server_ctx.get(), GetTestCertificate().get())); + ASSERT_TRUE(SSL_CTX_use_PrivateKey(server_ctx.get(), GetTestKey().get())); + ASSERT_TRUE(SSL_CTX_use_certificate(server_ctx.get(), + GetECDSATestCertificate().get())); + ASSERT_TRUE( + SSL_CTX_use_PrivateKey(server_ctx.get(), GetECDSATestKey().get())); + ASSERT_TRUE(SSL_CTX_use_certificate(server_ctx.get(), + GetED25519TestCertificate().get())); + ASSERT_TRUE( + SSL_CTX_use_PrivateKey(server_ctx.get(), GetED25519TestKey().get())); + + + // Versions prior to TLS1.3 need a valid authentication cipher suite to pair + // with the certificate. + if (version < TLS1_3_VERSION) { + ASSERT_TRUE(SSL_CTX_set_cipher_list(client_ctx.get(), + certificate_key_param().suite)); + } + + // We allow all possible sigalgs in this test, but either the ECDSA or ED25519 + // certificate could be chosen when using an |SSL_aECDSA| ciphersuite. + std::vector sigalgs = {SSL_SIGN_RSA_PSS_RSAE_SHA256}; + if (slot_index == SSL_PKEY_ED25519) { + sigalgs.push_back(SSL_SIGN_ED25519); + } else { + sigalgs.push_back(SSL_SIGN_ECDSA_SECP256R1_SHA256); + } + + StandardCertificateSlotIndexTests(client_ctx.get(), server_ctx.get(), sigalgs, + SSL_PKEY_ED25519, true); +} + TEST_P(MultipleCertificateSlotTest, MissingCertificate) { - if ((version == TLS1_1_VERSION || version == TLS1_VERSION) && - slot_index == SSL_PKEY_ED25519) { + if (version < TLS1_2_VERSION && slot_index == SSL_PKEY_ED25519) { // ED25519 is not supported in versions prior to TLS1.2. - return; + GTEST_SKIP(); } bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_method())); @@ -6637,10 +6677,9 @@ TEST_P(MultipleCertificateSlotTest, MissingCertificate) { } TEST_P(MultipleCertificateSlotTest, MissingPrivateKey) { - if ((version == TLS1_1_VERSION || version == TLS1_VERSION) && - slot_index == SSL_PKEY_ED25519) { + if (version < TLS1_2_VERSION && slot_index == SSL_PKEY_ED25519) { // ED25519 is not supported in versions prior to TLS1.2. - return; + GTEST_SKIP(); } bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_method())); diff --git a/ssl/ssl_x509.cc b/ssl/ssl_x509.cc index 112d0747ed..b6059d3554 100644 --- a/ssl/ssl_x509.cc +++ b/ssl/ssl_x509.cc @@ -195,7 +195,7 @@ static UniquePtr new_leafless_chain(void) { // ssl_cert_set_chain sets elements 1.. of |cert->chain| to the serialised // forms of elements of |chain|. It returns one on success or zero on error, in -// which case no change to |cert->chain| is made. It preverses the existing +// which case no change to |cert->chain| is made. It preserves the existing // leaf from |cert->chain|, if any. static bool ssl_cert_set_chain(CERT *cert, STACK_OF(X509) *chain) { if (!ssl_cert_check_cert_private_keys_usage(cert)) {