More static code analysis fixes by LinuxJedi · Pull Request #173 · wolfSSL/wolfPKCS11

LinuxJedi · 2026-03-25T10:59:43Z

Fix RSA exponent endianess
Fix CKM_TLS12_KEY_AND_MAC_DERIVE attributes
Fix leak in key wrap
Fix potential overflow in CK_TLS12_KEY_MAT_PARAMS
Fix HKDF NULL ptr dereference
Add wc_ForceZero where needed

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-1314

F-1315

F-1316, F-1317, F-1318

wolfSSL-Fenrir-bot

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.

Copilot

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_ForceZero for sensitive buffers.
Hardens C_DeriveKey around 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.

tests/rsa_exponent_test.c

src/crypto.c

tests/pkcs11test.c

dgarske · 2026-04-08T21:21:11Z

src/crypto.c

+                    tlsParams->ulIVSizeInBits  > ((CK_ULONG)0xFFFFFFFF / 2)) {
+                    return CKR_MECHANISM_PARAM_INVALID;
+                }
+                totalBits = (2 * tlsParams->ulMacSizeInBits) +


TLS totalBits sum can still overflow on 32-bit platforms

Suggested change

totalBits = (2 * tlsParams->ulMacSizeInBits) +

CK_ULONG a = 2 * tlsParams->ulMacSizeInBits;

CK_ULONG b = 2 * tlsParams->ulKeySizeInBits;

CK_ULONG c = 2 * tlsParams->ulIVSizeInBits;

if (a > (CK_ULONG)0xFFFFFFFF - b)

return CKR_MECHANISM_PARAM_INVALID;

totalBits = a + b;

if (totalBits > (CK_ULONG)0xFFFFFFFF - c)

return CKR_MECHANISM_PARAM_INVALID;

totalBits += c;

dgarske · 2026-04-08T21:21:43Z

src/crypto.c

                }
-                keyLen = *(word32*)lenAttr->pValue;
+                XMEMCPY(&reqLen, lenAttr->pValue, sizeof(CK_ULONG));
+                if (reqLen == 0 || reqLen > (CK_ULONG)0xFFFFFFFF) {


reqLen > (CK_ULONG)0xFFFFFFFF is unreachable on 32-bit

Suggested change

if (reqLen == 0 || reqLen > (CK_ULONG)0xFFFFFFFF) {

/* On 64-bit, CK_ULONG may exceed word32 range */

if (reqLen == 0 || reqLen > (CK_ULONG)0xFFFFFFFF) {

dgarske · 2026-04-08T21:22:22Z

tests/rsa_exponent_test.c

+    }                                                                      \
+    while (0)
+
+static int verbose = 0;


The verbose variable is set from command-line arguments on line 531 but is never read to control behavior. The test output is unconditional regardless of -v.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-09T09:44:58Z

tests/rsa_exponent_test.c

+static unsigned char exp_65537[] = { 0x01, 0x00, 0x01 };
+
+/* Non-palindromic exponent: 65539 = 0x010003
+ * If byte-order is reversed, this becomes 0x030001 = 196611.


The comment’s decimal conversion is incorrect: 0x030001 equals 196609 (not 196611). Please fix the numeric value so the explanation of the byte-reversal case is accurate.

Suggested change

* If byte-order is reversed, this becomes 0x030001 = 196611.

* If byte-order is reversed, this becomes 0x030001 = 196609.

Copilot · 2026-04-09T09:44:59Z

src/crypto.c

+                CK_ULONG reqLen;
+                if (!lenAttr || !lenAttr->pValue) {
+                    return CKR_ATTRIBUTE_VALUE_INVALID;
+                }
+                if (lenAttr->ulValueLen != sizeof(CK_ULONG)) {
+                    return CKR_ATTRIBUTE_VALUE_INVALID;
                }


When HKDF expand requires CKA_VALUE_LEN, this code returns CKR_ATTRIBUTE_VALUE_INVALID both when the attribute is missing and when it has an invalid value. Elsewhere in crypto.c, missing required template attributes return CKR_TEMPLATE_INCOMPLETE (and CKR_ATTRIBUTE_VALUE_INVALID is used for NULL pValue/wrong ulValueLen). Consider returning CKR_TEMPLATE_INCOMPLETE when lenAttr is NULL, and reserve CKR_ATTRIBUTE_VALUE_INVALID for NULL pValue / wrong size / out-of-range values.

Copilot · 2026-04-09T09:44:59Z

tests/pkcs11test.c

+        { CKM_HKDF_DERIVE, &paramsExpand, sizeof(paramsExpand) };
+
+    /* Expand template with NULL pValue for CKA_VALUE_LEN.
+     * This triggers the NULL dereference at crypto.c:7870.


This comment hard-codes an internal source line reference ("crypto.c:7870"), which will drift as the file changes (and is already inaccurate after this fix). Prefer referencing the condition being tested (NULL CKA_VALUE_LEN pValue) and/or the issue ID, without a specific file:line pointer.

Suggested change

* This triggers the NULL dereference at crypto.c:7870.

* This exercises the previously failing NULL CKA_VALUE_LEN pValue case.

LinuxJedi added 6 commits March 25, 2026 10:32

Fix RSA exponent endianess

47feff0

The default is a palindrome, so was missed. F-1311

Fix CKM_TLS12_KEY_AND_MAC_DERIVE attributes

1648653

When `derivedKey` is not `NULL`, don't set the attribute states. F-1312

Fix leak in key wrap

2f61030

F-1313

Fix potential overflow in CK_TLS12_KEY_MAT_PARAMS

48884e6

F-1314

Fix HKDF NULL ptr dereference

3dce681

F-1315

Add wc_ForceZero where needed

0c894a8

F-1316, F-1317, F-1318

LinuxJedi requested review from Copilot and wolfSSL-Fenrir-bot March 25, 2026 10:59

Copilot started reviewing on behalf of LinuxJedi March 25, 2026 11:04 View session

wolfSSL-Fenrir-bot reviewed Mar 25, 2026

View reviewed changes

Copilot AI reviewed Mar 25, 2026

View reviewed changes

tests/rsa_exponent_test.c Show resolved Hide resolved

src/crypto.c Show resolved Hide resolved

src/crypto.c Show resolved Hide resolved

tests/pkcs11test.c Outdated Show resolved Hide resolved

tests/pkcs11test.c Outdated Show resolved Hide resolved

Address things flagged in review

052e15b

LinuxJedi assigned wolfSSL-Bot Mar 25, 2026

dgarske requested changes Apr 8, 2026

View reviewed changes

dgarske assigned LinuxJedi Apr 8, 2026

Address PR comments

e61e5da

Copilot AI review requested due to automatic review settings April 9, 2026 09:34

Copilot started reviewing on behalf of LinuxJedi April 9, 2026 09:39 View session

Copilot AI reviewed Apr 9, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More static code analysis fixes#173

More static code analysis fixes#173
LinuxJedi wants to merge 8 commits intowolfSSL:masterfrom
LinuxJedi:f-fixes5

LinuxJedi commented Mar 25, 2026

Uh oh!

wolfSSL-Fenrir-bot left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dgarske Apr 8, 2026

Uh oh!

dgarske Apr 8, 2026

Uh oh!

dgarske Apr 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 9, 2026

Uh oh!

Copilot AI Apr 9, 2026

Uh oh!

Copilot AI Apr 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

-                totalBits = (2 * tlsParams->ulMacSizeInBits) +
+                CK_ULONG a = 2 * tlsParams->ulMacSizeInBits;
+                CK_ULONG b = 2 * tlsParams->ulKeySizeInBits;
+                CK_ULONG c = 2 * tlsParams->ulIVSizeInBits;
+                if (a > (CK_ULONG)0xFFFFFFFF - b)
+                    return CKR_MECHANISM_PARAM_INVALID;
+                totalBits = a + b;
+                if (totalBits > (CK_ULONG)0xFFFFFFFF - c)
+                    return CKR_MECHANISM_PARAM_INVALID;
+                totalBits += c;

	if (reqLen == 0 \|\| reqLen > (CK_ULONG)0xFFFFFFFF) {
	/* On 64-bit, CK_ULONG may exceed word32 range */
	if (reqLen == 0 \|\| reqLen > (CK_ULONG)0xFFFFFFFF) {

	* If byte-order is reversed, this becomes 0x030001 = 196611.
	* If byte-order is reversed, this becomes 0x030001 = 196609.

	* This triggers the NULL dereference at crypto.c:7870.
	* This exercises the previously failing NULL CKA_VALUE_LEN pValue case.

Conversation

LinuxJedi commented Mar 25, 2026

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #173

Low (1)

Incomplete integer overflow protection in TLS key material size computation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dgarske Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

dgarske Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

dgarske Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants