-
Notifications
You must be signed in to change notification settings - Fork 1k
X509_verify_cert: avoid mutating shared store cert stack #10997
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -632,37 +632,6 @@ static int X509StoreMoveCert(WOLFSSL_STACK *certs_stack, | |
| return WOLFSSL_FAILURE; | ||
| } | ||
|
|
||
| /* Remove the first node referencing `cert` (by pointer identity) from `stack`. | ||
| * The certificate object itself is not freed - the stack only holds a borrowed | ||
| * reference. Returns WOLFSSL_SUCCESS if a node was removed, WOLFSSL_FAILURE if | ||
| * `cert` was not present, or WOLFSSL_FATAL_ERROR if `stack`/`cert` is NULL. | ||
| * The only caller performs best-effort cleanup and intentionally ignores the | ||
| * return value. | ||
| * | ||
| * Walks the linked list once (O(n)) rather than indexing with | ||
| * wolfSSL_sk_X509_value() per position (which would re-walk from the head each | ||
| * time, O(n^2)). */ | ||
| static int X509StoreRemoveCert(WOLFSSL_STACK *stack, WOLFSSL_X509 *cert) { | ||
| WOLFSSL_STACK* node; | ||
| int idx; | ||
| int num; | ||
|
|
||
| if (stack == NULL || cert == NULL) | ||
| return WOLFSSL_FATAL_ERROR; | ||
|
|
||
| num = wolfSSL_sk_X509_num(stack); | ||
| for (node = stack, idx = 0; idx < num && node != NULL; | ||
| node = node->next, idx++) { | ||
| if (node->data.x509 == cert) { | ||
| (void)wolfSSL_sk_pop_node(stack, idx); | ||
| return WOLFSSL_SUCCESS; | ||
| } | ||
| } | ||
|
|
||
| return WOLFSSL_FAILURE; | ||
| } | ||
|
|
||
|
|
||
| /* Push x509 onto the ctx chain with its own reference, like OpenSSL. | ||
| * The chain owns a reference to each of its certs. */ | ||
| static int X509StoreChainPush(WOLF_STACK_OF(WOLFSSL_X509)* chain, | ||
|
|
@@ -679,6 +648,29 @@ static int X509StoreChainPush(WOLF_STACK_OF(WOLFSSL_X509)* chain, | |
| return ret; | ||
| } | ||
|
|
||
| /* Returns 1 if `cert` (by pointer identity) is present in `stack`, else 0. | ||
| * Used to keep a candidate that already failed verification off the reported | ||
| * chain. */ | ||
| static int X509StoreCertInStack(WOLF_STACK_OF(WOLFSSL_X509)* stack, | ||
| WOLFSSL_X509* cert) | ||
| { | ||
| int i; | ||
| int num; | ||
|
|
||
| if (stack == NULL || cert == NULL) | ||
| return 0; | ||
|
|
||
| /* Index by logical position like the other helpers in this file rather | ||
| * than walking raw nodes. */ | ||
| num = wolfSSL_sk_X509_num(stack); | ||
| for (i = 0; i < num; i++) { | ||
| if (wolfSSL_sk_X509_value(stack, i) == cert) | ||
| return 1; | ||
| } | ||
|
|
||
| return 0; | ||
| } | ||
|
|
||
| /* Current certificate failed, but it is possible there is an | ||
| * alternative cert with the same subject key which will work. | ||
| * Retry until all possible candidate certs are exhausted. */ | ||
|
|
@@ -716,11 +708,15 @@ static int X509DerEquals(WOLFSSL_X509* cur, WOLFSSL_X509* x509) | |
| } | ||
|
|
||
| /* Returns 1 if x509's DER matches an entry in either origTrustedSk (an | ||
| * immutable snapshot of the caller's trusted set captured before any | ||
| * intermediates were injected for this verification call) or in | ||
| * store->trusted. Returns 0 otherwise. Used by the | ||
| * X509_V_FLAG_PARTIAL_CHAIN fallback to confirm that a chain actually | ||
| * terminates at a caller-trusted certificate. */ | ||
| * immutable snapshot of the caller's trusted set - store->certs or the | ||
| * set0_trusted_stack override - captured before any intermediates were | ||
| * injected for this verification call) or in store->trusted. Returns 0 | ||
| * otherwise. Used by the X509_V_FLAG_PARTIAL_CHAIN fallback to confirm that | ||
| * a chain actually terminates at a caller-trusted certificate. | ||
| * NOTE: origTrustedSk is a private snapshot, but store->trusted is read live | ||
| * and unlocked here (as it is at the terminal issuer lookup); this mitigation | ||
| * is deliberately asymmetric, so a single X509_STORE must not be shared across | ||
| * threads verifying concurrently. */ | ||
| static int X509StoreCertIsTrusted(WOLFSSL_X509_STORE* store, | ||
| WOLFSSL_X509* x509, WOLF_STACK_OF(WOLFSSL_X509)* origTrustedSk) | ||
| { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⚪ [Info] Snapshot mitigation is asymmetric: store->trusted and setTrustedSk are still walked live during verification Sites:
So after this PR one shared stack ( Recommendation: For consistency with the |
||
|
|
@@ -859,13 +855,11 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx) | |
| int ret = WC_NO_ERR_TRACE(WOLFSSL_FAILURE); | ||
| int done = 0; | ||
| int added = 0; | ||
| int i = 0; | ||
| int numFailedCerts = 0; | ||
| int depth = 0; | ||
| int origDepth = 0; | ||
| WOLFSSL_X509 *issuer = NULL; | ||
| WOLFSSL_X509 *orig = NULL; | ||
| WOLF_STACK_OF(WOLFSSL_X509)* certs = NULL; | ||
| WOLF_STACK_OF(WOLFSSL_X509)* callerTrusted = NULL; | ||
| WOLF_STACK_OF(WOLFSSL_X509)* certsToUse = NULL; | ||
| WOLF_STACK_OF(WOLFSSL_X509)* failedCerts = NULL; | ||
| WOLF_STACK_OF(WOLFSSL_X509)* origTrustedSk = NULL; | ||
|
|
@@ -876,51 +870,47 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx) | |
| return WOLFSSL_FATAL_ERROR; | ||
| } | ||
|
|
||
| certs = ctx->store->certs; | ||
|
|
||
| /* Chain building mutates the working stack: caller-supplied intermediates | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 [Medium] Both shallow dups re-walk the shared store stack, so the two snapshots can disagree - take the second one from certsToUse Moving the PARTIAL_CHAIN trust check onto its own snapshot addresses the round-1 comment, but taking that snapshot from certsToUse = wolfSSL_shallow_sk_dup(callerTrusted);
origTrustedSk = wolfSSL_shallow_sk_dup(callerTrusted);The two walks are independent and unsynchronised, so they are not guaranteed to observe the same set. If thread B calls The residual points are worth a note rather than a change, since the PR already documents (972-979) that concurrent verification on one store stays unsupported:
Suggestion: Reword the 865-878 comment (and the PR description) to say the copy removes the write-side corruption but does not make concurrent use of one X509_STORE safe, since the copy is itself an unlocked read - consistent with the NOTE already added at lines 972-979. |
||
| * are appended and X509VerifyCertSetupRetry moves failed certs out of it. | ||
| * store->certs is shared by every connection using this store and | ||
| * setTrustedSk is owned by the caller, so build a per-verification shallow | ||
| * copy (certsToUse) and leave both untouched. This removes the write-side | ||
| * corruption a concurrent verification used to inflict on those stacks, but | ||
| * it does NOT make concurrent use of one X509_STORE safe: the dup is itself | ||
| * an unlocked read, and store->trusted is still walked live (see the NOTE | ||
| * below). A single store must not be shared across threads that verify | ||
| * concurrently. | ||
| * | ||
| * The X509_V_FLAG_PARTIAL_CHAIN fallback needs the set of certs that were | ||
| * caller-trusted before any intermediates were injected. Snapshot it from | ||
| * certsToUse - the private copy, taken before addAllButSelfSigned() injects | ||
| * intermediates - rather than walking the shared stack a second time, so | ||
| * the two snapshots are provably identical. Both dups hold borrowed | ||
| * references and are shallow-freed at exit. */ | ||
| callerTrusted = ctx->store->certs; | ||
| if (ctx->setTrustedSk != NULL) { | ||
|
Frauschi marked this conversation as resolved.
|
||
| certs = ctx->setTrustedSk; | ||
| callerTrusted = ctx->setTrustedSk; | ||
| } | ||
|
|
||
| if (certs == NULL && | ||
| wolfSSL_sk_X509_num(ctx->ctxIntermediates) > 0) { | ||
| certsToUse = wolfSSL_sk_X509_new_null(); | ||
| if (certsToUse == NULL) { | ||
| if (callerTrusted != NULL) { | ||
| certsToUse = wolfSSL_shallow_sk_dup(callerTrusted); | ||
| if (certsToUse != NULL) | ||
| origTrustedSk = wolfSSL_shallow_sk_dup(certsToUse); | ||
| if (origTrustedSk == NULL) { | ||
| ret = WOLFSSL_FAILURE; | ||
| goto exit; | ||
| } | ||
| ret = addAllButSelfSigned(certsToUse, ctx->ctxIntermediates, NULL); | ||
| /* certsToUse holds only injected intermediates, none are trusted, so | ||
| * leave origTrustedSk NULL (empty snapshot). */ | ||
| certs = certsToUse; | ||
| } | ||
| else { | ||
| /* Snapshot the caller-trusted entries before injecting the | ||
| * caller-supplied untrusted intermediates. Only the entries already | ||
| * present count as trusted for the partial-chain check below, and | ||
| * we need a stable reference because X509VerifyCertSetupRetry may | ||
| * remove nodes from `certs` during chain building. */ | ||
| if (certs != NULL && wolfSSL_sk_X509_num(certs) > 0) { | ||
| int j; | ||
| int n = wolfSSL_sk_X509_num(certs); | ||
| origTrustedSk = wolfSSL_sk_X509_new_null(); | ||
| if (origTrustedSk == NULL) { | ||
| ret = WOLFSSL_FAILURE; | ||
| goto exit; | ||
| } | ||
| for (j = 0; j < n; j++) { | ||
| if (wolfSSL_sk_X509_push(origTrustedSk, | ||
| wolfSSL_sk_X509_value(certs, j)) <= 0) { | ||
| ret = WOLFSSL_FAILURE; | ||
| goto exit; | ||
| } | ||
| } | ||
| } | ||
| /* Add the intermediates provided on init to the list of untrusted | ||
| * intermediates to be used. They are removed again from `certs` in the | ||
| * exit cleanup (by identity, recomputed from ctxIntermediates). */ | ||
| ret = addAllButSelfSigned(certs, ctx->ctxIntermediates, NULL); | ||
| certsToUse = wolfSSL_sk_X509_new_null(); | ||
| } | ||
| if (certsToUse == NULL) { | ||
| ret = WOLFSSL_FAILURE; | ||
| goto exit; | ||
| } | ||
| /* Add the intermediates provided on init to the list of untrusted | ||
| * intermediates to be used. */ | ||
| ret = addAllButSelfSigned(certsToUse, ctx->ctxIntermediates, NULL); | ||
| if (ret != WOLFSSL_SUCCESS) { | ||
| goto exit; | ||
| } | ||
|
|
@@ -956,7 +946,7 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx) | |
| issuer = NULL; | ||
|
|
||
| /* Try to find an untrusted issuer first */ | ||
| ret = X509StoreGetIssuerEx(&issuer, certs, | ||
| ret = X509StoreGetIssuerEx(&issuer, certsToUse, | ||
| ctx->current_cert); | ||
| if (ret == WOLFSSL_SUCCESS) { | ||
| if (ctx->current_cert == issuer) { | ||
|
|
@@ -991,9 +981,17 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx) | |
| } | ||
| } | ||
| #endif | ||
| /* NOTE: this loads the caller-supplied intermediate into the | ||
| * shared ctx->store->cm as a WOLFSSL_TEMP_CA, and the unload paths | ||
| * drop *all* WOLFSSL_TEMP_CA signers in that CertManager, not only | ||
| * the ones added here. This copy removes the working-stack race, | ||
| * but two threads running X509_verify_cert() against the same | ||
| * X509_STORE still contend on store->cm. Concurrent verification | ||
| * on a single shared store therefore remains unsupported; callers | ||
| * needing it must use a store per thread. */ | ||
| ret = X509StoreAddCa(ctx->store, issuer, WOLFSSL_TEMP_CA); | ||
| if (ret != WOLFSSL_SUCCESS) { | ||
| X509VerifyCertSetupRetry(ctx, certs, failedCerts, | ||
| X509VerifyCertSetupRetry(ctx, certsToUse, failedCerts, | ||
| &depth, origDepth); | ||
| continue; | ||
| } | ||
|
|
@@ -1002,7 +1000,7 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx) | |
| if (ret != WOLFSSL_SUCCESS) { | ||
| if ((origDepth - depth) <= 1) | ||
| added = 0; | ||
| X509VerifyCertSetupRetry(ctx, certs, failedCerts, | ||
| X509VerifyCertSetupRetry(ctx, certsToUse, failedCerts, | ||
| &depth, origDepth); | ||
| continue; | ||
| } | ||
|
|
@@ -1058,7 +1056,7 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx) | |
| * above; the depth>0/done==0 success path accepts it. */ | ||
| break; | ||
| } else { | ||
| X509VerifyCertSetupRetry(ctx, certs, failedCerts, | ||
| X509VerifyCertSetupRetry(ctx, certsToUse, failedCerts, | ||
| &depth, origDepth); | ||
| continue; | ||
| } | ||
|
|
@@ -1082,7 +1080,16 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx) | |
| ctx->setTrustedSk, ctx->current_cert); | ||
| } | ||
| #endif | ||
| if (issuer != NULL) { | ||
| /* A candidate that already failed verification (moved to | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 [Medium] The new failedCerts guard is narrowly reachable and untested, and when it does fire it drops the anchor off the reported chain This is the guard added for the round-1 review comment, and the fix is right in principle - thanks. Two things to settle before it lands. It currently has no coverage. I verified this by mutation on the PR head (7051bc9): neutralising the guard so it always pushes ( That matches the reachability analysis: Also worth a word in the comment: under When it does fire, Suggestion: Add a case that puts the store's real anchor in the CertManager (e.g. an SSL_CTX-owned store, or CertManagerLoadCA) while a same-subject rejected candidate sits in the set0_trusted_stack, then assert X509_STORE_CTX_get0_chain() does not contain the rejected cert. Also note in the comment that under WOLFSSL_SIGNER_DER_CERT the issuer is a CM copy and the identity check does not apply. |
||
| * failedCerts by the retry path) must not terminate the reported | ||
| * chain. setTrustedSk / store->trusted are searched by name+AKID, | ||
| * not by signature, and setTrustedSk is no longer pruned during | ||
| * chain building, so X509StoreGetIssuerEx can return a same-subject | ||
| * cert that was tried and rejected. | ||
| * Under WOLFSSL_SIGNER_DER_CERT the issuer above is a freshly | ||
| * allocated CM copy, never pointer-equal to a failedCerts entry, so | ||
| * this guard is a no-op on that path. */ | ||
| if (issuer != NULL && !X509StoreCertInStack(failedCerts, issuer)) { | ||
| X509StoreChainPush(ctx->chain, issuer); | ||
| } | ||
|
|
||
|
|
@@ -1115,40 +1122,12 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx) | |
| } | ||
|
|
||
| exit: | ||
| /* Copy back failed certs. */ | ||
| numFailedCerts = wolfSSL_sk_X509_num(failedCerts); | ||
| for (i = 0; i < numFailedCerts; i++) | ||
| { | ||
| wolfSSL_sk_X509_push(certs, wolfSSL_sk_X509_pop(failedCerts)); | ||
| } | ||
| wolfSSL_sk_X509_pop_free(failedCerts, NULL); | ||
| /* failedCerts, certsToUse and origTrustedSk hold only borrowed references; | ||
| * free the stack nodes, not the certs. All three are per-verification | ||
| * stacks (certsToUse/origTrustedSk are shallow dups of the caller's set), | ||
| * so none of the caller's own stacks are touched here. */ | ||
| wolfSSL_sk_X509_free(failedCerts); | ||
|
|
||
| /* Remove the caller-supplied intermediates that addAllButSelfSigned | ||
| * appended to `certs` during chain building, restoring it to its original | ||
| * contents. Remove them by pointer identity from the same stack they were | ||
| * added to (store->certs in the common case, or the caller's setTrustedSk | ||
| * via X509_STORE_CTX_set0_trusted_stack), recomputed from ctxIntermediates | ||
| * with the same self-signed filter as the add. | ||
| * | ||
| * Identity removal - not a saved count + positional pop - is required: | ||
| * X509VerifyCertSetupRetry reorders `certs` during chain building, so | ||
| * popping N entries off the top could drop a legitimate trusted entry and | ||
| * leave an injected intermediate behind, which a later verification reusing | ||
| * this store/ctx would then snapshot as a trust anchor. certsToUse is the | ||
| * throwaway certs==NULL path and is freed wholesale below, so skip it. */ | ||
| if (ctx != NULL && certsToUse == NULL && certs != NULL && | ||
| ctx->ctxIntermediates != NULL) { | ||
| int n = wolfSSL_sk_X509_num(ctx->ctxIntermediates); | ||
| for (i = 0; i < n; i++) { | ||
| WOLFSSL_X509* inter = | ||
| wolfSSL_sk_X509_value(ctx->ctxIntermediates, i); | ||
| if (inter != NULL && | ||
| wolfSSL_X509_NAME_cmp(&inter->issuer, &inter->subject) | ||
| != 0) { | ||
| X509StoreRemoveCert(certs, inter); | ||
| } | ||
| } | ||
| } | ||
| /* Remove intermediates that were added to CM */ | ||
| if (ctx != NULL) { | ||
| if (ctx->store != NULL) { | ||
|
|
@@ -1160,13 +1139,8 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx) | |
| ctx->current_cert = orig; | ||
| } | ||
| } | ||
| if (certsToUse != NULL) { | ||
| wolfSSL_sk_X509_free(certsToUse); | ||
| } | ||
| if (origTrustedSk != NULL) { | ||
| /* Shallow free: only the snapshot's stack nodes, not the X509s. */ | ||
| wolfSSL_sk_X509_free(origTrustedSk); | ||
| } | ||
| wolfSSL_sk_X509_free(certsToUse); | ||
| wolfSSL_sk_X509_free(origTrustedSk); | ||
|
|
||
| /* Enforce hostname / IP verification from X509_VERIFY_PARAM if set. | ||
| * Always check against the leaf (end-entity) certificate, captured in | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 [Low] X509StoreCertInStack walks nodes without bounding by wolfSSL_sk_X509_num()
🔧 NIT
conventionThe new helper iterates the raw node list (
for (node = stack; node != NULL; node = node->next)) while every sibling helper in this file bounds the walk by the stack count -X509StoreMoveCert()useswolfSSL_sk_X509_num(), and theX509StoreRemoveCert()deleted by this PR usedidx < num && node != NULLfor exactly that reason. It is safe today (a wolfSSL empty stack is a single head node withdata.x509 == NULL, andcertis NULL-checked, so no false positive), but it relies on the node-list length always matchingnum, an invariant the rest of the file does not assume.Also raised by the
bugsscan (src/x509_str.c:651-668):The new helper iterates raw nodes (
for (node = stack; node != NULL; node = node->next)) and never consultswolfSSL_sk_X509_num(). The helper it replaces in the same diff,X509StoreRemoveCert(), deliberately bounded its walk withnum = wolfSSL_sk_X509_num(stack)and carried a comment explaining that a wolfSSL stack's logical length ishead->num, not the physical node count.Today this is still correct for the only argument passed (
failedCerts): that stack is only ever appended to viawolfSSL_sk_push(), an empty head node carriesdata.x509 == NULL, andcertis checked non-NULL, so no false positive is possible. But the two representations are not interchangeable in general -wolfSSL_sk_insert()incrementsstack->numbefore linking the new node (src/ssl_sk.c:767-778), sonumand the node chain are transiently inconsistent - and any future caller that passes a stack which has had entries removed positionally, or a stack shared with another writer, gets a silently different answer than every other iteration site in this file.Since the guard at line 1077 decides whether a certificate is reported in
ctx->chain, a wrong answer here is a wrong verified chain, not just a cosmetic difference.Suggestion: