F-3271 - https://fenrir.wolfssl.com/finding/3271 - Use TPM2_Packet_AppendSensitive in TPM2_LoadExternal and extend roundtrip test coverage

Author: aidangarske

src/fwtpm/fwtpm_command.c

@@ -434,7 +434,8 @@ static void FwLookupEntityAuth(FWTPM_CTX* ctx, TPM_HANDLE handle,
  * neither the trip count nor per-iteration work depends on the secret
  * authValSz. Trailing zeros on either side are treated as insignificant
  * (matches TCG reference for authValues padded to nameAlg digest size).
- * Returns 1 on mismatch, 0 on match. */
+ * Returns 1 on mismatch, 0 on match. Precondition: pwSz and avSz must
+ * each be <= TPM_MAX_DIGEST_SIZE; out-of-range inputs fail closed. */
 static int FwCtAuthCompare(const byte* password, int pwSz,
     const byte* authVal, int avSz)
 {
@@ -443,13 +444,19 @@ static int FwCtAuthCompare(const byte* password, int pwSz,
     volatile byte diff = 0;
     int ci;
 
+    if (pwSz < 0 || avSz < 0 ||
+            pwSz > TPM_MAX_DIGEST_SIZE || avSz > TPM_MAX_DIGEST_SIZE) {
+        return 1;
+    }
+
     XMEMSET(zeroAuth, 0, sizeof(zeroAuth));
     avPtr = (authVal != NULL) ? authVal : zeroAuth;
 
     for (ci = 0; ci < TPM_MAX_DIGEST_SIZE; ci++) {
-        /* 0xFF if ci < bound, else 0x00 (bounds are at most 64) */
-        byte pwMask = (byte)-(((unsigned)(ci - pwSz)) >> 31);
-        byte avMask = (byte)-(((unsigned)(ci - avSz)) >> 31);
+        /* 0xFF if ci < bound, else 0x00. Use UINT32 (guaranteed 32-bit
+         * wolfTPM typedef) so the >> 31 shift is always well-defined. */
+        byte pwMask = (byte)-((UINT32)(ci - pwSz) >> 31);
+        byte avMask = (byte)-((UINT32)(ci - avSz) >> 31);
         byte overlap = (byte)(pwMask & avMask);
         /* Overlap region: bytes must match */
         diff |= (byte)((password[ci] ^ avPtr[ci]) & overlap);

src/tpm2.c

@@ -1304,10 +1304,11 @@ TPM_RC TPM2_PCR_Extend(PCR_Extend_In* in)
         int i;
         TPM2_Packet packet;
         CmdInfo_t info = {0,0,0,0};
+        UINT32 count;
         info.inHandleCnt = 1;
         info.flags = (CMD_FLAG_AUTH_USER1);
 
-        UINT32 count = in->digests.count;
+        count = in->digests.count;
         if (count > HASH_COUNT)
             count = HASH_COUNT;
 
@@ -1723,34 +1724,14 @@ TPM_RC TPM2_LoadExternal(LoadExternal_In* in, LoadExternal_Out* out)
         TPM2_Packet_Init(ctx, &packet);
 
         st = TPM2_Packet_AppendAuth(&packet, ctx, &info);
+        /* Reading sensitive.any.size is valid regardless of sensitiveType:
+         * every TPM2B variant in TPMU_SENSITIVE_COMPOSITE has UINT16 size
+         * at offset 0, so the .any view reliably reflects the populated
+         * typed member (common-initial-sequence aliasing). */
         if (in->inPrivate.sensitiveArea.authValue.size > 0 ||
             in->inPrivate.sensitiveArea.seedValue.size > 0 ||
             in->inPrivate.sensitiveArea.sensitive.any.size > 0) {
-
-            in->inPrivate.size = 2 + /* sensitiveType */
-                2 + in->inPrivate.sensitiveArea.authValue.size +
-                2 + in->inPrivate.sensitiveArea.seedValue.size +
-                2 + in->inPrivate.sensitiveArea.sensitive.any.size;
-            TPM2_Packet_AppendU16(&packet, in->inPrivate.size);
-
-            TPM2_Packet_AppendU16(&packet,
-                in->inPrivate.sensitiveArea.sensitiveType);
-            TPM2_Packet_AppendU16(&packet,
-                in->inPrivate.sensitiveArea.authValue.size);
-            TPM2_Packet_AppendBytes(&packet,
-                in->inPrivate.sensitiveArea.authValue.buffer,
-                in->inPrivate.sensitiveArea.authValue.size);
-            TPM2_Packet_AppendU16(&packet,
-                in->inPrivate.sensitiveArea.seedValue.size);
-            TPM2_Packet_AppendBytes(&packet,
-                in->inPrivate.sensitiveArea.seedValue.buffer,
-                in->inPrivate.sensitiveArea.seedValue.size);
-
-            TPM2_Packet_AppendU16(&packet,
-                in->inPrivate.sensitiveArea.sensitive.any.size);
-            TPM2_Packet_AppendBytes(&packet,
-                in->inPrivate.sensitiveArea.sensitive.any.buffer,
-                in->inPrivate.sensitiveArea.sensitive.any.size);
+            TPM2_Packet_AppendSensitive(&packet, &in->inPrivate);
         }
         else {
             TPM2_Packet_AppendU16(&packet, 0);
@@ -3326,22 +3307,24 @@ TPM_RC TPM2_SetCommandCodeAuditStatus(SetCommandCodeAuditStatus_In* in)
         int i;
         TPM2_Packet packet;
         CmdInfo_t info = {0,0,0,0};
+        UINT32 setCount;
+        UINT32 clearCount;
         info.inHandleCnt = 1;
         info.flags = (CMD_FLAG_AUTH_USER1);
 
+        setCount = in->setList.count;
+        clearCount = in->clearList.count;
+        if (setCount > MAX_CAP_CC)
+            setCount = MAX_CAP_CC;
+        if (clearCount > MAX_CAP_CC)
+            clearCount = MAX_CAP_CC;
+
         TPM2_Packet_Init(ctx, &packet);
 
         TPM2_Packet_AppendU32(&packet, in->auth);
 
         TPM2_Packet_AppendAuth(&packet, ctx, &info);
 
-        UINT32 setCount = in->setList.count;
-        UINT32 clearCount = in->clearList.count;
-        if (setCount > MAX_CAP_CC)
-            setCount = MAX_CAP_CC;
-        if (clearCount > MAX_CAP_CC)
-            clearCount = MAX_CAP_CC;
-
         TPM2_Packet_AppendU16(&packet, in->auditAlg);
 
         TPM2_Packet_AppendU32(&packet, setCount);

tests/unit_tests.c

@@ -2059,6 +2059,52 @@ static void test_TPM2_Sensitive_Roundtrip(void)
     AssertIntEQ(XMEMCMP(sensOut.sensitiveArea.sensitive.ecc.buffer,
         rsaPriv, sizeof(rsaPriv)), 0);
 
+    /* KEYEDHASH sensitive roundtrip */
+    XMEMSET(&sensIn, 0, sizeof(sensIn));
+    sensIn.sensitiveArea.sensitiveType = TPM_ALG_KEYEDHASH;
+    sensIn.sensitiveArea.sensitive.bits.size = sizeof(rsaPriv);
+    XMEMCPY(sensIn.sensitiveArea.sensitive.bits.buffer, rsaPriv,
+        sizeof(rsaPriv));
+
+    XMEMSET(buf, 0, sizeof(buf));
+    XMEMSET(&packet, 0, sizeof(packet));
+    packet.buf = buf;
+    packet.size = sizeof(buf);
+
+    TPM2_Packet_AppendSensitive(&packet, &sensIn);
+
+    packet.pos = 0;
+    XMEMSET(&sensOut, 0, sizeof(sensOut));
+    TPM2_Packet_ParseSensitive(&packet, &sensOut);
+
+    AssertIntEQ(sensOut.sensitiveArea.sensitiveType, TPM_ALG_KEYEDHASH);
+    AssertIntEQ(sensOut.sensitiveArea.sensitive.bits.size, sizeof(rsaPriv));
+    AssertIntEQ(XMEMCMP(sensOut.sensitiveArea.sensitive.bits.buffer,
+        rsaPriv, sizeof(rsaPriv)), 0);
+
+    /* SYMCIPHER sensitive roundtrip */
+    XMEMSET(&sensIn, 0, sizeof(sensIn));
+    sensIn.sensitiveArea.sensitiveType = TPM_ALG_SYMCIPHER;
+    sensIn.sensitiveArea.sensitive.sym.size = sizeof(rsaPriv);
+    XMEMCPY(sensIn.sensitiveArea.sensitive.sym.buffer, rsaPriv,
+        sizeof(rsaPriv));
+
+    XMEMSET(buf, 0, sizeof(buf));
+    XMEMSET(&packet, 0, sizeof(packet));
+    packet.buf = buf;
+    packet.size = sizeof(buf);
+
+    TPM2_Packet_AppendSensitive(&packet, &sensIn);
+
+    packet.pos = 0;
+    XMEMSET(&sensOut, 0, sizeof(sensOut));
+    TPM2_Packet_ParseSensitive(&packet, &sensOut);
+
+    AssertIntEQ(sensOut.sensitiveArea.sensitiveType, TPM_ALG_SYMCIPHER);
+    AssertIntEQ(sensOut.sensitiveArea.sensitive.sym.size, sizeof(rsaPriv));
+    AssertIntEQ(XMEMCMP(sensOut.sensitiveArea.sensitive.sym.buffer,
+        rsaPriv, sizeof(rsaPriv)), 0);
+
     printf("Test TPM Wrapper:\tSensitive roundtrip:\t\tPassed\n");
 }