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

@@ -789,10 +789,17 @@ void TPM2_Packet_ParsePoint(TPM2_Packet* packet, TPM2B_ECC_POINT* point)
     TPM2_Packet_ParseEccPoint(packet, &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 && point->size > 0) {
+        if (pointStartPos + point->size <= packet->size) {
+            packet->pos = pointStartPos + point->size;
+        }
+        else {
+            packet->pos = packet->size;
+        }
     }
 }
 
@@ -1149,10 +1156,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,11 +5770,21 @@ 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. Note: digest size is a weak discriminator -
+         * SM3-256 / SHA3-256 also produce 32 bytes; callers needing those
+         * must pass nameAlg explicitly. */
         switch (authPolicySz) {
         #ifndef NO_SHA
             case TPM_SHA_DIGEST_SIZE:
@@ -5830,6 +5852,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

@@ -1578,13 +1578,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 +1600,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 +1648,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 +2539,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

tests/unit_tests.c

@@ -592,6 +592,7 @@ static void test_wolfTPM2_SetAuthHandle_PolicyAuthOffset(void)
 static void test_wolfTPM2_StartSession_SaltedEncryptAttrs(void)
 {
 #if !defined(WOLFTPM2_NO_WOLFCRYPT)
+    int rc;
     WOLFTPM2_DEV dev;
     WOLFTPM2_KEY tpmKey;
     WOLFTPM2_SESSION session;
@@ -601,17 +602,26 @@ static void test_wolfTPM2_StartSession_SaltedEncryptAttrs(void)
     XMEMSET(&tpmKey, 0, sizeof(tpmKey));
     XMEMSET(&session, 0, sizeof(session));
 
+    /* Initialize so TPM2_GetNonceNoLock and dependent code paths have a
+     * valid context. Skip if no TPM is reachable. */
+    rc = wolfTPM2_Init(&dev, TPM2_IoCb, NULL);
+    if (rc != 0) {
+        printf("Test TPM Wrapper:\tStartSession salted enc attrs:\tSkipped\n");
+        return;
+    }
+
     /* tpmKey with a non-NULL handle, no auth */
     tpmKey.handle.hndl = 0x80000000;
 
-    /* Best effort - if no TPM is present the call returns early after the
-     * SetAuth path, which is what we want to inspect. */
+    /* The call will fail later (no real key with that handle) but the
+     * SetAuth path that sets sessionAttributes runs first. */
     (void)wolfTPM2_StartSession(&dev, &session, &tpmKey, NULL,
         TPM_SE_HMAC, TPM_ALG_CFB);
 
     AssertIntEQ((int)(dev.session[0].sessionAttributes & expected),
         (int)expected);
 
+    wolfTPM2_Cleanup(&dev);
     printf("Test TPM Wrapper:\tStartSession salted enc attrs:\tPassed\n");
 #endif
 }
@@ -1902,11 +1912,11 @@ static void test_TPM2_ECC_Parameters_EcdaaResponseParse(void)
     printf("Test TPM Wrapper:\tEcdaaResponseParse:\t\tPassed\n");
 }
 
-/* TPM2_Packet_ParseSignature must explicitly recognize TPM_ALG_NULL as a
- * zero-payload signature so subsequent fields stay aligned. The previous
- * default-fallthrough lumped TPM_ALG_NULL together with unknown algorithms,
- * making the property "Parse(Append(NULL signature)) consumes exactly the
- * sigAlg bytes" depend on undocumented behavior. */
+/* TPM2_Packet_AppendSignature / ParseSignature must explicitly recognize
+ * TPM_ALG_NULL as a zero-payload signature so subsequent fields stay
+ * aligned. The previous default-fallthrough lumped TPM_ALG_NULL together
+ * with unknown algorithms, making the property "Parse(Append(NULL))
+ * consumes exactly the sigAlg bytes" depend on undocumented behavior. */
 static void test_TPM2_ParseSignature_NullAlg(void)
 {
     TPM2_Packet packet;
@@ -1936,6 +1946,29 @@ static void test_TPM2_ParseSignature_NullAlg(void)
     sentinel = (UINT16)((buf[packet.pos] << 8) | buf[packet.pos + 1]);
     AssertIntEQ(sentinel, 0xDEAD);
 
+    /* Round-trip: Append a TPM_ALG_NULL signature into a fresh packet and
+     * verify only the 2-byte sigAlg was written. A future regression that
+     * drops the explicit case (defaulting to silent fallthrough) would
+     * still pass for Parse but the Append side is also locked in here. */
+    XMEMSET(buf, 0, sizeof(buf));
+    XMEMSET(&packet, 0, sizeof(packet));
+    XMEMSET(&sig, 0, sizeof(sig));
+    sig.sigAlg = TPM_ALG_NULL;
+    packet.buf = buf;
+    packet.size = sizeof(buf);
+    packet.pos = 0;
+    TPM2_Packet_AppendSignature(&packet, &sig);
+    AssertIntEQ(packet.pos, 2);
+    AssertIntEQ(buf[0], (byte)((TPM_ALG_NULL >> 8) & 0xFF));
+    AssertIntEQ(buf[1], (byte)(TPM_ALG_NULL & 0xFF));
+
+    /* Re-parse confirms the round-trip. */
+    XMEMSET(&sig, 0, sizeof(sig));
+    packet.pos = 0;
+    TPM2_Packet_ParseSignature(&packet, &sig);
+    AssertIntEQ(sig.sigAlg, TPM_ALG_NULL);
+    AssertIntEQ(packet.pos, 2);
+
     printf("Test TPM Wrapper:\tParseSignature NULL alg:\tPassed\n");
 }
 
