diff --git a/bssl-compat/source/SSL_get_servername.cc b/bssl-compat/source/SSL_get_servername.cc index a6fc3485f9..145ec03984 100644 --- a/bssl-compat/source/SSL_get_servername.cc +++ b/bssl-compat/source/SSL_get_servername.cc @@ -53,12 +53,18 @@ const char *SSL_get_servername(const SSL *ssl, const int type) { size_t len; // Extract the bytes from the client hello SNI extension, if present. - if (ossl_SSL_client_hello_get0_ext(const_cast(ssl), TLSEXT_TYPE_server_name, &p, &len)) { + if (ossl_SSL_client_hello_get0_ext(const_cast(ssl), type, &p, &len)) { if ((p = extract_ext_str_value(p, len, type)) != nullptr) { // The string pointed to by p is len bytes long but NOT null-terminated. // Therefore, we have to make a null-terminated copy of it for returning. char *copy {ossl_OPENSSL_strndup(reinterpret_cast(p), len)}; + // The free func (registered with SSL_get_ex_new_index() above) is only + // called when the SSL object is finally freed. Therefore, we need to + // explicitly free any ex data value that may in the slot from previous + // calls on the same SSL object. + ossl_OPENSSL_free(SSL_get_ex_data(ssl, index)); + // Squirel away the copy in the SSL object's ext data so it won't leak. if (SSL_set_ex_data(const_cast(ssl), index, copy) == 0) { ossl_OPENSSL_free(copy); diff --git a/bssl-compat/test/test_ssl.cc b/bssl-compat/test/test_ssl.cc index d14fda27bf..1612c90e30 100644 --- a/bssl-compat/test/test_ssl.cc +++ b/bssl-compat/test/test_ssl.cc @@ -1463,6 +1463,59 @@ TEST(SSLTest, test_SSL_get_servername_inside_select_certificate_cb) { } +/** + * @brief This test exercises a leak in SSL_get_servername() + * + * If SSL_get_servername() was invoked multiple times from the same certificate + * selection callback, it was leaking the string value that was returned from + * the previous invocation(s). + * + * Note that the string returned by the _last_ SSL_get_servername() invocation, + * inside a certificate selection callback, does _not_ leak i.e. if + * SSL_get_servername() is only called once during a callback, there is no leak. + * It only leaks when SSL_get_servername() is called more than once during the + * same callback. + */ +TEST(SSLTest, test_SSL_get_servername_leak_inside_select_certificate_cb) { + static const char SERVERNAME[] { "www.example.com" }; + + 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 a certificate selection callback which calls SSL_get_servername() 5 times. + // This will result in 4 leaks if the SSL_get_servername() 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_get_servername(client_hello->ssl, TLSEXT_NAMETYPE_host_name); + SSL_get_servername(client_hello->ssl, TLSEXT_NAMETYPE_host_name); + SSL_get_servername(client_hello->ssl, TLSEXT_NAMETYPE_host_name); + SSL_get_servername(client_hello->ssl, TLSEXT_NAMETYPE_host_name); + SSL_get_servername(client_hello->ssl, TLSEXT_NAMETYPE_host_name); + 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 + 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])); + ASSERT_TRUE(SSL_set_tlsext_host_name(client_ssl.get(), SERVERNAME)); + SSL_set_connect_state(client_ssl.get()); + + ASSERT_TRUE(CompleteHandshakes(client_ssl.get(), server_ssl.get())); +} + + TEST(SSLTest, test_SSL_get_servername_null_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 };