Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
214 changes: 94 additions & 120 deletions src/x509_str.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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.

Copy link
Copy Markdown
Contributor

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 convention

The 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() uses wolfSSL_sk_X509_num(), and the X509StoreRemoveCert() deleted by this PR used idx < num && node != NULL for exactly that reason. It is safe today (a wolfSSL empty stack is a single head node with data.x509 == NULL, and cert is NULL-checked, so no false positive), but it relies on the node-list length always matching num, an invariant the rest of the file does not assume.

Also raised by the bugs scan (src/x509_str.c:651-668):

The new helper iterates raw nodes (for (node = stack; node != NULL; node = node->next)) and never consults wolfSSL_sk_X509_num(). The helper it replaces in the same diff, X509StoreRemoveCert(), deliberately bounded its walk with num = wolfSSL_sk_X509_num(stack) and carried a comment explaining that a wolfSSL stack's logical length is head->num, not the physical node count.

Today this is still correct for the only argument passed (failedCerts): that stack is only ever appended to via wolfSSL_sk_push(), an empty head node carries data.x509 == NULL, and cert is checked non-NULL, so no false positive is possible. But the two representations are not interchangeable in general - wolfSSL_sk_insert() increments stack->num before linking the new node (src/ssl_sk.c:767-778), so num and 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:

Suggested change
/* Returns 1 if `cert` (by pointer identity) is present in `stack`, else 0.
int idx;
int num = wolfSSL_sk_X509_num(stack);
for (node = stack, idx = 0; idx < num && node != NULL;
node = node->next, idx++) {
if (node->data.x509 == cert)
return 1;
}

* 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. */
Expand Down Expand Up @@ -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)
{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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
🔧 NIT

Sites: src/x509_str.c:729-735,1062-1069
The PR's rationale for snapshotting is that "another thread may add to or detach store->certs while this verification is in flight, so a borrowed pointer could be reordered or freed underneath us" (src/x509_str.c:871-878). That rationale applies verbatim to ctx->store->trusted, which wolfSSL_X509_STORE_add_cert() also appends to under no lock (src/x509_str.c:2059) - yet store->trusted is still walked live in two places the PR touched or relies on:

  • X509StoreCertIsTrusted() (src/x509_str.c:729-735) iterates store->trusted directly on every PARTIAL_CHAIN fallback. Only the origTrustedSk half of that function got a snapshot; the store->trusted half did not.
  • the terminal issuer lookup (src/x509_str.c:1062-1069) calls X509StoreGetIssuerEx() on ctx->store->trusted / ctx->setTrustedSk live, immediately before the new X509StoreCertInStack(failedCerts, issuer) guard.

So after this PR one shared stack (store->certs) is snapshotted while its sibling (store->trusted) is not, for no stated reason. The PR is honest that full concurrency is not achieved - the new NOTE at src/x509_str.c:972-979 records that caller-supplied intermediates are still loaded into the shared ctx->store->cm as WOLFSSL_TEMP_CA and that the unload drops all temp CAs in that CertManager, so two threads verifying against one X509_STORE still corrupt each other's temporary trust set. This is a design observation, not a defect introduced by the diff: the change is a net improvement (no verification path mutates store->certs or the caller's setTrustedSk any more, which removes the previous concurrent-verify reorder/use-after-free on those stacks), but the shared-store verification path as a whole is still not thread safe.

Recommendation: For consistency with the store->certs treatment, either snapshot store->trusted the same way at the top of wolfSSL_X509_verify_cert() and pass the snapshot into X509StoreCertIsTrusted() and the terminal issuer lookup, or - preferably - introduce a single WOLFSSL_X509_STORE-level mutex guarding certs, trusted, and owned, taken by wolfSSL_X509_STORE_add_cert() and by the snapshot/lookup paths here. Until then, promote the store->cm NOTE at src/x509_str.c:972-979 into the public API documentation for X509_STORE/X509_verify_cert so callers know a store must not be shared across concurrently verifying threads.

