Add WOLFSSL_X509_TINY minimal-extension profile + WOLFSSL_X509_VERIFY_ONLY#10823
Add WOLFSSL_X509_TINY minimal-extension profile + WOLFSSL_X509_VERIFY_ONLY#10823aidangarske wants to merge 3 commits into
Conversation
|
retest this please |
…SSL_X509_VERIFY_ONLY + fail-closed cert test
372c75a to
a7aaa51
Compare
|
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 5 total — 2 posted, 3 skipped
2 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Info] Stale/misleading #endif comment on the reworked NAME_CONS_OID guard —
wolfcrypt/src/asn.c:21278 - [Info] Inconsistent preprocessor-directive indentation in reworked extension guards —
wolfcrypt/src/asn.c:21252-21310
Skipped findings
- [Medium]
WOLFSSL_X509_VERIFY_ONLY silently undefines an explicit user WOLFSSL_KEY_GEN/WOLFSSL_CERT_GEN - [Medium]
TINY hard-rejects any certificate carrying nameConstraints, including trusted CA certs - [Low]
Test coverage: cert_x509_tiny_test does not exercise VERIFY_ONLY or SAN-stripping/add-back paths
Review generated by Skoll
dgarske
left a comment
There was a problem hiding this comment.
| Config | __TEXT bytes |
vs baseline |
|---|---|---|
| baseline (neither macro) | 56,535 | - |
WOLFSSL_X509_VERIFY_ONLY |
53,747 | -2,788 (-4.9%) |
WOLFSSL_X509_TINY |
53,767 | -2,768 (-4.9%) |
| both | 50,979 | -5,556 (-9.8%) |
| static word32 SetAlgoIDImpl(int algoOID, byte* output, int type, int curveSz, byte absentParams); | ||
| #ifndef NO_CERTS | ||
| static int DecodeAltNames(const byte* input, word32 sz, DecodedCert* cert); | ||
| #if !defined(WOLFSSL_X509_TINY) || defined(WOLFSSL_X509_TINY_CRL_DP) |
There was a problem hiding this comment.
These new ASN options need to be documented at top of asn.c:
- WOLFSSL_X509_VERIFY_ONLY - drops cert/key generation and forces
WOLFSSL_NO_PEMunless a gen add-back (WOLFSSL_X509_CERT_GEN/WOLFSSL_X509_KEY_GEN) orWOLFSSL_X509_PEMis set. - WOLFSSL_X509_TINY - compiles out optional X.509 extension decoders behind per-feature
WOLFSSL_X509_TINY_<F>add-back macros. RequiresWOLFSSL_ASN_TEMPLATE(enforced with#error).
| /* Cert/key generation add-backs need the PEM helpers, so don't strip PEM. */ | ||
| #if !defined(WOLFSSL_X509_PEM) && !defined(WOLFSSL_NO_PEM) && \ | ||
| !defined(WOLFSSL_X509_CERT_GEN) && !defined(WOLFSSL_X509_KEY_GEN) | ||
| #define WOLFSSL_NO_PEM |
There was a problem hiding this comment.
WOLFSSL_X509_VERIFY_ONLY forces WOLFSSL_NO_PEM, which disables WOLFSSL_PEM_TO_DER (confirmed in settings.h). A verify-only TLS client that loads its trust anchors in PEM (wolfSSL_CTX_load_verify_locations / ..._buffer with ..._FILETYPE_PEM) loses PEM decoding and can only load DER anchors. There is an escape hatch - define WOLFSSL_X509_PEM (or a gen add-back) to keep PEM - but it is opt-in and non-obvious. This is a functional footgun to document prominently, not a verification-logic hole.
| break; | ||
| #endif /* !WOLFSSL_X509_TINY || WOLFSSL_X509_TINY_SKI */ | ||
|
|
||
| #if !defined(WOLFSSL_X509_TINY) || defined(WOLFSSL_X509_TINY_POLICIES) |
There was a problem hiding this comment.
TINY + WOLFSSL_NO_ASN_STRICT can silently accept a critical stripped extension (low severity, but an inconsistency). The fully removed cases - CRL-DP, certificatePolicies, inhibitAnyPolicy, policyConstraints, Netscape, OCSP no-check - fall through to default:, and default: only rejects a critical-unknown extension inside #ifndef WOLFSSL_NO_ASN_STRICT. So under WOLFSSL_NO_ASN_STRICT, a critical policyConstraints/inhibitAnyPolicy/CRL-DP is ignored rather than rejected. By contrast AKI/SKI/AIA (explicit stubs) and NAME_CONS reject unconditionally. The practical security impact is limited: wolfSSL parses policyConstraints/inhibitAnyPolicy only into flags (extPolicyConstRxpSet etc.) and never enforces the RFC 5280 6.1 policy tree in the verify path, so what is lost is the RFC 5280 4.2 "MUST reject an unrecognized critical extension" strictness, not active enforcement. Still, the inconsistent fail-closed behavior across the stripped set is a rough edge. Suggest either a build-time #warning/#error on WOLFSSL_X509_TINY && WOLFSSL_NO_ASN_STRICT, or give the fully-removed cases the same unconditional critical-reject stub as AKI/SKI/AIA.
There was a problem hiding this comment.
Fixed so when TINY is on, a critical extension it can't decode is always rejected with ASN_CRIT_EXT_E, regardless of NO_ASN_STRICT
…osed on critical stripped extensions under TINY
92c94ae to
c970a81
Compare
Two opt-in, macro-driven profiles for the template X.509 parser. WOLFSSL_X509_TINY strips optional extension/SAN decoding (with per-feature WOLFSSL_X509_TINY_* add-backs); WOLFSSL_X509_VERIFY_ONLY drives a verify-only footprint. Fail-closed on critical/nameConstraints, revocation locators forced when OCSP/CRL enabled. Default build byte-identical; includes a TINY cert test.
Saves ~3.4 KB off the reachable P-256 cert parser (asn.c __TEXT 59,673 -> 56,221 bytes, -Os; ~5.8%). with other size option we can get to aprox 18% down with
WOLFSSL_X509_TINY, WOLFSSL_X509_VERIFY_ONLY, IGNORE_NAME_CONSTRAINTS, WOLFSSL_NO_ASN_STRICT