Fix: asn - reject critical certificatePolicies that exceed MAX_CERTPOL_NB in both back-ends - #11293
Fix: asn - reject critical certificatePolicies that exceed MAX_CERTPOL_NB in both back-ends#11293jackctj117 wants to merge 2 commits into
Conversation
Sorry, something went wrong.
|
Can one of the admins verify this patch? |
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
This pull request tightens X.509 certificatePolicies extension parsing to better align with RFC 5280 processing rules: if a critical certificatePolicies extension contains more policies than wolfSSL can store (MAX_CERTPOL_NB), the extension is now rejected (reported as ASN_CRIT_EXT_E) instead of being silently truncated.
Changes:
- Updated DecodeCertPolicy() in both ASN backends to accept a critical flag and reject over-limit critical certificatePolicies extensions (while still allowing over-limit non-critical ones).
- Adjusted DecodeExtensionType() to preserve ASN_CRIT_EXT_E for deferral/handling consistent with other unsupported critical extensions.
- Added API + whitebox tests to cover limit/over-limit behavior for both critical and non-critical cases.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file| File | Description |
|---|---|
| wolfcrypt/src/asn.c | Adds critical parameter to DecodeCertPolicy() (template backend) and propagates ASN_CRIT_EXT_E from DecodeExtensionType(). |
| wolfcrypt/src/asn_orig.c | Mirrors the critical handling + over-limit critical rejection logic for the non-template backend. |
| tests/unit-mcdc/test_asn_ext_whitebox.c | Updates whitebox calls for new signature and adds coverage for the post-loop critical rejection path. |
| tests/api/test_asn.h | Registers the new API test entry point. |
| tests/api/test_asn.c | Adds an API test that builds synthetic certificatePolicies values to validate at-limit and over-limit behavior for critical vs non-critical. |
wolfcrypt/src/asn.c:22110
- In DecodeCertPolicy() the loop condition compares absolute idx (offset into input) against total_length (content length returned by GetASN_Sequence). This mixes units and can cause malformed non-empty SEQUENCEs to be treated as having “no policies” (e.g., small total_length values), which can lead to accepting invalid non-critical extensions and misclassifying critical ones.
Use idx < sz (or idx < policyEnd) as the bound since GetASN_Sequence(..., complete=1) already enforces the SEQUENCE spans the entire input buffer.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
| /* Build a certificatePolicies extnValue with n PolicyInformation entries, | ||
| * each SEQUENCE { policyIdentifier OID 1.2.3.4.(k+1) }. Emits a short- or | ||
| * long-form outer length as needed so it stays valid DER for any n; returns | ||
| * the encoded length. out must hold 3 + 8*n bytes. n <= 127 keeps the OID | ||
| * arc single-byte. */ |
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11293
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Sorry, something went wrong.
|
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
This pull request strengthens certificate policy extension handling to comply with RFC 5280. It ensures that critical certificatePolicies extensions with more policies than can be stored are now correctly rejected, rather than silently accepting only the maximum allowed. The changes add new tests, update function signatures, and enhance logic for both the main and template ASN.1 decoders, as well as associated unit tests.
Certificate policy extension handling:
Testing improvements: