Conversation
|
JacobBarthelmeh
requested review from
wolfSSL-Fenrir-bot
and
a lite review from Copilot
September 18, 2026 18:09
wolfSSL-Fenrir-bot
previously requested changes
Sep 18, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11500
Scan targets checked: wolfssl-src, wolfssl-bugs
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.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11500
Scan targets checked: wolfssl-src, wolfssl-bugs
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
wolfSSL-Fenrir-bot
dismissed
their stale review
September 18, 2026 19:26
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
JacobBarthelmeh
previously approved these changes
Sep 18, 2026
A client with both OCSP and CRL enabled skipped the CRL check for a certificate carrying no OCSP responder URL. OcspNoUrlPolicy() maps OCSP_NO_URL onto 0 for the historical soft-fail default, which is indistinguishable from a responder answering CERT_GOOD, so the CRL fallback never ran. Decide whether the CRL check is still needed from the raw OCSP result, before the no-URL policy collapses it, for both the leaf and the chain. A chain certificate whose OCSP lookup was handed to a staple is the one case where that result is not in yet, since the CertificateStatus entry answering it has not been parsed. Take the CRL check there anyway, while the DecodedCert is alive, but keep only CRL_CERT_REVOKED from it: an inconclusive CRL_MISSING would otherwise reject a certificate whose valid staple is still on its way, which no CRL ever said anything about. The tests are gated out under WOLFSSL_CRL_ALLOW_MISSING_CDP, where a certificate naming no distribution point has its CRL check skipped by design and the in-tree test certificates name none. test_ocsp_checkall_staple_crl_missing covers both directions with a fully stapled chain under OCSP and CRL CHECKALL: an empty CRL store no longer rejects it, and certs/ocsp/root-ca-crl-revoked.pem, which gencrls.sh now produces beside the other OCSP CRLs, still does.
…t follow it Under WOLFSSL_SMALL_CERT_VERIFY, ProcessPeerCertParse() merged sigRet back only when the parse returned 0, so any parse error hid the signature result. ParseCertRelative(VERIFY) runs its checks in a fixed order: decode, signer lookup, ConfirmSignature(), name constraints, validity dates, critical extensions. That order is what makes overriding date errors a sound policy for a device with no real-time clock, since a date error can only reach the application for a certificate whose signature already verified. Splitting the signature check out inverted it. Let a non-zero sigRet take precedence over exactly those five errors that ParseCertRelative() only reaches after the signature has been confirmed. Errors raised before that point keep priority as they do in a full verify, a parse error is still reported unchanged when the signature was good, and the raw public key exemption for the leaf is unaffected. The test builds a leaf naming the 2048-bit test root as its issuer, signed either by the root or by another key, in and out of its validity window, and runs a memio handshake with a date tolerant callback. It anchors the expected result to what wc_CheckCertSignature() makes of each leaf.
…sued for Without NO_SESSION_CACHE_REF, wolfSSL_get_session() hands back a ClientSession reference into the process-global cache rather than a session object, and ClientSessionToSession() resolves it against nothing but a hash of the entry's session ID. That ID does not change when the entry is overwritten, so a reference kept across an overwrite still resolved, to contents it was never issued for. Give every cache write a generation number and record it in the ClientSession, so a reference resolves only while the entry still holds the contents it was issued for. The numbers come from a counter on the SessionRow rather than from the entry, so that under SESSION_CACHE_DYNAMIC_MEM a freed and reallocated entry cannot restart at a value an outstanding reference still holds. The generation is not advanced when an overwrite carries the same master secret, the benign re-add of an unchanged session; without that an older reference would silently fall back to a full handshake. The comparison runs through ConstantCompare so its timing says nothing about the cached secret. WOLFSSL_CACHE_VERSION goes to 3: the ClientCache, the SessionRow prefix and the WOLFSSL_SESSION layouts written by the session-cache save functions all change. The test builds the sequence out of two real handshakes on separate contexts, behind two controls so it cannot pass by refusing everything. Every profile in the os-check matrix that builds the unit tests defines NO_SESSION_CACHE_REF, which compiles the reference path out, so a session-cache-ref entry configures one that does not.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Three independent hardening fixes from the same audit: two in certificate verification, one in the client session cache. They are unrelated beyond where they were found, so each commit stands alone and can be cherry-picked or dropped without touching the others.
Revocation checking. A peer certificate naming no OCSP responder skipped the CRL fallback, because the soft-fail policy collapsed the no-URL result onto success before the fallback decision was taken. That decision is now taken from the raw OCSP result, for both the leaf and the chain.
Certificate verification. Under
WOLFSSL_SMALL_CERT_VERIFYthe separate signature check was merged back only when the parse succeeded, so it could be reported as one of the later parse errors instead. It now takes precedence over exactly those errors thatParseCertRelative()reaches only after the signature is confirmed.Session cache. Without
NO_SESSION_CACHE_REF, a client cache reference resolved against the session ID alone. Cache writes now carry a generation number recorded in theClientSession, so a reference resolves only while the entry still holds the contents it was issued for.Behaviour changes
CRL_MISSINGinstead of completing.test_ocsp_no_url_crl_not_loadedcovers this.WOLFSSL_SMALL_CERT_VERIFY, a certificate whose signature does not verify now reports the signature error rather than a validity-date, name-constraint, path-length or critical-extension error raised later in the same parse.WOLFSSL_CACHE_VERSIONgoes to 3: theClientCache,SessionRowprefix andWOLFSSL_SESSIONlayouts written by the session-cache save functions all change.Thanks to Anthropic for the reports.