Merge pull request #453 from tedjpoole/fix_ssl_set_ocsp_response_exception_safety_1_34

[bp/1.34] Fixed potential bssl-compat leaks

Author: tedjpoole

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,15 +101,19 @@ 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<uint8_t, decltype(&OPENSSL_free)> cleanup(
+    const_cast<uint8_t*>(client_hello.extensions), 
+    OPENSSL_free
+  );
+
   enum ssl_select_cert_result_t result;
 
   {
     ActiveSelectCertificateCb active(ssl);
     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;

bssl-compat/source/SSL_set_ocsp_response.cc

@@ -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;
       }
     })};
@@ -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<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)};
@@ -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;
+      }
     }
   }
 

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<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
+
+
+/**
+ * @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<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 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<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
+  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());
+
+  // 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