Fix leak in bssl-compat SSL_set_ocsp_response()

Signed-off-by: Ted Poole <tpoole@redhat.com>

Author: tedjpoole

bssl-compat/source/SSL_set_ocsp_response.cc

@@ -24,7 +24,14 @@
 typedef std::pair<void*,size_t> 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<OcspResponse*>(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<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));
     }

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<SSL_CTX> server_ctx(SSL_CTX_new(TLS_server_method()));
+  bssl::UniquePtr<SSL_CTX> 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<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());
+
+  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