Repository navigation
certman: require keyCertSign on self-issued peer intermediates - #1294
Open
yosuke-wolfssl wants to merge 1 commit into
Open
yosuke-wolfssl wants to merge 1 commit into
yosuke-wolfssl wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused security fix is consistent with certificate validation requirements and has targeted regression coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Strengthens certificate-chain validation by enforcing keyCertSign for self-issued peer intermediates.
Changes:
- Removes the unsafe
selfSignedexemption. - Adds self-issued certificate generation and regression coverage.
- Covers peer chains that include the trusted root.
| File | Description |
|---|---|
src/certman.c |
Enforces keyCertSign for all peer intermediates with KeyUsage. |
tests/unit.c |
Tests valid and invalid self-issued intermediates and appended roots. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1294
Scan targets checked: wolfssh-src, wolfssh-bugs
Coverage: 2 of 2 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
- CertManIntermediateIsCA() demotes a CA intermediate whose KeyUsage extension lacks keyCertSign whether or not it is self-issued. - certmanForgeCert() takes the subject name from issuerCert when cn is NULL, and with WOLFSSL_CERT_EXT sets the subject and authority key ids. - certmanIsSelfIssued() reports whether a cert's issuer and subject name hashes match; tests/unit.c includes asn.h for it. - certmanCheckIntermediate() gains selfIssued, which gives the intermediate the root's subject name and fails with -935 when the forged cert is not self-issued, and appendRoot, which also sends the trusted root at the end of the chain. A promoted case passes only on WS_SUCCESS or WS_CERT_PROFILE_E. - test_CertMan_PromoteValidCaIntermediate() covers a self-issued intermediate with and without keyCertSign, and a chain that ends in the trusted root.
yosuke-wolfssl
force-pushed
the
fix/certman
branch
from
October 12, 2026 23:58
6c55a8e to
90b361d
Compare
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.
Problem
CertManIntermediateIsCA()skipped the keyCertSign check for any peer intermediate withDecodedCert.selfSignedset. In released wolfSSL that flag is only an issuer-name == subject-name compare, so a self-issued intermediate (for example a CA key rollover cert signed by its parent's key) whose KeyUsage lacks keyCertSign was promoted into the trust store and could then sign leaves. This check is the only keyCertSign gate for peer intermediates, since they are loaded asWOLFSSL_USER_CA, which wolfSSL does not check. Exploiting it needs a trusted CA to have issued such a cert, so this closes a policy gap rather than an open hole.Fix (
src/certman.c)CertManIntermediateIsCA()no longer exemptsselfSigned: every promoted intermediate whose KeyUsage extension is present must assert keyCertSign. The field is no longer read, so no wolfSSL version guard is needed.selfSignedthen means the cert's own key verifies it), the exemption can return behind a version check.Tests (
tests/unit.c)WS_SUCCESS, orWS_CERT_PROFILE_Efrom the FPKI profile check), not just avoidWS_CERT_NO_SIGNER_E.Verification
make check14/14 against wolfSSL v5.9.4 (--enable-all); gcc-13 preflight clean across 6 configs;unit.testpasses against an RSA-off wolfSSL.