commit 1685bd140f6eeb6939c73756be70c888dde32c5e
author David Benjamin <davidben@google.com> Tue Nov 21 12:01:38 2023 -0500
committer Boringssl LUCI CQ <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Mon Nov 27 16:21:21 2023 +0000
tree 5adccad72fd24e40e4cc5d11e6e10d52d127c4e0
parent b3d1666b989c39c6e2f78d9c37de79b308c57a92

Fix leak if X509_STORE_CTX_init is called on a previously initialized context

This wasn't possible when X509_STORE_CTX was stack-allocated because
X509_STORE_CTX_init needed to account for an uninitialized struct. But
now it is always initialized, so we can avoid this footgun. This also
matches what OpenSSL does nowadays.

Change-Id: I266be58204b8cd374fa4896c1c66a35ffaa762ea
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64141
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

crypto/x509/x509_test.cc

diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index fda412f..d6500f2 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -3409,6 +3409,18 @@
   EXPECT_FALSE(X509_STORE_CTX_init(ctx.get(), nullptr, leaf.get(), nullptr));
 }
 
+TEST(X509Test, StoreCtxReuse) {
+  bssl::UniquePtr<X509> leaf(CertFromPEM(kLeafPEM));
+  ASSERT_TRUE(leaf);
+  bssl::UniquePtr<X509_STORE> store(X509_STORE_new());
+  ASSERT_TRUE(store);
+  bssl::UniquePtr<X509_STORE_CTX> ctx(X509_STORE_CTX_new());
+  ASSERT_TRUE(ctx);
+  ASSERT_TRUE(X509_STORE_CTX_init(ctx.get(), store.get(), leaf.get(), nullptr));
+  // Re-initializing |ctx| should not leak memory.
+  ASSERT_TRUE(X509_STORE_CTX_init(ctx.get(), store.get(), leaf.get(), nullptr));
+}
+
 TEST(X509Test, BasicConstraints) {
   const uint32_t kFlagMask = EXFLAG_CA | EXFLAG_BCONS | EXFLAG_INVALID;
 

crypto/x509/x509_vfy.c

diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index c78faf4..5eca5c7 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -1648,13 +1648,8 @@
 
 int X509_STORE_CTX_init(X509_STORE_CTX *ctx, X509_STORE *store, X509 *x509,
                         STACK_OF(X509) *chain) {
-  // TODO(davidben): This is a remnant of when |X509_STORE_CTX| was a
-  // stack-allocatable function. Now that it is heap-allocated, we don't need to
-  // worry about uninitialized memory in |ctx|. Move the memset to
-  // |X509_STORE_CTX_cleanup| and call |X509_STORE_CTX_cleanup| here so callers
-  // don't leak memory when re-initializing a previously initialized
-  // |X509_STORE_CTX|.
-  OPENSSL_memset(ctx, 0, sizeof(X509_STORE_CTX));
+  X509_STORE_CTX_cleanup(ctx);
+
   ctx->ctx = store;
   ctx->cert = x509;
   ctx->untrusted = chain;
@@ -1781,7 +1776,7 @@
   sk_X509_pop_free(ctx->chain, X509_free);
   ctx->chain = NULL;
   CRYPTO_free_ex_data(&g_ex_data_class, ctx, &(ctx->ex_data));
-  OPENSSL_memset(&ctx->ex_data, 0, sizeof(CRYPTO_EX_DATA));
+  OPENSSL_memset(ctx, 0, sizeof(X509_STORE_CTX));
 }
 
 void X509_STORE_CTX_set_depth(X509_STORE_CTX *ctx, int depth) {