Handle TPMs that lack SHA-1, disable EncryptDecrypt, or run a command longer than the TIS wait budget - #607
Handle TPMs that lack SHA-1, disable EncryptDecrypt, or run a command longer than the TIS wait budget#607dgarske wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved correctness, validation, coverage, and portability issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Improves TPM compatibility for oversized signatures, long-running commands, missing SHA-1 support, and disabled EncryptDecrypt commands.
Changes:
- Adds signature input-buffer checks.
- Uses real-time TIS wait budgets with fallback behavior.
- Adds capability-aware handling in example tests.
File summaries
| File | Description |
|---|---|
wolftpm/tpm2.h |
Defines the minimum TPM input-buffer size. |
wolftpm/tpm2_types.h |
Adds timing hooks and timeout configuration. |
src/tpm2_wrap.c |
Preflights signature sizes before verification. |
src/tpm2_tis.c |
Applies elapsed-time TIS wait budgets. |
examples/wrap/wrap_test.c |
Gates SHA-1-dependent verification by capability. |
examples/pqc/pqc_ctrl.c |
Handles oversized Hash-ML-DSA verification. |
examples/native/native_test.c |
Handles SHA-1 availability and disabled EncryptDecrypt paths. |
Review details
Suppressed comments (4)
examples/native/native_test.c:118
- [Medium][CWE-703] Returning 0 for every
TPM2_GetCapabilityfailure makes the callers print that SHA-1 is unsupported and continue. A transport error or malformed capability response can therefore skip both PCR tests and still reachNative test passed; only a successful query with no matching algorithm should be treated as an unsupported optional feature. Preserve the query error separately from the support boolean.
if (rc != TPM_RC_SUCCESS) {
return 0;
examples/native/native_test.c:1721
- [Medium][CWE-754] If the second call returns
TPM_RC_COMMAND_CODEorTPM_RC_DISABLED, this branch clearsrcbut leavesperform_EncryptDecrypt2true and falls through to the comparison at lines 1730-1743.TPM2_EncryptDecrypt2()only populatescmdOuton success (src/tpm2.c:2770-2781), so the previous output is treated as decrypted data and the example reportsTPM_RC_TESTINGinstead of skipping; preserve the response code through the skip or bypass that comparison.
if (native_is_cmd_unavailable_or_disabled(rc)) { /* unsupported or disabled */
printf("TPM2_EncryptDecrypt2: Is not a supported feature without enabling due to export controls\n");
rc = 0;
src/tpm2_wrap.c:5777
- [Medium][CWE-130]
TPM_PT_INPUT_BUFFERis the maximum serialized command size, not a per-parameter limit. TreatingsigSz <= TPM_MIN_INPUT_BUFFERas automatically safe can still send an oversized packet—for example,VerifyDigestSignaturewithcontextSz=255andsigSz=1024already exceeds a 1024-byte input buffer, and a cap just above a valid PQ signature has the same issue. Compare the complete command size or reserve the fixed handle/digest/context/signature overhead before taking this shortcut.
if (sigSz <= TPM_MIN_INPUT_BUFFER) {
return TPM_RC_SUCCESS;
wolftpm/tpm2_types.h:734
- [Medium] The generic
FREERTOSarm expandsxTaskGetTickCount()andportTICK_PERIOD_MS, but this header does not include the FreeRTOS/task headers.src/tpm2_tis.cincludes onlytpm2_tis.hand now necessarily expands this macro, so a normal-DFREERTOSlibrary build can fail with undefined identifiers unless every integrator pre-includes platform headers; include the required headers or require anXTPM_GET_TIMEMSoverride.
#elif defined(WOLFSSL_ESPIDF) || defined(FREERTOS)
#define XTPM_GET_TIMEMS() \
((word32)xTaskGetTickCount() * (word32)portTICK_PERIOD_MS)
- Files reviewed: 7/7 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
0bb9730 to
c58a890
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved validation, fallback, and example-correctness issues remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (6)
examples/native/native_test.c:115
- [Medium, CWE-20]
TPM_CAP_ALGSonly says that the TPM implements SHA-1; it does not say that a SHA-1 PCR bank is allocated. A TPM with SHA-1 crypto support but no SHA-1 PCR selection still takes both new branches and issuesPCR_Read/PolicyPCR, which can fail and abort the test. QueryTPM_CAP_PCRSand verify that the requested PCR is present in the SHA-1 selection, astest_pcr_bank_allocateddoes.
in.capability = TPM_CAP_ALGS;
in.property = alg;
in.propertyCount = 1;
examples/native/native_test.c:118
- [Low] A failed capability query is silently converted into “SHA-1 unsupported” (CWE-390). A transport or TPM error will therefore be printed as a skip and the native test can continue and report success instead of exposing the failure. Preserve the query return code separately from the unsupported result, as
wolfTPM2_IsAlgSupported()already does, and abort/report on query failure.
rc = TPM2_GetCapability(&in, &out);
if (rc != TPM_RC_SUCCESS) {
return 0;
examples/pqc/pqc_ctrl.c:505
- [Medium, CWE-754] This branch reports
PASSand returns success without verifyingsig; the referencedexamples/pqc/mldsa_host_verifyhelper is not present in the tree. A bad signature or signing regression therefore passes the example whenever the TPM input buffer is too small. Perform an actual host verification before returning success, or report this result as skipped/non-verified.
printf("PASS HashML-DSA-%-3s signdigest (sig %d bytes); on-TPM "
"verify unavailable, signature exceeds input buffer\n",
mldsaName(ps), sigSz);
rc = TPM_RC_SUCCESS;
goto exit_quiet;
src/tpm2_tis.c:456
- [Medium] The fallback is only selected when the start tick is zero; if
XTPM_GET_TIMEMS()fails after a nonzero start, the implementation returns zero and unsigned subtraction wraps, making the wait expire immediately instead of falling back to the counter (CWE-834). Preserve the last valid reading or make the clock API report validity on every call before applying the elapsed-time check.
if (to->haveStart) {
/* unsigned subtraction stays correct across the word32 wrap */
return ((word32)(XTPM_GET_TIMEMS() - to->start) >= TPM_TIMEOUT_MS) ?
1 : 0;
src/tpm2_wrap.c:5824
- [High, CWE-400] This guard covers only the two one-shot verification wrappers.
wolfTPM2_VerifySequenceComplete()also appends aTPMT_SIGNATUREand is used by the PQC example and benchmark with 2420–4627-byte ML-DSA signatures; on a TPM reporting a 1024-byte input buffer it can still send the oversized command and trigger the hang this check is meant to prevent. Apply the same preflight to that wrapper or centralize it across the verification paths.
rc = wolfTPM2_CheckSigInputBuffer(sigSz);
if (rc != TPM_RC_SUCCESS) {
return rc;
}
wolftpm/tpm2_types.h:728
- [Medium, CWE-670] When a port supplies
XTPM_GET_TIMEMS, this outer guard skips the auto-detection block but never definesWOLFTPM_HAVE_MONOTONIC_MS; the TIS timeout helpers therefore compile to the old iteration-only path. The advertised custom clock hook is ineffective, including for freestanding users that provide their own clock. Treat a user-supplied hook as enabling the monotonic path before the standard-header guard, or explicitly require and document the second macro.
#if !defined(XTPM_GET_TIMEMS) && !defined(WOLFTPM_NO_MONOTONIC_MS) && \
!defined(WOLFTPM_NO_STD_HEADERS)
- Files reviewed: 7/7 changed files
- Comments generated: 4
- Review effort level: Lite
| inputBuffer = out.capabilityData.data.tpmProperties.tpmProperty[0].value; | ||
| if (inputBuffer > 0 && (UINT32)sigSz > inputBuffer) { |
| #elif defined(CLOCK_MONOTONIC) || defined(__linux__) | ||
| #include <time.h> | ||
| static inline word32 XTPM_GET_TIMEMS(void) |
| if (native_is_cmd_unavailable_or_disabled(rc)) { /* unsupported or disabled */ | ||
| printf("TPM2_EncryptDecrypt2: Is not a supported feature without enabling due to export controls\n"); | ||
| rc = 0; |
| if (rc != TPM_RC_SUCCESS || | ||
| out.capabilityData.data.tpmProperties.count == 0) { |
Four independent robustness fixes, found running the stock examples against a current SPI TPM on a Raspberry Pi 5.
Oversized signatures no longer hang the TPM.
wolfTPM2_VerifyHashTicket()andwolfTPM2_VerifyDigestSignature()now check the signature againstTPM_PT_INPUT_BUFFERand returnBUFFER_Ebefore sending, rather than leaving the TPM to discover it; post-quantum signatures exceed that cap routinely and not every TPM rejects the oversized parameter cleanly, with at least one ceasing to respond until a hardware reset. The capability read is skipped at or below the 1024-byteTPM_MIN_INPUT_BUFFERfloor, so RSA and ECC verifies cost no extra round trip.TIS waits are bounded by real time instead of an iteration count.
TPM2_TIS_WaitForStatus()andTPM2_TIS_GetBurstCount()now useTPM_TIMEOUT_MS(default 60000) through a newXTPM_GET_TIMEMS(), so the budget no longer varies with host speed and scheduler; a roughly 21 second RSA key generation previously timed out intermittently on the same command that succeeded moments earlier. Ports without a monotonic clock keep the existing counter unchanged.Examples skip SHA-1 on TPMs that do not implement it.
wrap_testandnative_testcarried hard-coded SHA-1 test vectors and PCR bank selections that abort the run withTPM_RC_HASHon an increasing share of current parts; all three sites now queryTPM_CAP_ALGSfirst and skip with the reason printed.native_testtolerates EncryptDecrypt being disabled rather than absent. It already skipped the test onTPM_RC_COMMAND_CODE, but a TPM that implements the command and ships it switched off answersTPM_RC_DISABLED; both codes now go through one helper, and the second call site gains the response-code masking it was missing.Hardware / test status
Validated on a Raspberry Pi 5 driving an SPI TPM over spidev with no kernel TPM driver in the path, against a part that exercises all four paths: no SHA-1,
TPM2_EncryptDecryptdisabled, a 1024-byteTPM_PT_INPUT_BUFFER, and roughly 21 second RSA-2048 key generation.native_testnow runs to completion where it previously aborted at the first SHA-1 PCR read, andexamples/bench/benchcompletes its post-quantum rows instead of hanging partway.Also run against the firmware TPM and the simulator with no change in results:
make check, the SPDM suite in both TCG and PSK modes, and the tpm2-tools compatibility suite. The no-clock fallback is compile-verified with-DWOLFTPM_NO_MONOTONIC_MS.Scope
TPM_TIMEOUT_MSis a single global budget rather than a per-command duration derived from the TPM's ownTPM_PT_*timeout and duration properties. That is the more correct answer and is left as follow-on work.