From 95c808bbfb8cf6ad38a02aa348da516eaf7852b1 Mon Sep 17 00:00:00 2001 From: Colton Willey Date: Tue, 9 Jun 2026 18:42:54 -0700 Subject: [PATCH] x509: fix use-after-free in wolfSSL_X509_STORE_get0_objects wolfSSL_X509_STORE_get0_objects builds its list from CertManager certs (owned by the list) and borrowed store->certs entries, using store->numAdded to mark the borrowed tail so they aren't freed. When the store has a CRL, a CRL object is appended after those certs, so the count freed a cert still owned by store->certs. The next get0_objects call then returned a dangling pointer - the use-after-free reported with HAProxy under ASAN. The list now takes its own reference on every cert (wolfSSL_X509_up_ref), so it owns all its entries and is freed uniformly, and the numAdded bookkeeping (and its struct field) is removed. Fixes #10123 --- src/x509_str.c | 108 ++++++++++++++------------------- tests/api/test_ossl_x509_str.c | 66 ++++++++++++++++++++ tests/api/test_ossl_x509_str.h | 3 + wolfssl/ssl.h | 1 - 4 files changed, 114 insertions(+), 64 deletions(-) diff --git a/src/x509_str.c b/src/x509_str.c index abdb42b4e21..fe59330af4b 100644 --- a/src/x509_str.c +++ b/src/x509_str.c @@ -1403,8 +1403,6 @@ WOLFSSL_X509_STORE* wolfSSL_X509_STORE_new(void) store->crl = store->cm->crl; #endif - store->numAdded = 0; - #if defined(OPENSSL_EXTRA) || defined(WOLFSSL_WPAS_SMALL) /* Link store's new Certificate Manager to self by default */ @@ -1440,30 +1438,41 @@ WOLFSSL_X509_STORE* wolfSSL_X509_STORE_new(void) } #ifdef OPENSSL_ALL -static void X509StoreFreeObjList(WOLFSSL_X509_STORE* store, - WOLF_STACK_OF(WOLFSSL_X509_OBJECT)* objs) +/* Free a list from wolfSSL_X509_STORE_get0_objects(). The list owns a reference + * to each entry, so pop_free drops exactly one ref per cert/CRL. */ +static void X509StoreFreeObjList(WOLF_STACK_OF(WOLFSSL_X509_OBJECT)* objs) { - int i; - WOLFSSL_X509_OBJECT *obj = NULL; - int cnt = store->numAdded; + wolfSSL_sk_X509_OBJECT_pop_free(objs, NULL); +} - /* -1 here because it is later used as an index value into the object stack. - * With there being the chance that the only object in the stack is one from - * the numAdded to the store >= is used when comparing to 0. */ - i = wolfSSL_sk_X509_OBJECT_num(objs) - 1; - while (cnt > 0 && i >= 0) { - /* The inner X509 is owned by somebody else, NULL out the reference */ - obj = (WOLFSSL_X509_OBJECT *)wolfSSL_sk_X509_OBJECT_value(objs, i); - if (obj != NULL) { - obj->type = (WOLFSSL_X509_LOOKUP_TYPE)0; - obj->data.ptr = NULL; - } - cnt--; - i--; - } +#if defined(WOLFSSL_SIGNER_DER_CERT) && !defined(NO_FILESYSTEM) +/* Add x509 to objs in a new X509_OBJECT, taking a reference so the list owns it + * independently of its source (store->certs or the CM-decode stack). */ +static int X509StoreAddCertToObjs(WOLF_STACK_OF(WOLFSSL_X509_OBJECT)* objs, + WOLFSSL_X509* x509) +{ + WOLFSSL_X509_OBJECT* obj; - wolfSSL_sk_X509_OBJECT_pop_free(objs, NULL); + if (wolfSSL_X509_up_ref(x509) != WOLFSSL_SUCCESS) { + WOLFSSL_MSG("wolfSSL_X509_up_ref error"); + return WOLFSSL_FAILURE; + } + obj = wolfSSL_X509_OBJECT_new(); + if (obj == NULL) { + WOLFSSL_MSG("wolfSSL_X509_OBJECT_new error"); + wolfSSL_X509_free(x509); /* undo up_ref */ + return WOLFSSL_FAILURE; + } + obj->type = WOLFSSL_X509_LU_X509; + obj->data.x509 = x509; + if (wolfSSL_sk_X509_OBJECT_push(objs, obj) <= 0) { + WOLFSSL_MSG("wolfSSL_sk_X509_OBJECT_push error"); + wolfSSL_X509_OBJECT_free(obj); + return WOLFSSL_FAILURE; + } + return WOLFSSL_SUCCESS; } +#endif /* WOLFSSL_SIGNER_DER_CERT && !NO_FILESYSTEM */ #endif void wolfSSL_X509_STORE_free(WOLFSSL_X509_STORE* store) @@ -1504,7 +1513,7 @@ void wolfSSL_X509_STORE_free(WOLFSSL_X509_STORE* store) #endif #ifdef OPENSSL_ALL if (store->objs != NULL) { - X509StoreFreeObjList(store, store->objs); + X509StoreFreeObjList(store->objs); } #endif #if defined(OPENSSL_EXTRA) || defined(WOLFSSL_WPAS_SMALL) @@ -2197,8 +2206,7 @@ WOLF_STACK_OF(WOLFSSL_X509_OBJECT)* wolfSSL_X509_STORE_get0_objects( { WOLFSSL_STACK* ret = NULL; WOLFSSL_STACK* cert_stack = NULL; -#if ((defined(WOLFSSL_SIGNER_DER_CERT) && !defined(NO_FILESYSTEM)) || \ - (defined(HAVE_CRL))) +#ifdef HAVE_CRL WOLFSSL_X509_OBJECT* obj = NULL; #endif #if defined(WOLFSSL_SIGNER_DER_CERT) && !defined(NO_FILESYSTEM) @@ -2215,7 +2223,7 @@ WOLF_STACK_OF(WOLFSSL_X509_OBJECT)* wolfSSL_X509_STORE_get0_objects( if (store->objs != NULL) { #if defined(WOLFSSL_SIGNER_DER_CERT) && !defined(NO_FILESYSTEM) /* want to update objs stack by cm stack again before returning it*/ - X509StoreFreeObjList(store, store->objs); + X509StoreFreeObjList(store->objs); store->objs = NULL; #else if (wolfSSL_sk_X509_OBJECT_num(store->objs) == 0) { @@ -2234,41 +2242,18 @@ WOLF_STACK_OF(WOLFSSL_X509_OBJECT)* wolfSSL_X509_STORE_get0_objects( } #if defined(WOLFSSL_SIGNER_DER_CERT) && !defined(NO_FILESYSTEM) + /* Two sources, each ref'd into the list: CM-decoded certs (cert_stack) and + * store->certs (compat intermediates not in the CM). */ cert_stack = wolfSSL_CertManagerGetCerts(store->cm); - store->numAdded = 0; - if (cert_stack == NULL && wolfSSL_sk_X509_num(store->certs) > 0) { - cert_stack = wolfSSL_sk_X509_new_null(); - if (cert_stack == NULL) { - WOLFSSL_MSG("wolfSSL_sk_X509_OBJECT_new error"); + for (i = 0; i < wolfSSL_sk_X509_num(cert_stack); i++) { + x509 = (WOLFSSL_X509 *)wolfSSL_sk_X509_value(cert_stack, i); + if (X509StoreAddCertToObjs(ret, x509) != WOLFSSL_SUCCESS) goto err_cleanup; - } } for (i = 0; i < wolfSSL_sk_X509_num(store->certs); i++) { - if (wolfSSL_sk_X509_push(cert_stack, - wolfSSL_sk_X509_value(store->certs, i)) > 0) { - store->numAdded++; - } - } - /* Do not modify stack until after we guarantee success to - * simplify cleanup logic handling cert merging above */ - for (i = 0; i < wolfSSL_sk_X509_num(cert_stack); i++) { - x509 = (WOLFSSL_X509 *)wolfSSL_sk_value(cert_stack, i); - obj = wolfSSL_X509_OBJECT_new(); - if (obj == NULL) { - WOLFSSL_MSG("wolfSSL_X509_OBJECT_new error"); + x509 = (WOLFSSL_X509 *)wolfSSL_sk_X509_value(store->certs, i); + if (X509StoreAddCertToObjs(ret, x509) != WOLFSSL_SUCCESS) goto err_cleanup; - } - if (wolfSSL_sk_X509_OBJECT_push(ret, obj) <= 0) { - WOLFSSL_MSG("wolfSSL_sk_X509_OBJECT_push error"); - wolfSSL_X509_OBJECT_free(obj); - goto err_cleanup; - } - obj->type = WOLFSSL_X509_LU_X509; - obj->data.x509 = x509; - } - - while (wolfSSL_sk_X509_num(cert_stack) > 0) { - wolfSSL_sk_X509_pop(cert_stack); } #endif @@ -2295,20 +2280,17 @@ WOLF_STACK_OF(WOLFSSL_X509_OBJECT)* wolfSSL_X509_STORE_get0_objects( } #endif + /* Drop cert_stack's refs; the list holds its own. */ if (cert_stack) wolfSSL_sk_X509_pop_free(cert_stack, NULL); store->objs = ret; return ret; err_cleanup: + /* ret and cert_stack hold disjoint refs; free both. */ if (ret != NULL) - X509StoreFreeObjList(store, ret); - if (cert_stack != NULL) { - while (store->numAdded > 0) { - wolfSSL_sk_X509_pop(cert_stack); - store->numAdded--; - } + X509StoreFreeObjList(ret); + if (cert_stack != NULL) wolfSSL_sk_X509_pop_free(cert_stack, NULL); - } return NULL; } #endif /* OPENSSL_ALL */ diff --git a/tests/api/test_ossl_x509_str.c b/tests/api/test_ossl_x509_str.c index 96218daafad..acccca1814e 100644 --- a/tests/api/test_ossl_x509_str.c +++ b/tests/api/test_ossl_x509_str.c @@ -1671,6 +1671,72 @@ int test_X509_STORE_get0_objects(void) return EXPECT_RESULT(); } +/* Regression test for #10123: an X509_STORE with an intermediate in + * store->certs, enumerated repeatedly with a CRL present. get0_objects used to + * free the borrowed intermediate, faulting on a later deref. */ +int test_X509_STORE_get0_objects_extern_repeat(void) +{ + EXPECT_DECLS; +#if defined(OPENSSL_ALL) && defined(WOLFSSL_SIGNER_DER_CERT) && \ + !defined(NO_FILESYSTEM) && !defined(NO_TLS) && !defined(NO_WOLFSSL_DIR) && \ + !defined(NO_RSA) + X509_STORE *store = NULL; + STACK_OF(X509_OBJECT) *objs = NULL; + int call; + int i; + + ExpectNotNull(store = X509_STORE_new()); + /* self-signed root -> store->trusted + CertManager */ + ExpectIntEQ(X509_STORE_load_locations(store, caCertFile, NULL), + WOLFSSL_SUCCESS); + /* non-self-signed intermediate -> store->certs (the borrowed cert) */ + ExpectIntEQ(X509_STORE_load_locations(store, + "./certs/intermediate/ca-int-cert.pem", NULL), WOLFSSL_SUCCESS); + + /* Repeated get0 + deref; the unfixed code faults once the borrowed + * intermediate is freed. */ + for (call = 0; call < 4 && EXPECT_SUCCESS(); call++) { + int x509Cnt = 0; + #ifdef HAVE_CRL + int sawCrl = 0; + #endif + ExpectNotNull(objs = X509_STORE_get0_objects(store)); + for (i = 0; i < sk_X509_OBJECT_num(objs) && EXPECT_SUCCESS(); i++) { + X509_OBJECT *obj = (X509_OBJECT*)sk_X509_OBJECT_value(objs, i); + if (X509_OBJECT_get_type(obj) == X509_LU_X509) { + X509 *x509 = X509_OBJECT_get0_X509(obj); + X509_NAME *nm = NULL; + ExpectNotNull(x509); + ExpectNotNull(nm = X509_get_subject_name(x509)); + /* the deref that used to read freed memory; assert only when + * SHA is built in (X509_NAME_hash returns 0 under NO_SHA) */ + #ifndef NO_SHA + ExpectIntNE(X509_NAME_hash(nm), 0); + #else + (void)X509_NAME_hash(nm); + #endif + x509Cnt++; + } + #ifdef HAVE_CRL + else if (X509_OBJECT_get_type(obj) == X509_LU_CRL) { + sawCrl = 1; + } + #endif + } + /* exactly root + intermediate; == catches a lost or duplicated cert */ + ExpectIntEQ(x509Cnt, 2); + #ifdef HAVE_CRL + /* the trailing CRL is what mis-shifted the old cleanup; assert it's + * present so the trigger stays exercised */ + ExpectIntEQ(sawCrl, 1); + #endif + } + + X509_STORE_free(store); +#endif + return EXPECT_RESULT(); +} + int test_wolfSSL_X509_STORE_get1_certs(void) { EXPECT_DECLS; diff --git a/tests/api/test_ossl_x509_str.h b/tests/api/test_ossl_x509_str.h index a3fe8bfebdf..8b2ad3bdd2a 100644 --- a/tests/api/test_ossl_x509_str.h +++ b/tests/api/test_ossl_x509_str.h @@ -38,6 +38,7 @@ int test_wolfSSL_X509_STORE_set_flags(void); int test_wolfSSL_X509_STORE(void); int test_wolfSSL_X509_STORE_load_locations(void); int test_X509_STORE_get0_objects(void); +int test_X509_STORE_get0_objects_extern_repeat(void); int test_wolfSSL_X509_STORE_get1_certs(void); int test_wolfSSL_X509_STORE_set_get_crl(void); int test_wolfSSL_X509_CA_num(void); @@ -63,6 +64,8 @@ int test_wolfSSL_CTX_set_cert_store(void); TEST_DECL_GROUP("ossl_x509_store", \ test_wolfSSL_X509_STORE_load_locations), \ TEST_DECL_GROUP("ossl_x509_store", test_X509_STORE_get0_objects), \ + TEST_DECL_GROUP("ossl_x509_store", \ + test_X509_STORE_get0_objects_extern_repeat), \ TEST_DECL_GROUP("ossl_x509_store", test_wolfSSL_X509_STORE_get1_certs), \ TEST_DECL_GROUP("ossl_x509_store", test_wolfSSL_X509_STORE_set_get_crl), \ TEST_DECL_GROUP("ossl_x509_store", test_wolfSSL_X509_CA_num), \ diff --git a/wolfssl/ssl.h b/wolfssl/ssl.h index 713c5a55c07..6cd8798ae1c 100644 --- a/wolfssl/ssl.h +++ b/wolfssl/ssl.h @@ -879,7 +879,6 @@ struct WOLFSSL_X509_STORE { WOLF_STACK_OF(WOLFSSL_X509)* certs; WOLF_STACK_OF(WOLFSSL_X509)* trusted; WOLF_STACK_OF(WOLFSSL_X509)* owned; - word32 numAdded; /* Number of objs in objs that are in certs sk */ }; #if defined(OPENSSL_EXTRA) || defined(WOLFSSL_WPAS_SMALL)