Conversation
The default is a palindrome, so was missed. F-1311
When `derivedKey` is not `NULL`, don't set the attribute states. F-1312
F-1313
F-1316, F-1317, F-1318
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #173
Scan targets checked: wolfpkcs11-bugs, wolfpkcs11-src
Findings: 1
Low (1)
Incomplete integer overflow protection in TLS key material size computation
File: src/crypto.c:7950-7963
Function: C_DeriveKey
Category: Integer overflows
The PR reworks the TLS key material size computation to use a CK_ULONG totalBits intermediate variable and adds a check totalBits > (CK_ULONG)0xFFFFFFFF before truncating to word32. However, the individual multiplications 2 * tlsParams->ulMacSizeInBits, 2 * tlsParams->ulKeySizeInBits, and 2 * tlsParams->ulIVSizeInBits and their sum can all overflow CK_ULONG before the range check. On 32-bit platforms where CK_ULONG is 32 bits, the > 0xFFFFFFFF check is tautologically false and provides no protection at all. An attacker providing crafted mechanism parameters (e.g., ulMacSizeInBits = 0x80000004) could cause the arithmetic to wrap to a small totalBits/keyLen, resulting in a small derivedKey allocation while the individual field sizes used later to partition the buffer remain large. This is an improvement over the prior code (which had direct word32 truncation), but the overflow protection is incomplete for the scenario it was designed to address.
CK_ULONG totalBits;
totalBits = (2 * tlsParams->ulMacSizeInBits) +
(2 * tlsParams->ulKeySizeInBits) +
(2 * tlsParams->ulIVSizeInBits);
if (totalBits == 0)
return CKR_MECHANISM_PARAM_INVALID;
if ((totalBits % 8) != 0)
return CKR_MECHANISM_PARAM_INVALID;
totalBits /= 8;
if (totalBits > (CK_ULONG)0xFFFFFFFF)
return CKR_MECHANISM_PARAM_INVALID;
keyLen = (word32)totalBits;Recommendation: Add upper-bound validation on individual fields before arithmetic, or use overflow-safe multiplication. For example, check that each of ulMacSizeInBits, ulKeySizeInBits, and ulIVSizeInBits is less than CK_ULONG_MAX / 2 (or a reasonable maximum like 0xFFFFFFFF) before computing totalBits. This ensures the 2 * val multiplications and their sum cannot wrap.
This review was generated automatically by Fenrir. Findings are non-blocking.
There was a problem hiding this comment.
Pull request overview
This PR addresses several static analysis findings and hardening fixes across wolfPKCS11’s cryptographic operations and test suite, including RSA exponent handling, HKDF robustness, TLS 1.2 derivation sizing, and AES key wrap resource cleanup.
Changes:
- Adds a new RSA exponent endianness regression test and a new HKDF regression test for NULL
CKA_VALUE_LEN. - Fixes/cleans up internal AES key wrap state teardown and expands use of
wc_ForceZerofor sensitive buffers. - Hardens
C_DeriveKeyaround HKDF expand template parsing and TLS 1.2 key material length computation; updates sanitizer CI to exercise AES key wrap.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/rsa_exponent_test.c | New standalone regression test for RSA public exponent byte-order in key generation. |
| tests/pkcs11test.c | Adds HKDF regression test targeting NULL CKA_VALUE_LEN expand crash. |
| tests/include.am | Registers new RSA exponent test program in Automake test targets. |
| src/internal.c | Fixes RSA exponent conversion order, adds AES key wrap session cleanup, and zeros sensitive buffers. |
| src/crypto.c | Prevents HKDF NULL deref and adds overflow guard structure for TLS12 key material sizing. |
| .github/workflows/sanitizer-tests.yml | Enables AES key wrap in sanitizer CI to cover the affected code paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
CKM_TLS12_KEY_AND_MAC_DERIVEattributes