Skoll review fixes

Author: aidangarske

src/fwtpm/fwtpm_command.c

@@ -11129,8 +11129,12 @@ static TPM_RC FwParseAttestParams(TPM2_Packet* cmd, int cmdSize,
          * hashAlg per Part 2 Sec. 11.2.1.5. */
         if (*sigScheme == TPM_ALG_ECDAA) {
             UINT16 ecdaaCount;
-            TPM2_Packet_ParseU16(cmd, &ecdaaCount);
-            (void)ecdaaCount;
+            if (cmd->pos + 2 > cmdSize)
+                rc = TPM_RC_COMMAND_SIZE;
+            if (rc == 0) {
+                TPM2_Packet_ParseU16(cmd, &ecdaaCount);
+                (void)ecdaaCount;
+            }
         }
     }
 
@@ -11441,8 +11445,12 @@ static TPM_RC FwCmd_CertifyCreation(FWTPM_CTX* ctx, TPM2_Packet* cmd,
          * hashAlg per Part 2 Sec. 11.2.1.5. */
         if (sigScheme == TPM_ALG_ECDAA) {
             UINT16 ecdaaCount;
-            TPM2_Packet_ParseU16(cmd, &ecdaaCount);
-            (void)ecdaaCount;
+            if (cmd->pos + 2 > cmdSize)
+                rc = TPM_RC_COMMAND_SIZE;
+            if (rc == 0) {
+                TPM2_Packet_ParseU16(cmd, &ecdaaCount);
+                (void)ecdaaCount;
+            }
         }
     }
 
@@ -11655,13 +11663,6 @@ static TPM_RC FwCmd_NV_Certify(FWTPM_CTX* ctx, TPM2_Packet* cmd,
             &qualifyingData, &sigScheme, &sigHashAlg);
     }
 
