Fix six Fenrir findings in the fwTPM and the examples

examples/boot/secure_rot.c

@@ -83,6 +83,7 @@ int TPM2_Boot_SecureROT_Example(void* userCtx, int argc, char *argv[])
     enum wc_HashType hashType = WC_HASH_TYPE_SHA256;
     byte digest[WC_MAX_DIGEST_SIZE];
     int digestSz = 0;
+    int i;
     byte authBuf[WC_SHA256_DIGEST_SIZE];
     int authBufSz = 0;
 
@@ -230,7 +231,18 @@ int TPM2_Boot_SecureROT_Example(void* userCtx, int argc, char *argv[])
                 nvAttributes, digestSz, authBuf, authBufSz);
             if (rc == TPM_RC_NV_DEFINED) {
                 printf("Warning: NV Index 0x%x already exists!\n", nvIndex);
-                rc = 0;
+                /* Only reuse an index this tool would have created, so a
+                 * planted index cannot hold the root of trust */
+                rc = wolfTPM2_NVReadPublic(&dev, nvIndex, &nvPublic);
+                if (rc == 0 && ((nvPublic.attributes & ~(TPMA_NV_WRITTEN |
+                        TPMA_NV_WRITELOCKED | TPMA_NV_READLOCKED)) !=
+                        nvAttributes || nvPublic.dataSize != digestSz ||
+                        nvPublic.nameAlg != WOLFTPM2_WRAP_DIGEST ||
+                        nvPublic.authPolicy.size != 0)) {
+                    printf("NV Index 0x%x attributes, size, name algorithm "
+                        "or policy do not match\n", nvIndex);
+                    rc = TPM_RC_NV_DEFINED;
+                }
             }
             wolfTPM2_SetAuthHandle(&dev, 0, &nv.handle);
         }
@@ -262,7 +274,10 @@ int TPM2_Boot_SecureROT_Example(void* userCtx, int argc, char *argv[])
     rc = wolfTPM2_NVReadAuth(&dev, &nv, nvIndex, digest, (word32*)&digestSz, 0);
     if (rc == 0) {
         printf("Read Public Key Hash (%d)\n", digestSz);
-        TPM2_PrintBin(digest, digestSz);
+        for (i = 0; i < digestSz; i++) {
+            printf("%02x", digest[i]);
+        }
+        printf("\n");
     }
     else if ((rc & RC_MAX_FMT1) == TPM_RC_HANDLE) {
         printf("NV index does not exist\n");

examples/keygen/create_primary.c

@@ -335,10 +335,12 @@ int TPM2_CreatePrimaryKey_Example(void* userCtx, int argc, char *argv[])
 
     if (persistHandle > 0) {
     #ifndef WOLFTPM_WINAPI
-        /* Move storage key into persistent NV */
+        /* Move storage key into persistent NV. EvictControl takes the
+         * owner or platform hierarchy, never endorsement. */
         printf("Storing Primary key to handle 0x%08x\n", persistHandle);
-        rc = wolfTPM2_NVStoreKey(&dev, hierarchy, primary,
-            persistHandle);
+        rc = wolfTPM2_NVStoreKey(&dev,
+            hierarchy == TPM_RH_PLATFORM ? TPM_RH_PLATFORM : TPM_RH_OWNER,
+            primary, persistHandle);
         if (rc != TPM_RC_SUCCESS) goto exit;
     #else
         printf("Windows TBS does not allow persisting handles to "

examples/management/flush.c

@@ -32,6 +32,7 @@
 #include <examples/tpm_test.h>
 
 #include <stdio.h>
+#include <errno.h>
 
 #ifndef WOLFTPM2_NO_WRAPPER
 /******************************************************************************/
@@ -59,7 +60,25 @@ int TPM2_Flush_Tool(void* userCtx, int argc, char *argv[])
     FlushContext_In flushCtx;
 
     if (argc == 2) {
-        handle = (word32)XSTRTOUL(argv[1], NULL, 0);
+        char* end = NULL;
+        unsigned long val;
+        if (XSTRCMP(argv[1], "-?") == 0 ||
+            XSTRCMP(argv[1], "-h") == 0 ||
+            XSTRCMP(argv[1], "--help") == 0) {
+            usage();
+            return 0;
+        }
+        /* strtoul accepts a sign and wraps it, so require a leading digit */
+        errno = 0;
+        val = XSTRTOUL(argv[1], &end, 0);
+        if (argv[1][0] < '0' || argv[1][0] > '9' || *end != '\0' ||
+                errno == ERANGE || val == 0 ||
+                (unsigned long)(word32)val != val) {
+            printf("Invalid handle %s\n", argv[1]);
+            usage();
+            return -1;
+        }
+        handle = (TPM_HANDLE)val;
     }
     else if (argc == 1) {
         allTransientObjects = 1;
@@ -78,7 +97,7 @@ int TPM2_Flush_Tool(void* userCtx, int argc, char *argv[])
     }
     printf("wolfTPM2_Init: success\n");
 
-    if (allTransientObjects || handle == 0) {
+    if (allTransientObjects) {
         /* Flush key objects */
         for (handle=0x80000000; handle < 0x8000000A; handle++) {
             flushCtx.flushHandle = handle;

examples/run_examples.sh

@@ -121,6 +121,9 @@ RESULT=$?
 ./examples/keygen/create_primary -rsa -oh -auth=ThisIsMyStorageKeyAuth -store=0x81000200 >> $TPMPWD/run.out 2>&1
 RESULT=$?
 [ $RESULT -ne 0 ] && echo -e "create primary owner rsa key stored failed! $RESULT" && exit 1
+./examples/keygen/create_primary -rsa -eh -store=0x81000203 >> $TPMPWD/run.out 2>&1
+RESULT=$?
+[ $RESULT -ne 0 ] && echo -e "create primary endorsement rsa key stored failed! $RESULT" && exit 1
 
 # Dictionary Attack (DA / noDA) check. Signs with a DA-protected key (rides
 # through any TPM_RC_RETRY) and shows a noDA key never trips lockout. The
@@ -147,7 +150,27 @@ if [ $WOLFCRYPT_ENABLE -eq 1 ]; then
     RESULT=$?
     [ $RESULT -ne 0 ] && echo -e "certify RSA IDevID with IAK failed! $RESULT" && exit 1
 
+    # Help and a bad handle must not flush the keys loaded above
+    ./examples/management/flush -h >> $TPMPWD/run.out 2>&1
+    RESULT=$?
+    [ $RESULT -ne 0 ] && echo -e "flush help failed! $RESULT" && exit 1
+    # A bad handle is rejected before the tool contacts the TPM. The last value
+    # overflows unsigned long on 32 and 64 bit targets.
+    for BADHANDLE in notahandle -2147483648 0x180000000 0x10000000000000000; do
+        FLUSH_OUT=$(./examples/management/flush $BADHANDLE 2>&1)
+        RESULT=$?
+        echo "$FLUSH_OUT" >> $TPMPWD/run.out
+        [ $RESULT -eq 0 ] && echo -e "flush accepted handle $BADHANDLE!" && exit 1
+        case "$FLUSH_OUT" in
+            *"Preparing to free"*)
+                echo -e "flush contacted the TPM for handle $BADHANDLE!" && exit 1 ;;
+            *"Invalid handle"*) ;;
+            *) echo -e "flush did not reject handle $BADHANDLE!" && exit 1 ;;
+        esac
+    done
     ./examples/management/flush 0x80000000 >> $TPMPWD/run.out 2>&1
+    RESULT=$?
+    [ $RESULT -ne 0 ] && echo -e "flush 0x80000000 failed! $RESULT" && exit 1
     ./examples/management/flush 0x80000001 >> $TPMPWD/run.out 2>&1
 
     ./examples/keygen/create_primary -ecc -eh -iak -keep >> $TPMPWD/run.out 2>&1
@@ -1051,6 +1074,39 @@ if [ $WOLFCRYPT_ENABLE -eq 1 ] && [ $WOLFCRYPT_DEFAULT -eq 0 ] && [ $NO_FILESYST
     ./examples/boot/secure_rot -nvindex=0x1400201 >> $TPMPWD/run.out 2>&1
     RESULT=$?
     [ $RESULT -eq 0 ] && echo -e "secure rot write ecc384 read no auth! $RESULT" && exit 1
+
+    # A pre-existing index that differs is rejected and left unchanged. The hash
+    # is shorter than the index, so without the check the write would succeed.
+    ./examples/boot/secure_rot -nvindex=0x1400203 -authstr=test -hash=e77dd3112a27948a3f2d87f32dc69ebeed0b3344c5d7726f5742f4f0c0f451aa >> $TPMPWD/run.out 2>&1
+    RESULT=$?
+    [ $RESULT -ne 0 ] && echo -e "secure rot create 0x1400203 failed! $RESULT" && exit 1
+    ROT_OUT=$(./examples/boot/secure_rot -nvindex=0x1400203 -authstr=test -hash=be4213f8b3b986639e69ed0ea8b49d94aabbccdd 2>&1)
+    RESULT=$?
+    echo "$ROT_OUT" >> $TPMPWD/run.out
+    [ $RESULT -eq 0 ] && echo -e "secure rot accepted an index of another size!" && exit 1
+    case "$ROT_OUT" in
+        *"do not match"*) ;;
+        *) echo -e "secure rot did not report the index mismatch!" && exit 1 ;;
+    esac
+    ROT_OUT=$(./examples/boot/secure_rot -nvindex=0x1400203 -authstr=test 2>&1)
+    echo "$ROT_OUT" >> $TPMPWD/run.out
+    case "$ROT_OUT" in
+        *e77dd3112a27948a3f2d87f32dc69ebeed0b3344c5d7726f5742f4f0c0f451aa*) ;;
+        *) echo -e "secure rot changed a rejected index!" && exit 1 ;;
+    esac
+
+    # Same size, but other attributes and a policy
+    ./examples/nvram/extend -nvindex=0x1400204 -aes >> $TPMPWD/run.out 2>&1
+    RESULT=$?
+    [ $RESULT -ne 0 ] && echo -e "nv extend create 0x1400204 failed! $RESULT" && exit 1
+    ROT_OUT=$(./examples/boot/secure_rot -nvindex=0x1400204 -authstr=test -hash=e77dd3112a27948a3f2d87f32dc69ebeed0b3344c5d7726f5742f4f0c0f451aa 2>&1)
+    RESULT=$?
+    echo "$ROT_OUT" >> $TPMPWD/run.out
+    [ $RESULT -eq 0 ] && echo -e "secure rot accepted an index with other attributes!" && exit 1
+    case "$ROT_OUT" in
+        *"do not match"*) ;;
+        *) echo -e "secure rot did not report the attribute mismatch!" && exit 1 ;;
+    esac
 fi
 
 # Seal/Unseal (PCR Policy)

src/fwtpm/fwtpm_command.c

@@ -21239,12 +21239,13 @@ static TPM_RC FwCheckPolicyAssertions(FWTPM_CTX* ctx,
             }
         }
     }
-    /* PolicyTemplate binds only the creation template (Part 3 Sec.23.19);
-     * it does not restrict the command, that is PolicyCommandCode's role. */
-    if (rc == 0 && sess->templateHash.size > 0 &&
-        (cmdCode == TPM_CC_Create || cmdCode == TPM_CC_CreatePrimary ||
-         cmdCode == TPM_CC_CreateLoaded)) {
-        if (FwComputeTemplateHash(sess->authHash, cmdBuf, cmdSize, cpStart,
+    /* A template hash can only match Create, CreatePrimary or CreateLoaded,
+     * so any other command fails the policy, as in the reference
+     * CompareTemplateHash. */
+    if (rc == 0 && sess->templateHash.size > 0) {
+        if ((cmdCode != TPM_CC_Create && cmdCode != TPM_CC_CreatePrimary &&
+                cmdCode != TPM_CC_CreateLoaded) ||
+            FwComputeTemplateHash(sess->authHash, cmdBuf, cmdSize, cpStart,
                 digest, &digestSz) != 0) {
             rc = TPM_RC_POLICY_FAIL;
         }
@@ -22461,6 +22462,13 @@ int FWTPM_ProcessCommand(FWTPM_CTX* ctx,
                 rspBuf[rspHandleEnd + 3]);
         }
         rspParamStart = rspHandleEnd + 4;
+        /* Never trust paramSize past what the handler wrote */
+        if (rspParamStart > rspPkt.pos) {
+            rspParamSzVal = 0;
+        }
+        else if (rspParamSzVal > (UINT32)(rspPkt.pos - rspParamStart)) {
+            rspParamSzVal = (UINT32)(rspPkt.pos - rspParamStart);
+        }
 #ifndef FWTPM_NO_PARAM_ENC
         rspParamEnd = rspParamStart + (int)rspParamSzVal;
 #endif

src/fwtpm/fwtpm_tis.c

@@ -84,6 +84,7 @@ static void TisHandleRegAccess(FWTPM_CTX* ctx, FWTPM_TIS_REGS* regs)
      * (STS/GO/FIFO). ACCESS and read-only ID/INT registers are not gated. */
     int isOwner;
     FWTPM_DECLARE_BUF(localCmd, FWTPM_TIS_FIFO_SIZE);
+    FWTPM_DECLARE_BUF(localRsp, FWTPM_TIS_FIFO_SIZE);
 
     isOwner = (ctx->tisLocality >= 0 && loc == ctx->tisLocality);
 
@@ -151,14 +152,18 @@ static void TisHandleRegAccess(FWTPM_CTX* ctx, FWTPM_TIS_REGS* regs)
                         FWTPM_TIS_BURST_COUNT);
                 }
                 if (val & FWTPM_STS_GO) {
-                    /* Execute command - copy to local buffer first to
-                     * prevent TOCTOU if cmd_buf is in shared memory */
+                    /* Execute command - use local command and response
+                     * buffers to prevent TOCTOU if the registers are in
+                     * shared memory */
                     UINT32 localCmdLen;
                     int rspSize = 0;
                     int procRc = TPM_RC_FAILURE;
                     TPM_RC rc = TPM_RC_SUCCESS; /* FWTPM_ALLOC_BUF sets on OOM */
 
                     FWTPM_ALLOC_BUF(localCmd, FWTPM_TIS_FIFO_SIZE);
+                    if (rc == TPM_RC_SUCCESS) {
+                        FWTPM_ALLOC_BUF(localRsp, FWTPM_TIS_FIFO_SIZE);
+                    }
                     if (rc == TPM_RC_SUCCESS) {
                         localCmdLen = regs->cmd_len;
                         if (localCmdLen > FWTPM_TIS_FIFO_SIZE) {
@@ -172,12 +177,24 @@ static void TisHandleRegAccess(FWTPM_CTX* ctx, FWTPM_TIS_REGS* regs)
 
                         /* Gated to the owner above, so loc == ctx->tisLocality:
                          * execute under the addressed (owning) locality. */
-                        rspSize = (int)sizeof(regs->rsp_buf);
+                        rspSize = FWTPM_TIS_FIFO_SIZE;
                         procRc = FWTPM_ProcessCommand(ctx,
                             localCmd, (int)localCmdLen,
-                            regs->rsp_buf, &rspSize, loc);
-                        FWTPM_FREE_BUF(localCmd);
+                            localRsp, &rspSize, loc);
+                        if (procRc == TPM_RC_SUCCESS && rspSize > 0 &&
+                                rspSize <= FWTPM_TIS_FIFO_SIZE) {
+                            XMEMCPY(regs->rsp_buf, localRsp, rspSize);
+                        }
+                        else {
+                            rspSize = 0;
+                        }
                     }
+                    TPM2_ForceZero(localRsp,
+                        FWTPM_SIZEOF_BUF(localRsp, FWTPM_TIS_FIFO_SIZE));
+                    TPM2_ForceZero(localCmd,
+                        FWTPM_SIZEOF_BUF(localCmd, FWTPM_TIS_FIFO_SIZE));
+                    FWTPM_FREE_BUF(localRsp);
+                    FWTPM_FREE_BUF(localCmd);
                     if (procRc != TPM_RC_SUCCESS || rspSize == 0) {
                         /* Build minimal error response
                          * TPM_ST_NO_SESSIONS = 0x8001 (big-endian) */