Fix non-minimal DER encoding of caller-supplied certificate serials - #11433
Fix non-minimal DER encoding of caller-supplied certificate serials#11433embhorn wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Fixes certificate generation to emit minimal DER-encoded serial numbers (and reject invalid serials) to avoid strict ASN.1 parse failures and OOB reads when callers provide malformed serialSz.
Changes:
- Normalize caller-supplied serials to minimal DER INTEGER encoding (strip redundant leading zeros) and bound
serialSztoCTC_SERIAL_SIZE. - Reject zero serials in strict configurations to match RFC 5280 and decoder behavior.
- Add/adjust tests and fixtures to validate encoding, parsing round-trips, and zero-serial behavior; update doxygen comments for the serial contract.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfcrypt/src/asn.c | Normalizes/validates serial encoding during cert generation and rejects invalid serials. |
| tests/unit-mcdc/test_asn_cert_whitebox.c | Updates MC/DC fixtures to keep testing zero-serial parsing by patching DER after body generation. |
| tests/api/test_asn.h | Registers the new serial encoding API test. |
| tests/api/test_asn.c | Adds unit test to verify serial TLV encoding and parse round-trips across serial shapes. |
| doc/dox_comments/header_files/asn_public.h | Documents the updated serial number behavior and error conditions. |
Suppressed comments (1)
wolfcrypt/src/asn.c:1
- The new rejection can trigger for caller-supplied serials as well, but the message says “for generated certificate”, which is misleading. Consider changing it to something neutral and actionable like “Serial number must be positive (non-zero)” (optionally mentioning the controlling macro in a comment rather than the runtime string).
💡 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
There was a problem hiding this comment.
Fenrir Automated Review — PR #11433
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-bugs
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
|
|
retest this please |
Description
MakeAnyCert()passedcert->serialto the encoder verbatim. The template encoder adds a sign pad when the MSB is set but never strips redundant leading zeros, so a fixed-width serial like 00 00 00 01 was emitted as 02 04 00 00 00 01 rather than 02 01 01. Generation returned success and the certificate then failed to parse in the same build with ASN_PARSE_E (-140), "Zero not required on INTEGER". Strict third party parsers reject it too. The original ASN back end was unaffected; it normalizes viaSetSerialNumber().MakeAnyCert()now strips leading zeros, matching the existing fix for the same defect class inStoreECC_DSA_Sig_Bin(). It also boundsserialSztoCTC_SERIAL_SIZE, closing an out of bounds read when a caller set a size past the 20 byte array, and rejects a zero serial per RFC 5280 4.1.2.2 under the same macros that gate the decoder's serial-0 check, so one configuration governs both directions.Fixes zd22432
Testing
test_wc_MakeCert_serial_encodingasserts the emitted serial TLV byte for byte across six serial shapes and round trips each certificate throughwc_ParseCert(); it fails without the asn.c change. The zero-serial MC/DC fixtures now patch the serial betweenwc_MakeCert()andwc_SignCert(), since the generator no longer emits one. make check and the white-box smoke suite pass.Fix confirmed by customer
Checklist