diff --git a/bssl-compat/source/SSL_CTX_set_select_certificate_cb.cc b/bssl-compat/source/SSL_CTX_set_select_certificate_cb.cc index f3b7e1a87e..e577cd0224 100644 --- a/bssl-compat/source/SSL_CTX_set_select_certificate_cb.cc +++ b/bssl-compat/source/SSL_CTX_set_select_certificate_cb.cc @@ -15,9 +15,9 @@ typedef enum ssl_select_cert_result_t (*select_certificate_cb_t)(const SSL_CLIEN * in_select_certificate_cb(ssl) function to query whether or not we are * executing within a SSL_CTX_set_select_certificate_cb() callback for that SSL * object, or not. - * - * This mechanism is used by the SSL_get_servername() function to provide a - * different implementation depending on it's invocation context. + * + * This mechanism is used by SSL_get_servername() & SSL_set_ocsp_response() + * to provide different behavior depending on invocation context. */ class ActiveSelectCertificateCb { public: @@ -27,14 +27,14 @@ class ActiveSelectCertificateCb { ~ActiveSelectCertificateCb() { SSL_set_ex_data(ssl_, index(), nullptr); } + static bool isActive(const SSL *ssl) { + return SSL_get_ex_data(ssl, index()) != nullptr; + } + private: static int index() { - static int index = SSL_get_ex_new_index(0, nullptr, nullptr, nullptr, - +[](void *, void *ptr, CRYPTO_EX_DATA *, int, long, void*) { - if (ptr) ossl_OPENSSL_free(ptr); - }); + static int index = SSL_get_ex_new_index(0, nullptr, nullptr, nullptr, nullptr); return index; } - private: SSL *ssl_; }; @@ -43,7 +43,7 @@ class ActiveSelectCertificateCb { * callback invocation for the specified SSL object. */ bool in_select_certificate_cb(const SSL *ssl) { - return SSL_get_ex_data(ssl, ActiveSelectCertificateCb::index()) != nullptr; + return ActiveSelectCertificateCb::isActive(ssl); } @@ -101,6 +101,12 @@ static int ssl_ctx_client_hello_cb(SSL *ssl, int *alert, void *arg) { return ossl_SSL_CLIENT_HELLO_ERROR; } + // Ensure extensions are freed even if the callback throws + std::unique_ptr cleanup( + const_cast(client_hello.extensions), + OPENSSL_free + ); + enum ssl_select_cert_result_t result; { @@ -108,8 +114,6 @@ static int ssl_ctx_client_hello_cb(SSL *ssl, int *alert, void *arg) { result = callback(&client_hello); } - OPENSSL_free((void*)client_hello.extensions); - switch (result) { case ssl_select_cert_success: return ossl_SSL_CLIENT_HELLO_SUCCESS; case ssl_select_cert_retry: return ossl_SSL_CLIENT_HELLO_RETRY; diff --git a/bssl-compat/source/SSL_set_ocsp_response.cc b/bssl-compat/source/SSL_set_ocsp_response.cc index 299716e8a2..8f3df92116 100644 --- a/bssl-compat/source/SSL_set_ocsp_response.cc +++ b/bssl-compat/source/SSL_set_ocsp_response.cc @@ -21,14 +21,12 @@ #include "SSL_CTX_set_select_certificate_cb.h" -typedef std::pair OcspResponse; +typedef std::pair,size_t> OcspResponse; static int index() { static int index {ossl.ossl_SSL_get_ex_new_index(0, nullptr, nullptr, nullptr, +[](void *, void *ptr, CRYPTO_EX_DATA *, int, long, void*) { - if(ptr) { - OcspResponse *resp {reinterpret_cast(ptr)}; - ossl.ossl_OPENSSL_free(resp->first); + if(OcspResponse *resp = reinterpret_cast(ptr)) { delete resp; } })}; @@ -44,10 +42,11 @@ static int ssl_apply_deferred_ocsp_response_cb(SSL *ssl, void *arg) { if (resp) { ossl.ossl_SSL_set_ex_data(ssl, index(), nullptr); - if (ossl.ossl_SSL_set_tlsext_status_ocsp_resp(ssl, resp->first, resp->second) == 0) { - return ossl_SSL_TLSEXT_ERR_ALERT_FATAL; + if (ossl.ossl_SSL_set_tlsext_status_ocsp_resp(ssl, resp->first.get(), resp->second) == 1) { + resp->first.release(); // ossl_SSL_set_tlsext_status_ocsp_resp() took ownership + return ossl_SSL_TLSEXT_ERR_OK; } - return ossl_SSL_TLSEXT_ERR_OK; + return ossl_SSL_TLSEXT_ERR_ALERT_FATAL; } return ossl_SSL_TLSEXT_ERR_NOACK; @@ -60,7 +59,12 @@ static int ssl_apply_deferred_ocsp_response_cb(SSL *ssl, void *arg) { * ossl_SSL_CTX_set_tlsext_status_cb() later on. */ extern "C" int SSL_set_ocsp_response(SSL *ssl, const uint8_t *response, size_t response_len) { - if (void *response_copy {ossl.ossl_OPENSSL_memdup(response, response_len)}) { + std::unique_ptr response_copy( + ossl.ossl_OPENSSL_memdup(response, response_len), + OPENSSL_free + ); + + if (response_copy) { if (in_select_certificate_cb(ssl)) { SSL_CTX *ctx {ossl.ossl_SSL_get_SSL_CTX(ssl)}; @@ -84,15 +88,21 @@ extern "C" int SSL_set_ocsp_response(SSL *ssl, const uint8_t *response, size_t r // squirreled away already. If so, delete it first, so we don't just // overwrite it and create a leak. if(OcspResponse *prev = reinterpret_cast(ossl.ossl_SSL_get_ex_data(ssl, index()))) { - ossl.ossl_OPENSSL_free(prev->first); delete prev; } - // Store the OCSP response bytes for the callback to pick up later - return ossl.ossl_SSL_set_ex_data(ssl, index(), new OcspResponse(response_copy, response_len)); + // Store the OcspResponse bytes for the callback to pick up later + std::unique_ptr resp = std::make_unique(std::move(response_copy), response_len); + if (ossl.ossl_SSL_set_ex_data(ssl, index(), resp.get()) == 1) { + resp.release(); // ossl_SSL_set_ex_data() took ownership + return 1; + } } - else { - return ossl.ossl_SSL_set_tlsext_status_ocsp_resp(ssl, response_copy, response_len); + else { // We're not in a select certificate callback, so we set it directly + if (ossl.ossl_SSL_set_tlsext_status_ocsp_resp(ssl, response_copy.get(), response_len) == 1) { + response_copy.release(); // ossl_SSL_set_tlsext_status_ocsp_resp() took ownership + return 1; + } } } diff --git a/bssl-compat/source/test/test_ssl.cc b/bssl-compat/source/test/test_ssl.cc index 4c97a1bc01..11f8e39f96 100644 --- a/bssl-compat/source/test/test_ssl.cc +++ b/bssl-compat/source/test/test_ssl.cc @@ -1671,6 +1671,112 @@ TEST(SSLTest, test_SSL_set_ocsp_response_leak_inside_select_certificate_cb) { } +#ifdef BSSL_COMPAT +/** + * @brief This test exercises a leak that occurs in SSL_set_ocsp_response() if + * it returns early due to an error, when it is called from within a certificate + * selection callback. + * + * Without a fix for the leak, running this test under valgrind or similar + * memory checker tool will report the memory leak. + * + * Note that because this test uses knowledge of the internals of the + * SSL_set_ocsp_response() implementation, in bssl-compat, in order to provoke + * the leak, it will not work the same on BoringSSL proper. + */ +TEST(SSLTest, test_SSL_set_ocsp_response_early_return_leak_inside_select_certificate_cb) { + TempFile server_2_key_pem { server_2_key_pem_str }; + TempFile server_2_cert_chain_pem { server_2_cert_chain_pem_str }; + + static const uint8_t OCSP_RESPONSE[] { 1, 2, 3, 4, 5 }; + + int sockets[2]; + ASSERT_EQ(0, socketpair(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0, sockets)); + SocketCloser close[] { sockets[0], sockets[1] }; + + bssl::UniquePtr server_ctx(SSL_CTX_new(TLS_server_method())); + bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_client_method())); + + // Register a dummy tlsext status callback. This will provoke the + // SSL_set_ocsp_response() call, inside the certificate selection callback, to + // fail and return early. This in turn will cause the leak to occur if the fix + // is not in place. + SSL_CTX_set_tlsext_status_cb(server_ctx.get(), [](SSL *ssl, void *arg) -> int {return 0;}); + + // Set up server with a select certificate callback that calls + // SSL_set_ocsp_response() - which will return early and leak because of the + // dummy status callback we installed above. + SSL_CTX_set_select_certificate_cb(server_ctx.get(), [](const SSL_CLIENT_HELLO *client_hello) -> ssl_select_cert_result_t { + if (SSL_set_ocsp_response(client_hello->ssl, OCSP_RESPONSE, sizeof(OCSP_RESPONSE)) == 1) { + return ssl_select_cert_success; + } + return ssl_select_cert_error; + }); + ASSERT_TRUE(SSL_CTX_use_certificate_chain_file(server_ctx.get(), server_2_cert_chain_pem.path())); + ASSERT_TRUE(SSL_CTX_use_PrivateKey_file(server_ctx.get(), server_2_key_pem.path(), SSL_FILETYPE_PEM)); + bssl::UniquePtr server_ssl(SSL_new(server_ctx.get())); + ASSERT_TRUE(SSL_set_fd(server_ssl.get(), sockets[0])); + SSL_set_accept_state(server_ssl.get()); + + // Set up client with ocsp stapling enabled + SSL_CTX_set_verify(client_ctx.get(), SSL_VERIFY_NONE, nullptr); + bssl::UniquePtr client_ssl(SSL_new(client_ctx.get())); + ASSERT_TRUE(SSL_set_fd(client_ssl.get(), sockets[1])); + SSL_set_connect_state(client_ssl.get()); + SSL_enable_ocsp_stapling(client_ssl.get()); + + // We expect this to fail because the SSL_set_ocsp_response() call inside the + // certificate selection callback above will return early with an error, + // causing the certificate selection callback to fail, which in turn will + // cause the handshake to fail. + ASSERT_FALSE(CompleteHandshakes(client_ssl.get(), server_ssl.get())); +} +#endif // BSSL_COMPAT + + +/** + * @brief This test exercises a leak in SSL_CTX_set_select_certificate_cb() when + * the certificate selection callback throws an exception. + * + * Without a fix for the leak, running this test under valgrind or similar + * memory checker tool will report the memory leak. + */ +TEST(SSLTest, test_SSL_CTX_set_select_certificate_cb_leak_from_callback_exception) { + TempFile server_2_key_pem { server_2_key_pem_str }; + TempFile server_2_cert_chain_pem { server_2_cert_chain_pem_str }; + + int sockets[2]; + ASSERT_EQ(0, socketpair(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0, sockets)); + SocketCloser close[] { sockets[0], sockets[1] }; + + bssl::UniquePtr server_ctx(SSL_CTX_new(TLS_server_method())); + bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_client_method())); + + // Set up server with a select certificate callback that raises an exception + SSL_CTX_set_select_certificate_cb(server_ctx.get(), [](const SSL_CLIENT_HELLO *client_hello) -> ssl_select_cert_result_t { + throw std::runtime_error("Intentional exception to test for memory leaks"); + }); + + ASSERT_TRUE(SSL_CTX_use_certificate_chain_file(server_ctx.get(), server_2_cert_chain_pem.path())); + ASSERT_TRUE(SSL_CTX_use_PrivateKey_file(server_ctx.get(), server_2_key_pem.path(), SSL_FILETYPE_PEM)); + bssl::UniquePtr server_ssl(SSL_new(server_ctx.get())); + ASSERT_TRUE(SSL_set_fd(server_ssl.get(), sockets[0])); + SSL_set_accept_state(server_ssl.get()); + + // Set up client + SSL_CTX_set_verify(client_ctx.get(), SSL_VERIFY_NONE, nullptr); + bssl::UniquePtr client_ssl(SSL_new(client_ctx.get())); + ASSERT_TRUE(SSL_set_fd(client_ssl.get(), sockets[1])); + SSL_set_connect_state(client_ssl.get()); + + // Handshake will fail because of the exception in the callback + EXPECT_THROW( + CompleteHandshakes(client_ssl.get(), server_ssl.get()), + std::runtime_error + ); +} + + /** * Test that setting a TLS alert and returning ssl_verify_invalid, from a * callback installed via SSL_CTX_set_custom_verify(), results in a handshake