Add X509_STORE_get1_objects

davidben · t8m · commit 08cecb4448e9 · 2024-01-15T16:29:54.000+01:00
X509_STORE_get0_objects returns a pointer to the X509_STORE's storage, but this function is a bit deceptive. It is practically unusable in a multi-threaded program. See, for example, RUSTSEC-2023-0072, a security vulnerability caused by this OpenSSL API. One might think that, if no other threads are mutating the X509_STORE, it is safe to read the resulting list. However, the documention does not mention that other logically-const operations on the X509_STORE, notably certifcate verifications when a hash_dir is installed, will, under a lock, write to the X509_STORE. The X509_STORE also internally re-sorts the list on the first query. If the caller knows to call X509_STORE_lock and X509_STORE_unlock, it can work around this. But this is not obvious, and the documentation does not discuss how X509_STORE_lock is very rarely safe to use. E.g. one cannot call any APIs like X509_STORE_add_cert or X509_STORE_CTX_get1_issuer while holding the lock because those functions internally expect to take the lock. (X509_STORE_lock is another such API which is not safe to export as public API.) Rather than leave all this to the caller to figure out, the API should have returned a shallow copy of the list, refcounting the values. Then it could be internally locked and the caller can freely inspect the result without synchronization with the X509_STORE. Reviewed-by: Neil Horman <nhorman@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #23224)
diff --git a/crypto/x509/x509_lu.c b/crypto/x509/x509_lu.c
@@ -583,6 +583,36 @@ STACK_OF(X509_OBJECT) *X509_STORE_get0_objects(const X509_STORE *xs)
     return xs->objs;
 }
 
