Upstream merge 2024-03-21

crypto/CMakeLists.txt

@@ -484,9 +484,7 @@ add_library(
   x509/x_attrib.c
   x509/x_crl.c
   x509/x_exten.c
-  x509/x_info.c
   x509/x_name.c
-  x509/x_pkey.c
   x509/x_pubkey.c
   x509/x_req.c
   x509/x_sig.c

crypto/pem/pem_info.c

@@ -69,6 +69,37 @@
 #include <openssl/rsa.h>
 #include <openssl/x509.h>
 
+
+static X509_PKEY *X509_PKEY_new(void) {
+  return OPENSSL_zalloc(sizeof(X509_PKEY));
+}
+
+static void X509_PKEY_free(X509_PKEY *x) {
+  if (x == NULL) {
+    return;
+  }
+
+  EVP_PKEY_free(x->dec_pkey);
+  OPENSSL_free(x);
+}
+
+static X509_INFO *X509_INFO_new(void) {
+  return OPENSSL_zalloc(sizeof(X509_INFO));
+}
+
+void X509_INFO_free(X509_INFO *x) {
+  if (x == NULL) {
+    return;
+  }
+
+  X509_free(x->x509);
+  X509_CRL_free(x->crl);
+  X509_PKEY_free(x->x_pkey);
+  OPENSSL_free(x->enc_data);
+  OPENSSL_free(x);
+}
+
+
 STACK_OF(X509_INFO) *PEM_X509_INFO_read(FILE *fp, STACK_OF(X509_INFO) *sk,
                                         pem_password_cb *cb, void *u) {
   BIO *b = BIO_new_fp(fp, BIO_NOCLOSE);

crypto/x509/x509_lu.c

@@ -65,10 +65,22 @@
 #include "../internal.h"
 #include "internal.h"
 
-X509_LOOKUP *X509_LOOKUP_new(X509_LOOKUP_METHOD *method) {
-  X509_LOOKUP *ret;
 
-  ret = (X509_LOOKUP *)OPENSSL_zalloc(sizeof(X509_LOOKUP));
+static int X509_OBJECT_idx_by_subject(STACK_OF(X509_OBJECT) *h, int type,
+                                      X509_NAME *name);
+static X509_OBJECT *X509_OBJECT_retrieve_by_subject(
+      STACK_OF(X509_OBJECT) *h, int type, X509_NAME *name);
+static X509_OBJECT *X509_OBJECT_retrieve_match(STACK_OF(X509_OBJECT) *h,
+                                               X509_OBJECT *x);
+static int X509_OBJECT_up_ref_count(X509_OBJECT *a);
+
+static X509_LOOKUP *X509_LOOKUP_new(X509_LOOKUP_METHOD *method);
+static int X509_LOOKUP_by_subject(X509_LOOKUP *ctx, int type, X509_NAME *name,
+                                  X509_OBJECT *ret);
+static int X509_LOOKUP_shutdown(X509_LOOKUP *ctx);
+
+static X509_LOOKUP *X509_LOOKUP_new(X509_LOOKUP_METHOD *method) {
+  X509_LOOKUP *ret = OPENSSL_zalloc(sizeof(X509_LOOKUP));
   if (ret == NULL) {
     return NULL;
   }
@@ -91,18 +103,7 @@ void X509_LOOKUP_free(X509_LOOKUP *ctx) {
   OPENSSL_free(ctx);
 }
 
-int X509_LOOKUP_init(X509_LOOKUP *ctx) {
-  if (ctx->method == NULL) {
-    return 0;
-  }
-  if (ctx->method->init != NULL) {
-    return ctx->method->init(ctx);
-  } else {
-    return 1;
-  }
-}
-
-int X509_LOOKUP_shutdown(X509_LOOKUP *ctx) {
+static int X509_LOOKUP_shutdown(X509_LOOKUP *ctx) {
   if (ctx->method == NULL) {
     return 0;
   }
@@ -125,14 +126,18 @@ int X509_LOOKUP_ctrl(X509_LOOKUP *ctx, int cmd, const char *argc, long argl,
   }
 }
 
-int X509_LOOKUP_by_subject(X509_LOOKUP *ctx, int type, X509_NAME *name,
-                           X509_OBJECT *ret) {
+static int X509_LOOKUP_by_subject(X509_LOOKUP *ctx, int type, X509_NAME *name,
+                                  X509_OBJECT *ret) {
   if ((ctx->method == NULL) || (ctx->method->get_by_subject == NULL)) {
     return 0;
   }
   if (ctx->skip) {
     return 0;
   }
+  // Note |get_by_subject| leaves |ret| in an inconsistent state. It has
+  // pointers to an |X509| or |X509_CRL|, but has not bumped the refcount yet.
+  // For now, the caller is expected to fix this, but ideally we'd fix the
+  // |X509_LOOKUP| convention itself.
   return ctx->method->get_by_subject(ctx, type, name, ret) > 0;
 }
 
@@ -217,21 +222,6 @@ int X509_STORE_up_ref(X509_STORE *store) {
   return 1;
 }
 
-static void cleanup(X509_OBJECT *a) {
-  if (a == NULL) {
-    return;
-  }
-  if (a->type == X509_LU_X509) {
-    X509_free(a->data.x509);
-  } else if (a->type == X509_LU_CRL) {
-    X509_CRL_free(a->data.crl);
-  } else {
-    // abort();
-  }
-
-  OPENSSL_free(a);
-}
-
 void X509_STORE_free(X509_STORE *vfy) {
   size_t j;
   STACK_OF(X509_LOOKUP) *sk;
@@ -254,7 +244,7 @@ void X509_STORE_free(X509_STORE *vfy) {
     X509_LOOKUP_free(lu);
   }
   sk_X509_LOOKUP_free(sk);
-  sk_X509_OBJECT_pop_free(vfy->objs, cleanup);
+  sk_X509_OBJECT_pop_free(vfy->objs, X509_OBJECT_free);
 
   if (vfy->param) {
     X509_VERIFY_PARAM_free(vfy->param);
@@ -328,7 +318,7 @@ static int x509_store_add(X509_STORE *ctx, void *x, int is_crl) {
     return 0;
   }
 
-  X509_OBJECT *const obj = (X509_OBJECT *)OPENSSL_malloc(sizeof(X509_OBJECT));
+  X509_OBJECT *const obj = X509_OBJECT_new();
   if (obj == NULL) {
     return 0;
   }
@@ -354,8 +344,7 @@ static int x509_store_add(X509_STORE *ctx, void *x, int is_crl) {
   CRYPTO_MUTEX_unlock_write(&ctx->objs_lock);
 
   if (!added) {
-    X509_OBJECT_free_contents(obj);
-    OPENSSL_free(obj);
+    X509_OBJECT_free(obj);
   }
 
   return ret;
@@ -370,14 +359,18 @@ int X509_STORE_add_crl(X509_STORE *ctx, X509_CRL *x) {
 }
 
 X509_OBJECT *X509_OBJECT_new(void) {
-  X509_OBJECT *ret = OPENSSL_zalloc(sizeof(X509_OBJECT));
-  if (ret == NULL) {
-    return NULL;
+  return OPENSSL_zalloc(sizeof(X509_OBJECT));
+}
+
+void X509_OBJECT_free(X509_OBJECT *obj) {
+  if (obj == NULL) {
+    return;
   }
-  return ret;
+  X509_OBJECT_free_contents(obj);
+  OPENSSL_free(obj);
 }
 
-int X509_OBJECT_up_ref_count(X509_OBJECT *a) {
+static int X509_OBJECT_up_ref_count(X509_OBJECT *a) {
   switch (a->type) {
     case X509_LU_X509:
       X509_up_ref(a->data.x509);
@@ -398,11 +391,8 @@ void X509_OBJECT_free_contents(X509_OBJECT *a) {
       X509_CRL_free(a->data.crl);
       break;
   }
-}
 
-void X509_OBJECT_free(X509_OBJECT *a) {
-  X509_OBJECT_free_contents(a);
-  OPENSSL_free(a);
+  OPENSSL_memset(a, 0, sizeof(X509_OBJECT));
 }
 
 int X509_OBJECT_get_type(const X509_OBJECT *a) { return a->type; }
@@ -488,13 +478,13 @@ static int x509_object_idx_cnt(STACK_OF(X509_OBJECT) *h, int type,
   return (int)idx;
 }
 
-int X509_OBJECT_idx_by_subject(STACK_OF(X509_OBJECT) *h, int type,
-                               X509_NAME *name) {
+static int X509_OBJECT_idx_by_subject(STACK_OF(X509_OBJECT) *h, int type,
+                                      X509_NAME *name) {
   return x509_object_idx_cnt(h, type, name, NULL);
 }
 
-X509_OBJECT *X509_OBJECT_retrieve_by_subject(STACK_OF(X509_OBJECT) *h, int type,
-                                             X509_NAME *name) {
+X509_OBJECT *X509_OBJECT_retrieve_by_subject(STACK_OF(X509_OBJECT) *h,
+                                                    int type, X509_NAME *name) {
   int idx;
   idx = X509_OBJECT_idx_by_subject(h, type, name);
   if (idx == -1) {
@@ -589,8 +579,8 @@ STACK_OF(X509_CRL) *X509_STORE_get1_crls(X509_STORE_CTX *ctx, X509_NAME *nm) {
   return sk;
 }
 
-X509_OBJECT *X509_OBJECT_retrieve_match(STACK_OF(X509_OBJECT) *h,
-                                        X509_OBJECT *x) {
+static X509_OBJECT *X509_OBJECT_retrieve_match(STACK_OF(X509_OBJECT) *h,
+                                               X509_OBJECT *x) {
   sk_X509_OBJECT_sort(h);
   size_t idx;
   if (!sk_X509_OBJECT_find_awslc(h, &idx, x)) {

crypto/x509/x509_test.cc

@@ -1851,7 +1851,7 @@ TEST(X509Test, MatchFoundSetsPeername) {
   char *peername = nullptr;
   EXPECT_NE(1, X509_check_host(leaf.get(), kWrongHostname, strlen(kWrongHostname), 0, &peername));
   ASSERT_EQ(nullptr, peername);
-  
+
   EXPECT_EQ(1, X509_check_host(leaf.get(), kHostname, strlen(kHostname), 0, &peername));
   EXPECT_STREQ(peername, kHostname);
   OPENSSL_free(peername);
@@ -3888,6 +3888,18 @@ TEST(X509Test, NullStore) {
   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;
 
@@ -5074,15 +5086,8 @@ TEST(X509Test, AddDuplicates) {
       ASSERT_TRUE(X509_STORE_lock(store.get()));
       // Sleep after taking the lock to cause contention. Sleep longer than the
       // adder half of threads to ensure we hold the lock while they contend
-      // for it. |X509_OBJECT_retrieve_by_subject| is called because it doesn't
-      // take a lock on the store, thus avoiding deadlock.
+      // for it.
       std::this_thread::sleep_for(std::chrono::microseconds(11 + (sleep_buf[0] % 5)));
-      EXPECT_TRUE(X509_OBJECT_retrieve_by_subject(
-        store->objs, X509_LU_X509, X509_get_subject_name(a.get())
-      ));
-      EXPECT_TRUE(X509_OBJECT_retrieve_by_subject(
-        store->objs, X509_LU_X509, X509_get_subject_name(b.get())
-      ));
       ASSERT_TRUE(X509_STORE_unlock(store.get()));
     });
   }

crypto/x509/x509_vfy.c

@@ -1659,18 +1659,7 @@ int X509_STORE_CTX_purpose_inherit(X509_STORE_CTX *ctx, int def_purpose,
 }
 
 X509_STORE_CTX *X509_STORE_CTX_new(void) {
-  X509_STORE_CTX *ctx;
-  ctx = (X509_STORE_CTX *)OPENSSL_zalloc(sizeof(X509_STORE_CTX));
-  if (!ctx) {
-    return NULL;
-  }
-  // NO-OP: struct already zeroed
-  //X509_STORE_CTX_zero(ctx);
-  return ctx;
-}
-
-void X509_STORE_CTX_zero(X509_STORE_CTX *ctx) {
-  OPENSSL_memset(ctx, 0, sizeof(X509_STORE_CTX));
+  return OPENSSL_zalloc(sizeof(X509_STORE_CTX));
 }
 
 void X509_STORE_CTX_free(X509_STORE_CTX *ctx) {
@@ -1683,7 +1672,8 @@ void X509_STORE_CTX_free(X509_STORE_CTX *ctx) {
 
 int X509_STORE_CTX_init(X509_STORE_CTX *ctx, X509_STORE *store, X509 *x509,
                         STACK_OF(X509) *chain) {
-  X509_STORE_CTX_zero(ctx);
+  X509_STORE_CTX_cleanup(ctx);
+
   ctx->ctx = store;
   ctx->cert = x509;
   ctx->untrusted = chain;
@@ -1810,7 +1800,7 @@ void X509_STORE_CTX_cleanup(X509_STORE_CTX *ctx) {
   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) {