From: David Benjamin Date: Mon, 11 Dec 2023 06:47:25 +0000 (-0500) Subject: Add X509_STORE_get1_objects X-Git-Tag: openssl-3.3.0-alpha1~303 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=08cecb4448e990f7914ec1df97b1ee0ca9031643;p=thirdparty%2Fopenssl.git Add X509_STORE_get1_objects 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 Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/23224) --- diff --git a/crypto/x509/x509_lu.c b/crypto/x509/x509_lu.c index 0ca7cb960d4..3fa4fee1e1e 100644 --- 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 index 559c137285b..d9dfb1b656f 100644 --- 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. 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 and B 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 and B 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 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_STORE_get1_all_certs() returns a pointer to a stack of the retrieved @@ -51,6 +61,7 @@ L B and B were added in OpenSSL 1.1.0. B was added in OpenSSL 3.0. +B was added in OpenSSL 3.3. =head1 COPYRIGHT diff --git a/include/openssl/x509_vfy.h.in b/include/openssl/x509_vfy.h.in index 7a478d117ae..2166ef0b064 100644 --- 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 index 4a736071ae6..001ed570d3c 100644 --- 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 index 9d92ca67cad..f6e5a90ad73 100644 --- 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: