diff --git a/bssl-compat/source/SSL_set_ocsp_response.cc b/bssl-compat/source/SSL_set_ocsp_response.cc index 2383c8e159..299716e8a2 100644 --- a/bssl-compat/source/SSL_set_ocsp_response.cc +++ b/bssl-compat/source/SSL_set_ocsp_response.cc @@ -24,7 +24,14 @@ typedef std::pair OcspResponse; static int index() { - static int index {ossl.ossl_SSL_get_ex_new_index(0, nullptr, nullptr, nullptr, nullptr)}; + 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); + delete resp; + } + })}; return index; } @@ -72,6 +79,15 @@ extern "C" int SSL_set_ocsp_response(SSL *ssl, const uint8_t *response, size_t r return 0; } + // If we have been called previously, from within the same select + // certificate callback invocation, there will be an OcspReponse object + // 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)); } diff --git a/bssl-compat/source/test/test_ssl.cc b/bssl-compat/source/test/test_ssl.cc index abbba7114c..48552dc7a9 100644 --- a/bssl-compat/source/test/test_ssl.cc +++ b/bssl-compat/source/test/test_ssl.cc @@ -1612,6 +1612,66 @@ TEST(SSLTest, test_SSL_set_ocsp_response_inside_select_certificate_cb) { } +/** + * @brief This test exercises a leak in SSL_set_ocsp_response() + * + * This test exercises a leak in SSL_set_ocsp_response() that occurs when it is + * invoked multiple times from within the same certificate selection callback + * i.e. without the fix, running this test under valgrind or similar memory + * checker tool will report the memory leak. + * + * Note that the leak does _not_ occur if SSL_set_ocsp_response() is only called + * _once_ from within the same certificate selection callback. It is only the + * additional calls that leak. + */ +TEST(SSLTest, test_SSL_set_ocsp_response_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())); + + // Set up server with a select certificate callback that calls + // SSL_set_ocsp_response() 5 times. This will result in 4 leaks if the + // SSL_set_ocsp_response() fix is not in place. + SSL_CTX_set_select_certificate_cb(server_ctx.get(), [](const SSL_CLIENT_HELLO *client_hello) -> ssl_select_cert_result_t { + SSL_set_ocsp_response(client_hello->ssl, OCSP_RESPONSE, sizeof(OCSP_RESPONSE)); + SSL_set_ocsp_response(client_hello->ssl, OCSP_RESPONSE, sizeof(OCSP_RESPONSE)); + SSL_set_ocsp_response(client_hello->ssl, OCSP_RESPONSE, sizeof(OCSP_RESPONSE)); + SSL_set_ocsp_response(client_hello->ssl, OCSP_RESPONSE, sizeof(OCSP_RESPONSE)); + SSL_set_ocsp_response(client_hello->ssl, OCSP_RESPONSE, sizeof(OCSP_RESPONSE)); + return ssl_select_cert_success; + }); + 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()); + + ASSERT_TRUE(CompleteHandshakes(client_ssl.get(), server_ssl.get())); + + // Check that the client received the OCSP response ok + const uint8_t *ocsp_resp_data{}; + size_t ocsp_resp_len{}; + SSL_get0_ocsp_response(client_ssl.get(), &ocsp_resp_data, &ocsp_resp_len); + ASSERT_EQ(sizeof(OCSP_RESPONSE), ocsp_resp_len); + ASSERT_EQ(0, memcmp(OCSP_RESPONSE, ocsp_resp_data, ocsp_resp_len)); +} + + /** * 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