@@ -2224,6 +2257,44 @@ static void test_wolfTPM2_GetKeyTemplate_KeyedHash_Scheme(void)
 #endif
 }
 
+/* wolfTPM2_VerifyHashTicket must apply the same RSA-strict / ECDSA-permissive
+ * digest size policy as wolfTPM2_SignHashScheme. The bounds check fires
+ * before any TPM call so this test does not require a working TPM. */
+static void test_wolfTPM2_VerifyHashTicket_DigestSize(void)
+{
+#if !defined(WOLFTPM2_NO_WOLFCRYPT) && !defined(NO_RSA)
+    int rc;
+    WOLFTPM2_DEV dev;
+    WOLFTPM2_KEY key;
+    byte digest[TPM_MAX_DIGEST_SIZE];
+    byte sig[MAX_RSA_KEY_BYTES];
+
+    XMEMSET(&dev, 0, sizeof(dev));
+    XMEMSET(&key, 0, sizeof(key));
+    XMEMSET(digest, 0xCC, sizeof(digest));
+    XMEMSET(sig, 0, sizeof(sig));
+    key.handle.hndl = 0x80000000;
+    key.pub.publicArea.type = TPM_ALG_RSA;
+
+    /* SHA-256 digest (32) + hashAlg=SHA512 -> RSA mismatch -> BUFFER_E */
+    rc = wolfTPM2_VerifyHashTicket(&dev, &key, sig, 256, digest, 32,
+        TPM_ALG_RSASSA, TPM_ALG_SHA512, NULL);
+    AssertIntEQ(rc, BUFFER_E);
+
+    /* Oversized digest (64) + hashAlg=SHA256 -> BUFFER_E */
+    rc = wolfTPM2_VerifyHashTicket(&dev, &key, sig, 256, digest, 64,
+        TPM_ALG_RSASSA, TPM_ALG_SHA256, NULL);
+    AssertIntEQ(rc, BUFFER_E);
+
+    /* Negative digestSz -> BUFFER_E */
+    rc = wolfTPM2_VerifyHashTicket(&dev, &key, sig, 256, digest, -1,
+        TPM_ALG_RSASSA, TPM_ALG_SHA256, NULL);
+    AssertIntEQ(rc, BUFFER_E);
+
+    printf("Test TPM Wrapper:\tVerifyHashTicket size:\t\tPassed\n");
+#endif
+}
+
 /* wolfTPM2_NVCreateAuthPolicy must derive nameAlg from authPolicySz so
  * the policy digest hash matches the index's nameAlg. Bug-mode hardcoded
  * SHA-256 nameAlg, which made SHA-384/SHA-512 policies unsatisfiable.
@@ -2354,6 +2425,12 @@ static void test_TPM2_BrainpoolCurveMapping(void)
     AssertIntEQ(TPM2_GetTpmCurve(ECC_SECP256R1), TPM_ECC_NIST_P256);
     AssertIntEQ(TPM2_GetWolfCurve(TPM_ECC_NIST_P256), ECC_SECP256R1);
 
+    /* TPM2_GetCurveSize must report the correct byte size for the new
+     * Brainpool curve IDs (32 / 48 / 64). */
+    AssertIntEQ(TPM2_GetCurveSize(TPM_ECC_BP_P256_R1), 32);
+    AssertIntEQ(TPM2_GetCurveSize(TPM_ECC_BP_P384_R1), 48);
+    AssertIntEQ(TPM2_GetCurveSize(TPM_ECC_BP_P512_R1), 64);
+
     printf("Test TPM Wrapper:\tBrainpool curve mapping:\tPassed\n");
 #endif
 }
@@ -3752,6 +3829,7 @@ int unit_tests(int argc, char *argv[])
     test_TPM2_BrainpoolCurveMapping();
     test_wolfTPM2_RsaEncryptDecrypt_OversizedBufferE();
     test_wolfTPM2_SignHashScheme_DigestSize();
+    test_wolfTPM2_VerifyHashTicket_DigestSize();
     test_wolfTPM2_NVCreateAuthPolicy_NameAlg();
     test_wolfTPM2_GetKeyTemplate_KeyedHash_Scheme();
     test_wolfTPM2_LoadEccPublicKey_Ex();