fwtpm: peer-review batch 1 (quick wins)

- NV_Write: reject non-ordinary/PIN NV types (CWE-697 counter reset)
- wolfTPM2_SetAuth/SetAuthHandleName: validate bounds before mutating
  session state so failures don't leave auth.size oversized
- wolfTPM2_SetAuthHandle policyAuth: clamp name.size before XMEMCPY
- EventSequenceComplete: authHandleCnt 1->2 (sequenceHandle was unauth'd)
- NV_ChangeAuth: zero newAuthBuf stack copy before return
- FwRspParamsEnd: guard against rsp->pos < paramStart underflow
- wolfTPM2_ECDHGen: remove duplicate ForceZero

Author: dgarske

src/fwtpm/fwtpm_command.c

@@ -119,7 +119,7 @@ void FwRspParamsEnd(TPM2_Packet* rsp, UINT16 cmdTag,
     int paramSzPos, int paramStart)
 {
     /* Patch parameterSize if sessions */
-    if (paramSzPos >= 0) {
+    if (paramSzPos >= 0 && rsp->pos >= paramStart) {
         int paramSize = rsp->pos - paramStart;
         int savedPos = rsp->pos;
         rsp->pos = paramSzPos;
@@ -9732,6 +9732,18 @@ static TPM_RC FwCmd_NV_Write(FWTPM_CTX* ctx, TPM2_Packet* cmd,
         rc = FW_NV_HANDLE_ERR_2;
     }
 
+    /* Per TPM 2.0 Part 3 §31.3, NV_Write only valid for ordinary and PIN
+     * indices. Counter, Bits, and Extend types must use their dedicated
+     * commands (NV_Increment, NV_SetBits, NV_Extend). Allowing NV_Write on
+     * counters would let an attacker reset a monotonic counter to zero. */
+    if (rc == 0) {
+        UINT32 nt = (nv->nvPublic.attributes & TPMA_NV_TPM_NT) >> 4;
+        if (nt != TPM_NT_ORDINARY && nt != TPM_NT_PIN_FAIL &&
+            nt != TPM_NT_PIN_PASS) {
+            rc = TPM_RC_ATTRIBUTES;
+        }
+    }
+
     if (rc == 0 && (nv->nvPublic.attributes & TPMA_NV_WRITELOCKED)) {
         rc = TPM_RC_NV_LOCKED;
     }
@@ -10158,6 +10170,9 @@ static TPM_RC FwCmd_NV_ChangeAuth(FWTPM_CTX* ctx, TPM2_Packet* cmd,
         FwRspNoParams(rsp, cmdTag);
     }
 
+    /* Zero stack copy of new auth value before returning */
+    TPM2_ForceZero(newAuthBuf, sizeof(newAuthBuf));
+
     return rc;
 }
 
@@ -12064,7 +12079,7 @@ static const FWTPM_CMD_ENTRY fwCmdTable[] = {
     { TPM_CC_HashSequenceStart,  FwCmd_HashSequenceStart,    0, 0, 0, FW_CMD_FLAG_ENC },
     { TPM_CC_SequenceUpdate,     FwCmd_SequenceUpdate,       1, 1, 0, FW_CMD_FLAG_ENC },
     { TPM_CC_SequenceComplete,   FwCmd_SequenceComplete,     1, 1, 0, FW_CMD_FLAG_ENC | FW_CMD_FLAG_DEC },
-    { TPM_CC_EventSequenceComplete, FwCmd_EventSequenceComplete, 2, 1, 0, FW_CMD_FLAG_ENC },
+    { TPM_CC_EventSequenceComplete, FwCmd_EventSequenceComplete, 2, 2, 0, FW_CMD_FLAG_ENC },
     /* --- ECC --- */
 #ifdef HAVE_ECC
     { TPM_CC_ECDH_KeyGen,        FwCmd_ECDH_KeyGen,          1, 0, 0, FW_CMD_FLAG_DEC },

src/tpm2_wrap.c

@@ -1664,15 +1664,17 @@ int wolfTPM2_SetAuth(WOLFTPM2_DEV* dev, int index,
     }
 #endif
 
+    /* Validate bounds before mutating session state so that an oversized
+     * auth value doesn't leave the session with an invalid auth.size that
+     * subsequent code paths could read past the buffer end. */
+    if (auth != NULL && auth->size > sizeof(session->auth.buffer)) {
+        return BUFFER_E;
+    }
     XMEMSET(session, 0, sizeof(TPM2_AUTH_SESSION));
     session->sessionHandle = sessionHandle;
     session->sessionAttributes = sessionAttributes;
     if (auth) {
         session->auth.size = auth->size;
-        /* Note: returns error instead of truncating for security (v3.11+) */
-        if (session->auth.size > sizeof(session->auth.buffer)) {
-            return BUFFER_E;
-        }
         XMEMCPY(session->auth.buffer, auth->buffer, session->auth.size);
     }
     if (name) {
@@ -1731,7 +1733,10 @@ int wolfTPM2_SetAuthHandle(WOLFTPM2_DEV* dev, int index,
             XMEMCPY(&session->auth.buffer[authDigestSz], handle->auth.buffer,
                 handle->auth.size);
             session->name.size = handle->name.size;
-            XMEMCPY(session->name.name, handle->name.name, handle->name.size);
+            if (session->name.size > sizeof(session->name.name)) {
+                session->name.size = sizeof(session->name.name); /* truncate */
+            }
+            XMEMCPY(session->name.name, handle->name.name, session->name.size);
             return TPM_RC_SUCCESS;
         }
         auth = &handle->auth;
@@ -1754,24 +1759,21 @@ int wolfTPM2_SetAuthHandleName(WOLFTPM2_DEV* dev, int index,
     session = &dev->session[index];
 
     if (handle->auth.size > 0) {
+        /* Validate bounds before mutating session.auth so a failure leaves
+         * session->auth.size unchanged rather than oversized. */
+        if (handle->auth.size > sizeof(session->auth.buffer)) {
+            return BUFFER_E;
+        }
         if (session->sessionHandle == TPM_RS_PW) {
             /* password based authentication */
             session->auth.size = handle->auth.size;
-            /* Note: returns error instead of truncating for security (v3.11+) */
-            if (session->auth.size > sizeof(session->auth.buffer)) {
-                return BUFFER_E;
-            }
             XMEMCPY(session->auth.buffer, handle->auth.buffer,
                 session->auth.size);
         }
         else {
             if (handle->policyPass) {
                 /* use policy password directly */
                 session->auth.size = handle->auth.size;
-                /* Note: returns error instead of truncating for security (v3.11+) */
-                if (session->auth.size > sizeof(session->auth.buffer)) {
-                    return BUFFER_E;
-                }
                 XMEMCPY(session->auth.buffer, handle->auth.buffer,
                     session->auth.size);
                 session->policyPass = handle->policyPass;
@@ -5112,7 +5114,6 @@ int wolfTPM2_ECDHGen(WOLFTPM2_DEV* dev, WOLFTPM2_KEY* privKey,
         ecdhOut.pubPoint.size);
 #endif
 
-    TPM2_ForceZero(&ecdhOut, sizeof(ecdhOut));
     TPM2_ForceZero(&ecdhOut, sizeof(ecdhOut));
     return rc;
 }