Skip to content

Fixed potential leaks in SSL_set_ocsp_response() #448

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tedjpoole wants to merge 1 commit into
base: release/v1.35
Choose a base branch
from
Open

Fixed potential leaks in SSL_set_ocsp_response() #448

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 23 additions & 13 deletions bssl-compat/source/SSL_set_ocsp_response.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,12 @@
#include "SSL_CTX_set_select_certificate_cb.h"


typedef std::pair<void*,size_t> OcspResponse;
typedef std::pair<std::unique_ptr<void, decltype(&OPENSSL_free)>,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<OcspResponse*>(ptr)};
ossl.ossl_OPENSSL_free(resp->first);
if(OcspResponse *resp = reinterpret_cast<OcspResponse*>(ptr)) {
delete resp;
}
})};
Expand All @@ -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;
Expand All @@ -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<void, decltype(&OPENSSL_free)> 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)};
Expand All @@ -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<OcspResponse*>(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<OcspResponse> resp = std::make_unique<OcspResponse>(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;
}
}
}

Expand Down
63 changes: 63 additions & 0 deletions bssl-compat/source/test/test_ssl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1671,6 +1671,69 @@ 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<SSL_CTX> server_ctx(SSL_CTX_new(TLS_server_method()));
bssl::UniquePtr<SSL_CTX> 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<SSL> 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<SSL> 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


/**
* 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
Expand Down