ASN.1 portability and a KeyUsage encoding fix - #11171
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves ASN.1-related portability (avoiding int/word32 pointer type punning) and fixes a functional KeyUsage BIT STRING encoding bug affecting decipherOnly on ASN template encoding paths, with a new regression test to validate KeyUsage round-trips.
Changes:
- Remove several instances of pointer type punning by introducing correctly typed length variables (e.g.,
word32/int) and explicit conversions. - Add 16-bit BIT STRING sizing/encoding helpers for ASN templates to correctly encode KeyUsage values that require a second content byte (notably
decipherOnly). - Add an API test covering KeyUsage round-trip cases including
decipherOnlyand mixed-bit combinations.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfcrypt/src/evp.c | Avoids (word32*)&int type punning when exporting ECC keys to PKCS#8. |
| wolfcrypt/src/asn.c | Adds template BIT STRING16 support for KeyUsage, fixes multiple type-punning sites, and adjusts several integer widths for portability. |
| wolfcrypt/src/asn_orig.c | Fixes 16-bit int shift/promotion issue in KeyUsage decoding. |
| tests/api/test_asn.h | Registers the new KeyUsage decipherOnly regression test. |
| tests/api/test_asn.c | Adds a KeyUsage round-trip regression test covering decipherOnly and combinations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
9f1e639 to
7a789c4
Compare
|
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11171
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 4
4 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
| static int ECC_populate_EVP_PKEY(WOLFSSL_EVP_PKEY* pkey, WOLFSSL_EC_KEY *key) | ||
| { | ||
| int derSz = 0; | ||
| word32 derSzOut = 0; |
There was a problem hiding this comment.
🔵 [Low] derSzOut is unused when HAVE_PKCS8 is not defined · Dead/unreachable code
The new derSzOut local is declared unconditionally but every reference (lines 9909-9916) sits inside #ifdef HAVE_PKCS8. In a NO_PKCS8 build the variable is never referenced, producing a -Wunused-variable warning that breaks -Werror configurations.
Fix: Move the derSzOut declaration inside the #ifdef HAVE_PKCS8 block.
| * bit string header, so capacities in a narrow band just below the exact | ||
| * encoding size get accepted and overrun. | ||
| */ | ||
| #if !defined(NO_ASN) && !defined(NO_RSA) && !defined(NO_CERTS) && \ |
There was a problem hiding this comment.
⚪ [Info] Doc comment for test_wc_SignCert_buffer_bounds separated from its function · Documentation
The TEST_KEYUSAGE_DECIPHER_ONLY macro and new test are inserted between the comment block (lines 2614–2623) describing wc_SignCert() buffer bounds and the test_wc_SignCert_buffer_bounds() function it documents. As a result, the comment now appears to document the test_wc_DecodeKeyUsage_decipherOnly test instead of its intended target.
Fix: Move the new #if block and test_wc_DecodeKeyUsage_decipherOnly() function below test_wc_SignCert_buffer_bounds() so its doc comment remains adjacent to the function it documents.
| { "encipherOnly,decipherOnly", (word16)(KEYUSE_ENCIPHER_ONLY | | ||
| KEYUSE_DECIPHER_ONLY) }, | ||
| /* A first-byte-only value must keep encoding in a single byte. */ | ||
| { "digitalSignature", KEYUSE_DIGITAL_SIG }, |
There was a problem hiding this comment.
🔵 [Low] KeyUsage round-trip cases all encode to unusedBits == 7 · Missing edge-case coverage on a function the PR also changed
All four cases (0x8000, 0x8080, 0x8001, 0x0080) have a last significant byte of 0x80, so the trailing-zero-bit loop in the new SetASN_BitString16() is only ever exercised at unusedBits==7. Counts 0-6 and the lastByte != 0 false branch are untested, contradicting the PR's "all nine bits" claim.
Fix: Add cases for the remaining low-byte bits (cRLSign, keyCertSign, keyAgreement, dataEncipherment, keyEncipherment, nonRepudiation) so each unused-bit count is covered.
| * bit string header, so capacities in a narrow band just below the exact | ||
| * encoding size get accepted and overrun. | ||
| */ | ||
| #if !defined(NO_ASN) && !defined(NO_RSA) && !defined(NO_CERTS) && \ |
There was a problem hiding this comment.
⚪ [Info] New test inserted between a doc comment and the function it documents · Dead/unreachable code
The comment block at lines 2614-2623 describes wc_SignCert() bounds behaviour for test_wc_SignCert_buffer_bounds() at line 2714, but the new TEST_KEYUSAGE_DECIPHER_ONLY guard and key-usage test were inserted between them, so the comment now appears to document the key-usage test.
Fix: Move the new guard macro and test_wc_DecodeKeyUsage_decipherOnly() below test_wc_SignCert_buffer_bounds().
ASN.1 portability and a KeyUsage encoding fix
Part 2 of 4 from an internal automated source review. Independent of the other three; no shared files.
Behaviour change
While writing the regression test for the key usage decode, a functional bug turned up in the encoder.
wc_SetKeyUsage(cert, "decipherOnly")produced a certificate whose KeyUsage extension parsed back asdigitalSignature. decipherOnly is bit 8, the only KeyUsage value needing a second BIT STRING content byte, and the ASN template encoder wrote the two bytes in the wrong order. The non-template encoder was already correct, and the fix adds the missing 16-bit sibling of the existingSetASN_BitString32()rather than changing shared code - the TSPfailInfoBIT STRING deliberately uses the opposite word layout and would otherwise have broken.Certificates generated with
decipherOnlyby an affected build carry the wrong usage and should be reissued.Testing
make checkpasses on--enable-alland--enable-all --enable-lms.New test: KeyUsage round trip covering all nine bits and several combinations (
tests/api/test_asn.c).