Skip to content

x509: fix use-after-free in wolfSSL_X509_STORE_get0_objects #10650

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
ColtonWilley wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

x509: fix use-after-free in wolfSSL_X509_STORE_get0_objects #10650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 45 additions & 63 deletions src/x509_str.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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) {
Expand All @@ -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;
Comment on lines 2247 to 2251
}
}
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

Expand All @@ -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 */
Expand Down
66 changes: 66 additions & 0 deletions tests/api/test_ossl_x509_str.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 3 additions & 0 deletions tests/api/test_ossl_x509_str.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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), \
Expand Down
1 change: 0 additions & 1 deletion wolfssl/ssl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading