Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Signature-failure overrides remain cacheable, and selective CA overrides can now break leaf verification within the same handshake.
2 open issues
What changed in this PR
Prevents certain certificate verification overrides from adding untrusted chain CAs to wolfSSL’s shared trust cache.
Changes:
- Preserves the pre-callback error to block both cache-admission paths.
- Adds and registers a regression test checking trust before and after an overridden handshake.
| File | Description |
|---|---|
| tests/api/test_ssl_cert.h | Declares and registers the regression test. |
| tests/api/test_ssl_cert.c | Tests that an overridden handshake does not persist trust. |
| src/internal.c | Blocks caching for missing-signer and self-signed errors. |
Review effort: Balanced
|
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11830
Scan targets checked: wolfssl-src, wolfssl-bugs
Not reviewed (no scan target covers them): doc/dox_comments/header_files/ssl.h, tests/api/test_ssl_cert.h
Coverage: 2 of 2 in-scope changed file(s) opened by the reviewer
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
Review tier: Lite
| * runs on an error. The errors left out here were raised | ||
| * after the signature verified, so the key is authentic and | ||
| * the rest of the chain needs the cached entry. */ | ||
| if (preCbRet == WC_NO_ERR_TRACE(ASN_NO_SIGNER_E) || |
There was a problem hiding this comment.
Skip-cache denylist misses pre-signature errors such as ASN_PATHLEN_INV_E, so an override still caches an… · Certificate and trust chain validation bypass
ParseCertRelative returns ASN_PATHLEN_INV_E (asn.c:26084/26118) and hash errors such as HASH_TYPE_E before ConfirmSignature checks the signature, and none of these is in the four-code list. When a callback overrides one of them, skipAddCA stays 0 and AddCA stores the forged CA in the context-wide CertManager as a trust anchor for later connections.
Suggested fix: Use an allowlist instead: cache only when the signature was confirmed (record a sig-verified flag in DecodedCert), or treat every nonzero preCbRet as skip except errors known to come after the signature check.
Basis: RFC 5280 section 6.1.3(a)(1): verifying the certificate signature with the working issuer public key is a required step of basic path validation, so a certificate whose signature was not verified must not become a trust anchor.
Related known findings (similar but distinct; listed for context, not part of this finding)
- F-16248: that finding covers the ordering in asn.c
ParseCertRelative, where the path-length check runs before the signature check inside one handshake. This finding is about the new skip condition in internal.cProcessPeerCerts, which still lets that unauthenticated CA be cached for the whole context. - F-14457 (assigned): File/function: both are in the ProcessPeerCerts chain-CA loop, around DoVerifyCallback and AddCA. Faulting operation: both end with AddCA caching a peer-supplied chain CA in the shared CertManager after a callback override. F-14457 reports that no skip exists, so any overridden error is cached. The candidate reports that the skip condition (skipAddCA, checked against the four-code denylist) misses pre-signature errors such as ASN_PATHLEN_INV_E and HASH_TYPE_E. Root cause: F-14457 is an override th
There was a problem hiding this comment.
Confirmed. Both return ASN_PATHLEN_INV_E sites sit inside if (type != CERT_TYPE && cert->isCA && cert->ca && ...) and run ahead of ConfirmSignature(), so the issuer is resolved, isCA is set, AddCA() is reachable and the signature was never examined.
Your structural point is the one that mattered. This was the third code the list was missing, so rather than add a fourth, the enumeration is gone. ParseCertRelative() now sets DecodedCert.sigVerified once every signature the certificate needed, including a dual-algorithm certificate's alternative signature, has been confirmed against the issuer it found, and ProcessPeerCerts() caches a chain CA only when that flag is set. That also covers the return ret at the Renesas TSIP/FSPSM key-index decode, which is likewise ahead of the signature check. Under WOLFSSL_SMALL_CERT_VERIFY the flag is taken from wc_CheckCertSignature(), since ParseCertRelative() runs with VERIFY_NAME there.
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
A chain certificate that fails verification with either a missing signer or a self-signed chain is not rooted in a trusted certificate. If the application's verification callback overrides the error, the certificate is still added to the context's certificate manager as a chain CA. Since these entries act as trust anchors for all subsequent connections using the same context, and the verification callback is only invoked on errors, the application is never asked to verify that certificate again. Record the verification result before invoking the callback and skip adding the certificate for these two error codes. This prevents caching through both the AddCA path and the pending CA path for Certificate Status Request v2. Other errors remain unchanged because the certificate is rooted in a trusted anchor, and the rest of the chain is verified against the cached entry.
The verify callback for the chain CA override test sat outside the build condition that guards the test body. A configuration that compiles the test out, such as --enable-leantls, which defines WOLFSSL_NO_TLS12, left the callback defined but unused and the build failed on -Werror=unused-function. Name the condition once and guard both the callback and the test body with it so the two cannot drift apart.
A chain certificate can reach the verify callback with its signature unconfirmed in more ways than a missing signer. A forged certificate that names a loaded root as its issuer is looked up successfully and then fails ConfirmSignature, and a signature algorithm that does not match the issuer key fails before the signature is checked at all. Neither certificate has been authenticated, so neither may be cached when the callback clears the error. Add both error codes to the set that skips the add, and correct the comment: the question is whether the signature verified against a trusted anchor, not whether an issuer was found. The errors left out are raised after the signature verified, so the key is authentic and the rest of the chain needs the cached entry. Add a regression test that presents an intermediate carrying the test root's issuer name and authority key identifier but signed with a key the root does not hold.
check-api-guards.py matches the enclosing #if chain as text, so naming the conditions in a macro hid NO_FILESYSTEM and the other guards from it.
Accepting a chain CA that is not in the trust store now decides that one certificate rather than the rest of the chain, because the CA is no longer cached. Pin that: a callback accepting only the chain CA ends in ASN_NO_SIGNER_E on the leaf, the same chain completes when the callback accepts the leaf as well, and the CA is not a trust anchor afterwards either way. Record the scope of an override in the verify callback documentation, which described the mode flags only.
Enumerating the error codes that mean a chain certificate's signature was never confirmed left gaps. ASN_PATHLEN_INV_E is one: the path length check returns it before ConfirmSignature runs, so an override cached a certificate nothing had authenticated. Record the outcome instead. ParseCertRelative sets a flag once every signature the certificate needed has been confirmed against the issuer it found, and a chain CA is cached only when that flag is set. Errors raised after the signature verified still cache, so a certificate waived for its OCSP status, extended key usage, dates or revocation behaves as before. Under WOLFSSL_SMALL_CERT_VERIFY the signature is checked separately by wc_CheckCertSignature, so the flag is taken from that result there. The flag joins the existing bitfield run, leaving sizeof(DecodedCert) unchanged.
Accepting a chain certificate whose signature was never confirmed does not complete a handshake that requested OCSP stapling. The staple is verified against the certificate manager, which does not hold the accepted certificate, so the responder's own certificate cannot be verified and the handshake ends with a fatal alert the verify callback never sees. A live OCSP lookup fails for the same reason but its error does reach the callback. Name the one hook that can accept the stapled case. The text also described the override without saying what happens without one, so say that first.
The text said an acceptance applied to the handshake in progress only. That holds for a certificate whose signature was never confirmed, which is not cached, but not for one whose signature was confirmed and which failed a later check: the date, revocation, OCSP and key usage checks all run after the signature verified, so an expired or revoked intermediate waved through still becomes a trust anchor for later connections from the same context. Say which case is which instead.
6ad7735 to
7974c4a
Compare
|
@claude review |
|
retest this please |
1 similar comment
|
retest this please |
There was a problem hiding this comment.
🟡 Changes recommended
A critical issue allows a dual-algorithm CA into the shared cache without verifying its alternative signature when a path-length error is overridden with both certificate features enabled.
1 open issue
2 resolved since last review
Review effort: Balanced
| /* ParseCertRelative() ran with VERIFY_NAME and left the signature to | ||
| * wc_CheckCertSignature() above, so it could not record the result. */ | ||
| if (args->dCert != NULL) { | ||
| args->dCert->sigVerified = (verify == VERIFY_NAME) && (sigRet == 0); |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11830
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-src, wolfssl-bugs
Not reviewed (no scan target covers them): doc/dox_comments/header_files/ssl.h, tests/api/test_ssl_cert.h
Coverage: 3 of 3 in-scope changed file(s) opened by the reviewer
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
Do not cache chain CAs whose signature was never confirmed - F14457
Problem
In
ProcessPeerCerts(), a chain certificate can reach the verify callback with its signature unconfirmed in several ways: no signer was found, the certificate was self-signed, the signature failed against the issuer that was found, the signature algorithm did not match the issuer key, or a check that runs ahead of the signature (the RFC 5280 6.1.4(l) path length check, for one) returned first. When the application's verify callback overrides that error,DoVerifyCallback()clearsretand the certificate was added to the context's certificate manager byAddCA(..., WOLFSSL_CHAIN_CA, NO_VERIFY).signer->typeis only read byFreeSignerTableType(), so aWOLFSSL_CHAIN_CAentry is indistinguishable from an explicitly loaded trust anchor during verification. BecauseSSL_CM(ssl)is theWOLFSSL_CTX-wide certificate manager, the certificate became a trust anchor for every later connection from that context. Those connections verify successfully, andDoVerifyCallback()only invokes the callback on an error, so the application was never asked about that certificate again. Only an explicitwolfSSL_CTX_UnloadIntermediateCerts()removed the entry.The
WOLFSSL_ALT_CERT_CHAINSand Apple native validation bypasses already refuse to cache for two of those error codes. The verify callback path refused for none of them.Fix
The first attempt listed the error codes that mean the signature was never confirmed. That list could not be kept complete:
ParseCertRelative()has around thirty return points ahead ofConfirmSignature(), and hardware ports add their own.ASN_PATHLEN_INV_Eis one that the list missed, returned from the path length check with the issuer already resolved and the signature never examined.So the result is recorded rather than inferred.
ParseCertRelative()setsDecodedCert.sigVerifiedonce every signature the certificate needed, including the alternative signature of a dual-algorithm certificate, has been confirmed against the issuer it found.ProcessPeerCerts()then caches a chain CA only when that flag is set:This covers both the
AddCA()path and theProcessPeerCertAddPendingCA()path used underHAVE_CERTIFICATE_STATUS_REQUEST_V2, and it does not have to be revisited when a new error code is added ahead of the signature check.Errors raised after the signature verified leave the flag set and still cache, because the chain CA cache is also the chain building workspace within a single handshake:
ParseCertRelative()finds issuers only throughGetCA(), so refusing to cache an intermediate ends the chain there. A certificate waived for its OCSP status, extended key usage, dates or revocation is rooted in a trusted anchor, so caching it creates no new trust root, andtest_override_alt_cert_chainandtest_wolfSSL_chain_ca_ext_key_usagekeep passing.Under
WOLFSSL_SMALL_CERT_VERIFYthe signature is checked separately bywc_CheckCertSignature()andParseCertRelative()runs withVERIFY_NAME, so the flag is taken from that result inProcessPeerCertParse().Behavioral change
This changes what a verify callback can do with a chain CA that is not in the trust store. Accepting such a CA now decides that one certificate rather than the rest of the chain: the certificate it signed has no signer, so the callback is asked about that certificate in its turn instead of being told it verified. An application that means to accept the chain accepts each certificate, which completes the handshake exactly as before.
Unaffected: applications with no verify callback, applications using
WOLFSSL_VERIFY_NONE, and callbacks that accept a chain CA the context already trusts, which is the common case of a server resending its root.How far an acceptance reaches now depends on why the certificate was rejected, and the documentation says so. A chain CA whose signature was confirmed and which failed a later check is still cached, so accepting an expired or revoked intermediate still makes it a trust anchor for later connections from the same context and the callback is not asked about it again. That is deliberate: the date, revocation, OCSP and key usage checks all run after the signature verified, and refusing to cache for them ends the chain inside the same handshake, which is what regressed
test_override_alt_cert_chainandtest_wolfSSL_chain_ca_ext_key_usagewhen tried.The previous behavior reported
preverify_okat depth 0 for a leaf verified against a key the application had just waved through, so the check the application believed it was making at depth 0 was relative to an unauthenticated anchor.OCSP is the one case the verify callback does not resolve. A stapled response is verified against the certificate manager, which no longer holds the accepted certificate, so the responder's own certificate cannot be verified:
ProcessCSR_ex()returnsBAD_CERTIFICATE_STATUS_ERRORandDoCertificateStatus()sends a fatal alert without consulting the verify callback. OnlyocspStatusVerifyCb, set withwolfSSL_CTX_SetOCSP_StatusVerifyCb(), can accept that result. Measured on one build with only the library sources swapped: a client that accepts every certificate in an untrusted chain and requests stapling completes the handshake before this change and ends withBAD_CERTIFICATE_STATUS_ERRORafter it. A live OCSP lookup fails for the same reason, but its error reaches the verify callback and is accepted there, so that path completes on both. Such a configuration was asking a responder authorised by a key the application itself waved through, so the staple was never meaningful assurance, but the failure is abrupt and the callback does not see it. Restoring the handshake-scoped availability of accepted chain CAs, by way of theOcspResponse.pendingCAsmechanism that Certificate Status Request v2 already uses, is tracked separately.Test
Three tests in
tests/api/test_ssl_cert.c, each failing before the fix:test_wolfSSL_chain_ca_not_cached_on_overridepresentscerts/intermediate/server-chain-short.pemto a client that trusts only the test root, so the chain certificate has no signer. The samewolfSSL_CertManagerVerify()call on the leaf runs before and after the handshake and must fail both times.test_wolfSSL_forged_chain_ca_not_cachedpresents an intermediate carrying the test root's issuer name and authority key identifier but signed with a key the root does not hold, so the signer lookup succeeds andConfirmSignature()does not.test_wolfSSL_chain_ca_override_is_handshake_scopedpins the behavioral change above: a callback that accepts only the chain CA ends inASN_NO_SIGNER_Eon the leaf, the same chain completes when the callback accepts the leaf as well, and the CA is not a trust anchor afterwards in either case.make checkwith no failures on:./configure --enable-ocsp --enable-crl --enable-opensslextra(WOLFSSL_ALT_CERT_CHAINSoff, the configuration the new code actually runs in): 7 passed, 7 skipped, 0 failed./configure --enable-all(WOLFSSL_ALT_CERT_CHAINSon): 17 passed, 7 skipped, 0 failedAlso built and run clean on
./configure(default), on--enable-all --enable-static --disable-shared CPPFLAGS="-DWOLFSSL_TEST_STATIC_BUILD", and on--enable-leantls --enable-session-ticket --enable-sni --enable-opensslextra.tests/api/check-api-guards.py origin/masterreports the changed call sites covered. Skipped suites are the ones needing external network or anopensslbinary.Checklist