Merge pull request #169 from LinuxJedi/f-fixes2

More static analysis fixes

Author: dgarske

src/crypto.c

@@ -3209,6 +3209,8 @@ CK_RV C_Decrypt(CK_SESSION_HANDLE hSession, CK_BYTE_PTR pEncryptedData,
             if (!WP11_Session_IsOpInitialized(session, WP11_INIT_AES_GCM_DEC))
                 return CKR_OPERATION_NOT_INITIALIZED;
 
+            if (ulEncryptedDataLen < (CK_ULONG)WP11_AesGcm_GetTagBits(session) / 8)
+                return CKR_ENCRYPTED_DATA_LEN_RANGE;
             decDataLen = (word32)ulEncryptedDataLen -
                                             WP11_AesGcm_GetTagBits(session) / 8;
             if (pData == NULL) {
@@ -3230,6 +3232,8 @@ CK_RV C_Decrypt(CK_SESSION_HANDLE hSession, CK_BYTE_PTR pEncryptedData,
             if (!WP11_Session_IsOpInitialized(session, WP11_INIT_AES_CCM_DEC))
                 return CKR_OPERATION_NOT_INITIALIZED;
 
+            if (ulEncryptedDataLen < (CK_ULONG)WP11_AesCcm_GetMacLen(session))
+                return CKR_ENCRYPTED_DATA_LEN_RANGE;
             decDataLen = (word32)ulEncryptedDataLen -
                                             WP11_AesCcm_GetMacLen(session);
             if (pData == NULL) {
@@ -3294,9 +3298,10 @@ CK_RV C_Decrypt(CK_SESSION_HANDLE hSession, CK_BYTE_PTR pEncryptedData,
             if (!WP11_Session_IsOpInitialized(session, WP11_INIT_AES_KEYWRAP_DEC))
                 return CKR_OPERATION_NOT_INITIALIZED;
 
-            /* AES Key Wrap unwrapping reduces the size by 8 bytes (the
-             * integrity check value). If using padding then its even smaller
-             * but we can't know the final size without decrypting first. */
+            /* AES Key Wrap ciphertext is at least two semiblocks: one data
+             * semiblock plus the 8-byte integrity check value. */
+            if (ulEncryptedDataLen < 2 * KEYWRAP_BLOCK_SIZE)
+                return CKR_ENCRYPTED_DATA_LEN_RANGE;
             decDataLen = (word32)(ulEncryptedDataLen - KEYWRAP_BLOCK_SIZE);
             if (pData == NULL) {
                 *pulDataLen = decDataLen;
@@ -3312,12 +3317,31 @@ CK_RV C_Decrypt(CK_SESSION_HANDLE hSession, CK_BYTE_PTR pEncryptedData,
             if (mechanism == CKM_AES_KEY_WRAP_PAD) {
                 int i;
                 byte padValue = pData[decDataLen - 1];
-                if (padValue > KEYWRAP_BLOCK_SIZE || padValue > decDataLen)
-                    return CKR_ENCRYPTED_DATA_LEN_RANGE;
-                for (i = 0; i < padValue; i++) {
-                    if (pData[decDataLen - 1 - i] != padValue)
-                        return CKR_ENCRYPTED_DATA_INVALID;
+                unsigned int badPad = 0;
+                unsigned int inPad;
+
+                /* Constant-time range check: padValue must be 1..KEYWRAP_BLOCK_SIZE
+                 * and must not exceed decDataLen. */
+                badPad |= ((unsigned int)padValue - 1) >> 31;
+                badPad |= ((unsigned int)KEYWRAP_BLOCK_SIZE -
+                           (unsigned int)padValue) >> 31;
+                badPad |= ((unsigned int)decDataLen -
+                           (unsigned int)padValue) >> 31;
+
+                /* Constant-time padding byte verification.
+                 * Always iterate KEYWRAP_BLOCK_SIZE times to avoid leaking
+                 * padValue through iteration count. */
+                for (i = 0; i < KEYWRAP_BLOCK_SIZE; i++) {
+                    /* Full mask: all-ones when i < padValue, else 0 */
+                    inPad = 0 - (((unsigned int)i -
+                                   (unsigned int)padValue) >> 31);
+                    badPad |= inPad &
+                              ((unsigned int)pData[decDataLen - 1 - i] ^
+                               (unsigned int)padValue);
                 }
+
+                if (badPad != 0)
+                    return CKR_ENCRYPTED_DATA_INVALID;
                 decDataLen -= padValue;
             }
             *pulDataLen = decDataLen;
@@ -3623,6 +3647,9 @@ CK_RV C_DecryptFinal(CK_SESSION_HANDLE hSession, CK_BYTE_PTR pLastPart,
             if (!WP11_Session_IsOpInitialized(session, WP11_INIT_AES_GCM_DEC))
                 return CKR_OPERATION_NOT_INITIALIZED;
 
+            if (WP11_AesGcm_EncDataLen(session) <
+                                            WP11_AesGcm_GetTagBits(session) / 8)
+                return CKR_ENCRYPTED_DATA_LEN_RANGE;
             decPartLen = WP11_AesGcm_EncDataLen(session) -
                                             WP11_AesGcm_GetTagBits(session) / 8;
             if (pLastPart == NULL) {

src/internal.c

@@ -102,6 +102,17 @@
     #error "wolfTPM and MAXQ10XX are incompatible with each other."
 #endif
 
+/* wc_ForceZero was added in wolfSSL 5.8.4. Provide a fallback for older
+ * versions to securely zero sensitive memory. */
+#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
+
 /* Helper to get size of struct field */
 #define FIELD_SIZE(type, field) (sizeof(((type *)0)->field))
 
@@ -2738,7 +2749,10 @@ int WP11_Object_Copy(WP11_Object *src, WP11_Object *dest)
                         }
                     }
 
-                    XFREE(derBuf, NULL, DYNAMIC_TYPE_TMP_BUFFER);
+                    if (derBuf != NULL) {
+                        wc_ForceZero(derBuf, derSz);
+                        XFREE(derBuf, NULL, DYNAMIC_TYPE_TMP_BUFFER);
+                    }
 
                     /* Free destination key on failure */
                     if (ret != 0) {
@@ -2811,7 +2825,7 @@ int WP11_Object_Copy(WP11_Object *src, WP11_Object *dest)
 
                     /* Clean up */
                     if (derBuf != NULL) {
-                        XMEMSET(derBuf, 0, derSz); /* Clear sensitive data */
+                        wc_ForceZero(derBuf, derSz);
                         XFREE(derBuf, NULL, DYNAMIC_TYPE_TMP_BUFFER);
                     }
 
@@ -7819,8 +7833,8 @@ int WP11_Session_SetCcmParams(WP11_Session* session, int dataSz,
         ret = BAD_FUNC_ARG;
 
     if (ret == 0) {
-        ccm->dataSz = dataSz;
         XMEMSET(ccm, 0, sizeof(*ccm));
+        ccm->dataSz = dataSz;
         XMEMCPY(ccm->iv, iv, ivSz);
         ccm->ivSz = ivSz;
         if (aad != NULL) {
@@ -8285,8 +8299,7 @@ void WP11_Object_Free(WP11_Object* object)
     #endif
         if ((object->type == CKK_AES || object->type == CKK_GENERIC_SECRET ||
              object->type == CKK_HKDF) && object->data.symmKey != NULL) {
-            /* TODO: ForceZero */
-            XMEMSET(object->data.symmKey->data, 0, object->data.symmKey->len);
+            wc_ForceZero(object->data.symmKey->data, object->data.symmKey->len);
             XFREE(object->data.symmKey, NULL, DYNAMIC_TYPE_AES);
             object->data.symmKey = NULL;
         }
@@ -9931,6 +9944,9 @@ static int GetEcbCheckValue(WP11_Object* secret, byte* dataOut,
     XFREE(hash, NULL, DYNAMIC_TYPE_TMP_BUFFER);
     XFREE(input, NULL, DYNAMIC_TYPE_TMP_BUFFER);
 
+    if (ret != 0)
+        return CKR_FUNCTION_FAILED;
+
     return CKR_OK;
 }
 #endif
@@ -12991,7 +13007,26 @@ int WP11_AesCbcPad_DecryptFinal(unsigned char* dec, word32* decSz,
     ret = wc_AesCbcDecrypt(&cbc->aes, cbc->partial, cbc->partial,
                                                                 cbc->partialSz);
     if (ret == 0) {
+        byte padBad;
+
         padCnt = cbc->partial[AES_BLOCK_SIZE-1];
+
+        /* Validate PKCS#7 padding in constant time:
+         * padCnt must be 1..AES_BLOCK_SIZE and all padding bytes must equal
+         * padCnt. */
+        padBad = (byte)(0 - (padCnt == 0));
+        padBad |= (byte)(0 - (padCnt > AES_BLOCK_SIZE));
+        for (i = 0; i < AES_BLOCK_SIZE; i++) {
+            /* inPad is 0xFF when i is in the padding region, 0x00 otherwise */
+            byte inPad = (byte)(0 -
+                ((unsigned)(AES_BLOCK_SIZE - 1 - i) < (unsigned)padCnt));
+            padBad |= inPad & (cbc->partial[i] ^ padCnt);
+        }
+        if (padBad) {
+            ret = BAD_PADDING_E;
+        }
+    }
+    if (ret == 0) {
         outSz = AES_BLOCK_SIZE - (padCnt & (0 - (padCnt <= AES_BLOCK_SIZE)));
         for (i = 0; i < AES_BLOCK_SIZE; i++) {
             mask = (size_t)0 - (i != outSz);

src/slot.c

@@ -328,7 +328,7 @@ static CK_MECHANISM_TYPE mechanismList[] = {
 #ifdef WC_RSA_PSS
     CKM_RSA_PKCS_PSS,
 #ifndef NO_SHA
-    CKM_SHA1_RSA_PKCS,
+    CKM_SHA1_RSA_PKCS_PSS,
 #endif
 #ifdef WOLFSSL_SHA224
     CKM_SHA224_RSA_PKCS_PSS,
@@ -579,12 +579,10 @@ static CK_MECHANISM_INFO rsaPssMechInfo = {
     1024, 4096, CKF_SIGN | CKF_VERIFY
 };
 #endif
-#ifndef NO_SHA256
 static CK_MECHANISM_INFO shaRsaPkcsMechInfo = {
     1024, 4096, CKF_SIGN | CKF_VERIFY
 };
 #endif
-#endif
 #ifdef HAVE_ECC
 /* Info on EC key generation mechanism. */
 static CK_MECHANISM_INFO ecKgMechInfo = {

src/wolfpkcs11.c

@@ -448,7 +448,7 @@ CK_RV C_GetInterfaceList(CK_INTERFACE_PTR pInterfacesList, CK_ULONG_PTR pulCount
         return CKR_BUFFER_TOO_SMALL;
     }
 
-    memcpy(pInterfacesList, interfaces, NUM_INTERFACES * sizeof(CK_INTERFACE));
+    XMEMCPY(pInterfacesList, interfaces, NUM_INTERFACES * sizeof(CK_INTERFACE));
     *pulCount = NUM_INTERFACES;
 
     return CKR_OK;