-    /* Per Part 3 Sec. 31.16.1: TPM_RH_NULL signHandle requires non-NULL
-     * scheme and hash algorithm, otherwise return TPM_RC_SCHEME. */
-    if (rc == 0 && signHandle == TPM_RH_NULL &&
-            (sigScheme == TPM_ALG_NULL || sigHashAlg == TPM_ALG_NULL)) {
-        rc = TPM_RC_SCHEME;
-    }
-
     /* size, offset */
     if (rc == 0) {
         TPM2_Packet_ParseU16(cmd, &readSize);
@@ -11739,10 +11740,13 @@ static TPM_RC FwCmd_NV_Certify(FWTPM_CTX* ctx, TPM2_Packet* cmd,
             int hashSz;
             enum wc_HashType wcDigH;
 
-            /* Resolve hash from signing key when scheme/hash is NULL */
+            /* Resolve hash from signing key when scheme/hash is NULL.
+             * keyScheme is filled in by the by-pointer interface but
+             * unused here - we only need the resolved hashAlg. */
             if (hashAlg == TPM_ALG_NULL) {
                 UINT16 keyScheme = TPM_ALG_NULL;
                 FwResolveSignScheme(sigObj, &keyScheme, &hashAlg);
+                (void)keyScheme;
             }
             wcDigH = FwGetWcHashType(hashAlg);
             hashSz = TPM2_GetHashDigestSize(hashAlg);

src/tpm2_packet.c

@@ -786,13 +786,28 @@ void TPM2_Packet_ParsePoint(TPM2_Packet* packet, TPM2B_ECC_POINT* point)
 
     TPM2_Packet_ParseU16(packet, &point->size);
     pointStartPos = (packet != NULL) ? packet->pos : 0;
-    TPM2_Packet_ParseEccPoint(packet, &point->point);
+    /* Skip the inner ECC point parse when the outer size is zero. A
+     * malformed blob with size=0 but nonzero inner x.size/y.size would
+     * otherwise advance packet->pos and desync subsequent fields. */
+    if (point->size > 0) {
+        TPM2_Packet_ParseEccPoint(packet, &point->point);
+    }
+    else {
+        XMEMSET(&point->point, 0, sizeof(point->point));
+    }
 
     /* Resync packet position to end of declared outer size so inner
-     * x.size / y.size disagreement can't desynchronize subsequent fields */
-    if (packet != NULL && point->size > 0 &&
-            pointStartPos + point->size <= packet->size) {
-        packet->pos = pointStartPos + point->size;
+     * x.size / y.size disagreement can't desynchronize subsequent fields.
+     * If the declared outer size runs past the buffer, clamp to packet end
+     * so subsequent reads return an out-of-bounds sentinel rather than
+     * leaving the position wherever the inner parses landed. */
+    if (packet != NULL) {
+        if (pointStartPos + point->size <= packet->size) {
+            packet->pos = pointStartPos + point->size;
+        }
+        else {
+            packet->pos = packet->size;
+        }
     }
 }
 
@@ -1149,10 +1164,16 @@ void TPM2_Packet_ParsePublic(TPM2_Packet* packet, TPM2B_PUBLIC* pub)
 
         /* Resync packet position to end of declared outer size so inner
          * parses can't cause field drift if declared size and actual
-         * inner consumption disagree */
-        if (packet != NULL &&
-                pubStartPos + pub->size <= packet->size) {
-            packet->pos = pubStartPos + pub->size;
+         * inner consumption disagree. If the declared outer size runs
+         * past the buffer, clamp to packet end so subsequent reads
+         * return an out-of-bounds sentinel. */
+        if (packet != NULL) {
+            if (pubStartPos + pub->size <= packet->size) {
+                packet->pos = pubStartPos + pub->size;
+            }
+            else {
+                packet->pos = packet->size;
+            }
         }
     }
 }

src/tpm2_wrap.c

@@ -3550,6 +3550,11 @@ int wolfTPM2_LoadEccPublicKey_ex(WOLFTPM2_DEV* dev, WOLFTPM2_KEY* key,
         return BUFFER_E;
     if (eccPubYSz > sizeof(pub.publicArea.unique.ecc.y.buffer))
         return BUFFER_E;
+    /* TPMS_SCHEME_ECDAA requires a 'count' field this helper cannot
+     * supply. Callers needing ECDAA must build TPM2B_PUBLIC manually and
+     * use wolfTPM2_LoadPublicKey directly. */
+    if (scheme == TPM_ALG_ECDAA)
+        return BAD_FUNC_ARG;
 
     XMEMSET(&pub, 0, sizeof(pub));
     pub.publicArea.type = TPM_ALG_ECC;
@@ -5521,8 +5526,10 @@ int wolfTPM2_RsaEncrypt(WOLFTPM2_DEV* dev, WOLFTPM2_KEY* key,
     #endif
     }
 
-    /* Plaintext copy lingers in the rsaEncIn stack frame after return -
-     * scrub it on every exit path. */
+    /* Plaintext copy lingers in rsaEncIn after the XMEMCPY above. Scrub
+     * before returning so the stack frame doesn't leak it. The early
+     * BUFFER_E returns above happen before the XMEMCPY so they don't
+     * need this path. */
     TPM2_ForceZero(&rsaEncIn, sizeof(rsaEncIn));
     return rc;
 }
@@ -5725,9 +5732,14 @@ int wolfTPM2_UnloadHandle(WOLFTPM2_DEV* dev, WOLFTPM2_HANDLE* handle)
 /* nv is the populated handle and auth */
 /* auth and authSz are optional NV authentication */
 /* authPolicy and authPolicySz are optional policy digest */
-int wolfTPM2_NVCreateAuthPolicy(WOLFTPM2_DEV* dev, WOLFTPM2_HANDLE* parent,
+/* nameAlg is the index name hash algorithm; must match the hash that
+ *   produced authPolicy when one is supplied, otherwise the policy can
+ *   never be satisfied. Pass TPM_ALG_NULL to use WOLFTPM2_WRAP_DIGEST
+ *   (default) or auto-infer from authPolicySz for SHA-only policies. */
+int wolfTPM2_NVCreateAuthPolicy_ex(WOLFTPM2_DEV* dev, WOLFTPM2_HANDLE* parent,
     WOLFTPM2_NV* nv, word32 nvIndex, word32 nvAttributes, word32 maxSize,
-    const byte* auth, int authSz, const byte* authPolicy, int authPolicySz)
+    const byte* auth, int authSz, const byte* authPolicy, int authPolicySz,
+    TPMI_ALG_HASH nameAlg)
 {
     int rc, rctmp;
     NV_DefineSpace_In in;
@@ -5758,30 +5770,36 @@ int wolfTPM2_NVCreateAuthPolicy(WOLFTPM2_DEV* dev, WOLFTPM2_HANDLE* parent,
         XMEMCPY(in.auth.buffer, auth, in.auth.size);
     }
     in.publicInfo.nvPublic.nvIndex = nvIndex;
-    /* When a policy digest is supplied, nameAlg must match the hash that
-     * produced it - otherwise the policy can never be satisfied. Infer
-     * nameAlg from authPolicySz when possible, defaulting to
-     * WOLFTPM2_WRAP_DIGEST for the no-policy case. */
-    if (authPolicy != NULL && authPolicySz > 0) {
+    if (nameAlg != TPM_ALG_NULL) {
+        /* Caller supplied an explicit nameAlg; use it as-is and only
+         * sanity-check against the policy digest size. */
+        if (authPolicy != NULL && authPolicySz > 0) {
+            int expectSz = TPM2_GetHashDigestSize(nameAlg);
+            if (expectSz <= 0 || expectSz != authPolicySz)
+                return BAD_FUNC_ARG;
+        }
+        in.publicInfo.nvPublic.nameAlg = nameAlg;
+    }
+    else if (authPolicy != NULL && authPolicySz > 0) {
+        /* No explicit nameAlg supplied. Infer from policy digest size for
+         * SHA-only policies. The TPM stores the policy digest verbatim,
+         * so we don't need wolfCrypt to support the hash here - the
+         * caller has already computed the digest. SM3-256 / SHA3-256
+         * also produce 32 bytes; callers needing those must pass
+         * nameAlg explicitly via the _ex API. */
         switch (authPolicySz) {
-        #ifndef NO_SHA
             case TPM_SHA_DIGEST_SIZE:
                 in.publicInfo.nvPublic.nameAlg = TPM_ALG_SHA1;
                 break;
-        #endif
             case TPM_SHA256_DIGEST_SIZE:
                 in.publicInfo.nvPublic.nameAlg = TPM_ALG_SHA256;
                 break;
-        #ifdef WOLFSSL_SHA384
             case TPM_SHA384_DIGEST_SIZE:
                 in.publicInfo.nvPublic.nameAlg = TPM_ALG_SHA384;
                 break;
-        #endif
-        #ifdef WOLFSSL_SHA512
             case TPM_SHA512_DIGEST_SIZE:
                 in.publicInfo.nvPublic.nameAlg = TPM_ALG_SHA512;
                 break;
-        #endif
             default:
                 return BAD_FUNC_ARG;
         }
@@ -5830,6 +5848,15 @@ int wolfTPM2_NVCreateAuthPolicy(WOLFTPM2_DEV* dev, WOLFTPM2_HANDLE* parent,
     return rc;
 }
 
+int wolfTPM2_NVCreateAuthPolicy(WOLFTPM2_DEV* dev, WOLFTPM2_HANDLE* parent,
+    WOLFTPM2_NV* nv, word32 nvIndex, word32 nvAttributes, word32 maxSize,
+    const byte* auth, int authSz, const byte* authPolicy, int authPolicySz)
+{
+    return wolfTPM2_NVCreateAuthPolicy_ex(dev, parent, nv, nvIndex,
+        nvAttributes, maxSize, auth, authSz, authPolicy, authPolicySz,
+        TPM_ALG_NULL);
+}
+
 int wolfTPM2_NVCreateAuth(WOLFTPM2_DEV* dev, WOLFTPM2_HANDLE* parent,
     WOLFTPM2_NV* nv, word32 nvIndex, word32 nvAttributes, word32 maxSize,
     const byte* auth, int authSz)

tests/fwtpm_unit_tests.c

@@ -1096,9 +1096,11 @@ static UINT32 CreatePrimaryHelper(FWTPM_CTX* ctx, TPM_ALG_ID alg)
     return GetU32BE(gRsp + TPM2_HEADER_SIZE);
 }
 
-#ifdef HAVE_ECC
+#if defined(HAVE_ECC) && !defined(FWTPM_NO_ATTESTATION)
 /* Build a non-restricted ECC-P256 sign-capable primary (for tests that
- * require TPMA_OBJECT_sign and a key with no scheme bound at create time). */
+ * require TPMA_OBJECT_sign and a key with no scheme bound at create time).
+ * Only consumed by the attestation tests, so gated to avoid
+ * -Werror=unused-function in FWTPM_NO_ATTESTATION builds. */
 static int BuildCreatePrimaryEccSignCmd(byte* buf)
 {
     int pos = 0;
@@ -1155,7 +1157,7 @@ static UINT32 CreatePrimaryEccSignHelper(FWTPM_CTX* ctx)
     if (GetRspRC(gRsp) != TPM_RC_SUCCESS) return 0;
     return GetU32BE(gRsp + TPM2_HEADER_SIZE);
 }
-#endif
+#endif /* HAVE_ECC && !FWTPM_NO_ATTESTATION */
 
 /* Helper: flush a handle */
 static void FlushHandle(FWTPM_CTX* ctx, UINT32 handle)
@@ -1578,13 +1580,15 @@ static void test_fwtpm_nv_counter(void)
 }
 
 #ifndef FWTPM_NO_ATTESTATION
+#ifdef HAVE_ECC
 /* TPMT_SIG_SCHEME parser must consume the extra UINT16 count field for
  * TPMS_SCHEME_ECDAA per Part 2 Sec. 11.2.1.5. Sending a Quote with
  * inScheme.scheme = TPM_ALG_ECDAA followed by hashAlg + count + a single
  * PCR selection (count=1) verifies that subsequent PCRselect parsing
  * is not desynchronized. With the bug, pcrSelectionsCount reads as 0
  * because the count field is interpreted as the high 16 bits of the
- * PCR-selection count. */
+ * PCR-selection count. ECC-only because the RSA sign path rejects
+ * TPM_ALG_ECDAA with TPM_RC_SCHEME. */
 static void test_fwtpm_quote_ecdaa_scheme(void)
 {
     FWTPM_CTX ctx;
@@ -1598,11 +1602,7 @@ static void test_fwtpm_quote_ecdaa_scheme(void)
     memset(&ctx, 0, sizeof(ctx));
     AssertIntEQ(fwtpm_test_startup(&ctx), 0);
 
-#ifdef HAVE_ECC
     keyH = CreatePrimaryHelper(&ctx, TPM_ALG_ECC);
-#else
-    keyH = CreatePrimaryHelper(&ctx, TPM_ALG_RSA);
-#endif
     AssertIntNE(keyH, 0);
 
     /* Build TPM2_Quote with ECDAA inScheme. */
@@ -1650,6 +1650,7 @@ static void test_fwtpm_quote_ecdaa_scheme(void)
     FWTPM_Cleanup(&ctx);
     printf("Test fwTPM:\tQuote(ECDAA scheme):\t\tPassed\n");
 }
+#endif /* HAVE_ECC */
 
 #ifdef HAVE_ECC
 /* TPM2_CertifyCreation inScheme parser must consume the extra UINT16
@@ -2540,8 +2541,8 @@ int fwtpm_unit_tests(int argc, char *argv[])
     test_fwtpm_nv_counter();
 #ifndef FWTPM_NO_ATTESTATION
     test_fwtpm_nv_certify_digest_mode();
-    test_fwtpm_quote_ecdaa_scheme();
 #ifdef HAVE_ECC
+    test_fwtpm_quote_ecdaa_scheme();
     test_fwtpm_sign_ecdaa_scheme();
     test_fwtpm_certify_creation_ecdaa_scheme();
 #endif