Reject non-CA peer intermediate certs#1021
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds stricter rules for promoting peer-supplied intermediate certificates into the cert manager trust store, preventing promotion of non-CA intermediates and adding regression coverage for issue F-5851.
Changes:
- Introduces
CertManIntermediateIsCA()to gate intermediate promotion on BasicConstraints CA=TRUE (and KeyUsage keyCertSign when present). - Updates certificate chain verification to avoid trusting non-CA intermediates.
- Adds unit tests that reproduce the non-CA intermediate promotion attack and validate promotion of legitimate CA intermediates.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/unit.c | Adds regression/positive tests that exercise intermediate promotion behavior using forged ECC certificates. |
| src/certman.c | Adds CA-check helper and gates intermediate promotion during chain verification. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1021
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
- Add CertManIntermediateIsCA: require isCA and, for non-self-signed intermediates that carry a KeyUsage extension, the keyCertSign bit before promoting a cert. - Only promote a verified intermediate into the trust store when it is actually a CA; otherwise fail with WS_CERT_NO_SIGNER_E. - Prevents a peer-supplied end-entity cert at an intermediate position from being trusted to issue certs for arbitrary SSH principals. - Gate keyCertSign on ALLOW_INVALID_CERTSIGN and on the KeyUsage extension being present, matching wolfSSL's AddCA loader. - Add regression tests: non-CA intermediate is not promoted, and a valid CA intermediate (with and without KeyUsage) still is. Issue: F-5851
OCSP_NEED_URL meant the cert has no AIA OCSP URL and no default responder is set, so OCSP cannot run. Treat it as not-revoked instead of failing the whole verification.
| #if defined(WOLFSSH_TEST_INTERNAL) && defined(WOLFSSH_CERTS) && \ | ||
| defined(WOLFSSL_CERT_GEN) && !defined(WOLFSSH_NO_ECDSA) && \ | ||
| !defined(NO_FILESYSTEM) | ||
| #define WOLFSSH_TEST_CERTMAN_PROMOTE | ||
| #include <wolfssl/wolfcrypt/asn_public.h> | ||
| #include <wolfssl/wolfcrypt/ecc.h> | ||
| #include <wolfssh/certman.h> | ||
| #endif |
| else if (ret == OCSP_NEED_URL) { | ||
| /* The cert carries no OCSP responder URL and certman has | ||
| * no default responder configured, so OCSP cannot be | ||
| * performed. Treat as not revoked rather than failing the | ||
| * whole verification. */ |
| /* Append a length-prefixed cert (the framing VerifyCerts_buffer expects). | ||
| * chainCap is the capacity of chain; on insufficient space the chain is left | ||
| * unchanged so a cert-size regression surfaces as a failed assertion rather | ||
| * than memory corruption. The subtractions are ordered to avoid word32 | ||
| * underflow. */ |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1021
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
| WLOG(WS_LOG_CERTMAN, "ocsp lookup: ocsp revoked"); | ||
| ret = WS_CERT_REVOKED_E; | ||
| } | ||
| else if (ret == OCSP_NEED_URL) { |
There was a problem hiding this comment.
⚪ [Info] OCSP_NEED_URL softens revocation from hard-fail to soft-fail · Authentication bypass
OCSP_NEED_URL is now mapped to WS_SUCCESS, so any chain cert lacking an AIA OCSP URL (with no default responder) skips revocation checking despite WOLFSSL_OCSP_CHECKALL being enabled. Bounded impact since the AIA URL is signed and not attacker-strippable.
Fix: Confirm soft-fail is intended; optionally gate it behind a config flag or require a default OCSP responder to keep hard-fail enforcement.
Issue: F-5851