From a792f8804773f9c6c8fa55a8d9a502d56bd79b2b Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 11 Mar 2024 15:59:01 -0400 Subject: [PATCH 1/2] Fix a number of cases overwriting certificates, keys, etc. with SSL_CREDENTIAL Field-by-field setters make the worst APIs. This fixes the following: - Calling SSL_CTX_set_chain_and_key twice should override the old one (Regression from SSL_CREDENTIAL.) - Various APIs forgot to clear the old chain before appending new ones. (Regression from SSL_CREDENTIAL.) - Switching between a custom private key and a concrete one should not leave the old one lying around. (I think this was always broken.) Add tests for all of these cases. Change-Id: Ief7b3aecf2ada3b123d79d4eddf464c65d5f7d0d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66907 Commit-Queue: David Benjamin Auto-Submit: David Benjamin Reviewed-by: Bob Beck Commit-Queue: Bob Beck --- include/openssl/ssl.h | 11 ++- ssl/internal.h | 4 + ssl/ssl_cert.cc | 1 + ssl/ssl_credential.cc | 13 +++ ssl/ssl_test.cc | 212 ++++++++++++++++++++++++++++++++++++------ ssl/ssl_x509.cc | 1 + 6 files changed, 207 insertions(+), 35 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index d10bb02b9..d73f9da9d 100644 --- a/include/openssl/ssl.h +++ b/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/ssl/internal.h b/ssl/internal.h index 0e557398a..0c2c2f86d 100644 --- a/ssl/internal.h +++ b/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/ssl/ssl_cert.cc b/ssl/ssl_cert.cc index 39798ba7e..e30ec7395 100644 --- a/ssl/ssl_cert.cc +++ b/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/ssl/ssl_credential.cc b/ssl/ssl_credential.cc index f78709868..f4bb55eba 100644 --- a/ssl/ssl_credential.cc +++ b/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/ssl/ssl_test.cc b/ssl/ssl_test.cc index 9247dc33a..503ad5f0d 100644 --- a/ssl/ssl_test.cc +++ b/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/ssl/ssl_x509.cc b/ssl/ssl_x509.cc index d7f10834d..66c32102f 100644 --- a/ssl/ssl_x509.cc +++ b/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 || From 5ee4e9512e9a99f97c4a3fad397034028b3457c2 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 6 Mar 2024 16:48:35 -0500 Subject: [PATCH 2/2] Add BIO_FP_TEXT This CL allows us to reduce the patch set on CPython. BIO_new_fp is the FILE* analog of BIO_new_fd. However, it behaves very strangely w.r.t. Windows file translation modes. Instead of simply inheriting the FILE* as the caller constructed it, it unconditionally overrides the file's translation mode! This is surprising. Moreover, if you change the mode without flushing the file, weird things happen, as Windows documentation discusses: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/setmode?view=msvc-170 This leaks all the way up to calling code, because callers need to pass a matching BIO_FP_TEXT to the FILE* they made. To be source-compatible with such callers, notably CPython, we need to at least provide BIO_FP_TEXT. I first tried to fully match OpenSSL's semantics, but OpenSSL's semantics are quite dangerous. Code tested on POSIX, calling BIO_new_fp(some_file, BIO_NOCLOSE), without much thought, is subtly broken on Windows. It will change the mode of any file passed into it to binary! Our own code runs into this. BIO_new_file internally calls BIO_new_fp. In OpenSSL, they need to re-parse the mode string and figure out the right flag. ASN1_STRING_print_ex_fp doesn't even know which is the right one. In OpenSSL, they actually call fwrite manually. We wrap it in a BIO and then use the BIO version, because it makes no sense to not use the abstraction we already have lying around. But that is incompatible with OpenSSL's semantics. So instead I've opted to make BIO_FP_TEXT switch the mode, but no flag just leaves the mode alone. This is slightly OpenSSL-incompatible because this code will work in OpenSSL, but continue to not work in BoringSSL: // Oops, I actually wanted binary but forgot to use "rb" FILE *f = fopen("blah", "r"); // But bio fixed it for me! BIO *bio = BIO_new_fp(f, BIO_NOCLOSE); But callers should have passed "rb" if they wanted binary. This is also preexisting and no one has noticed. I think it's far more likely that applications *aren't* expecting BIO_new_fp to secretly change the input FILE's mode. If we ever need to, we can adopt OpenSSL's semantics and then add BIO_FP_LEAVE_MY_FILE_ALONE. But those are worse defaults. Change-Id: I2905673c523eb24312c15d3000cbe34a66602700 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66809 Reviewed-by: Bob Beck Commit-Queue: David Benjamin Auto-Submit: David Benjamin --- crypto/bio/bio_test.cc | 96 +++++++++++++++++++++++++++++++++++------- crypto/bio/file.c | 25 ++++++++--- include/openssl/bio.h | 51 +++++++++++++++++++--- 3 files changed, 144 insertions(+), 28 deletions(-) diff --git a/crypto/bio/bio_test.cc b/crypto/bio/bio_test.cc index d44c9dddf..075de0e95 100644 --- a/crypto/bio/bio_test.cc +++ b/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/crypto/bio/file.c b/crypto/bio/file.c index 9b2a6ca0a..e68a898c3 100644 --- a/crypto/bio/file.c +++ b/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/include/openssl/bio.h b/include/openssl/bio.h index 93f3c0c1a..89cdc861c 100644 --- a/include/openssl/bio.h +++ b/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|