From ec93a9b7337d4f4eef3ff054f2c18347448c3fd1 Mon Sep 17 00:00:00 2001 From: David Garske Date: Thu, 25 Jun 2026 15:17:07 -0700 Subject: [PATCH] peer review fixes --- .gitignore | 6 ++++++ examples/keygen/create_primary.c | 17 ++++++++++++++--- examples/nvram/counter.c | 15 +++++++++------ examples/nvram/store.c | 15 +++++++++------ examples/pcr/quote.c | 15 +++++++++------ examples/run_examples.sh | 14 ++++++++++++++ examples/wrap/wrap_test.c | 15 +++++++++------ src/fwtpm/fwtpm_command.c | 10 ++++++++-- zephyr/samples/wolftpm_wrap_test/CMakeLists.txt | 3 +++ 9 files changed, 81 insertions(+), 29 deletions(-) diff --git a/.gitignore b/.gitignore index 70091d78..494a8ace 100644 --- a/.gitignore +++ b/.gitignore @@ -41,6 +41,9 @@ build/ examples/wrap/wrap_test examples/wrap/caps examples/wrap/hmac +examples/wrap/getrandom +examples/wrap/hash +examples/wrap/encrypt_decrypt examples/native/native_test examples/bench/bench examples/csr/csr @@ -56,6 +59,7 @@ examples/pcr/reset examples/timestamp/clock_set examples/management/flush examples/management/tpmclear +examples/management/da_check pkcs7tpmsigned.p7s pkcs7tpmsignedex.p7s examples/tls/tls_server @@ -70,6 +74,7 @@ examples/keygen/keyload examples/keygen/keygen examples/keygen/keyimport examples/keygen/external_import +examples/keygen/ecdh examples/pqc/mldsa_sign examples/pqc/mlkem_encap examples/pqc/pqc_mssim_e2e @@ -207,6 +212,7 @@ examples/firmware/*.MANIFESTHASH src/fwtpm/fwtpm_server fwtpm_nv.bin fwtpm_test_nv.bin +fwtpm_test_nv.bin.key # Fuzz artifacts (corpus generated at runtime by gen_corpus.py) tests/fuzz/corpus/ diff --git a/examples/keygen/create_primary.c b/examples/keygen/create_primary.c index 190dfabd..fcbfa0c9 100644 --- a/examples/keygen/create_primary.c +++ b/examples/keygen/create_primary.c @@ -90,9 +90,20 @@ static void usage(void) #ifdef WOLFTPM_MLDSA static int mldsaParamSet(const char* optVal, TPMI_MLDSA_PARAMETER_SET* ps) { - int n = XATOI(optVal); + int n; + const char* p; + + if (optVal[0] == '\0') { /* missing or empty suffix, use default */ + *ps = TPM_MLDSA_65; + return TPM_RC_SUCCESS; + } + /* reject non-digit input (e.g. -mldsa=abc) before XATOI */ + for (p = optVal; *p != '\0'; p++) { + if (*p < '0' || *p > '9') + return TPM_RC_FAILURE; + } + n = XATOI(optVal); switch (n) { - case 0: /* missing or empty suffix, use default */ case 65: *ps = TPM_MLDSA_65; return TPM_RC_SUCCESS; case 44: *ps = TPM_MLDSA_44; return TPM_RC_SUCCESS; case 87: *ps = TPM_MLDSA_87; return TPM_RC_SUCCESS; @@ -147,7 +158,7 @@ int TPM2_CreatePrimaryKey_Example(void* userCtx, int argc, char *argv[]) argv[argc-1] + 7 : ""; if (mldsaParamSet(optVal, &mldsaPs) != TPM_RC_SUCCESS) { usage(); - return 0; + return -1; } alg = TPM_ALG_MLDSA; } diff --git a/examples/nvram/counter.c b/examples/nvram/counter.c index 59095f4c..5782087d 100644 --- a/examples/nvram/counter.c +++ b/examples/nvram/counter.c @@ -74,6 +74,7 @@ int TPM2_NVRAM_Counter_Example(void* userCtx, int argc, char *argv[]) #if defined(WOLFTPM_MLKEM) || defined(WOLFTPM_MLDSA) TPM_ALG_ID pqcParamEncAlg = TPM_ALG_NULL; int pqcParamSet = 0; + int pqcRc = 0; WOLFTPM2_KEY pqcKey; #endif word32 nvIndex = TPM2_DEMO_NV_COUNTER_INDEX; @@ -121,13 +122,15 @@ int TPM2_NVRAM_Counter_Example(void* userCtx, int argc, char *argv[]) paramEncAlg = TPM_ALG_XOR; } #if defined(WOLFTPM_MLKEM) || defined(WOLFTPM_MLDSA) - else if (parsePqcParamEncArg(argv[argc-1], &pqcParamEncAlg, - &pqcParamSet) != 0) { - /* PQC option; an unsupported parameter set (alg left NULL) is a - * fatal typo, not a silent drop of parameter encryption. */ - if (pqcParamEncAlg == TPM_ALG_NULL) { + else if ((pqcRc = parsePqcParamEncArg(argv[argc-1], &pqcParamEncAlg, + &pqcParamSet)) != 0) { + /* An unsupported parameter set (parser returns -1) is a fatal typo, + * not a silent drop of parameter encryption. Branch on the parser + * result directly: pqcParamEncAlg may already hold a valid alg from + * an earlier PQC option (argv is parsed right-to-left). */ + if (pqcRc < 0) { usage(); - return 0; + return -1; } } #endif diff --git a/examples/nvram/store.c b/examples/nvram/store.c index 383db785..97528506 100644 --- a/examples/nvram/store.c +++ b/examples/nvram/store.c @@ -82,6 +82,7 @@ int TPM2_NVRAM_Store_Example(void* userCtx, int argc, char *argv[]) #if defined(WOLFTPM_MLKEM) || defined(WOLFTPM_MLDSA) TPM_ALG_ID pqcParamEncAlg = TPM_ALG_NULL; int pqcParamSet = 0; + int pqcRc = 0; WOLFTPM2_KEY pqcKey; #endif int partialStore = 0; @@ -135,13 +136,15 @@ int TPM2_NVRAM_Store_Example(void* userCtx, int argc, char *argv[]) partialStore = PUBLIC_PART_ONLY; } #if defined(WOLFTPM_MLKEM) || defined(WOLFTPM_MLDSA) - else if (parsePqcParamEncArg(argv[argc-1], &pqcParamEncAlg, - &pqcParamSet) != 0) { - /* PQC option; an unsupported parameter set (alg left NULL) is a - * fatal typo, not a silent drop of parameter encryption. */ - if (pqcParamEncAlg == TPM_ALG_NULL) { + else if ((pqcRc = parsePqcParamEncArg(argv[argc-1], &pqcParamEncAlg, + &pqcParamSet)) != 0) { + /* An unsupported parameter set (parser returns -1) is a fatal typo, + * not a silent drop of parameter encryption. Branch on the parser + * result directly: pqcParamEncAlg may already hold a valid alg from + * an earlier PQC option (argv is parsed right-to-left). */ + if (pqcRc < 0) { usage(); - return 0; + return -1; } } #endif diff --git a/examples/pcr/quote.c b/examples/pcr/quote.c index 634d9e14..bdf7e3bb 100644 --- a/examples/pcr/quote.c +++ b/examples/pcr/quote.c @@ -91,6 +91,7 @@ int TPM2_PCR_Quote_Test(void* userCtx, int argc, char *argv[]) #if defined(WOLFTPM_MLKEM) || defined(WOLFTPM_MLDSA) TPM_ALG_ID pqcParamEncAlg = TPM_ALG_NULL; int pqcParamSet = 0; + int pqcRc = 0; WOLFTPM2_KEY pqcKey; #endif #if !defined(NO_FILESYSTEM) && !defined(NO_WRITE_TEMP_FILES) @@ -140,13 +141,15 @@ int TPM2_PCR_Quote_Test(void* userCtx, int argc, char *argv[]) paramEncAlg = TPM_ALG_XOR; } #if defined(WOLFTPM_MLKEM) || defined(WOLFTPM_MLDSA) - else if (parsePqcParamEncArg(argv[argc-1], &pqcParamEncAlg, - &pqcParamSet) != 0) { - /* PQC option; an unsupported parameter set (alg left NULL) is a - * fatal typo, not a silent drop of parameter encryption. */ - if (pqcParamEncAlg == TPM_ALG_NULL) { + else if ((pqcRc = parsePqcParamEncArg(argv[argc-1], &pqcParamEncAlg, + &pqcParamSet)) != 0) { + /* An unsupported parameter set (parser returns -1) is a fatal typo, + * not a silent drop of parameter encryption. Branch on the parser + * result directly: pqcParamEncAlg may already hold a valid alg from + * an earlier PQC option (argv is parsed right-to-left). */ + if (pqcRc < 0) { usage(); - return 0; + return -1; } } #endif diff --git a/examples/run_examples.sh b/examples/run_examples.sh index bdc3e12b..e0b549fc 100755 --- a/examples/run_examples.sh +++ b/examples/run_examples.sh @@ -383,6 +383,20 @@ if [ $ENABLE_V185 -eq 1 ]; then [ $RESULT -ne 0 ] && echo -e "create_primary mldsa=$PS failed! $RESULT" && exit 1 done + echo -e "PQC usage-error checks (invalid parameter sets must be rejected)" + # These return before touching the TPM; a zero exit means an invalid + # parameter set was silently accepted as the default. + ./examples/keygen/create_primary -mldsa=0 -oh >> $TPMPWD/run.out 2>&1 + [ $? -eq 0 ] && echo -e "create_primary -mldsa=0 should fail!" && exit 1 + ./examples/keygen/create_primary -mldsa=abc -oh >> $TPMPWD/run.out 2>&1 + [ $? -eq 0 ] && echo -e "create_primary -mldsa=abc should fail!" && exit 1 + # An invalid PQC option must stay fatal even after a valid one (argv is + # parsed right-to-left), in either argument order. + ./examples/wrap/wrap_test -aes -mldsa=999 -mlkem=768 >> $TPMPWD/run.out 2>&1 + [ $? -eq 0 ] && echo -e "wrap_test -mldsa=999 -mlkem=768 should fail!" && exit 1 + ./examples/wrap/wrap_test -aes -mlkem=768 -mldsa=999 >> $TPMPWD/run.out 2>&1 + [ $? -eq 0 ] && echo -e "wrap_test -mlkem=768 -mldsa=999 should fail!" && exit 1 + echo -e "PQC parameter encryption (ML-KEM salt / ML-DSA bind)" # ML-KEM as the param-enc session salt, ML-DSA as the param-enc session # bind; exercise AES-CFB and XOR across child-create, attestation and NV. diff --git a/examples/wrap/wrap_test.c b/examples/wrap/wrap_test.c index 7ed67738..e4b03e1f 100644 --- a/examples/wrap/wrap_test.c +++ b/examples/wrap/wrap_test.c @@ -138,6 +138,7 @@ int TPM2_Wrapper_TestArgs(void* userCtx, int argc, char *argv[]) * (ML-KEM) or bind (ML-DSA) key. */ TPM_ALG_ID pqcParamEncAlg = TPM_ALG_NULL; int pqcParamSet = 0; + int pqcRc = 0; WOLFTPM2_KEY pqcKey; #endif @@ -178,13 +179,15 @@ int TPM2_Wrapper_TestArgs(void* userCtx, int argc, char *argv[]) paramEncAlg = TPM_ALG_XOR; } #if defined(WOLFTPM_MLKEM) || defined(WOLFTPM_MLDSA) - else if (parsePqcParamEncArg(argv[argc-1], &pqcParamEncAlg, - &pqcParamSet) != 0) { - /* PQC option; an unsupported parameter set (alg left NULL) is a - * fatal typo, not a silent drop of parameter encryption. */ - if (pqcParamEncAlg == TPM_ALG_NULL) { + else if ((pqcRc = parsePqcParamEncArg(argv[argc-1], &pqcParamEncAlg, + &pqcParamSet)) != 0) { + /* An unsupported parameter set (parser returns -1) is a fatal typo, + * not a silent drop of parameter encryption. Branch on the parser + * result directly: pqcParamEncAlg may already hold a valid alg from + * an earlier PQC option (argv is parsed right-to-left). */ + if (pqcRc < 0) { usage(); - return 0; + return -1; } } #endif diff --git a/src/fwtpm/fwtpm_command.c b/src/fwtpm/fwtpm_command.c index c8c59da2..ac06cf0e 100644 --- a/src/fwtpm/fwtpm_command.c +++ b/src/fwtpm/fwtpm_command.c @@ -224,7 +224,7 @@ static int FwBuildParamEncKey(FWTPM_Session* sess, TPM_HANDLE authHandle, static int FwParamDecryptCmd(FWTPM_CTX* ctx, FWTPM_Session* sess, TPM_HANDLE authHandle, byte* paramData, UINT32 paramSz) { - int rc; + int rc = TPM_RC_FAILURE; byte keyBuf[TPM_MAX_DIGEST_SIZE * 2]; int keyBufSz = 0; @@ -247,6 +247,9 @@ static int FwParamDecryptCmd(FWTPM_CTX* ctx, FWTPM_Session* sess, sess->nonceTPM.buffer, sess->nonceTPM.size, paramData, paramSz, 0); /* decrypt */ } + else { + rc = TPM_RC_SYMMETRIC; /* unsupported param-enc cipher */ + } TPM2_ForceZero(keyBuf, sizeof(keyBuf)); (void)ctx; @@ -257,7 +260,7 @@ static int FwParamDecryptCmd(FWTPM_CTX* ctx, FWTPM_Session* sess, static int FwParamEncryptRsp(FWTPM_CTX* ctx, FWTPM_Session* sess, TPM_HANDLE authHandle, byte* paramData, UINT32 paramSz) { - int rc; + int rc = TPM_RC_FAILURE; byte keyBuf[TPM_MAX_DIGEST_SIZE * 2]; int keyBufSz = 0; @@ -281,6 +284,9 @@ static int FwParamEncryptRsp(FWTPM_CTX* ctx, FWTPM_Session* sess, sess->nonceCaller.buffer, sess->nonceCaller.size, paramData, paramSz, 1); /* encrypt */ } + else { + rc = TPM_RC_SYMMETRIC; /* unsupported param-enc cipher */ + } TPM2_ForceZero(keyBuf, sizeof(keyBuf)); (void)ctx; diff --git a/zephyr/samples/wolftpm_wrap_test/CMakeLists.txt b/zephyr/samples/wolftpm_wrap_test/CMakeLists.txt index 27ef27f4..61066091 100644 --- a/zephyr/samples/wolftpm_wrap_test/CMakeLists.txt +++ b/zephyr/samples/wolftpm_wrap_test/CMakeLists.txt @@ -4,6 +4,9 @@ project(wolftpm_wrap_test) # Include source code for wrap test target_sources(app PRIVATE ${ZEPHYR_WOLFTPM_MODULE_DIR}/examples/wrap/wrap_test.c) +# wrap_test calls PQC param-enc helpers from examples/tpm_test_keys.c under +# WOLFTPM_MLKEM / WOLFTPM_MLDSA; compile it so those references resolve. +target_sources(app PRIVATE ${ZEPHYR_WOLFTPM_MODULE_DIR}/examples/tpm_test_keys.c) target_sources(app PRIVATE ${app_sources}) # Include header files