+static X509_OBJECT *x509_object_dup(const X509_OBJECT *obj)
+{
+    X509_OBJECT *ret = X509_OBJECT_new();
+    if (ret == NULL)
+        return NULL;
+
+    ret->type = obj->type;
+    ret->data = obj->data;
+    X509_OBJECT_up_ref_count(ret);
+    return ret;
+}
+
+STACK_OF(X509_OBJECT) *X509_STORE_get1_objects(X509_STORE *store)
+{
+    STACK_OF(X509_OBJECT) *objs;
+
+    if (store == NULL) {
+        ERR_raise(ERR_LIB_X509, ERR_R_PASSED_NULL_PARAMETER);
+        return NULL;
+    }
+
+    if (!x509_store_read_lock(store))
+        return NULL;
+
+    objs = sk_X509_OBJECT_deep_copy(store->objs, x509_object_dup,
+                                    X509_OBJECT_free);
+    X509_STORE_unlock(store);
+    return objs;
+}
+
 STACK_OF(X509) *X509_STORE_get1_all_certs(X509_STORE *store)
 {
     STACK_OF(X509) *sk;
diff --git a/doc/man3/X509_STORE_get0_param.pod b/doc/man3/X509_STORE_get0_param.pod
@@ -3,7 +3,7 @@
 =head1 NAME
 
 X509_STORE_get0_param, X509_STORE_set1_param,
-X509_STORE_get0_objects, X509_STORE_get1_all_certs
+X509_STORE_get1_objects, X509_STORE_get0_objects, X509_STORE_get1_all_certs
 - X509_STORE setter and getter functions
 
 =head1 SYNOPSIS
@@ -12,6 +12,7 @@ X509_STORE_get0_objects, X509_STORE_get1_all_certs
 
  X509_VERIFY_PARAM *X509_STORE_get0_param(const X509_STORE *xs);
  int X509_STORE_set1_param(X509_STORE *xs, const X509_VERIFY_PARAM *pm);
+ STACK_OF(X509_OBJECT) *X509_STORE_get1_objects(X509_STORE *xs);
  STACK_OF(X509_OBJECT) *X509_STORE_get0_objects(const X509_STORE *xs);
  STACK_OF(X509) *X509_STORE_get1_all_certs(X509_STORE *xs);
 
@@ -23,9 +24,15 @@ X509_STORE_get0_param() retrieves an internal pointer to the verification
 parameters for I<xs>. The returned pointer must not be freed by the
 calling application
 
+X509_STORE_get1_objects() returns a snapshot of all objects in the store's X509
+cache. The cache contains B<X509> and B<X509_CRL> objects. The caller is
+responsible for freeing the returned list.
+
 X509_STORE_get0_objects() retrieves an internal pointer to the store's
 X509 object cache. The cache contains B<X509> and B<X509_CRL> objects. The
-returned pointer must not be freed by the calling application.
+returned pointer must not be freed by the calling application. If the store is
+shared across multiple threads, it is not safe to use the result of this
+function. Use X509_STORE_get1_objects() instead, which avoids this problem.
 
 X509_STORE_get1_all_certs() returns a list of all certificates in the store.
 The caller is responsible for freeing the returned list.
@@ -37,6 +44,9 @@ B<X509_VERIFY_PARAM> structure.
 
 X509_STORE_set1_param() returns 1 for success and 0 for failure.
 
+X509_STORE_get1_objects() returns a pointer to a stack of the retrieved
+objects on success, else NULL.
+
 X509_STORE_get0_objects() returns a pointer to a stack of B<X509_OBJECT>.
 
 X509_STORE_get1_all_certs() returns a pointer to a stack of the retrieved
@@ -51,6 +61,7 @@ L<X509_STORE_new(3)>
 B<X509_STORE_get0_param> and B<X509_STORE_get0_objects> were added in
 OpenSSL 1.1.0.
 B<X509_STORE_get1_certs> was added in OpenSSL 3.0.
+B<X509_STORE_get1_objects> was added in OpenSSL 3.3.
 
 =head1 COPYRIGHT
 
diff --git a/include/openssl/x509_vfy.h.in b/include/openssl/x509_vfy.h.in
@@ -400,6 +400,7 @@ int X509_STORE_lock(X509_STORE *xs);
 int X509_STORE_unlock(X509_STORE *xs);
 int X509_STORE_up_ref(X509_STORE *xs);
 STACK_OF(X509_OBJECT) *X509_STORE_get0_objects(const X509_STORE *xs);
+STACK_OF(X509_OBJECT) *X509_STORE_get1_objects(X509_STORE *xs);
 STACK_OF(X509) *X509_STORE_get1_all_certs(X509_STORE *xs);
 STACK_OF(X509) *X509_STORE_CTX_get1_certs(X509_STORE_CTX *xs,
                                           const X509_NAME *nm);
diff --git a/test/x509_load_cert_file_test.c b/test/x509_load_cert_file_test.c
@@ -15,19 +15,32 @@ static const char *chain;
 
 static int test_load_cert_file(void)
 {
-    int ret = 0;
+    int ret = 0, i;
     X509_STORE *store = NULL;
     X509_LOOKUP *lookup = NULL;
     STACK_OF(X509) *certs = NULL;
+    STACK_OF(X509_OBJECT) *objs = NULL;
+
+    if (!TEST_ptr(store = X509_STORE_new())
+        || !TEST_ptr(lookup = X509_STORE_add_lookup(store, X509_LOOKUP_file()))
+        || !TEST_true(X509_load_cert_file(lookup, chain, X509_FILETYPE_PEM))
+        || !TEST_ptr(certs = X509_STORE_get1_all_certs(store))
+        || !TEST_int_eq(sk_X509_num(certs), 4)
+        || !TEST_ptr(objs = X509_STORE_get1_objects(store))
+        || !TEST_int_eq(sk_X509_OBJECT_num(objs), 4))
+        goto err;
+
+    for (i = 0; i < sk_X509_OBJECT_num(objs); i++) {
+        const X509_OBJECT *obj = sk_X509_OBJECT_value(objs, i);
+        if (!TEST_int_eq(X509_OBJECT_get_type(obj), X509_LU_X509))
+            goto err;
+    }
 
-    if (TEST_ptr(store = X509_STORE_new())
-        && TEST_ptr(lookup = X509_STORE_add_lookup(store, X509_LOOKUP_file()))
-        && TEST_true(X509_load_cert_file(lookup, chain, X509_FILETYPE_PEM))
-        && TEST_ptr(certs = X509_STORE_get1_all_certs(store))
-        && TEST_int_eq(sk_X509_num(certs), 4))
-        ret = 1;
+    ret = 1;
 
+err:
     OSSL_STACK_OF_X509_free(certs);
+    sk_X509_OBJECT_pop_free(objs, X509_OBJECT_free);
     X509_STORE_free(store);
     return ret;
 }
diff --git a/util/libcrypto.num b/util/libcrypto.num
@@ -5543,3 +5543,4 @@ OSSL_CMP_ITAV_get0_certProfile          ?	3_3_0	EXIST::FUNCTION:CMP
 OSSL_CMP_SRV_CTX_init_trans             ?	3_3_0	EXIST::FUNCTION:CMP
 EVP_DigestSqueeze                       ?	3_3_0	EXIST::FUNCTION:
 ERR_pop                                 ?	3_3_0	EXIST::FUNCTION:
+X509_STORE_get1_objects                 ?	3_3_0	EXIST::FUNCTION:
