More static analysis fixes

[LinuxJedi]

• Remove wrong #ifndef
• Add cyphertext too short decryption protection
• Use ForceZero instead of XMEMSET
• Clear key buffers before free
• Use constant time for AES key wrap padding
• Set CCM dataSz after buffer is cleared, not before
• Validate PKCS#7 padding bytes in AES-CBC-PAD decrypt
• Fix declaration of CKM_SHA1_RSA_PKCS_PSS
• Use XMEMCPY instead of memcpy
• Fix AES-ECB encrypt failure error handling

[Copilot]

Pull request overview

This PR tightens crypto error handling and constant-time padding validation in the wolfPKCS11 implementation, and adds targeted regression tests for two reported bugs.

Changes:

• Add new regression tests for AES-CBC-PAD PKCS#7 padding validation and AES-ECB check-value error propagation.
• Harden decrypt paths by rejecting too-short ciphertexts for AEAD modes and add constant-time padding checks for AES key wrap padding.
• Improve key material handling by forcing zeroization before freeing sensitive buffers and by propagating AES-ECB encrypt failures.

Reviewed changes

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

Show a summary per file

File	Description
tests/include.am	Registers two new test executables in the Automake test build.
tests/ecb_check_value_error_test.c	New test covering GetEcbCheckValue error propagation for invalid AES key sizes.
tests/aes_cbc_pad_padding_test.c	New test verifying AES-CBC-PAD rejects tampered PKCS#7 padding (one-shot and multipart).
src/wolfpkcs11.c	Replaces a raw memcpy with XMEMCPY for interface list copying.
src/slot.c	Fixes mechanism list/mech-info wiring for SHA1 RSA-PSS and cleans up mech-info guarding.
src/internal.c	Adds force-zeroization before frees, fixes CCM param init ordering, propagates ECB check-value failures, and validates CBC-PAD padding bytes in constant time.
src/crypto.c	Adds ciphertext-length guards for AEAD decrypt, constant-time keywrap-pad padding validation, and keywrap minimum length protection.

---

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

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

---

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

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #169

Scan targets checked: wolfpkcs11-bugs, wolfpkcs11-src
Findings: 4

Medium (2)

Possible wrong header included for wc_ForceZero declaration

File: src/internal.c:105-112
Function: N/A (file scope)
Category: API contract violations

The new wc_ForceZero compatibility shim includes <wolfssl/wolfcrypt/memory.h> for wolfSSL >= 5.8.4, expecting that header to declare wc_ForceZero. However, in wolfSSL, wc_ForceZero is historically declared in wolfssl/wolfcrypt/misc.h (and implemented in wolfcrypt/src/misc.c). If memory.h does not actually declare wc_ForceZero, the code would compile without the fallback definition and without a proper declaration, resulting in either an implicit-function-declaration warning/error (C99+) or a linker error. This would only manifest when building against wolfSSL >= 5.8.4 where the #else fallback is skipped. If misc.h is already included elsewhere in internal.c and provides wc_ForceZero inline, the memory.h include is harmless but misleading; if misc.h does not provide it (e.g., when WOLFSSL_MISC_INCLUDED is not defined), the build would fail.

#if defined(LIBWOLFSSL_VERSION_HEX) && LIBWOLFSSL_VERSION_HEX >= 0x05008004
    #include <wolfssl/wolfcrypt/memory.h>
#else
    static void wc_ForceZero(void* mem, size_t len) {
        volatile byte* p = (volatile byte*)mem;
        while (len--) *p++ = 0;
    }
#endif

Recommendation: Verify that wolfssl/wolfcrypt/memory.h actually declares wc_ForceZero in wolfSSL 5.8.4+. If the declaration is in wolfssl/wolfcrypt/misc.h instead, change the include to #include <wolfssl/wolfcrypt/misc.h>, or if the function is already available through an existing include, remove the unnecessary #include and add a comment explaining where the declaration comes from.

---

Insufficient minimum ciphertext length check for AES Key Wrap allows decDataLen of zero

File: src/crypto.c:3301-3302
Function: C_Decrypt
Category: Buffer overflows

The newly added underflow guard if (ulEncryptedDataLen < KEYWRAP_BLOCK_SIZE) uses KEYWRAP_BLOCK_SIZE (8 bytes) as the minimum, but AES Key Wrap requires at least two semiblocks (16 bytes). When ulEncryptedDataLen == KEYWRAP_BLOCK_SIZE, decDataLen becomes 0 (a word32). In the CKM_AES_KEY_WRAP_PAD branch, pData[decDataLen - 1] wraps to pData[0xFFFFFFFF], and the constant-time loop accesses pData[decDataLen - 1 - i] for i = 0..KEYWRAP_BLOCK_SIZE-1, all out-of-bounds reads. While the underlying wc_AesKeyUnWrap would likely fail for an 8-byte input (preventing the padding code from executing in practice), the bounds check itself is insufficient and relies on the unwrap implementation to prevent the buffer over-read.

if (ulEncryptedDataLen < KEYWRAP_BLOCK_SIZE)
                return CKR_ENCRYPTED_DATA_LEN_RANGE;
            decDataLen = (word32)(ulEncryptedDataLen - KEYWRAP_BLOCK_SIZE);
            ...
            if (mechanism == CKM_AES_KEY_WRAP_PAD) {
                byte padValue = pData[decDataLen - 1];

Recommendation: Change the minimum length check to 2 * KEYWRAP_BLOCK_SIZE (16 bytes), which is the actual minimum for AES Key Wrap ciphertext (one semiblock of data plus the 8-byte integrity check value): if (ulEncryptedDataLen < 2 * KEYWRAP_BLOCK_SIZE) return CKR_ENCRYPTED_DATA_LEN_RANGE;

---

Low (2)

Constant-time >> 31 idiom assumes exactly 32-bit unsigned int

File: src/crypto.c:3325-3340
Function: C_Decrypt
Category: Incorrect sizeof/type usage

The new constant-time padding validation for CKM_AES_KEY_WRAP_PAD uses >> 31 to extract the sign/overflow bit from unsigned int arithmetic, e.g. badPad |= ((unsigned int)padValue - 1) >> 31; and inPad = 0 - (((unsigned int)i - (unsigned int)padValue) >> 31);. This idiom only works correctly when unsigned int is exactly 32 bits. On platforms where unsigned int is 16 bits (legal per C standard, possible on embedded targets), the underflow wraps at 2^16 and bit 31 is never set, so the overflow detection silently fails. The analogous validation added in WP11_AesCbcPad_DecryptFinal (internal.c) correctly avoids this issue by using comparison-based masks: (byte)(0 - (padCnt == 0)) and ((unsigned)(AES_BLOCK_SIZE - 1 - i) < (unsigned)padCnt), which produce correct 0/1 results regardless of integer width. The two constant-time padding checks use inconsistent idioms for the same purpose.

badPad |= ((unsigned int)padValue - 1) >> 31;
badPad |= ((unsigned int)KEYWRAP_BLOCK_SIZE -
           (unsigned int)padValue) >> 31;
badPad |= ((unsigned int)decDataLen -
           (unsigned int)padValue) >> 31;

for (i = 0; i < KEYWRAP_BLOCK_SIZE; i++) {
    inPad = 0 - (((unsigned int)i -
                   (unsigned int)padValue) >> 31);

Recommendation: Use comparison-based constant-time masks consistent with the WP11_AesCbcPad_DecryptFinal approach, e.g. badPad |= (unsigned int)(0 - (padValue == 0)); and badPad |= (unsigned int)(0 - (padValue > KEYWRAP_BLOCK_SIZE)); and inPad = 0 - (unsigned int)(i < padValue);. This is both more portable and consistent with the other constant-time check in this PR.

---

PKCS#7 padding validation uses comparison operators that may not be constant-time

File: src/internal.c:13010-13023
Function: WP11_AesCbcPad_DecryptFinal
Category: Constant-time violations

The new constant-time PKCS#7 padding validation in WP11_AesCbcPad_DecryptFinal uses C boolean comparison operators (padCnt == 0, padCnt > AES_BLOCK_SIZE, and < in the loop) which produce 0 or 1 but may compile to conditional branches on some architectures/compilers. In contrast, the analogous constant-time padding check added in C_Decrypt for CKM_AES_KEY_WRAP_PAD uses purely arithmetic/bitwise operations (((unsigned int)padValue - 1) >> 31) that are more reliably branchless. The inconsistency means the CBC-PAD path has a weaker constant-time guarantee than the Key Wrap path.

padBad = (byte)(0 - (padCnt == 0));
        padBad |= (byte)(0 - (padCnt > AES_BLOCK_SIZE));
        for (i = 0; i < AES_BLOCK_SIZE; i++) {
            byte inPad = (byte)(0 -
                ((unsigned)(AES_BLOCK_SIZE - 1 - i) < (unsigned)padCnt));
            padBad |= inPad & (cbc->partial[i] ^ padCnt);
        }

Recommendation: Use the same arithmetic/bitwise pattern as the Key Wrap PAD code for consistency and stronger constant-time guarantees: padBad = (byte)(((unsigned)padCnt - 1) >> 31); for the zero check, and padBad |= (byte)(((unsigned)AES_BLOCK_SIZE - (unsigned)padCnt) >> 31); for the range check. Similarly, express the in-padding mask using subtraction and shift rather than <.

---

This review was generated automatically by Fenrir. Findings are non-blocking.

[LinuxJedi]

Fenrir scan results vary between invalid due to it not seeing the full context and very low priority for another PR. The second medium has been fixed though.