diff --git a/src/crypto/bio/bio_test.cc b/src/crypto/bio/bio_test.cc index d44c9dddf..075de0e95 100644 --- a/src/crypto/bio/bio_test.cc +++ b/src/crypto/bio/bio_test.cc @@ -50,6 +50,8 @@ using Socket = int; #define INVALID_SOCKET (-1) static int closesocket(int sock) { return close(sock); } static std::string LastSocketError() { return strerror(errno); } +static const int kOpenReadOnlyBinary = O_RDONLY; +static const int kOpenReadOnlyText = O_RDONLY; #else using Socket = SOCKET; static std::string LastSocketError() { @@ -57,6 +59,8 @@ static std::string LastSocketError() { snprintf(buf, sizeof(buf), "%d", WSAGetLastError()); return buf; } +static const int kOpenReadOnlyBinary = _O_RDONLY | _O_BINARY; +static const int kOpenReadOnlyText = O_RDONLY | _O_TEXT; #endif class OwnedSocket { @@ -673,21 +677,16 @@ TEST(BIOTest, Gets) { { SCOPED_TRACE("fd"); -#if defined(OPENSSL_WINDOWS) - int open_flags = _O_RDONLY | _O_BINARY; -#else - int open_flags = O_RDONLY; -#endif // Test |BIO_NOCLOSE|. - ScopedFD fd = file.OpenFD(open_flags); + ScopedFD fd = file.OpenFD(kOpenReadOnlyBinary); ASSERT_TRUE(fd.is_valid()); bssl::UniquePtr bio(BIO_new_fd(fd.get(), BIO_NOCLOSE)); ASSERT_TRUE(bio); check_bio_gets(bio.get()); // Test |BIO_CLOSE|. - fd = file.OpenFD(open_flags); + fd = file.OpenFD(kOpenReadOnlyBinary); ASSERT_TRUE(fd.is_valid()); bio.reset(BIO_new_fd(fd.get(), BIO_CLOSE)); ASSERT_TRUE(bio); @@ -706,22 +705,87 @@ TEST(BIOTest, Gets) { EXPECT_EQ(c, 'a'); } -// Test that, on Windows, |BIO_read_filename| opens files in binary mode. -TEST(BIOTest, BinaryMode) { +// Test that, on Windows, file BIOs correctly handle text vs binary mode. +TEST(BIOTest, FileMode) { if (SkipTempFileTests()) { GTEST_SKIP(); } - TemporaryFile file; - ASSERT_TRUE(file.Init("\r\n")); + TemporaryFile temp; + ASSERT_TRUE(temp.Init("hello\r\nworld")); - // Reading from the file should give back the exact bytes we put in. + auto expect_file_contents = [](BIO *bio, const std::string &str) { + // Read more than expected, to make sure we've reached the end of the file. + std::vector buf(str.size() + 100); + int len = BIO_read(bio, buf.data(), static_cast(buf.size())); + ASSERT_GT(len, 0); + EXPECT_EQ(Bytes(buf.data(), len), Bytes(str)); + }; + auto expect_binary_mode = [&](BIO *bio) { + expect_file_contents(bio, "hello\r\nworld"); + }; + auto expect_text_mode = [&](BIO *bio) { +#if defined(OPENSSL_WINDOWS) + expect_file_contents(bio, "hello\nworld"); +#else + expect_file_contents(bio, "hello\r\nworld"); +#endif + }; + + // |BIO_read_filename| should open in binary mode. bssl::UniquePtr bio(BIO_new(BIO_s_file())); ASSERT_TRUE(bio); - ASSERT_TRUE(BIO_read_filename(bio.get(), file.path().c_str())); - char buf[2]; - ASSERT_EQ(2, BIO_read(bio.get(), buf, 2)); - EXPECT_EQ(Bytes(buf, 2), Bytes("\r\n")); + ASSERT_TRUE(BIO_read_filename(bio.get(), temp.path().c_str())); + expect_binary_mode(bio.get()); + + // |BIO_new_file| should use the specified mode. + bio.reset(BIO_new_file(temp.path().c_str(), "rb")); + ASSERT_TRUE(bio); + expect_binary_mode(bio.get()); + + bio.reset(BIO_new_file(temp.path().c_str(), "r")); + ASSERT_TRUE(bio); + expect_text_mode(bio.get()); + + // |BIO_new_fp| inherits the file's existing mode by default. + ScopedFILE file = temp.Open("rb"); + ASSERT_TRUE(file); + bio.reset(BIO_new_fp(file.get(), BIO_NOCLOSE)); + ASSERT_TRUE(bio); + expect_binary_mode(bio.get()); + + file = temp.Open("r"); + ASSERT_TRUE(file); + bio.reset(BIO_new_fp(file.get(), BIO_NOCLOSE)); + ASSERT_TRUE(bio); + expect_text_mode(bio.get()); + + // However, |BIO_FP_TEXT| changes the file to be text mode, no matter how it + // was opened. + file = temp.Open("rb"); + ASSERT_TRUE(file); + bio.reset(BIO_new_fp(file.get(), BIO_NOCLOSE | BIO_FP_TEXT)); + ASSERT_TRUE(bio); + expect_text_mode(bio.get()); + + file = temp.Open("r"); + ASSERT_TRUE(file); + bio.reset(BIO_new_fp(file.get(), BIO_NOCLOSE | BIO_FP_TEXT)); + ASSERT_TRUE(bio); + expect_text_mode(bio.get()); + + // |BIO_new_fd| inherits the FD's existing mode. + ScopedFD fd = temp.OpenFD(kOpenReadOnlyBinary); + ASSERT_TRUE(fd.is_valid()); + bio.reset(BIO_new_fd(fd.get(), BIO_NOCLOSE)); + ASSERT_TRUE(bio); + expect_binary_mode(bio.get()); + + fd = temp.OpenFD(kOpenReadOnlyText); + ASSERT_TRUE(fd.is_valid()); + bio.reset(BIO_new_fd(fd.get(), BIO_NOCLOSE)); + ASSERT_TRUE(bio); + expect_text_mode(bio.get()); } // Run through the tests twice, swapping |bio1| and |bio2|, for symmetry. diff --git a/src/crypto/bio/file.c b/src/crypto/bio/file.c index 9b2a6ca0a..e68a898c3 100644 --- a/src/crypto/bio/file.c +++ b/src/crypto/bio/file.c @@ -73,6 +73,7 @@ #include +#include #include #include #include @@ -82,6 +83,10 @@ #include "../internal.h" +#if defined(OPENSSL_WINDOWS) +#include +#include +#endif #define BIO_FP_READ 0x02 #define BIO_FP_WRITE 0x04 @@ -122,14 +127,13 @@ BIO *BIO_new_file(const char *filename, const char *mode) { return ret; } -BIO *BIO_new_fp(FILE *stream, int close_flag) { +BIO *BIO_new_fp(FILE *stream, int flags) { BIO *ret = BIO_new(BIO_s_file()); - if (ret == NULL) { return NULL; } - BIO_set_fp(ret, stream, close_flag); + BIO_set_fp(ret, stream, flags); return ret; } @@ -196,6 +200,17 @@ static long file_ctrl(BIO *b, int cmd, long num, void *ptr) { break; case BIO_C_SET_FILE_PTR: file_free(b); + static_assert((BIO_CLOSE & BIO_FP_TEXT) == 0, + "BIO_CLOSE and BIO_FP_TEXT must not collide"); +#if defined(OPENSSL_WINDOWS) + // If |BIO_FP_TEXT| is not set, OpenSSL will switch the file to binary + // mode. BoringSSL intentionally diverges here because it means code + // tested under POSIX will inadvertently change the state of |FILE| + // objects when wrapping them in a |BIO|. + if (num & BIO_FP_TEXT) { + _setmode(_fileno(ptr), _O_TEXT); + } +#endif b->shutdown = (int)num & BIO_CLOSE; b->ptr = ptr; b->init = 1; @@ -287,8 +302,8 @@ int BIO_get_fp(BIO *bio, FILE **out_file) { return (int)BIO_ctrl(bio, BIO_C_GET_FILE_PTR, 0, (char *)out_file); } -int BIO_set_fp(BIO *bio, FILE *file, int close_flag) { - return (int)BIO_ctrl(bio, BIO_C_SET_FILE_PTR, close_flag, (char *)file); +int BIO_set_fp(BIO *bio, FILE *file, int flags) { + return (int)BIO_ctrl(bio, BIO_C_SET_FILE_PTR, flags, (char *)file); } int BIO_read_filename(BIO *bio, const char *filename) { diff --git a/src/include/openssl/bio.h b/src/include/openssl/bio.h index 93f3c0c1a..89cdc861c 100644 --- a/src/include/openssl/bio.h +++ b/src/include/openssl/bio.h @@ -473,22 +473,59 @@ OPENSSL_EXPORT int BIO_get_fd(BIO *bio, int *out_fd); OPENSSL_EXPORT const BIO_METHOD *BIO_s_file(void); // BIO_new_file creates a file BIO by opening |filename| with the given mode. -// See the |fopen| manual page for details of the mode argument. +// See the |fopen| manual page for details of the mode argument. On Windows, +// files may be opened in either binary or text mode so, as in |fopen|, callers +// must specify the desired option in |mode|. OPENSSL_EXPORT BIO *BIO_new_file(const char *filename, const char *mode); -// BIO_new_fp creates a new file BIO that wraps the given |FILE|. If -// |close_flag| is |BIO_CLOSE|, then |fclose| will be called on |stream| when -// the BIO is closed. -OPENSSL_EXPORT BIO *BIO_new_fp(FILE *stream, int close_flag); +// BIO_FP_TEXT indicates the |FILE| should be switched to text mode on Windows. +// It has no effect on non-Windows platforms. +#define BIO_FP_TEXT 0x10 + +// BIO_new_fp creates a new file BIO that wraps |file|. If |flags| contains +// |BIO_CLOSE|, then |fclose| will be called on |file| when the BIO is closed. +// +// On Windows, if |flags| contains |BIO_FP_TEXT|, this function will +// additionally switch |file| to text mode. This is not recommended, but may be +// required for OpenSSL compatibility. If |file| was not already in text mode, +// mode changes can cause unflushed data in |file| to be written in unexpected +// ways. See |_setmode| in Windows documentation for details. +// +// Unlike OpenSSL, if |flags| does not contain |BIO_FP_TEXT|, the translation +// mode of |file| is left as-is. In OpenSSL, |file| will be set to binary, with +// the same pitfalls as above. BoringSSL does not do this so that wrapping a +// |FILE| in a |BIO| will not inadvertently change its state. +// +// To avoid these pitfalls, callers should set the desired translation mode when +// opening the file. If targeting just BoringSSL, this is sufficient. If +// targeting both OpenSSL and BoringSSL, callers should set |BIO_FP_TEXT| to +// match the desired state of the file. +OPENSSL_EXPORT BIO *BIO_new_fp(FILE *file, int flags); // BIO_get_fp sets |*out_file| to the current |FILE| for |bio|. It returns one // on success and zero otherwise. OPENSSL_EXPORT int BIO_get_fp(BIO *bio, FILE **out_file); -// BIO_set_fp sets the |FILE| for |bio|. If |close_flag| is |BIO_CLOSE| then +// BIO_set_fp sets the |FILE| for |bio|. If |flags| contains |BIO_CLOSE| then // |fclose| will be called on |file| when |bio| is closed. It returns one on // success and zero otherwise. -OPENSSL_EXPORT int BIO_set_fp(BIO *bio, FILE *file, int close_flag); +// +// On Windows, if |flags| contains |BIO_FP_TEXT|, this function will +// additionally switch |file| to text mode. This is not recommended, but may be +// required for OpenSSL compatibility. If |file| was not already in text mode, +// mode changes can cause unflushed data in |file| to be written in unexpected +// ways. See |_setmode| in Windows documentation for details. +// +// Unlike OpenSSL, if |flags| does not contain |BIO_FP_TEXT|, the translation +// mode of |file| is left as-is. In OpenSSL, |file| will be set to binary, with +// the same pitfalls as above. BoringSSL does not do this so that wrapping a +// |FILE| in a |BIO| will not inadvertently change its state. +// +// To avoid these pitfalls, callers should set the desired translation mode when +// opening the file. If targeting just BoringSSL, this is sufficient. If +// targeting both OpenSSL and BoringSSL, callers should set |BIO_FP_TEXT| to +// match the desired state of the file. +OPENSSL_EXPORT int BIO_set_fp(BIO *bio, FILE *file, int flags); // BIO_read_filename opens |filename| for reading and sets the result as the // |FILE| for |bio|. It returns one on success and zero otherwise. The |FILE| diff --git a/src/include/openssl/ssl.h b/src/include/openssl/ssl.h index d10bb02b9..d73f9da9d 100644 --- a/src/include/openssl/ssl.h +++ b/src/include/openssl/ssl.h @@ -919,8 +919,9 @@ OPENSSL_EXPORT int SSL_CREDENTIAL_set1_private_key(SSL_CREDENTIAL *cred, OPENSSL_EXPORT int SSL_CREDENTIAL_set1_signing_algorithm_prefs( SSL_CREDENTIAL *cred, const uint16_t *prefs, size_t num_prefs); -// SSL_CREDENTIAL_set1_cert_chain sets |cred|'s certificate chain to |num_cert|s -// certificates from |certs|. It returns one on success and zero on error. +// SSL_CREDENTIAL_set1_cert_chain sets |cred|'s certificate chain, starting from +// the leaf, to |num_cert|s certificates from |certs|. It returns one on success +// and zero on error. OPENSSL_EXPORT int SSL_CREDENTIAL_set1_cert_chain(SSL_CREDENTIAL *cred, CRYPTO_BUFFER *const *certs, size_t num_certs); @@ -1002,11 +1003,13 @@ OPENSSL_EXPORT int SSL_CTX_use_certificate(SSL_CTX *ctx, X509 *x509); 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 -// success and zero on failure. +// success and zero on failure. If |ctx| had a private key or +// |SSL_PRIVATE_KEY_METHOD| previously configured, it is replaced. OPENSSL_EXPORT int SSL_CTX_use_PrivateKey(SSL_CTX *ctx, EVP_PKEY *pkey); // SSL_use_PrivateKey sets |ssl|'s private key to |pkey|. It returns one on -// success and zero on failure. +// success and zero on failure. If |ssl| had a private key or +// |SSL_PRIVATE_KEY_METHOD| previously configured, it is replaced. OPENSSL_EXPORT int SSL_use_PrivateKey(SSL *ssl, EVP_PKEY *pkey); // SSL_CTX_set0_chain sets |ctx|'s certificate chain, excluding the leaf, to diff --git a/src/ssl/internal.h b/src/ssl/internal.h index 0e557398a..0c2c2f86d 100644 --- a/src/ssl/internal.h +++ b/src/ssl/internal.h @@ -1640,6 +1640,10 @@ struct ssl_credential_st : public bssl::RefCounted { bool SetLeafCert(bssl::UniquePtr leaf, bool discard_key_on_mismatch); + // ClearIntermediateCerts clears intermediate certificates in the certificate + // chain, while preserving the leaf. + void ClearIntermediateCerts(); + // AppendIntermediateCert appends |cert| to the certificate chain. If there is // no leaf certificate configured, it leaves a placeholder null in |chain|. It // returns one on success and zero on error. diff --git a/src/ssl/ssl_cert.cc b/src/ssl/ssl_cert.cc index 39798ba7e..e30ec7395 100644 --- a/src/ssl/ssl_cert.cc +++ b/src/ssl/ssl_cert.cc @@ -191,6 +191,7 @@ static int cert_set_chain_and_key( return 0; } + cert->default_credential->ClearCertAndKey(); if (!SSL_CREDENTIAL_set1_cert_chain(cert->default_credential.get(), certs, num_certs)) { return 0; diff --git a/src/ssl/ssl_credential.cc b/src/ssl/ssl_credential.cc index f78709868..f4bb55eba 100644 --- a/src/ssl/ssl_credential.cc +++ b/src/ssl/ssl_credential.cc @@ -203,6 +203,16 @@ bool ssl_credential_st::SetLeafCert(UniquePtr leaf, return true; } +void ssl_credential_st::ClearIntermediateCerts() { + if (chain == nullptr) { + return; + } + + while (sk_CRYPTO_BUFFER_num(chain.get()) > 1) { + CRYPTO_BUFFER_free(sk_CRYPTO_BUFFER_pop(chain.get())); + } +} + bool ssl_credential_st::AppendIntermediateCert(UniquePtr cert) { if (!UsesX509()) { OPENSSL_PUT_ERROR(SSL, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); @@ -249,6 +259,7 @@ int SSL_CREDENTIAL_set1_private_key(SSL_CREDENTIAL *cred, EVP_PKEY *key) { } cred->privkey = UpRef(key); + cred->key_method = nullptr; return 1; } @@ -259,6 +270,7 @@ int SSL_CREDENTIAL_set_private_key_method( return 0; } + cred->privkey = nullptr; cred->key_method = key_method; return 1; } @@ -275,6 +287,7 @@ int SSL_CREDENTIAL_set1_cert_chain(SSL_CREDENTIAL *cred, return 0; } + cred->ClearIntermediateCerts(); for (size_t i = 1; i < num_certs; i++) { if (!cred->AppendIntermediateCert(UpRef(certs[i]))) { return 0; diff --git a/src/ssl/ssl_test.cc b/src/ssl/ssl_test.cc index 9247dc33a..503ad5f0d 100644 --- a/src/ssl/ssl_test.cc +++ b/src/ssl/ssl_test.cc @@ -1317,6 +1317,34 @@ static bssl::UniquePtr KeyFromPEM(const char *pem) { PEM_read_bio_PrivateKey(bio.get(), nullptr, nullptr, nullptr)); } +static bssl::UniquePtr BufferFromPEM(const char *pem) { + bssl::UniquePtr bio(BIO_new_mem_buf(pem, strlen(pem))); + char *name, *header; + uint8_t *data; + long data_len; + if (!PEM_read_bio(bio.get(), &name, &header, &data, + &data_len)) { + return nullptr; + } + OPENSSL_free(name); + OPENSSL_free(header); + + auto ret = bssl::UniquePtr( + CRYPTO_BUFFER_new(data, data_len, nullptr)); + OPENSSL_free(data); + return ret; +} + +static bssl::UniquePtr X509FromBuffer( + bssl::UniquePtr buffer) { + if (!buffer) { + return nullptr; + } + const uint8_t *derp = CRYPTO_BUFFER_data(buffer.get()); + return bssl::UniquePtr( + d2i_X509(NULL, &derp, CRYPTO_BUFFER_len(buffer.get()))); +} + static bssl::UniquePtr GetTestCertificate() { static const char kCertPEM[] = "-----BEGIN CERTIFICATE-----\n" @@ -1370,7 +1398,7 @@ static bssl::UniquePtr CreateContextWithTestCertificate( return ctx; } -static bssl::UniquePtr GetECDSATestCertificate() { +static bssl::UniquePtr GetECDSATestCertificateBuffer() { static const char kCertPEM[] = "-----BEGIN CERTIFICATE-----\n" "MIIBzzCCAXagAwIBAgIJANlMBNpJfb/rMAkGByqGSM49BAEwRTELMAkGA1UEBhMC\n" @@ -1384,9 +1412,14 @@ static bssl::UniquePtr GetECDSATestCertificate() { "BgcqhkjOPQQBA0gAMEUCIQDyoDVeUTo2w4J5m+4nUIWOcAZ0lVfSKXQA9L4Vh13E\n" "BwIgfB55FGohg/B6dGh5XxSZmmi08cueFV7mHzJSYV51yRQ=\n" "-----END CERTIFICATE-----\n"; - return CertFromPEM(kCertPEM); + return BufferFromPEM(kCertPEM); } +static bssl::UniquePtr GetECDSATestCertificate() { + return X509FromBuffer(GetECDSATestCertificateBuffer()); +} + + static bssl::UniquePtr GetECDSATestKey() { static const char kKeyPEM[] = "-----BEGIN PRIVATE KEY-----\n" @@ -1397,24 +1430,6 @@ static bssl::UniquePtr GetECDSATestKey() { return KeyFromPEM(kKeyPEM); } -static bssl::UniquePtr BufferFromPEM(const char *pem) { - bssl::UniquePtr bio(BIO_new_mem_buf(pem, strlen(pem))); - char *name, *header; - uint8_t *data; - long data_len; - if (!PEM_read_bio(bio.get(), &name, &header, &data, - &data_len)) { - return nullptr; - } - OPENSSL_free(name); - OPENSSL_free(header); - - auto ret = bssl::UniquePtr( - CRYPTO_BUFFER_new(data, data_len, nullptr)); - OPENSSL_free(data); - return ret; -} - static bssl::UniquePtr GetChainTestCertificateBuffer() { static const char kCertPEM[] = "-----BEGIN CERTIFICATE-----\n" @@ -1438,16 +1453,6 @@ static bssl::UniquePtr GetChainTestCertificateBuffer() { return BufferFromPEM(kCertPEM); } -static bssl::UniquePtr X509FromBuffer( - bssl::UniquePtr buffer) { - if (!buffer) { - return nullptr; - } - const uint8_t *derp = CRYPTO_BUFFER_data(buffer.get()); - return bssl::UniquePtr( - d2i_X509(NULL, &derp, CRYPTO_BUFFER_len(buffer.get()))); -} - static bssl::UniquePtr GetChainTestCertificate() { return X509FromBuffer(GetChainTestCertificateBuffer()); } @@ -4047,7 +4052,7 @@ TEST_P(SSLVersionTest, SSLClearFailsWithShedding) { ASSERT_FALSE(SSL_clear(server_.get())); } -static bool ChainsEqual(STACK_OF(X509) * chain, +static bool ChainsEqual(const STACK_OF(X509) *chain, const std::vector &expected) { if (sk_X509_num(chain) != expected.size()) { return false; @@ -4062,6 +4067,24 @@ static bool ChainsEqual(STACK_OF(X509) * chain, return true; } +static bool BuffersEqual(const STACK_OF(CRYPTO_BUFFER) *chain, + const std::vector &expected) { + if (sk_CRYPTO_BUFFER_num(chain) != expected.size()) { + return false; + } + + for (size_t i = 0; i < expected.size(); i++) { + const CRYPTO_BUFFER *buf = sk_CRYPTO_BUFFER_value(chain, i); + if (Bytes(CRYPTO_BUFFER_data(buf), CRYPTO_BUFFER_len(buf)) != + Bytes(CRYPTO_BUFFER_data(expected[i]), + CRYPTO_BUFFER_len(expected[i]))) { + return false; + } + } + + return true; +} + TEST_P(SSLVersionTest, AutoChain) { cert_ = GetChainTestCertificate(); ASSERT_TRUE(cert_); @@ -4630,6 +4653,133 @@ TEST(SSLTest, OverrideCertAndKey) { ASSERT_TRUE(SSL_CTX_use_PrivateKey(ctx.get(), key2.get())); } +TEST(SSLTest, OverrideKeyMethodWithKey) { + // Make an SSL_PRIVATE_KEY_METHOD that should never be called. + static const SSL_PRIVATE_KEY_METHOD kErrorMethod = { + [](SSL *ssl, uint8_t *out, size_t *out_len, size_t max_out, + uint16_t signature_algorithm, const uint8_t *in, + size_t in_len) { return ssl_private_key_failure; }, + [](SSL *ssl, uint8_t *out, size_t *out_len, size_t max_out, + const uint8_t *in, size_t in_len) { return ssl_private_key_failure; }, + [](SSL *ssl, uint8_t *out, size_t *out_len, size_t max_oun) { + return ssl_private_key_failure; + }, + }; + + bssl::UniquePtr key = GetTestKey(); + ASSERT_TRUE(key); + bssl::UniquePtr leaf = GetTestCertificate(); + ASSERT_TRUE(leaf); + + bssl::UniquePtr ctx(SSL_CTX_new(TLS_method())); + ASSERT_TRUE(ctx); + ASSERT_TRUE(SSL_CTX_use_certificate(ctx.get(), leaf.get())); + + // Configuring an |SSL_PRIVATE_KEY_METHOD| and then overwriting it with an + // |EVP_PKEY| should clear the |SSL_PRIVATE_KEY_METHOD|. + SSL_CTX_set_private_key_method(ctx.get(), &kErrorMethod); + ASSERT_TRUE(SSL_CTX_use_PrivateKey(ctx.get(), key.get())); + + bssl::UniquePtr client, server; + ASSERT_TRUE(ConnectClientAndServer(&client, &server, ctx.get(), ctx.get())); +} + +// Configuring a chain and then overwriting it with a different chain should +// clear the old one. +TEST(SSLTest, OverrideChain) { + bssl::UniquePtr key = GetChainTestKey(); + ASSERT_TRUE(key); + bssl::UniquePtr leaf = GetChainTestCertificate(); + ASSERT_TRUE(leaf); + bssl::UniquePtr ca = GetChainTestIntermediate(); + ASSERT_TRUE(ca); + + bssl::UniquePtr chain(sk_X509_new_null()); + ASSERT_TRUE(chain); + ASSERT_TRUE(bssl::PushToStack(chain.get(), bssl::UpRef(ca))); + + bssl::UniquePtr wrong_chain(sk_X509_new_null()); + ASSERT_TRUE(wrong_chain); + ASSERT_TRUE(bssl::PushToStack(wrong_chain.get(), bssl::UpRef(leaf))); + ASSERT_TRUE(bssl::PushToStack(wrong_chain.get(), bssl::UpRef(leaf))); + + bssl::UniquePtr ctx(SSL_CTX_new(TLS_method())); + ASSERT_TRUE(ctx); + ASSERT_TRUE(SSL_CTX_use_certificate(ctx.get(), leaf.get())); + ASSERT_TRUE(SSL_CTX_use_PrivateKey(ctx.get(), key.get())); + + // Configure one chain, then replace it with another. Note this API considers + // the chain to exclude the leaf. + ASSERT_TRUE(SSL_CTX_set1_chain(ctx.get(), wrong_chain.get())); + ASSERT_TRUE(SSL_CTX_set1_chain(ctx.get(), chain.get())); + + bssl::UniquePtr client, server; + ASSERT_TRUE(ConnectClientAndServer(&client, &server, ctx.get(), ctx.get())); + EXPECT_TRUE(ChainsEqual(SSL_get_peer_full_cert_chain(client.get()), + {leaf.get(), ca.get()})); +} + +TEST(SSLTest, OverrideChainAndKey) { + bssl::UniquePtr key1 = GetChainTestKey(); + ASSERT_TRUE(key1); + bssl::UniquePtr leaf1 = GetChainTestCertificateBuffer(); + ASSERT_TRUE(leaf1); + bssl::UniquePtr ca1 = GetChainTestIntermediateBuffer(); + ASSERT_TRUE(ca1); + bssl::UniquePtr key2 = GetECDSATestKey(); + ASSERT_TRUE(key2); + bssl::UniquePtr leaf2 = GetECDSATestCertificateBuffer(); + ASSERT_TRUE(leaf2); + + bssl::UniquePtr ctx(SSL_CTX_new(TLS_method())); + ASSERT_TRUE(ctx); + + // Configure one cert and key pair, then replace it with noather. + std::vector certs = {leaf1.get(), ca1.get()}; + ASSERT_TRUE(SSL_CTX_set_chain_and_key(ctx.get(), certs.data(), certs.size(), + key1.get(), nullptr)); + certs = {leaf2.get()}; + ASSERT_TRUE(SSL_CTX_set_chain_and_key(ctx.get(), certs.data(), certs.size(), + key2.get(), nullptr)); + + bssl::UniquePtr client, server; + ASSERT_TRUE(ConnectClientAndServer(&client, &server, ctx.get(), ctx.get())); + EXPECT_TRUE( + BuffersEqual(SSL_get0_peer_certificates(client.get()), {leaf2.get()})); +} + +TEST(SSLTest, OverrideCredentialChain) { + bssl::UniquePtr key = GetChainTestKey(); + ASSERT_TRUE(key); + bssl::UniquePtr leaf = GetChainTestCertificateBuffer(); + ASSERT_TRUE(leaf); + bssl::UniquePtr ca = GetChainTestIntermediateBuffer(); + ASSERT_TRUE(ca); + + std::vector chain = {leaf.get(), ca.get()}; + std::vector wrong_chain = {leaf.get(), leaf.get(), + leaf.get()}; + + bssl::UniquePtr ctx(SSL_CTX_new(TLS_method())); + ASSERT_TRUE(ctx); + bssl::UniquePtr cred(SSL_CREDENTIAL_new_x509()); + ASSERT_TRUE(cred); + + // Configure one chain (including the leaf), then replace it with another. + ASSERT_TRUE(SSL_CREDENTIAL_set1_cert_chain(cred.get(), wrong_chain.data(), + wrong_chain.size())); + ASSERT_TRUE( + SSL_CREDENTIAL_set1_cert_chain(cred.get(), chain.data(), chain.size())); + + ASSERT_TRUE(SSL_CREDENTIAL_set1_private_key(cred.get(), key.get())); + ASSERT_TRUE(SSL_CTX_add1_credential(ctx.get(), cred.get())); + + bssl::UniquePtr client, server; + ASSERT_TRUE(ConnectClientAndServer(&client, &server, ctx.get(), ctx.get())); + EXPECT_TRUE(BuffersEqual(SSL_get0_peer_certificates(client.get()), + {leaf.get(), ca.get()})); +} + TEST(SSLTest, SetChainAndKeyCtx) { bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_with_buffers_method())); ASSERT_TRUE(client_ctx); diff --git a/src/ssl/ssl_x509.cc b/src/ssl/ssl_x509.cc index d7f10834d..66c32102f 100644 --- a/src/ssl/ssl_x509.cc +++ b/src/ssl/ssl_x509.cc @@ -198,6 +198,7 @@ static void ssl_crypto_x509_cert_flush_cached_chain(CERT *cert) { // which case no change to |cert->chain| is made. It preverses the existing // leaf from |cert->chain|, if any. static bool ssl_cert_set1_chain(CERT *cert, STACK_OF(X509) *chain) { + cert->default_credential->ClearIntermediateCerts(); for (X509 *x509 : chain) { UniquePtr buffer = x509_to_buffer(x509); if (!buffer ||