diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 97fe96e20..04f187362 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -848,15 +848,22 @@ OPENSSL_EXPORT void SSL_CTX_set0_buffer_pool(SSL_CTX *ctx, // the wire) but does not include the leaf. Both client and server certificates // use these functions. // +// Prefer to configure the certificate before the private key. If configured in +// the other order, inconsistent private keys will be silently dropped, rather +// than return an error. Additionally, overwriting a previously-configured +// certificate and key pair only works if the certificate is configured first. +// // Certificates and keys may be configured before the handshake or dynamically // in the early callback and certificate callback. // SSL_CTX_use_certificate sets |ctx|'s leaf certificate to |x509|. It returns -// one on success and zero on failure. +// one on success and zero on failure. If |ctx| has a private key which is +// inconsistent with |x509|, the private key is silently dropped. OPENSSL_EXPORT int SSL_CTX_use_certificate(SSL_CTX *ctx, X509 *x509); // SSL_use_certificate sets |ssl|'s leaf certificate to |x509|. It returns one -// on success and zero on failure. +// on success and zero on failure. If |ssl| has a private key which is +// inconsistent with |x509|, the private key is silently dropped. OPENSSL_EXPORT int SSL_use_certificate(SSL *ssl, X509 *x509); // SSL_CTX_use_PrivateKey sets |ctx|'s private key to |pkey|. It returns one on @@ -990,14 +997,6 @@ SSL_get0_peer_delegation_algorithms(const SSL *ssl, // chain of |ssl|. OPENSSL_EXPORT void SSL_certs_clear(SSL *ssl); -// SSL_CTX_check_private_key returns one if the certificate and private key -// configured in |ctx| are consistent and zero otherwise. -OPENSSL_EXPORT int SSL_CTX_check_private_key(const SSL_CTX *ctx); - -// SSL_check_private_key returns one if the certificate and private key -// configured in |ssl| are consistent and zero otherwise. -OPENSSL_EXPORT int SSL_check_private_key(const SSL *ssl); - // SSL_CTX_get0_certificate returns |ctx|'s leaf certificate. OPENSSL_EXPORT X509 *SSL_CTX_get0_certificate(const SSL_CTX *ctx); @@ -5317,6 +5316,25 @@ OPENSSL_EXPORT int SSL_set1_curves_list(SSL *ssl, const char *curves); // returns this value, but we define this constant for compatibility. #define TLSEXT_nid_unknown 0x1000000 +// SSL_CTX_check_private_key returns one if |ctx| has both a certificate and +// private key, and zero otherwise. +// +// This function does not check consistency because the library checks when the +// certificate and key are individually configured. However, if the private key +// is configured before the certificate, inconsistent private keys are silently +// dropped. Some callers are inadvertently relying on this function to detect +// when this happens. +// +// Instead, callers should configure the certificate first, then the private +// key, checking for errors in each. This function is then unnecessary. +OPENSSL_EXPORT int SSL_CTX_check_private_key(const SSL_CTX *ctx); + +// SSL_check_private_key returns one if |ssl| has both a certificate and private +// key, and zero otherwise. +// +// See discussion in |SSL_CTX_check_private_key|. +OPENSSL_EXPORT int SSL_check_private_key(const SSL *ssl); + // Compliance policy configurations // diff --git a/ssl/internal.h b/ssl/internal.h index 13b97daa4..9e7a05b1c 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -3202,7 +3202,6 @@ bool ssl_is_key_type_supported(int key_type); // message on the error queue. bool ssl_compare_public_and_private_key(const EVP_PKEY *pubkey, const EVP_PKEY *privkey); -bool ssl_cert_check_private_key(const CERT *cert, const EVP_PKEY *privkey); bool ssl_get_new_session(SSL_HANDSHAKE *hs); bool ssl_encrypt_ticket(SSL_HANDSHAKE *hs, CBB *out, const SSL_SESSION *session); diff --git a/ssl/ssl_cert.cc b/ssl/ssl_cert.cc index d635fb35b..dbc4818eb 100644 --- a/ssl/ssl_cert.cc +++ b/ssl/ssl_cert.cc @@ -493,30 +493,6 @@ bool ssl_compare_public_and_private_key(const EVP_PKEY *pubkey, return false; } -bool ssl_cert_check_private_key(const CERT *cert, const EVP_PKEY *privkey) { - if (privkey == nullptr) { - OPENSSL_PUT_ERROR(SSL, SSL_R_NO_PRIVATE_KEY_ASSIGNED); - return false; - } - - if (cert->chain == nullptr || - sk_CRYPTO_BUFFER_value(cert->chain.get(), 0) == nullptr) { - OPENSSL_PUT_ERROR(SSL, SSL_R_NO_CERTIFICATE_ASSIGNED); - return false; - } - - CBS cert_cbs; - CRYPTO_BUFFER_init_CBS(sk_CRYPTO_BUFFER_value(cert->chain.get(), 0), - &cert_cbs); - UniquePtr pubkey = ssl_cert_parse_pubkey(&cert_cbs); - if (!pubkey) { - OPENSSL_PUT_ERROR(X509, X509_R_UNKNOWN_KEY_TYPE); - return false; - } - - return ssl_compare_public_and_private_key(pubkey.get(), privkey); -} - bool ssl_cert_check_key_usage(const CBS *in, enum ssl_key_usage_t bit) { CBS buf = *in; diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 81a980713..f95cd5db2 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -1724,17 +1724,36 @@ int SSL_has_pending(const SSL *ssl) { return SSL_pending(ssl) != 0 || !ssl->s3->read_buffer.empty(); } +static bool has_cert_and_key(const CERT *cert) { + // TODO(davidben): If |cert->key_method| is set, that should be fine too. + if (cert->privatekey == nullptr) { + OPENSSL_PUT_ERROR(SSL, SSL_R_NO_PRIVATE_KEY_ASSIGNED); + return false; + } + + if (cert->chain == nullptr || + sk_CRYPTO_BUFFER_value(cert->chain.get(), 0) == nullptr) { + OPENSSL_PUT_ERROR(SSL, SSL_R_NO_CERTIFICATE_ASSIGNED); + return false; + } + + return true; +} + int SSL_CTX_check_private_key(const SSL_CTX *ctx) { - return ssl_cert_check_private_key(ctx->cert.get(), - ctx->cert->privatekey.get()); + // There is no need to actually check consistency because inconsistent values + // can never be configured. + return has_cert_and_key(ctx->cert.get()); } int SSL_check_private_key(const SSL *ssl) { if (!ssl->config) { return 0; } - return ssl_cert_check_private_key(ssl->config->cert.get(), - ssl->config->cert->privatekey.get()); + + // There is no need to actually check consistency because inconsistent values + // can never be configured. + return has_cert_and_key(ssl->config->cert.get()); } long SSL_get_default_timeout(const SSL *ssl) { diff --git a/ssl/ssl_privkey.cc b/ssl/ssl_privkey.cc index 57116cd6c..b3bb2c802 100644 --- a/ssl/ssl_privkey.cc +++ b/ssl/ssl_privkey.cc @@ -83,11 +83,21 @@ static bool ssl_set_pkey(CERT *cert, EVP_PKEY *pkey) { return false; } - if (cert->chain != nullptr && - sk_CRYPTO_BUFFER_value(cert->chain.get(), 0) != nullptr && - // Sanity-check that the private key and the certificate match. - !ssl_cert_check_private_key(cert, pkey)) { - return false; + // If the leaf certificate has been configured, check it matches. + const CRYPTO_BUFFER *leaf = cert->chain != nullptr + ? sk_CRYPTO_BUFFER_value(cert->chain.get(), 0) + : nullptr; + if (leaf != nullptr) { + CBS cert_cbs; + CRYPTO_BUFFER_init_CBS(leaf, &cert_cbs); + UniquePtr pubkey = ssl_cert_parse_pubkey(&cert_cbs); + if (!pubkey) { + OPENSSL_PUT_ERROR(X509, X509_R_UNKNOWN_KEY_TYPE); + return false; + } + if (!ssl_compare_public_and_private_key(pubkey.get(), pkey)) { + return false; + } } cert->privatekey = UpRef(pkey); diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 0b4ad3ccd..d12d49c78 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -4556,6 +4556,82 @@ TEST(SSLTest, SetChainAndKeyMismatch) { ERR_clear_error(); } +TEST(SSLTest, CertThenKeyMismatch) { + bssl::UniquePtr ctx(SSL_CTX_new(TLS_method())); + ASSERT_TRUE(ctx); + + bssl::UniquePtr key = GetTestKey(); + ASSERT_TRUE(key); + bssl::UniquePtr leaf = GetChainTestCertificate(); + ASSERT_TRUE(leaf); + + // There is no key or certificate, so |SSL_CTX_check_private_key| fails. + EXPECT_FALSE(SSL_CTX_check_private_key(ctx.get())); + + // With only a certificate, |SSL_CTX_check_private_key| still fails. + ASSERT_TRUE(SSL_CTX_use_certificate(ctx.get(), leaf.get())); + EXPECT_FALSE(SSL_CTX_check_private_key(ctx.get())); + + // The private key does not match the certificate, so it should fail. + EXPECT_FALSE(SSL_CTX_use_PrivateKey(ctx.get(), key.get())); + + // Checking the private key fails, but this is really because there is still + // no private key. + EXPECT_FALSE(SSL_CTX_check_private_key(ctx.get())); + EXPECT_EQ(nullptr, SSL_CTX_get0_privatekey(ctx.get())); +} + +TEST(SSLTest, KeyThenCertMismatch) { + bssl::UniquePtr ctx(SSL_CTX_new(TLS_method())); + ASSERT_TRUE(ctx); + + bssl::UniquePtr key = GetTestKey(); + ASSERT_TRUE(key); + bssl::UniquePtr leaf = GetChainTestCertificate(); + ASSERT_TRUE(leaf); + + // There is no key or certificate, so |SSL_CTX_check_private_key| fails. + EXPECT_FALSE(SSL_CTX_check_private_key(ctx.get())); + + // With only a key, |SSL_CTX_check_private_key| still fails. + ASSERT_TRUE(SSL_CTX_use_PrivateKey(ctx.get(), key.get())); + EXPECT_FALSE(SSL_CTX_check_private_key(ctx.get())); + + // If configuring a certificate that doesn't match the key, configuration + // actually succeeds. We just silently drop the private key. + ASSERT_TRUE(SSL_CTX_use_certificate(ctx.get(), leaf.get())); + EXPECT_EQ(nullptr, SSL_CTX_get0_privatekey(ctx.get())); + + // Some callers configure the private key, then the certificate, and then + // expect |SSL_CTX_check_private_key| to check consistency. It does, but only + // by way of noticing there is no private key. The actual consistency check + // happened in |SSL_CTX_use_certificate|. + EXPECT_FALSE(SSL_CTX_check_private_key(ctx.get())); +} + +TEST(SSLTest, OverrideCertAndKey) { + // It is possible to override an existing certificate by configuring + // certificate, then key, due to |SSL_CTX_use_certificate|'s above silent + // dropping behavior. + bssl::UniquePtr ctx(SSL_CTX_new(TLS_method())); + ASSERT_TRUE(ctx); + + bssl::UniquePtr key = GetTestKey(); + ASSERT_TRUE(key); + bssl::UniquePtr leaf = GetTestCertificate(); + ASSERT_TRUE(leaf); + bssl::UniquePtr key2 = GetChainTestKey(); + ASSERT_TRUE(key2); + bssl::UniquePtr leaf2 = GetChainTestCertificate(); + ASSERT_TRUE(leaf2); + + ASSERT_TRUE(SSL_CTX_use_certificate(ctx.get(), leaf.get())); + ASSERT_TRUE(SSL_CTX_use_PrivateKey(ctx.get(), key.get())); + + ASSERT_TRUE(SSL_CTX_use_certificate(ctx.get(), leaf2.get())); + ASSERT_TRUE(SSL_CTX_use_PrivateKey(ctx.get(), key2.get())); +} + TEST(SSLTest, SetChainAndKeyCtx) { bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_with_buffers_method())); ASSERT_TRUE(client_ctx);