X509_verify_cert: avoid mutating shared store cert stack

[julek-wolfssl]

wolfSSL_X509_verify_cert built certificate chains directly on
ctx->store->certs, which is shared by every connection using the
store, with no locking. Caller intermediates were pushed onto it,
retries reordered it, and injected entries were removed again on
exit, so concurrent verifications could race and corrupt the shared
stack.

Now the verification works on a per-verification shallow copy of the
cert stack instead, leaving store->certs and setTrustedSk
read-only.

Fixes F-7229

[Copilot]

Pull request overview

This PR fixes a thread-safety/correctness issue in wolfSSL_X509_verify_cert() where verification previously built chains by mutating ctx->store->certs (a shared stack across connections) and sometimes ctx->setTrustedSk (caller-owned). The updated implementation performs chain building on a per-verification shallow copy of the trusted stack so shared/caller-owned stacks remain read-only.

Changes:

• Update wolfSSL_X509_verify_cert() to build and mutate certificate chains on a per-call shallow-duplicated stack instead of store->certs / setTrustedSk.
• Remove now-unneeded cleanup logic that attempted to “undo” intermediate injection and retry reordering on the shared stack.
• Add an API test that asserts store->certs contents and order remain unchanged after both successful and failing verifications (including a retry-path scenario).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
tests/api/test_ossl_x509_str.c	Adds coverage ensuring store->certs remains unmodified (contents + order) across verification, including retry behavior.
src/x509_str.c	Refactors verification to use a per-verification shallow copy of the trusted stack, avoiding mutation of shared/caller-owned stacks.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

retest this please

[github-actions]

MemBrowse Memory Report

gcc-arm-cortex-m4

• FLASH: .text +64 B (+0.0%, 200,917 B / 262,144 B, total: 77% used)

gcc-arm-cortex-m4-openssl-compat

• FLASH: .text +64 B (+0.0%, 771,092 B / 1,048,576 B, total: 74% used)

gcc-arm-cortex-m4-pkcs7

• FLASH: .text +64 B (+0.0%, 213,908 B / 262,144 B, total: 82% used)

gcc-arm-cortex-m4-rsa-only

• FLASH: .text -64 B (-0.0%, 326,104 B / 1,048,576 B, total: 31% used)

gcc-arm-cortex-m4-tls13

• FLASH: .text -64 B (-0.0%, 236,615 B / 262,144 B, total: 90% used)

gcc-arm-cortex-m7-pq

• FLASH: .text -64 B (-0.0%, 280,512 B / 1,048,576 B, total: 27% used)

gcc-arm-cortex-m7-tls13

• FLASH: .text -64 B (-0.0%, 236,679 B / 262,144 B, total: 90% used)

linuxkm-standard

• Data: __patchable_function_entries +16 B (+0.0%, 48,992 B)

stm32-sim-stm32h753

• FLASH: .text +1,792 B (+1.0%, 185,684 B / 2,097,152 B, total: 9% used)
  No memory changes detected for:
• gcc-arm-cortex-m0plus
• gcc-arm-cortex-m3
• gcc-arm-cortex-m4-baremetal
• gcc-arm-cortex-m4-crypto-only
• gcc-arm-cortex-m4-dtls13
• gcc-arm-cortex-m4-min-ecc
• gcc-arm-cortex-m4-pq
• gcc-arm-cortex-m4-sp-math
• gcc-arm-cortex-m4-tls12
• gcc-arm-cortex-m7
• linuxkm-pie

[Frauschi]

🐺 Skoll Code Review

Overall recommendation: APPROVE
Findings: 11 total — 5 posted, 6 skipped

Posted findings

• [Medium] Trusted-stack test does not cover the retry/reorder case the removed cleanup guarded against — tests/api/test_ossl_x509_str.c:1538-1577
• [Medium] setTrustedSk is no longer pruned by retries, so a failed candidate can now be appended to the reported chain — src/x509_str.c:851
• [Medium] origTrustedSk now aliases the live shared store->certs instead of a private snapshot — src/x509_str.c:850
• [Low] certs is now a redundant alias for certsToUse — src/x509_str.c:874
• [Low] Shared-state race only partly closed: verification still adds/removes temp CAs on the shared store->cm — src/x509_str.c:849

Skipped findings

• [Medium] Copy-on-verify fixes the stack race but X509_verify_cert is still not safe for concurrent use of one store
• [Low] Every verification now allocates one stack node per store certificate
• [Low] Shallow copy of store->certs is taken without synchronization, freezing a possibly inconsistent (num, node list) pair
• [Info] Verification still mutates the store's shared CertManager, so the shared-store race is only partly closed
• [Info] New regression test asserts the single-threaded invariant only; no concurrent verification coverage
• [Info] wolfSSL_shallow_sk_dup() walks the shared store->certs list without holding a lock

Review generated by Skoll via Claude/Codex

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[Frauschi]

Jenkins retest this please

[Frauschi]

🐺 Skoll Code Review

Overall recommendation: APPROVE
Findings: 6 total — 5 posted, 1 skipped

Posted findings

• [Medium] The new failedCerts guard is narrowly reachable and untested, and when it does fire it drops the anchor off the reported chain — src/x509_str.c:1071-1079
• [Medium] Both shallow dups re-walk the shared store stack, so the two snapshots can disagree - take the second one from certsToUse — src/x509_str.c:865-898
• [Low] X509StoreCertInStack walks nodes without bounding by wolfSSL_sk_X509_num() — src/x509_str.c:651-668
• [Low] Both new negative sub-cases assert only the return value, not ctx->error — tests/api/test_ossl_x509_str.c:1618-1627
• [Info] Snapshot mitigation is asymmetric: store->trusted and setTrustedSk are still walked live during verification — src/x509_str.c:729-735,1062-1069

Skipped findings

• [Info] X509_STORE_CTX_get1_issuer() from a verify callback no longer sees caller-supplied intermediates

Review generated by Skoll via Claude/Codex

[Frauschi]

🟡 [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.

[Frauschi]

🟡 [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.

[Frauschi]

🔵 [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;
	}

[Frauschi]

🔵 [Low] Both new negative sub-cases assert only the return value, not ctx->error
🔧 NIT test

The two failing verifications added by this PR assert ExpectIntEQ(X509_verify_cert(ctx), 0) and then re-check the caller stack, but never pin the error. Every sibling negative test in this file also does ExpectIntNE(X509_STORE_CTX_get_error(ctx), X509_V_OK) - see test_untrusted_inter_tampered, test_untrusted_inter_reused_store, test_untrusted_inter_no_stale_anchor, test_untrusted_inter_retry.

It matters more than usual here because of the path these two drive: X509VerifyCertSetupRetry() pops an already-empty ctx->chain, leaves ctx->current_cert == NULL, and the next X509StoreGetIssuerEx() bails to exit with WOLFSSL_FATAL_ERROR. That is exactly the path most likely to return failure with a stale or unset ctx->error. test_untrusted_inter_store_stack_unchanged (1753-1757) has the same gap.

Suggestion:

Suggested change

	/* Failing verification on the same trusted stack: no genuine issuer. */
	ExpectIntEQ(X509_verify_cert(ctx), 0);
	ExpectIntNE(X509_STORE_CTX_get_error(ctx), X509_V_OK);
	X509_STORE_CTX_free(ctx);

[Frauschi]

⚪ [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.