Skip to content

Fix partial chain verification #10170

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

Open
embhorn wants to merge 2 commits into
base: master
Choose a base branch
from
+95 −1
Open

Fix partial chain verification #10170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ac0e1f
Fix partial chain verification
embhorn Apr 8, 2026
4f904c6
Fix from review
embhorn Apr 8, 2026
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
59 changes: 58 additions & 1 deletion src/x509_str.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,6 +552,48 @@ static int X509VerifyCertSetupRetry(WOLFSSL_X509_STORE_CTX* ctx,
return ret;
}

/* Returns 1 if x509's DER matches an entry in either the head [0, origNum)
* slice of trustedCerts (which excludes intermediates 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. */
static int X509StoreCertIsTrusted(WOLFSSL_X509_STORE* store,
WOLFSSL_X509* x509, WOLF_STACK_OF(WOLFSSL_X509)* trustedCerts,
int origNum)
{
int i;
int n;
Copy link
Copy Markdown
Contributor

@dgarske dgarske Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [Low] Unused variable n declared at function scope
🔧 NIT style

The variable int n is declared at the top of X509StoreCertIsTrusted but is only used inside the if (store != NULL && store->trusted != NULL) block. While this follows C89 declaration-at-top style (common in the wolfSSL codebase), declaring n at the top and only assigning it conditionally may produce compiler warnings about uninitialized use on some static analysis tools. This is purely cosmetic since the usage is correct.

Recommendation: No action needed — this follows the existing C89-style convention in the codebase. If you want to silence picky analyzers, initialize n = 0 at declaration.

WOLFSSL_X509* cur;

if (x509 == NULL || x509->derCert == NULL)
return 0;

for (i = 0; i < origNum; i++) {
cur = wolfSSL_sk_X509_value(trustedCerts, i);
if (cur != NULL && cur->derCert != NULL &&
cur->derCert->length == x509->derCert->length &&
XMEMCMP(cur->derCert->buffer, x509->derCert->buffer,
x509->derCert->length) == 0) {
return 1;
}
}

if (store != NULL && store->trusted != NULL) {
n = wolfSSL_sk_X509_num(store->trusted);
for (i = 0; i < n; i++) {
cur = wolfSSL_sk_X509_value(store->trusted, i);
if (cur != NULL && cur->derCert != NULL &&
cur->derCert->length == x509->derCert->length &&
XMEMCMP(cur->derCert->buffer, x509->derCert->buffer,
x509->derCert->length) == 0) {
return 1;
}
}
}

return 0;
}

