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
- 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. - certmanIsSelfIssued() reports whether a cert's issuer and subject name hashes match. - 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. - test_CertMan_PromoteValidCaIntermediate() covers a self-issued intermediate with and without keyCertSign, and a chain that ends in the trusted root.
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
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. Introduced ine2b7ad5d; not in any release.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)The self-issued cases first confirm that the forged cert's issuer and subject name hashes match.
Verification
make check14/14 against wolfSSL v5.9.4 (--enable-all); gcc-13 preflight clean across 6 configs.