Harden PCR policy bounds checks

src/fwtpm/fwtpm_command.c

@@ -4396,13 +4396,20 @@ static TPM_RC FwCmd_ClearControl(FWTPM_CTX* ctx, TPM2_Packet* cmd,
     }
 
     if (rc == 0) {
+        int oldDisableClear = ctx->disableClear;
+
     #ifdef DEBUG_WOLFTPM
         printf("fwTPM: ClearControl(auth=0x%x, disable=%d)\n",
             authHandle, disable);
     #endif
         ctx->disableClear = (int)disable;
-        FWTPM_NV_SaveFlags(ctx);
-        FwRspNoParams(rsp, cmdTag);
+        rc = FWTPM_NV_SaveFlags(ctx);
+        if (rc != 0) {
+            ctx->disableClear = oldDisableClear;
+        }
+        else {
+            FwRspNoParams(rsp, cmdTag);
+        }
     }
 
     return rc;

src/tpm2_wrap.c

@@ -11069,14 +11069,22 @@ int wolfTPM2_PolicyPCRMake(TPM_ALG_ID pcrAlg, byte* pcrArray, word32 pcrArraySz,
     const byte* pcrDigest, word32 pcrDigestSz, byte* digest, word32* digestSz)
 {
     int rc;
+    int hashSz;
     TPM2_Packet packet;
     byte buf[sizeof(TPML_PCR_SELECTION)+WC_MAX_DIGEST_SIZE];
     TPML_PCR_SELECTION pcr;
 
     if (digest == NULL || digestSz == NULL || pcrArray == NULL ||
-        pcrArraySz == 0) {
+        pcrArraySz == 0 || (pcrDigest == NULL && pcrDigestSz > 0)) {
+        return BAD_FUNC_ARG;
+    }
+    hashSz = TPM2_GetHashDigestSize(pcrAlg);
+    if (hashSz <= 0) {
         return BAD_FUNC_ARG;
     }
+    if (*digestSz < (word32)hashSz) {
+        return BUFFER_E;
+    }
 
     /* Build PCRS (PCR Count and PCR Selection) */
     XMEMSET(&pcr, 0, sizeof(pcr));
@@ -11087,12 +11095,14 @@ int wolfTPM2_PolicyPCRMake(TPM_ALG_ID pcrAlg, byte* pcrArray, word32 pcrArraySz,
     TPM2_Packet_AppendPCR(&packet, &pcr);
 
     /* Copy the pcrDigest to the end of buffer */
-    if (packet.pos < 0 || (word32)packet.pos > (word32)sizeof(buf) ||
-        pcrDigestSz > (word32)sizeof(buf) - (word32)packet.pos) {
+    if (packet.overflow || packet.pos > packet.size ||
+        pcrDigestSz > (word32)(packet.size - packet.pos)) {
         return BUFFER_E;
     }
-    XMEMCPY(buf + packet.pos, pcrDigest, pcrDigestSz);
-    packet.pos += pcrDigestSz;
+    if (pcrDigestSz > 0) {
+        XMEMCPY(buf + packet.pos, pcrDigest, pcrDigestSz);
+        packet.pos += (int)pcrDigestSz;
+    }
 
     rc = wolfTPM2_PolicyHash(pcrAlg, digest, digestSz, TPM_CC_PolicyPCR,
         buf, packet.pos);

tests/fwtpm_unit_tests.c

@@ -9494,36 +9494,53 @@ static void test_fwtpm_pcr_properties_capability(void)
     rspSize = 0;
     FWTPM_ProcessCommand(&ctx, gCmd, pos, gRsp, &rspSize, 0);
     AssertIntEQ(GetRspRC(gRsp), TPM_RC_SUCCESS);
-    AssertTrue(rspSize > 0 && rspSize <= (int)sizeof(gRsp));
+    if (rspSize < TPM2_HEADER_SIZE + 9 ||
+        rspSize > (int)sizeof(gRsp)) {
+        AssertTrue(rspSize >= TPM2_HEADER_SIZE + 9 &&
+            rspSize <= (int)sizeof(gRsp));
+        FWTPM_Cleanup(&ctx);
+        return;
+    }
 
     /* header(10) + moreData(1) + capability(4) + count(4) + properties */
     p = TPM2_HEADER_SIZE + 1;
     cap = GetU32BE(gRsp + p); p += 4;
     AssertIntEQ(cap, TPM_CAP_PCR_PROPERTIES);
     count = GetU32BE(gRsp + p); p += 4;
     AssertIntGT((int)count, 0);
-    AssertTrue((int)count <= 32); /* bounded by the 32 records requested above */
+    if (count == 0 || count > 32) {
+        AssertTrue(count > 0 && count <= 32);
+        FWTPM_Cleanup(&ctx);
+        return;
+    }
 
     for (i = 0; i < (int)count; i++) {
-        AssertTrue(p + 5 <= rspSize); /* room for tag(4)+size(1) */
+        if (p > rspSize || rspSize - p < 5) {
+            AssertTrue(p <= rspSize && rspSize - p >= 5);
+            FWTPM_Cleanup(&ctx);
+            return;
+        }
         tag = GetU32BE(gRsp + p); p += 4;
         wireSz = gRsp[p]; p += 1;
+        if (wireSz <= 0 || p > rspSize || wireSz > rspSize - p) {
+            AssertTrue(wireSz > 0 && p <= rspSize &&
+                wireSz <= rspSize - p);
+            FWTPM_Cleanup(&ctx);
+            return;
+        }
         selSz = (wireSz > 8) ? 8 : wireSz;
-        AssertTrue(selSz > 0); /* select bytes present; keeps p inside gRsp */
-        AssertTrue(p + selSz <= rspSize);
         if (tag == TPM_PT_PCR_RESET_L0) {
-            memcpy(resetL0, gRsp + p, selSz); gotResetL0 = 1;
+            XMEMCPY(resetL0, gRsp + p, selSz); gotResetL0 = 1;
         }
         else if (tag == TPM_PT_PCR_RESET_L4) {
-            memcpy(resetL4, gRsp + p, selSz); gotResetL4 = 1;
+            XMEMCPY(resetL4, gRsp + p, selSz); gotResetL4 = 1;
         }
         else if (tag == TPM_PT_PCR_EXTEND_L0) {
-            memcpy(extendL0, gRsp + p, selSz); gotExtendL0 = 1;
+            XMEMCPY(extendL0, gRsp + p, selSz); gotExtendL0 = 1;
         }
         else if (tag == TPM_PT_PCR_DRTM_RESET) {
-            memcpy(drtm, gRsp + p, selSz); gotDrtm = 1;
+            XMEMCPY(drtm, gRsp + p, selSz); gotDrtm = 1;
         }
-        AssertTrue(p + wireSz <= rspSize); /* full record present on the wire */
         p += wireSz; /* advance past the select bytes */
     }
 
@@ -9669,6 +9686,54 @@ static void test_fwtpm_clear(void)
     fwtpm_pass("Clear(LOCKOUT):", 0);
 }
 
+#ifndef FWTPM_NO_NV
+static int fail_nv_write(void* ctx, word32 offset, const byte* buf,
+    word32 size)
+{
+    (void)ctx;
+    (void)offset;
+    (void)buf;
+    (void)size;
+    return TPM_RC_FAILURE;
+}
+
+/* ClearControl must not change disableClear or return success when its NV
+ * update fails. */
+static void test_fwtpm_clear_control_nv_failure(void)
+{
+    FWTPM_CTX ctx;
+    FWTPM_NV_HAL oldHal, failHal;
+    int rc, rspSize, pos;
+
+    memset(&ctx, 0, sizeof(ctx));
+    AssertIntEQ(fwtpm_test_startup(&ctx), 0);
+    AssertIntEQ(ctx.disableClear, 0);
+
+    oldHal = ctx.nvHal;
+    failHal = oldHal;
+    failHal.write = fail_nv_write;
+    AssertIntEQ(FWTPM_NV_SetHAL(&ctx, &failHal), TPM_RC_SUCCESS);
+
+    pos = 0;
+    PutU16BE(gCmd + pos, TPM_ST_SESSIONS); pos += 2;
+    PutU32BE(gCmd + pos, 0); pos += 4;
+    PutU32BE(gCmd + pos, TPM_CC_ClearControl); pos += 4;
+    PutU32BE(gCmd + pos, TPM_RH_PLATFORM); pos += 4;
+    pos = AppendPwAuth(gCmd, pos, NULL, 0);
+    gCmd[pos++] = 1;
+    PutU32BE(gCmd + 2, (UINT32)pos);
+    rspSize = 0;
+    rc = FWTPM_ProcessCommand(&ctx, gCmd, pos, gRsp, &rspSize, 0);
+    AssertIntEQ(rc, TPM_RC_SUCCESS);
+    AssertIntEQ(GetRspRC(gRsp), TPM_RC_FAILURE);
+    AssertIntEQ(ctx.disableClear, 0);
+
+    AssertIntEQ(FWTPM_NV_SetHAL(&ctx, &oldHal), TPM_RC_SUCCESS);
+    FWTPM_Cleanup(&ctx);
+    fwtpm_pass("ClearControl NV failure rollback:", 0);
+}
+#endif /* !FWTPM_NO_NV */
+
 /* Per Part 3 Sec.24.6 Table 134, TPM2_Clear has Auth Index 1, Auth Role USER
  * on @authHandle (TPM_RH_LOCKOUT or TPM_RH_PLATFORM). NO_SESSIONS leaves
  * cmdAuthCnt at 0, skipping every auth enforcement loop in
@@ -12285,6 +12350,9 @@ int fwtpm_unit_tests(int argc, char *argv[])
     test_fwtpm_sessions_short_authcount_rejected();
 #endif /* !FWTPM_NO_HASH_CMDS */
     test_fwtpm_sessions_missing_authsize_command_size();
+#ifndef FWTPM_NO_NV
+    test_fwtpm_clear_control_nv_failure();
+#endif /* !FWTPM_NO_NV */
     test_fwtpm_clear();
 
     printf("\nAll fwTPM unit tests passed!\n");

tests/unit_tests.c

@@ -1164,6 +1164,18 @@ static void test_TPM2_PCRSel(void)
 static void test_TPM2_Policy_NULL_Args(void)
 {
     int rc;
+    #ifndef WOLFTPM2_NO_WOLFCRYPT
+    const byte expectedDigest[TPM_SHA256_DIGEST_SIZE] = {
+        0x4e, 0x35, 0x3a, 0xbb, 0x5b, 0x73, 0xa0, 0x8b,
+        0x9c, 0x1c, 0x53, 0x1e, 0x02, 0x27, 0x9a, 0xa9,
+        0x39, 0xb6, 0xb5, 0x61, 0x2d, 0xe3, 0x59, 0x6d,
+        0x74, 0xfe, 0xd8, 0x99, 0x9b, 0xef, 0x13, 0xdf
+    };
+    byte pcrArray[1] = {0};
+    byte pcrDigest[sizeof(TPML_PCR_SELECTION) + WC_MAX_DIGEST_SIZE + 1] = {0};
+    byte digest[TPM_SHA256_DIGEST_SIZE];
+    word32 digestSz = (word32)sizeof(digest);
+    #endif
 
     /* Test NULL input handling for policy commands */
     rc = TPM2_PolicyPhysicalPresence(NULL);
@@ -1175,6 +1187,55 @@ static void test_TPM2_Policy_NULL_Args(void)
     rc = TPM2_PolicyPassword(NULL);
     AssertIntEQ(rc, BAD_FUNC_ARG);
 
+    #ifndef WOLFTPM2_NO_WOLFCRYPT
+    rc = wolfTPM2_PolicyPCRMake(TPM_ALG_SHA256, pcrArray,
+        (word32)sizeof(pcrArray), NULL, 0, NULL, &digestSz);
+    AssertIntEQ(rc, BAD_FUNC_ARG);
+
+    rc = wolfTPM2_PolicyPCRMake(TPM_ALG_SHA256, pcrArray,
+        (word32)sizeof(pcrArray), NULL, 0, digest, NULL);
+    AssertIntEQ(rc, BAD_FUNC_ARG);
+
+    rc = wolfTPM2_PolicyPCRMake(TPM_ALG_SHA256, NULL,
+        (word32)sizeof(pcrArray), NULL, 0, digest, &digestSz);
+    AssertIntEQ(rc, BAD_FUNC_ARG);
+
+    rc = wolfTPM2_PolicyPCRMake(TPM_ALG_SHA256, pcrArray, 0,
+        NULL, 0, digest, &digestSz);
+    AssertIntEQ(rc, BAD_FUNC_ARG);
+
+    rc = wolfTPM2_PolicyPCRMake(TPM_ALG_NULL, pcrArray,
+        (word32)sizeof(pcrArray), NULL, 0, digest, &digestSz);
+    AssertIntEQ(rc, BAD_FUNC_ARG);
+
+    /* A nonzero PCR digest size requires backing digest data. */
+    rc = wolfTPM2_PolicyPCRMake(TPM_ALG_SHA256, pcrArray,
+        (word32)sizeof(pcrArray), NULL, 1, digest, &digestSz);
+    AssertIntEQ(rc, BAD_FUNC_ARG);
+
+    /* An empty PCR digest is valid. */
+    XMEMSET(digest, 0, sizeof(digest));
+    digestSz = (word32)sizeof(digest);
+    rc = wolfTPM2_PolicyPCRMake(TPM_ALG_SHA256, pcrArray,
+        (word32)sizeof(pcrArray), NULL, 0, digest, &digestSz);
+    AssertIntEQ(rc, TPM_RC_SUCCESS);
+    AssertIntEQ(digestSz, (word32)sizeof(expectedDigest));
+    AssertIntEQ(XMEMCMP(digest, expectedDigest, sizeof(expectedDigest)), 0);
+
+    /* Reject a caller-declared output capacity below the hash size. */
+    digestSz = (word32)sizeof(digest) - 1;
+    rc = wolfTPM2_PolicyPCRMake(TPM_ALG_SHA256, pcrArray,
+        (word32)sizeof(pcrArray), NULL, 0, digest, &digestSz);
+    AssertIntEQ(rc, BUFFER_E);
+
+    /* Reject a PCR digest that cannot fit in the assembly buffer. */
+    digestSz = (word32)sizeof(digest);
+    rc = wolfTPM2_PolicyPCRMake(TPM_ALG_SHA256, pcrArray,
+        (word32)sizeof(pcrArray), pcrDigest, (word32)sizeof(pcrDigest), digest,
+        &digestSz);
+    AssertIntEQ(rc, BUFFER_E);
+    #endif
+
     printf("Test TPM2:        %-40s Passed\n", "Policy NULL Args:");
 }
 

wolftpm/tpm2_wrap.h

@@ -4897,17 +4897,24 @@ WOLFTPM_API int wolfTPM2_PolicyRefMake(TPM_ALG_ID pcrAlg, byte* digest, word32*
 
     \brief Utility for generating a policy PCR digest.
 
+    \note To start a fresh policy chain, zero the digest buffer and set
+        digestSz to the selected hash size before calling.
+
     \return TPM_RC_SUCCESS: successful
-    \return INPUT_SIZE_E: policyDigestSz is too small to hold the returned digest
+    \return BUFFER_E: digest is too small for the selected hash or the PCR
+        policy input exceeds the internal assembly buffer
     \return BAD_FUNC_ARG: check the provided arguments
 
-    \param pcrAlg the hash algorithm to use with pcr policy
-    \param pcrArray optional array of pcrs to be used when creating the tpm object
-    \param pcrArraySz length of the pcrArray
-    \param pcrDigest digest for the PCR(s) collected (can get using wolfTPM2_PCRGetDigest)
+    \param pcrAlg the supported hash algorithm to use with the PCR policy
+    \param pcrArray non-NULL array of PCRs to include in the policy
+    \param pcrArraySz number of entries in pcrArray; must be greater than zero
+    \param pcrDigest digest for the PCR(s) collected (can get using
+        wolfTPM2_PCRGetDigest); required when pcrDigestSz is nonzero
     \param pcrDigestSz size of the PCR digest
-    \param digest input/out digest
-    \param digestSz input/out digest size
+    \param digest input/output policy digest buffer
+    \param digestSz input/output: current digest size and buffer capacity on
+        input, which must be at least the selected hash size; selected hash
+        size on output
 
     \sa wolfTPM2_PolicyPCRMake
     \sa wolfTPM2_PolicyAuthorizeMake