/* Verifies certificate chain using WOLFSSL_X509_STORE_CTX
* returns 1 on success or <= 0 on failure.
*/
Expand All @@ -565,6 +607,7 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx)
int numFailedCerts = 0;
int depth = 0;
int origDepth = 0;
int origCertsNum = 0;
WOLFSSL_X509 *issuer = NULL;
WOLFSSL_X509 *orig = NULL;
WOLF_STACK_OF(WOLFSSL_X509)* certs = NULL;
Expand All @@ -587,8 +630,14 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx)
wolfSSL_sk_X509_num(ctx->ctxIntermediates) > 0) {
certsToUse = wolfSSL_sk_X509_new_null();
ret = addAllButSelfSigned(certsToUse, ctx->ctxIntermediates, NULL);
/* certsToUse holds only injected intermediates, none are trusted. */
origCertsNum = 0;
}
else {
/* Snapshot the count of pre-existing entries before injecting the
* caller-supplied untrusted intermediates. Only the entries already
* present count as trusted for the partial-chain check below. */
origCertsNum = (certs != NULL) ? wolfSSL_sk_X509_num(certs) : 0;
/* Add the intermediates provided on init to the list of untrusted
* intermediates to be used */
ret = addAllButSelfSigned(certs, ctx->ctxIntermediates, &numInterAdd);
Expand Down Expand Up @@ -677,9 +726,17 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx)
* a trusted CA in the CM */
ret = X509StoreVerifyCert(ctx);
if (ret != WOLFSSL_SUCCESS) {
/* WOLFSSL_PARTIAL_CHAIN may only terminate the chain at a
* certificate the caller actually trusts. The previous
* "added == 1" guard merely confirmed that some untrusted
* intermediate had been temporarily loaded into the
* CertManager during chain building, which would accept
* chains that never reach a trust anchor. Verify that
* ctx->current_cert is itself in the original trust set. */
if (((ctx->flags & WOLFSSL_PARTIAL_CHAIN) ||
(ctx->store->param->flags & WOLFSSL_PARTIAL_CHAIN)) &&
(added == 1)) {
X509StoreCertIsTrusted(ctx->store, ctx->current_cert,
certs, origCertsNum)) {
wolfSSL_sk_X509_push(ctx->chain, ctx->current_cert);
ret = WOLFSSL_SUCCESS;
} else {
Expand Down
37 changes: 37 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 @@ -785,6 +785,42 @@ static int test_wolfSSL_X509_STORE_CTX_ex11(X509_STORE_test_data *testData)
return EXPECT_RESULT();
}

static int test_wolfSSL_X509_STORE_CTX_ex_partial_chain_neg(
Copy link
Copy Markdown
Contributor

@dgarske dgarske Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Medium] No test for mixed trusted-store + untrusted-chain with partial chain
💡 SUGGEST test

The new negative test verifies that partial-chain fails when no certificate is trusted (empty store + untrusted intermediates). However, there is no corresponding test for the mixed scenario: a store that contains some trusted intermediates combined with additional untrusted intermediates provided via the chain argument. This scenario exercises the origCertsNum boundary logic most directly. The existing positive tests (ex10, ex11) place intermediates in the store but do not provide extra untrusted intermediates through X509_STORE_CTX_init, so the boundary between trusted and untrusted entries in certs is never tested.

Suggestion:

Suggested change
static int test_wolfSSL_X509_STORE_CTX_ex_partial_chain_neg(
Add a test where one intermediate (e.g., x509CaInt) is added to the store, another (e.g., x509CaInt2) is passed as untrusted, PARTIAL_CHAIN is set, and verify: (a) the chain verifies successfully because x509CaInt is trusted, and (b) if *only* x509CaInt2 is in the store and x509CaInt is untrusted, verify the chain still succeeds with the correct terminal cert.

X509_STORE_test_data *testData)
{
EXPECT_DECLS;
X509_STORE* store = NULL;
X509_STORE_CTX* ctx = NULL;
STACK_OF(X509)* untrusted = NULL;

/* Negative partial-chain test: with X509_V_FLAG_PARTIAL_CHAIN set, the
* intermediates are supplied ONLY as untrusted (passed through the
* X509_STORE_CTX_init "chain" argument and never added to the store).
* No certificate in the chain is in the store, so verification must
* fail. Pre-fix, wolfSSL_X509_verify_cert would incorrectly accept
* this chain because its partial-chain fallback only checked that some
* intermediate had been temporarily loaded into the CertManager, not
* that any chain certificate was actually trusted. */
ExpectNotNull(store = X509_STORE_new());
/* Intentionally do NOT add x509CaInt, x509CaInt2, or x509Ca. */
ExpectIntEQ(X509_STORE_set_flags(store, X509_V_FLAG_PARTIAL_CHAIN), 1);

ExpectNotNull(untrusted = sk_X509_new_null());
ExpectIntGT(sk_X509_push(untrusted, testData->x509CaInt2), 0);
ExpectIntGT(sk_X509_push(untrusted, testData->x509CaInt), 0);

ExpectNotNull(ctx = X509_STORE_CTX_new());
ExpectIntEQ(X509_STORE_CTX_init(ctx, store, testData->x509Leaf, untrusted),
1);
/* Must NOT verify: partial-chain does not relax the trust requirement. */
ExpectIntNE(X509_verify_cert(ctx), 1);

X509_STORE_CTX_free(ctx);
X509_STORE_free(store);
sk_X509_free(untrusted);
return EXPECT_RESULT();
}

#ifdef HAVE_ECC
static int test_wolfSSL_X509_STORE_CTX_ex12(void)
{
Expand Down Expand Up @@ -870,6 +906,7 @@ int test_wolfSSL_X509_STORE_CTX_ex(void)
ExpectIntEQ(test_wolfSSL_X509_STORE_CTX_ex9(&testData), 1);
ExpectIntEQ(test_wolfSSL_X509_STORE_CTX_ex10(&testData), 1);
ExpectIntEQ(test_wolfSSL_X509_STORE_CTX_ex11(&testData), 1);
ExpectIntEQ(test_wolfSSL_X509_STORE_CTX_ex_partial_chain_neg(&testData), 1);
#ifdef HAVE_ECC
ExpectIntEQ(test_wolfSSL_X509_STORE_CTX_ex12(), 1);
#endif
Expand Down
Loading