Expand Down Expand Up @@ -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;
Expand All @@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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
💡 SUGGEST question

Moving the PARTIAL_CHAIN trust check onto its own snapshot addresses the round-1 comment, but taking that snapshot from callerTrusted again re-reads the shared stack a second time:

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 X509_STORE_add_cert(store, C) between them, thread A verifies with a trust snapshot containing C while its working stack does not - and the PARTIAL_CHAIN fallback (1028-1032) can then accept a chain terminating at C even though C was never a chain-building candidate for this verification. The reverse interleaving rejects a terminus the verification did consider. That makes the trust decision depend on timing rather than on store contents at any single instant, and it is the one part of this that is free to fix: origTrustedSk needs the caller's set before addAllButSelfSigned() injects intermediates, which is exactly what certsToUse already holds at that point. Duplicating from the private copy removes the second shared-list walk and makes the two sets provably identical.

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:

  • The dup is itself an unlocked read. WOLFSSL_X509_STORE has no mutex over certs/trusted, and wolfSSL_sk_insert() bumps stack->num before linking the new node, so a concurrent add_cert can be observed mid-update (harmless today - wolfSSL_X509_check_issued() is NULL-safe on the trailing indices). X509StorePushCertsToCM() pop-frees the whole stack and NULLs it, which the walk cannot survive at all. So the write-side corruption is genuinely gone, but the comment at 865-878 and the PR/commit wording read as if the race is closed; they should say what the NOTE at 972-979 says.
  • origTrustedSk's only consumer is X509StoreCertIsTrusted() inside the PARTIAL_CHAIN branch, and both flag sources are readable at entry - so the snapshot could be taken only when the flag is set. (Not a new allocation class: the deleted code also built one node per store cert.)
  • Minor: origTrustedSk is NULL-checked inside the if (callerTrusted != NULL) block while certsToUse is checked by the shared test below. The result is correct - both reach goto exit and the exit path shallow-frees whichever succeeded - but it reads as if only one allocation is checked, and the second dup is still attempted after the first has failed.

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) {
Comment thread
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;
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand All @@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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
💡 SUGGEST test

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 (X509StoreCertInStack(...) || 1) leaves the entire tests/unit.test suite green, and an instrumented build that prints whenever the guard suppresses a push records zero firings across the ossl_x509_store group. So nothing in tree exercises it.

That matches the reachability analysis: failedCerts is only ever fed from certsToUse, a shallow dup of ctx->setTrustedSk or ctx->store->certs. In the default path the terminal lookup searches ctx->store->trusted, and wolfSSL_X509_STORE_add_cert() sorts self-signed certs into store->trusted and non-self-signed into store->certs - disjoint sets, so pointer identity can never match. The guard can only fire with setTrustedSk in use and the chain terminating through the done = 1 CertManager path. test_untrusted_inter_trusted_stack_unchanged terminates through the self-issued ctx->current_cert == issuer break instead, and asserts only the caller stack, never X509_STORE_CTX_get0_chain().

Also worth a word in the comment: under WOLFSSL_SIGNER_DER_CERT the guard is a silent no-op, since x509GetIssuerFromCM() returns a freshly allocated X509 that can never be pointer-equal to a failedCerts entry.

When it does fire, done = 1 still runs and the function reports success with ctx->chain missing its top element. X509StoreCheckPathLen() (same file, 764-840) treats sk_X509_value(ctx->chain, num - 1) as the trust anchor - it seeds the budget from that cert and skips it in the num-2 .. 1 loop. With the anchor suppressed, the last intermediate is mistaken for the anchor and is never charged against the path-length budget; if the chain drops below three entries the check returns early altogether. Separately, callers reading the last element of X509_STORE_CTX_get1_chain() to identify the anchor now get an intermediate. The verdict is unaffected either way (ctx->current_cert was verified against the CM before this point), so this is not a bypass - but the reported chain ends up inconsistent with both its internal consumer and the OpenSSL contract.

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);
}

Expand Down Expand Up @@ -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) {
Expand All @@ -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
Expand Down
Loading
Loading