New API for CRL unknown extension callback - #10961
Open
anhu wants to merge 4 commits into
Open
Conversation
|
Member
Author
|
jenkins retest this please. |
Member
Author
|
jenkins retest this please |
Adds public entry points mirroring the existing X.509 unknown extension callback so callers can register a handler for unrecognized CRL extensions instead of failing with ASN_CRIT_EXT_E.
Frauschi
reviewed
Jul 24, 2026
Frauschi
left a comment
Contributor
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 3 total — 3 posted, 0 skipped
Posted findings
- [Medium] CRL unknown-ext callback is a silent no-op in non-template (WOLFSSL_NO_ASN_TEMPLATE) builds —
wolfcrypt/src/asn_orig.c:9194, wolfcrypt/src/asn_orig.c:9394-9541 - [Low] No test coverage for the non-Ex setter wolfSSL_CertManagerSetCRLUnknownExtCallback —
tests/api/test_certman.c:3020-3190 - [Low] Positive-return sanitization untested on the CRL-level extension path —
tests/api/test_certman.c:3160-3190, wolfcrypt/src/asn.c:37351-37357
Review generated by Skoll via Claude/Codex
Frauschi
reviewed
Jul 27, 2026
Frauschi
left a comment
Contributor
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 25 total — 6 posted, 19 skipped
Posted findings
- [Medium] Accept-all CRL callback also silently bypasses recognized-but-unimplemented critical extensions (IDP, deltaCRLIndicator) —
wolfcrypt/src/asn.c:37323-37364 - [Medium] New CRL unknown-extension callback is a silent no-op on every OpenSSL-compat CRL load path —
src/crl.c:900-907 - [Medium] Zero-length entry-extension OID hands the callback an uninitialized 64-byte stack array —
wolfcrypt/src/asn.c:36814-36879 - [Medium] Registering a permissive callback makes CRL parsing STRICTER when DecodeObjectId cannot decode the OID —
wolfcrypt/src/asn.c:36872-36888 - [Medium] Observe-only callback silently disables RFC 5280 §5.3 enforcement for all critical CRL extensions, including Issuing Distribution Point —
wolfssl/ssl.h:4477-4494 - [Low] Registering an accepting CRL callback turns previously-tolerated unknown non-critical entry extensions into hard load failures —
wolfcrypt/src/asn.c:36858-36890
Skipped findings
- [Medium] Callback error code reaches the application differently depending on where the extension sits
- [Medium] Bare BAD_FUNC_ARG in test assertions instead of WC_NO_ERR_TRACE(BAD_FUNC_ARG)
- [Medium] GetCRLInfo callback wiring is untested
- [Medium] No test that the callback fires for a non-critical unknown CRL extension
- [Medium] New CRL unknown-extension callback is a silent no-op in non-template ASN builds, and the new unit tests fail there
- [Low] Registering a callback turns previously-skipped malformed entry extensions into hard load failures
- [Low] CRL-level dispatch reads the ASNGetData union through buffer.data instead of the buf + localIdx idiom used by the rest of the function
- [Low] Buffer-only callback tests are gated on !NO_FILESYSTEM
- [Low] Callback error codes are discarded by the caller, contradicting the preservation logic and the new ssl.h documentation
- [Low] Callback's negative error code is flattened to ASN_PARSE_E, contradicting the code's own comment and the X.509 callback it claims to mirror
- [Low] New unconditional ASN_PARSE_E when an unknown CRL entry extension's extnValue is malformed
- [Low] Ex callback and its context pointer are published and consumed as two unsynchronized accesses
- [Low] New setters return int while the adjacent unknown-extension setter returns void, and the return contract is undocumented
- [Info] Moved static test arrays keep function-body indentation at file scope
- [Info] New public API functions lack doxygen parameter/return blocks
- [Info] Cert-manager cert-side callback still has no Ex variant, leaving an asymmetric API surface
- [Info] New tests compare against bare BAD_FUNC_ARG instead of WC_NO_ERR_TRACE(BAD_FUNC_ARG)
- [Info] Callback/context pair is read unsynchronized and now fires from the CRL monitor thread
- [Info] Undocumented dispatch semantics: both callbacks chain, and neither can tell a CRL-level from a revoked-entry extension
Review generated by Skoll via Claude/Codex
Frauschi
reviewed
Jul 29, 2026
| * the output array, which would hand the unknown-extension callback an | ||
| * uninitialized OID buffer. The template path gets this check via | ||
| * GetASN_ObjectId(); this hand-rolled parser must do it explicitly. */ | ||
| if (GetASN_ObjectId(buff, oidContent, oidLen) != 0) { |
Contributor
There was a problem hiding this comment.
GetASN_ObjectId() is only compiled under WOLFSSL_ASN_TEMPLATE, so this causes a compile-error with the old non-template asn code (also the cause for the PRB-master failure).
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.
Adds public entry points mirroring the existing X.509 unknown extension callback so callers can register a handler for unrecognized CRL extensions instead of failing with ASN_CRIT_EXT_E.
Fixes ZD 21887