Add new Firmware TPM (fwTPM)

[dgarske]

Summary

• Add firmware TPM 2.0 server (fwTPM) implementing TPM 2.0 spec v1.38 (105/113 commands, 93% coverage)
• Spec-compliant primary key derivation: deterministic RSA (iterative KDFa prime generation), ECC (KDFa scalar, Q=d*G), KEYEDHASH/SYMCIPHER keys — same seed + template always produces the same key
• ChangePPS / ChangeEPS hierarchy commands with seed regeneration and cache flush
• NV storage with TLV journal format (flash-friendly, append-only)
• Socket (mssim) and TIS/SHM transports for desktop, UART transport for embedded
• STM32 Cortex-M33 bare-metal port with TrustZone (CMSE) support
• Shared crypto refactoring: extract KDFa/KDFe, AES-CFB, HMAC/Hash to tpm2_crypto.c
• Bounds-checked TPM2_Packet_ParseU16Buf variant for safer response parsing
• Auth validation: reject oversized auth values instead of silent truncation
• SIGTERM/SIGINT signal handler for graceful NV save on server kill

fwTPM Server

Core TPM 2.0 command processing in src/fwtpm/:

• fwtpm_command.c — 105 command handlers with full auth, sessions, parameter encryption
• fwtpm_nv.c — TLV journal NV storage (file-based default, HAL-abstracted for flash)
• fwtpm_io.c — Socket transport (mssim + swtpm protocol auto-detection)
• fwtpm_tis.c / fwtpm_tis_shm.c — TIS register interface via POSIX shared memory
• fwtpm_crypto.c — Key generation, sign/verify, seed encrypt/decrypt helpers
• Clock HAL with NV-persisted clockOffset

Build: ./configure --enable-fwtpm && make

Example: wolfSSL/wolftpm-examples#1

Primary Key Derivation (TPM 2.0 Part 1 Section 26)

• RSA: iterative KDFa prime generation with labels "RSA p" / "RSA q"
• ECC: KDFa scalar derivation, public point Q = d*G
• KEYEDHASH/SYMCIPHER: KDFa with algorithm-specific labels
• hashUnique = H(sensitiveCreate.data || unique) per Section 26.1
• Primary cache retained as performance optimization, no longer required for correctness

UART Transport (--enable-swtpm=uart)

New transport option for wolfTPM client library to communicate with embedded fwTPM over serial:

• ./configure --enable-swtpm=uart — uses termios serial I/O instead of TCP sockets
• TPM2_SWTPM_HOST env var selects serial device at runtime
• Same mssim protocol as socket transport (compatible with all wolfTPM examples)

Testing

• 311 tpm2-tools compatibility tests (scripts/tpm2_tools_test.sh)
• Full wolfTPM example suite (examples/run_examples.sh)
• libFuzzer harness with corpus generator (tests/fuzz/)
• m33mu Cortex-M33 emulator CI test (scripts/fwtpm_emu_test.sh)
• ASan / UBSan clean
• 20 CI matrix configurations (pedantic gcc/clang -Werror, sanitizers, feature-disable variants)

Fixed

[aidangarske]

Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 54 total — 35 posted, 19 skipped
34 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [Critical] ContextLoad restores raw FWTPM_Session struct from untrusted input without validation — src/fwtpm/fwtpm_command.c:2669
• [Critical] ContextSave serializes session key material in plaintext to client — src/fwtpm/fwtpm_command.c:2579
• [High] FwGetEntityName writes NV name without checking nameBufSz capacity first — src/fwtpm/fwtpm_command.c:316-319
• [High] Multiple command handlers use (void)cmdSize and skip bounds checking — src/fwtpm/fwtpm_command.c:2954,3045,6205
• [High] NV compaction erases storage before writing, creating a data-loss window — src/fwtpm/fwtpm_nv.c:1166-1179
• [Medium] PCR_Read digest parsing not converted to ParseU16Buf - packet desync on oversized data — src/tpm2.c:1251-1261
• [Medium] KDFe_ex requires non-NULL Z parameter but KDFe wrapper passes NULL when Z is NULL — src/tpm2_crypto.c:301-315
• [Medium] PCR_Extend skips dSize bytes on unsupported bank but dSize may be zero — src/fwtpm/fwtpm_command.c:1406-1409
• [Medium] HierarchyChangeAuth does not zero old auth buffer before overwriting — src/fwtpm/fwtpm_command.c:3064-3093
• [Medium] rpHash computed with first session's hashAlg but used for all sessions — src/fwtpm/fwtpm_command.c:12724-12732
• [Medium] FlushContext allows flushing persistent object handles — src/fwtpm/fwtpm_command.c:2486-2492
• [Medium] PolicyPassword/PolicyAuthValue HMAC bypass for trial sessions — src/fwtpm/fwtpm_command.c:12556-12572
• [Medium] Missing cmdSize validation in Rewrap before parsing multiple handles — src/fwtpm/fwtpm_command.c:4940-4951
• [Medium] Duplicate exports plaintext sensitive data when both symAlg and newParent are NULL — src/fwtpm/fwtpm_command.c:4839-4867
• [Medium] Missing DA counter reset on successful authentication — src/fwtpm/fwtpm_command.c:12508-12524
• [Medium] NV file writes not flushed to disk (no fsync), data can be lost on crash — src/fwtpm/fwtpm_nv.c:83-127
• [Medium] FwNvAppendEntry writes TLV header and value in two separate HAL calls, creating a torn-write window — src/fwtpm/fwtpm_nv.c:662-675
• [Medium] Private key DER buffer not zeroed on error paths in FwGenerateRsaKey and FwGenerateEccKey — src/fwtpm/fwtpm_crypto.c:410-466
• [Medium] FwDeriveWrapKey uses only first 32 bytes of parent private key, reducing HMAC key entropy — src/fwtpm/fwtpm_crypto.c:830-833
• [Medium] ECC primary key derivation skips range check against curve order — src/fwtpm/fwtpm_crypto.c:576-586
• [Medium] NV journal scan silently drops entries when slots are full — src/fwtpm/fwtpm_nv.c:864-872
• [Medium] FwWrapPrivate does not bounds-check total output size against outPriv->buffer capacity — src/fwtpm/fwtpm_crypto.c:1063-1073
• [Medium] FwUnwrapPrivate does not zero the decrypted sensitive buffer on HMAC integrity failure — src/fwtpm/fwtpm_crypto.c:1159-1163
• [Medium] RSA import via FwImportReconstructKey does not verify n = p * q — src/fwtpm/fwtpm_crypto.c:1847-1859
• [Medium] Large stack allocation in TIS command handler (4096 bytes) — src/fwtpm/fwtpm_tis.c:116
• [Medium] Shared memory TOCTOU: reg_len read without validation before reg_data access — src/fwtpm/fwtpm_tis.c:74
• [Medium] Server writes response directly into shared memory rsp_buf — src/fwtpm/fwtpm_tis.c:131-133
• [Low] wolfTPM2_SetAuth inconsistency: auth returns BUFFER_E but name silently truncates — src/tpm2_wrap.c:1670-1684
• [Low] TPM2_ParamEnc_AESCFB does not zero symKey on early return from KDFa failure — src/tpm2_param_enc.c:113-152
• [Low] PolicyPCR computes PCR digest without checking wc_HashUpdate return codes — src/fwtpm/fwtpm_command.c:7378-7384
• [Low] FwNvProcessEntry does not validate return codes from unmarshal functions for seed and auth tags — src/fwtpm/fwtpm_nv.c:748-780
• [Low] FwComputeUniqueHash does not check wc_HashInit return code before proceeding — src/fwtpm/fwtpm_crypto.c:100-116
• [Low] Signal handler uses signal() instead of sigaction() -- non-portable reset semantics — src/fwtpm/fwtpm_main.c:158-159
• [Low] Shared memory file uses regular file path instead of shm_open — src/fwtpm/fwtpm_tis_shm.c:73
• [Low] Missing FWTPM_Cleanup on early exit paths in main() argument parsing — src/fwtpm/fwtpm_main.c:129-143

Skipped findings

• [Low] TPM2_ConstantCompare missing NULL pointer checks
• [Low] UART transport SwTpmReceive uses blocking read with VTIME timeout but no retry logic
• [Info] TPM2_Packet_ParseU16Buf sets *size to clamped value, losing original wire size for callers
• [Low] UART SwTpmDisconnect sends SESSION_END but does not read acknowledgment
• [Low] TPM2_ParamEnc_XOR cast comparison may fail for large paramSz on platforms where int is 16-bit
• [Low] TPM2_Packet_ParseSensitiveCreate does not validate inner size consistency
• [Low] Integer overflow in PCR_Extend concat buffer indexing
• [Low] FwCredentialDeriveKeys uses hardcoded SHA-256 instead of parent's nameAlg
• [Low] Sensitive data in FWTPM_Object and FWTPM_NvIndex not zeroed on NV delete operations
• [Low] FwNvMarshalPublic relies on TPM2_Packet_AppendPublicArea without explicit size validation
• [Low] Signal handler correctness: g_ctx->running is volatile but g_ctx pointer itself is not
• [Low] Client lazy-init not thread-safe despite comment claiming lock protection
• [Low] Socket server does not set FD_CLOEXEC on accepted connections
• [Info] FWTPM_Cleanup zeros entire context including IO state after IO cleanup
• [Low] TIS response written to shared memory before status update creates brief inconsistency
• [Info] setsockopt return value not checked
• [Info] Listening socket binds to INADDR_LOOPBACK only -- intentional but undocumented
• [Info] Client shared memory open uses O_NOFOLLOW but not O_CLOEXEC on some paths
• [Low] No timeout on sem_wait in client HAL -- could block indefinitely

Review generated by Skoll

[aidangarske]

[$\color{purple}{\textsf{Critical}}$] ContextLoad restores raw FWTPM_Session struct from untrusted input without validation

ContextLoad deserializes a raw FWTPM_Session struct directly from the context blob provided by the caller using TPM2_Packet_ParseBytes. The only validation is a magic/version check and a size match. An attacker who captures or crafts a context blob can inject arbitrary session state including: a forged policyDigest (bypassing policy checks), preset isPasswordPolicy/isAuthValuePolicy flags, a chosen sessionKey, or manipulated nonceTPM/nonceCaller values. This effectively allows an attacker to forge arbitrary authorized sessions. The blob is not encrypted or integrity-protected (no HMAC over session state), so any entity that can observe a ContextSave response can modify and replay it with altered policy digests.

TPM2_Packet_ParseBytes(cmd, (byte*)&ctx->sessions[si],
    sizeof(FWTPM_Session));

Recommendation: Add an HMAC over the serialized session blob using a TPM-internal secret (e.g., a key derived from the hierarchy seed). Verify the HMAC on ContextLoad before restoring the session. Additionally, validate critical fields of the deserialized struct (sessionType, authHash, handle ranges) before accepting the session. Consider encrypting the session blob as well to protect session key material.

[aidangarske]

[$\color{purple}{\textsf{Critical}}$] ContextSave serializes session key material in plaintext to client

When saving a session context, the entire FWTPM_Session struct is serialized to the response blob without encryption. This includes sess->sessionKey.buffer (the HMAC session key derived from KDFa), sess->bindAuth, and all nonce values. Any entity that can read the ContextSave response (e.g., via interception on the transport layer) obtains the session's HMAC key and can forge command authorizations. The TPM 2.0 specification requires that context blobs be encrypted with an internal key to protect this sensitive material.

TPM2_Packet_AppendBytes(rsp, (byte*)sess, sizeof(FWTPM_Session));

Recommendation: Encrypt the session blob using a symmetric key derived from an internal TPM secret (e.g., context encryption key derived from the platform seed) before serializing to the response. Use AES-CFB or similar, with an HMAC for integrity. On ContextLoad, decrypt and verify integrity before restoring.

[aidangarske]

[$\color{red}{\textsf{High}}$] FwGetEntityName writes NV name without checking nameBufSz capacity first

In the NV index branch of FwGetEntityName, FwComputeNvName is called to write into nameBuf before checking whether the computed nvNameSz fits within nameBufSz. The function computes the NV name, writes it to nameBuf, and only then checks 'if (nvNameSz <= nameBufSz)'. If the NV name is larger than nameBufSz, the data has already been written past the buffer boundary before the size check rejects it. This is a write-before-check pattern that could cause a stack buffer overflow, depending on the caller's buffer size.

FwComputeNvName(nv, nameBuf, &nvNameSz);
if (nvNameSz <= nameBufSz) {
    return (int)nvNameSz;
}

Recommendation: Pass nameBufSz to FwComputeNvName so it can respect the buffer limit, or check the expected NV name size before calling FwComputeNvName. Alternatively, compute the name into a local temporary buffer and only copy to nameBuf after verifying it fits.

[aidangarske]

[$\color{red}{\textsf{High}}$] Multiple command handlers use (void)cmdSize and skip bounds checking

Several command handlers cast cmdSize to void and then proceed to parse variable-length fields from the command buffer without checking whether sufficient bytes remain. For example, FwCmd_ClearControl parses authHandle and then skipAuthArea, but parses the 'disable' byte without checking cmdSize. FwCmd_HierarchyChangeAuth parses a variable-length newAuth without first verifying the outer command has enough bytes for the TPM2B header. FwCmd_HMAC_Start parses keyHandle without checking cmdSize >= header + 4. While the dispatch layer validates minimum size for handles, the parameter parsing after the auth area often lacks bounds checks against cmdSize. A truncated command packet could cause reads beyond the command buffer.

(void)cmdSize;

TPM2_Packet_ParseU32(cmd, &authHandle);
if (cmdTag == TPM_ST_SESSIONS) rc = FwSkipAuthArea(cmd, cmdSize);

TPM2_Packet_ParseU16(cmd, &newAuthSize);
if (newAuthSize > (UINT16)sizeof(newAuthBuf)) {

Recommendation: Add explicit bounds checks (cmd->pos + N > cmdSize) before every parse operation, especially after FwSkipAuthArea which advances the position by a variable amount. Remove the (void)cmdSize suppressions and use cmdSize consistently for validation throughout each handler.

[aidangarske]

[$\color{red}{\textsf{High}}$] NV compaction erases storage before writing, creating a data-loss window

FWTPM_NV_Save erases the entire NV storage (line 1170) before writing the new compacted state. If a power loss, crash, or write error occurs between the erase and the final header update, all NV state (hierarchy seeds, persistent objects, NV indices) is permanently lost. For a TPM this is catastrophic: endorsement and owner seeds that were derived once and used to create keys will be unrecoverable, breaking all existing sealed data and attestation identities. The compaction is also triggered automatically when the journal fills up (FwNvAppendEntry line 652), meaning normal operation can trigger this window.

ctx->nvCompacting = 1;

/* Erase NV storage */
if (hal->erase != NULL) {
    rc = hal->erase(hal->ctx, 0, hal->maxSize);
}

/* Reset write position to after header */
ctx->nvWritePos = (word32)sizeof(FWTPM_NV_HEADER);

/* Write header (will be updated at end with final writePos) */
if (rc == 0) {
    rc = FwNvWriteHeader(ctx);
}

Recommendation: Implement a two-phase commit for compaction: write the new compacted image to a secondary region or shadow file first, then atomically swap (rename or update a pointer). Alternatively, write the compacted data starting after the current writePos and only update the header to point to the new base after all entries are written. On flash, a second bank with a generation counter provides crash-safe compaction.

[aidangarske]

[$\color{blue}{\textsf{Low}}$] FwNvProcessEntry does not validate return codes from unmarshal functions for seed and auth tags

For seed tags (OWNER_SEED, ENDORSEMENT_SEED, PLATFORM_SEED), the code checks valueLen >= FWTPM_SEED_SIZE but does not track or act on any error. For auth tags (OWNER_AUTH through LOCKOUT_AUTH), FwNvUnmarshalAuth is called but its return value is discarded. If the unmarshal fails (e.g., due to a corrupted auth size field that passes the length check but causes a partial read), the auth value in ctx may be left in a partially-initialized state. Since auth values control access to hierarchy operations, a corrupted auth could either lock out legitimate access or bypass authentication.

case FWTPM_NV_TAG_OWNER_AUTH:
    FwNvUnmarshalAuth(value, &vPos, vMax, &ctx->ownerAuth);
    break;

case FWTPM_NV_TAG_ENDORSEMENT_AUTH:
    FwNvUnmarshalAuth(value, &vPos, vMax, &ctx->endorsementAuth);
    break;

Recommendation: Check the return value of FwNvUnmarshalAuth and other unmarshal calls. On failure, either skip the entry entirely (leaving the previous value intact) or set the auth to a known safe state (e.g., empty/zero). At minimum, log a warning when unmarshal fails so corrupted NV images can be diagnosed.

[aidangarske]

[$\color{blue}{\textsf{Low}}$] FwComputeUniqueHash does not check wc_HashInit return code before proceeding

At line 102, the code checks 'if (rc == 0 && wc_HashInit(...) == 0)' which correctly guards against init failure, but rc is initialized to 0 and FWTPM_ALLOC_VAR may set it to TPM_RC_MEMORY (if WOLFTPM_SMALL_STACK is defined and malloc fails). However, if FWTPM_ALLOC_VAR sets rc != 0, the 'rc == 0' check causes the function to skip hashing and fall through to return 0 (success with 0-byte digest). The function uses 0 as a return value to indicate error, but the caller may not distinguish between 'allocation failed' and 'hash algorithm not supported' since both return 0.

FWTPM_ALLOC_VAR(hCtx, wc_HashAlg);
if (rc == 0 && wc_HashInit(hCtx, wcHash) == 0) {
    rc = wc_HashUpdate(hCtx, wcHash, keyData, (word32)keyDataSz);
    if (rc == 0) {
        rc = wc_HashFinal(hCtx, wcHash, outBuf);
    }
    wc_HashFree(hCtx, wcHash);
    if (rc == 0) {
        FWTPM_FREE_VAR(hCtx);
        return hSz;
    }
}
FWTPM_FREE_VAR(hCtx);
return 0;

Recommendation: If FWTPM_ALLOC_VAR fails (rc != 0), the function should still free the var (FWTPM_FREE_VAR is safe on NULL) and return 0, which it does. However, consider using a negative return value or a dedicated error code to distinguish memory allocation failure from hash algorithm failure, so callers can handle the conditions differently.

[aidangarske]

[$\color{blue}{\textsf{Low}}$] Signal handler uses signal() instead of sigaction() -- non-portable reset semantics

The signal handler is installed using signal() which has System V semantics on some platforms (the handler is reset to SIG_DFL after the first invocation). This means a second SIGTERM/SIGINT could kill the process before NV state is saved. The handler itself is safe (only sets a volatile int flag), but the registration method is fragile. Additionally, signal() in the server loop files (fwtpm_io.c:539, fwtpm_tis.c:410) uses signal(SIGPIPE, SIG_IGN) which has the same portability concern.

signal(SIGTERM, sigterm_handler);
signal(SIGINT, sigterm_handler);

Recommendation: Use sigaction() with SA_RESTART flag instead of signal() for portable, non-resetting signal handling. This ensures the handler remains installed after the first signal delivery and that interrupted system calls are restarted.

[aidangarske]

[$\color{blue}{\textsf{Low}}$] Shared memory file uses regular file path instead of shm_open

The shared memory is created via open("/tmp/fwtpm.shm", O_CREAT | O_RDWR | O_TRUNC | O_NOFOLLOW, 0600) using a regular filesystem path rather than shm_open(). While O_NOFOLLOW prevents symlink attacks, using /tmp is subject to other risks: (1) the file persists after a crash (no automatic cleanup), (2) on systems with sticky-bit /tmp, another user could pre-create the path and block the server. The server-side uses O_TRUNC which mitigates some concerns, and the client uses O_NOFOLLOW. However, using shm_open() with a proper POSIX shared memory name (the semaphore names already follow this pattern) would be more consistent and avoid /tmp risks.

fd = open(FWTPM_TIS_SHM_PATH, O_CREAT | O_RDWR | O_TRUNC | O_NOFOLLOW, 0600);

Recommendation: Consider using shm_open() instead of open() for the shared memory region, consistent with the POSIX semaphore usage. This places the shared memory in the POSIX shared memory namespace (/dev/shm on Linux) with proper semantics. Alternatively, document why regular file path is preferred (e.g., persistence across restarts) and ensure shm_unlink is called in cleanup.

[aidangarske]

[$\color{blue}{\textsf{Low}}$] Missing FWTPM_Cleanup on early exit paths in main() argument parsing

After FWTPM_Init(&ctx) succeeds (line 116), the port-parsing loop at lines 124-145 can return 1 for invalid port values without calling FWTPM_Cleanup(). This leaks the wolfCrypt RNG and skips wolfCrypt_Cleanup(). While this is a process-exit scenario where the OS reclaims resources, it means NV state may not be saved and wolfCrypt internal state is not properly torn down.

long port = strtol(argv[++i], NULL, 10);
if (port > 0 && port <= 65535) {
    ctx.cmdPort = (int)port;
}
else {
    fprintf(stderr, "Invalid port: %s\n", argv[i]);
    return 1;
}

Recommendation: Call FWTPM_Cleanup(&ctx) before the return 1 in the invalid port error paths, or restructure to parse all arguments before calling FWTPM_Init().

[aidangarske]

Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 70 total — 50 posted, 20 skipped
22 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [High] ECDH_ZGen and ZGen_2Phase accept peer points without curve validation — src/fwtpm/fwtpm_command.c:6909-6925,11863-11906
• [High] EC_Ephemeral stores private key in ctx without zeroization after ZGen_2Phase use — src/fwtpm/fwtpm_command.c:11694-11706,11928-11939
• [High] FwDecryptSeed ECC path: out-of-bounds read parsing ySz from encSeedBuf — src/fwtpm/fwtpm_crypto.c:1287
• [Medium] PolicyAuthorize hardcodes SHA-256 for aHash instead of using session hash algorithm — src/fwtpm/fwtpm_command.c:8002-8004
• [Medium] SequenceComplete and EventSequenceComplete do not zeroize HMAC/hash digest buffers — src/fwtpm/fwtpm_command.c:6535-6537,6680-6682
• [Medium] NV_ChangeAuth does not zeroize newAuthBuf stack buffer containing new auth value — src/fwtpm/fwtpm_command.c:10115-10162
• [Medium] FwPolicyParseSession silently discards FwSkipAuthArea errors causing parser desync — src/fwtpm/fwtpm_command.c:7571-7580
• [Medium] DictionaryAttackParameters allows setting maxTries to zero, disabling DA protection — src/fwtpm/fwtpm_command.c:10253-10261
• [Medium] FwWrapPrivate reuses AES encryption key as HMAC integrity key — src/fwtpm/fwtpm_crypto.c:1037-1052
• [Medium] FwEncryptSeed RSA path does not zero seedBuf on encryption failure — src/fwtpm/fwtpm_crypto.c:1393-1435
• [Medium] FwWrapPrivate does not zero plaintext sensitive buffer before freeing — src/fwtpm/fwtpm_crypto.c:1076-1085
• [Medium] FwCredentialWrap and FwCredentialUnwrap call wc_AesFree on potentially uninitialized Aes object — src/fwtpm/fwtpm_crypto.c:2547,2657
• [Medium] FwCredentialDeriveKeys does not zero symKey when INTEGRITY derivation fails — src/fwtpm/fwtpm_crypto.c:2496-2515
• [Medium] FwImportVerifyAndDecrypt does not zero plainSens buffer on AES decryption failure — src/fwtpm/fwtpm_crypto.c:1656-1684
• [Medium] KDFa_ex does not zeroize output key buffer on mid-loop HMAC failure — src/tpm2_crypto.c:133-149
• [Medium] KDFe_ex does not zeroize output key buffer on mid-loop hash failure — src/tpm2_crypto.c:256-274
• [Medium] SwTpmTransmit missing negative bufSz check allows huge write() via signed-to-unsigned cast — src/tpm2_swtpm.c:93-105
• [Medium] SWTPM response size integer overflow: int rspSz from uint32_t allows buffer overread — src/tpm2_swtpm.c:385-456
• [Medium] UART FD opened without O_CLOEXEC leaks TPM device to child processes — src/tpm2_swtpm.c:173
• [Medium] wolfTPM2_CreatePrimaryKey_ex: sensitive auth data not zeroized on success or error paths — src/tpm2_wrap.c:2446-2541
• [Medium] wolfTPM2_CreateKeySeal_ex: sensitive data not zeroized on TPM2_Create error path — src/tpm2_wrap.c:7774-7780
• [Medium] wolfTPM2_ImportPrivateKey: symmetric seed and encrypted sensitive not zeroized — src/tpm2_wrap.c:3097-3174
• [Low] EncryptDecrypt allows ECB mode for symmetric operations despite spec deprecation — src/fwtpm/fwtpm_command.c:10463-10491
• [Low] FwComputeHashUnique does not zero hashOut on wc_HashInit failure — src/fwtpm/fwtpm_crypto.c:121-150
• [Low] TPM2_Packet_ParseSensitiveCreate missing NULL check for packet parameter — src/tpm2_packet.c:772
• [high] Duplicate: encryptedDuplication bypass when newParentHandle is TPM_RH_NULL
• [medium] Clear uses XMEMSET instead of TPM2_ForceZero to erase old hierarchy auth values
• [medium] ContextSave uses XMEMSET instead of TPM2_ForceZero for session slot containing key material
• [medium] Duplicate inner wrapping: caller-supplied encKeyIn overrides symKeyBits without validation
• [medium] PCR_Reset missing lower bound check allows negative pcrIndex with large handle values
• [medium] PCR_Event missing lower bound check on pcrHandle
• [medium] Rewrap does not zero sensitive plainSens data in dupBuf on error paths
• [medium] Import does not zero encKeyBuf containing inner encryption key
• [medium] ObjectChangeAuth does not zero old authValue before overwriting with new auth
• [medium] Startup(CLEAR) does not zero old hierarchy seeds before generating new ones
• [medium] FwComputeSessionHmac does not check sessionKey.size against hmacKey buffer before memcpy
• [high] FWTPM_ProcessCommand writes response using FWTPM_MAX_COMMAND_SIZE bound into FWTPM_TIS_FIFO_SIZE buffer
• [medium] STS_COMMAND_READY and STS_GO processed in same write causes empty command execution
• [medium] Missing zeroization of localCmd stack buffer containing sensitive TPM command data
• [medium] Client FIFO read uses server-controlled rsp_len without upper-bound validation
• [medium] TIS FIFO write path does not validate locality before accepting commands
• [medium] FWTPM_ProcessCommand return value check misses negative rspSize
• [high] NV marshal buffer freed without zeroization leaks hierarchy seeds and private keys on heap
• [high] Stack-local FWTPM_Object and FWTPM_NvIndex containing private keys not zeroized in FwNvProcessEntry
• [medium] FWTPM_NV_SaveAuth stack buffer containing marshaled auth value not zeroized
• [medium] FwNvMarshal return values ignored in FWTPM_NV_Save can write truncated/corrupt NV entries
• [medium] FwNvFileWrite: ftell failure (-1) causes runaway loop in file extension code
• [medium] Deleted NV index and persistent object slots cleared with XMEMSET instead of ForceZero
• [medium] Locality value from network not validated allows values outside TPM spec range 0-4
• [medium] FWTPM_NV_Save resets nvWritePos before validating erase success, corrupting journal state on erase failure

Skipped findings

• [low] GetCapability propertyCount is attacker-controlled and limits output but not response buffer bounds
• [low] ClockSet does not validate parsing bounds for newTime after auth area
• [low] Duplicate inner wrapping uses hardcoded SHA-256 for integrity HMAC regardless of object nameAlg
• [low] CreatePrimary template hash uses raw struct bytes for cache key, creating non-deterministic cache hits
• [Low] PolicySigned extends policy digest even when entity name lookup returns zero length
• [Low] FwAppendCreationHashAndTicket: no bounds check on objNameSz before copying into ticketData
• [Low] FwSignDigestAndAppend: RSASSA PKCS#1 v1.5 encHash buffer on stack not zeroed after use
• [Low] TPM2_AesCfbEncrypt and TPM2_AesCfbDecrypt do not zeroize AES context on stack
• [Low] TPM2_HmacCompute writes full digest without bounds check when digestSz is NULL
• [Low] KDFa_ex integer overflow in sizeInBits computation for very large keySz
• [Low] KDFa_ex and KDFe_ex omit mandatory 0x00 separator when label is NULL
• [low] Server shared memory file not removed on abnormal termination
• [low] Client FWTPM_TIS_ClientDisconnect does not zero shared-memory pointer before unmap
• [low] TIS register read of scalar value zero-fills only 'len' bytes of reg_data, leaving stale data in remainder
• [low] Server TIS init zeroes register struct after HAL returns mapped pointer, erasing HAL metadata
• [low] FwNvMarshalPublic/FwNvUnmarshalPublic integer underflow if pos exceeds maxSz
• [low] FWTPM_NV_Save buf not zeroized before reallocation during NV index/persistent/cache loop
• [Low] wolfTPM2_SetAuthHandle policyAuth path copies name without bounds check
• [Low] UART VTIME of 200 (20 seconds) creates denial-of-service window for unresponsive fwTPM
• [Low] TPM2_Packet_ParseU16Buf silently truncates data when wireSize exceeds buffer capacity

Review generated by Skoll

[aidangarske]

🔴 [High] ECDH_ZGen and ZGen_2Phase accept peer points without curve validation
Category: CWE-20: Improper Input Validation

Both ECDH_ZGen and ZGen_2Phase import attacker-supplied ECC points using wc_ecc_import_unsigned() and immediately pass them to wc_ecc_shared_secret() without calling wc_ecc_check_key() to validate that the peer point lies on the expected curve. An attacker can submit an invalid-curve point (a point on a different, weaker curve with a small subgroup), causing wc_ecc_shared_secret() to compute a shared secret that leaks information about the static private key. This is a classic invalid curve attack that can recover the full private key in as few as a handful of queries. The TPM 2.0 spec Part 1 Section C.6.1 mandates point-on-curve validation for all ECC operations using externally supplied points. While wolfCrypt's wc_ecc_shared_secret may perform some internal validation depending on build options, there is no explicit check here, and many wolfCrypt configurations (especially on embedded targets) skip the expensive point validation unless ECC_TIMING_RESISTANT or similar is enabled.

rc = wc_ecc_import_unsigned(peerPub,
        inPoint.point.x.buffer, inPoint.point.y.buffer,
        NULL, wcCurve);
    /* No wc_ecc_check_key() call */
    /* Compute shared secret */
    if (rc == 0) {
        zSz = (word32)sizeof(zBuf);
        rc = wc_ecc_shared_secret(privKey, peerPub, zBuf, &zSz);

Recommendation: After wc_ecc_import_unsigned(), call wc_ecc_check_key(peerPub) and return TPM_RC_ECC_POINT if it fails. This validates the point is on the correct curve and has the expected order, blocking invalid curve attacks.

[aidangarske]

🔴 [High] EC_Ephemeral stores private key in ctx without zeroization after ZGen_2Phase use
Category: CWE-459: Incomplete Cleanup

EC_Ephemeral stores the ephemeral private key DER into ctx->ecEphemeralKey (line 11697) for later use by ZGen_2Phase. After ZGen_2Phase completes, neither function zeroizes ctx->ecEphemeralKey or sets ctx->ecEphemeralKeySz to 0. The ephemeral private key thus persists indefinitely in the FWTPM_CTX structure until overwritten by the next EC_Ephemeral call. This violates the principle that ephemeral keys must be destroyed after use. An attacker who can read firmware memory (via cold boot, fault injection, or a separate vulnerability) can recover the ephemeral private key and compute the Z2 shared secret, breaking the two-phase key exchange protocol. Furthermore, the stale ephemeral key could be replayed if a malicious caller invokes ZGen_2Phase again with the same counter value.

/* Cleanup */
    TPM2_ForceZero(z1Buf, sizeof(z1Buf));
    TPM2_ForceZero(z2Buf, sizeof(z2Buf));
    if (privAInit) wc_ecc_free(privKeyA);
    if (ephInit) wc_ecc_free(privEph);
    if (peerInit) wc_ecc_free(peerPub);
    FWTPM_FREE_VAR(privKeyA);
    /* Missing: TPM2_ForceZero(ctx->ecEphemeralKey, sizeof(ctx->ecEphemeralKey)); */
    /* Missing: ctx->ecEphemeralKeySz = 0; */

Recommendation: At the end of FwCmd_ZGen_2Phase, after the cleanup section, add: TPM2_ForceZero(ctx->ecEphemeralKey, sizeof(ctx->ecEphemeralKey)); ctx->ecEphemeralKeySz = 0; This ensures the ephemeral private key is destroyed immediately after it is consumed.

[aidangarske]

🔴 [High] FwDecryptSeed ECC path: out-of-bounds read parsing ySz from encSeedBuf
Category: Buffer Over-read

In the ECC path of FwDecryptSeed, after parsing xSz and the x-coordinate data, the code reads the 2-byte ySz field at line 1287 without first verifying that 2 more bytes are available. The preceding bounds check at line 1280 ensures 'p + xSz <= encSeedSz' (i.e., the x data fits), but after advancing p by xSz, reading FwLoadU16BE(encSeedBuf + p) requires 'p + 2 <= encSeedSz'. If an attacker provides an encSeedBuf where encSeedSz == 2 + xSz (just enough for the x header and data, but no y header), this reads 2 bytes past the end of the buffer. This is reachable via TPM2_Import or TPM2_StartAuthSession with a crafted encrypted seed.

if (rc == 0) {
    XMEMCPY(xBuf, encSeedBuf + p, xSz);
    p += xSz;
    ySz = FwLoadU16BE(encSeedBuf + p);  /* OOB if p+2 > encSeedSz */
    p += 2;
    if (ySz > MAX_ECC_BYTES || p + ySz > encSeedSz) {
        rc = TPM_RC_SIZE;
    }
}

Recommendation: Add a bounds check before reading the ySz field: after 'p += xSz;', insert 'if (p + 2 > encSeedSz) { rc = TPM_RC_SIZE; }' and guard the ySz read with 'if (rc == 0)'. This matches the pattern used for the xSz read at line 1278-1282.

[aidangarske]

🟠 [Medium] PolicyAuthorize hardcodes SHA-256 for aHash instead of using session hash algorithm
Category: CWE-327: Use of a Broken or Risky Cryptographic Algorithm

In PolicyAuthorize's ticket verification, the aHash (H(approvedPolicy || policyRef)) is computed using a hardcoded TPM_ALG_SHA256 (line 8003) rather than the session's authHash algorithm. Per TPM 2.0 Part 3 Section 23.16, aHash should use the nameAlg of the signing key or the session's hash algorithm. When a session uses SHA-384 or SHA-512, the ticket was originally computed by VerifySignature using the key's nameAlg. If the signing key uses SHA-384, the ticket HMAC was computed over aHash produced with SHA-384, but PolicyAuthorize recomputes aHash with SHA-256, producing a different value. This causes valid tickets to be rejected for non-SHA-256 sessions, and more critically, it creates a hash algorithm mismatch where a 256-bit aHash is compared against what might have been a 384/512-bit computation, potentially enabling collision-based forgery attacks against the weaker hash.

/* Step 1: aHash = H(approvedPolicy || policyRef) */
            aWcHash = FwGetWcHashType(TPM_ALG_SHA256);
            aHashSz = TPM2_GetHashDigestSize(TPM_ALG_SHA256);

Recommendation: Replace the hardcoded TPM_ALG_SHA256 with sess->authHash (or the signing key's nameAlg if available). Change to: aWcHash = FwGetWcHashType(sess->authHash); aHashSz = TPM2_GetHashDigestSize(sess->authHash); This matches the TPM spec and ensures consistent hash algorithm usage throughout the ticket verification chain.

[aidangarske]

🟠 [Medium] NV_ChangeAuth does not zeroize newAuthBuf stack buffer containing new auth value
Category: CWE-459: Incomplete Cleanup

FwCmd_NV_ChangeAuth reads the new NV index auth value into a stack-local buffer newAuthBuf[TPM_MAX_DIGEST_SIZE] but never calls TPM2_ForceZero() on it before returning. The new auth credential remains on the stack and can be recovered by reading stack memory. Other auth-changing commands in this file (e.g., HierarchyChangeAuth at line 1803, ObjectChangeAuth at line 3102) properly zeroize their newAuthBuf buffers. This inconsistency suggests the zeroization was simply overlooked for this handler.

TPMI_RH_NV_INDEX nvHandle;
    UINT16 newAuthSize = 0;
    byte newAuthBuf[TPM_MAX_DIGEST_SIZE];
    ...
    return rc;
    /* Missing: TPM2_ForceZero(newAuthBuf, sizeof(newAuthBuf)); */

Recommendation: Add TPM2_ForceZero(newAuthBuf, sizeof(newAuthBuf)) before the return statement in FwCmd_NV_ChangeAuth, consistent with other auth-changing handlers.

[aidangarske]

🟠 [Medium] UART FD opened without O_CLOEXEC leaks TPM device to child processes
Category: CWE-403: Exposure of File Descriptor to Unintended Control Sphere

The UART device file is opened with open(host, O_RDWR | O_NOCTTY) without the O_CLOEXEC flag. Since the UART fd is persistent (kept open across commands by design in the UART transport), any fork()+exec() in the application will leak this file descriptor to child processes. A child process could then read/write to the TPM UART device, sending arbitrary TPM commands or intercepting TPM responses. This is especially concerning for embedded systems where the UART provides direct access to a firmware TPM.

fd = open(host, O_RDWR | O_NOCTTY);

Recommendation: Add O_CLOEXEC to the open flags: fd = open(host, O_RDWR | O_NOCTTY | O_CLOEXEC); This ensures the file descriptor is automatically closed in child processes after exec().

[aidangarske]

🔵 [Low] EncryptDecrypt allows ECB mode for symmetric operations despite spec deprecation
Category: CWE-327: Use of a Broken or Risky Cryptographic Algorithm

FwEncryptDecryptCore supports TPM_ALG_ECB mode for AES encrypt/decrypt operations. ECB mode processes each block independently, producing identical ciphertext for identical plaintext blocks, leaking patterns in the data. While the TPM 2.0 spec technically allows ECB, it is not recommended for any security-sensitive use. The handler processes ECB block-by-block using wc_AesEncryptDirect/wc_AesDecryptDirect without any warning or restriction. Additionally, the ECB code path also outputs a full AES_BLOCK_SIZE ivOut (line 10524) even though ECB has no IV/chaining state, potentially leaking uninitialized AES internal state.

case TPM_ALG_ECB:
                if (inDataSize % AES_BLOCK_SIZE != 0) {
                    rc = TPM_RC_SIZE;
                    break;
                }
                ret = wc_AesSetKey(aes, obj->privKey,
                    (word32)obj->privKeySize, NULL,
                    decrypt ? AES_DECRYPTION : AES_ENCRYPTION);
                ...
                for (blk = 0; blk < inDataSize; blk += AES_BLOCK_SIZE) {
                    if (decrypt) {
                        ret = wc_AesDecryptDirect(aes, inData + blk, inData + blk);

Recommendation: Consider rejecting ECB mode with TPM_RC_MODE or at minimum, when mode is ECB, set ivOut size to 0 instead of outputting the full AES_BLOCK_SIZE of potentially uninitialized state. If ECB support is intentional, document the security implications.

[aidangarske]

🔵 [Low] FwComputeHashUnique does not zero hashOut on wc_HashInit failure
Category: Uninitialized Output

If wc_HashInit fails at line 133 (rc is set to a non-zero error), the function falls through to line 148 where it checks rc != 0 and returns 0 (indicating error). However, the hashOut buffer is never written to and contains whatever data was previously in the caller's buffer. The caller receives 0 (error indicator) but hashOut is uninitialized. If the caller does not check the return value carefully and uses hashOut as context for primary key derivation (its documented purpose per TPM 2.0 Part 1 Section 26.1), the derived key is based on uninitialized memory, making it unpredictable and potentially leaking stack contents into the key derivation.

rc = wc_HashInit(hCtx, wcHash);
    if (rc == 0 && sensData != NULL && sensDataSz > 0) {
        rc = wc_HashUpdate(hCtx, wcHash, sensData, (word32)sensDataSz);
    }
    ...
    if (rc == 0) {
        return hSz;
    }
    ...
    return 0;  /* hashOut never written on error */

Recommendation: Zero hashOut at the start of the function or on the error path: 'XMEMSET(hashOut, 0, hSz);' before the error return. This ensures that on failure, hashOut contains known-zero data rather than uninitialized memory.

[aidangarske]

🔵 [Low] TPM2_Packet_ParseSensitiveCreate missing NULL check for packet parameter
Category: Null Pointer Dereference

The function dereferences packet->pos on line 772 without first checking whether packet is NULL. Other packet parsing functions in this file (e.g., TPM2_Packet_ParseU8, TPM2_Packet_ParseBytes) guard against NULL packet. While callers in fwtpm_command.c always pass a valid packet, this is inconsistent with the defensive pattern used elsewhere in the packet API and could cause a crash if the function is called with a NULL packet in the future.

TPM_RC TPM2_Packet_ParseSensitiveCreate(TPM2_Packet* packet, int maxSize,
    TPM2B_AUTH* userAuth, byte* sensData, int sensDataBufSz,
    UINT16* sensDataSize)
{
    TPM_RC rc = TPM_RC_SUCCESS;
    ...
    if (packet->pos + 2 > maxSize) {

Recommendation: Add a NULL check at the top of the function: if (packet == NULL || userAuth == NULL) return BAD_FUNC_ARG; This is consistent with the defensive pattern in other parse functions.

[aidangarske]

🟠 [Medium] SequenceComplete and EventSequenceComplete do not zeroize HMAC/hash digest buffers
Category: CWE-459: Incomplete Cleanup

Both FwCmd_SequenceComplete and FwCmd_EventSequenceComplete declare stack-local byte digest[TPM_MAX_DIGEST_SIZE] buffers that hold HMAC or hash output, but neither function calls TPM2_ForceZero(digest, sizeof(digest)) before returning. By contrast, FwCmd_Hash (line 6029) and FwCmd_HMAC (line 6147) both properly zeroize their digest buffers. When these functions are used with HMAC sequences (where seq->isHmac == 1), the digest contains a keyed HMAC value. The HMAC output remaining on the stack can be recovered via stack memory inspection, especially on embedded systems without ASLR. EventSequenceComplete additionally leaves concat[TPM_MAX_DIGEST_SIZE * 2] and newPcrDigest[TPM_MAX_DIGEST_SIZE] buffers unzeroized.

/* FwCmd_SequenceComplete - end of function */
    FWTPM_FREE_BUF(dataBuf);
    return rc;
    /* Missing: TPM2_ForceZero(digest, sizeof(digest)); */

    /* FwCmd_EventSequenceComplete - end of function */
    FWTPM_FREE_BUF(dataBuf);
    return rc;
    /* Missing: TPM2_ForceZero(digest, sizeof(digest)); */

Recommendation: Add TPM2_ForceZero(digest, sizeof(digest)) before the return statement in both functions. For EventSequenceComplete, also add TPM2_ForceZero(concat, sizeof(concat)) and TPM2_ForceZero(newPcrDigest, sizeof(newPcrDigest)).

---

Note: Referenced line (src/fwtpm/fwtpm_command.c:6535-6537,6680-6682) is outside the diff hunk. Comment anchored to nearest changed region.

[aidangarske]

Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 22 total — 15 posted, 7 skipped
15 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [High] Sessions not flushed when continueSession bit is clear — indefinite session reuse — src/fwtpm/fwtpm_command.c:12809-12823
• [High] Import inner integrity HMAC not verified after inner decryption — src/fwtpm/fwtpm_command.c:4372-4412
• [High] FwImportReconstructKey: No validation that sensType matches objectPublic.type (type confusion) — src/fwtpm/fwtpm_crypto.c:1778-1888
• [High] FwDeriveEccPrimaryKey/FwDerivePrime: KDFa context counter starts at 0 instead of 1 — src/fwtpm/fwtpm_crypto.c:552,658
• [Medium] TPM2_ParamEnc_CmdRequest silently succeeds without encrypting on unrecognized symmetric algorithm — src/tpm2_param_enc.c:193-235
• [Medium] TPM2_ParamDec_CmdResponse silently succeeds without decrypting on unrecognized symmetric algorithm — src/tpm2_param_enc.c:237-281
• [Medium] HierarchyControl accepts but does not enforce hierarchy disable — src/fwtpm/fwtpm_command.c:3024-3028
• [Medium] wolfTPM2_SetAuthHandle policyAuth path copies name without bounds check — src/tpm2_wrap.c:1733-1734
• [Medium] FwDerivePrime: Insufficient Miller-Rabin rounds for RSA prime generation — src/fwtpm/fwtpm_crypto.c:687
• [Medium] CreatePrimary template hash uses raw struct bytes for cache key — src/fwtpm/fwtpm_command.c:2183-2203
• [Medium] PolicySigned skips signature verification for trial sessions — src/fwtpm/fwtpm_command.c:8298-8345
• [Medium] FwComputeSessionHmac: sessionKey copy lacks bounds check — src/fwtpm/fwtpm_command.c:467-476
• [Medium] FwImportParseSensitive: totalSensSize parsed but not validated — src/fwtpm/fwtpm_crypto.c:1701-1703
• [Medium] FwMarshalSensitive vs FwMarshalSensitiveStd: Incompatible formats risk silent key corruption — src/fwtpm/fwtpm_crypto.c:872-955
• [Medium] FwEncryptSeed ECC: Seed not zeroed when encSeed packing fails — src/fwtpm/fwtpm_crypto.c:1511-1543

Skipped findings

• [Low] TPM2_HmacVerify leaks size-mismatch via timing side-channel
• [Low] TPM2_Packet_ParseSensitiveCreate: inSensSize not initialized at declaration
• [Low] ChangeEPS/ChangePPS use pendingClear which flushes ALL transient objects
• [Low] KDFa_ex label=NULL omits 0x00 separator required by TPM 2.0 spec
• [Low] FwDeriveRsaPrimaryKey: No gcd(e, p-1) check before CRT computation
• [Info] TPM2_AesCfbEncrypt/Decrypt do not zero AES key schedule from stack after wc_AesFree
• [Info] TPM2_HashCompute does not validate output buffer size when digestSz is NULL

Review generated by Skoll

[aidangarske]

🔴 [High] Sessions not flushed when continueSession bit is clear — indefinite session reuse
Category: Security

Per TPM 2.0 spec Part 1 Section 19.6.4, when a session's continueSession attribute is NOT set in the response, the TPM MUST flush the session. The fwTPM explicitly does NOT flush sessions after command processing. The comment at lines 12810-12813 states 'Sessions are NOT flushed here — the calling tool's ESYS layer flushes its own auth session handle'. This violates the spec: a one-shot HMAC session remains valid indefinitely in the fwTPM, allowing unlimited reuse of a session that should have been invalidated. An attacker who obtains or intercepts a session handle can reuse it for subsequent commands without re-authenticating, even after the legitimate client has closed the session from its side.

/* Deferred clear: flush transient objects after response auth is built.
 * Sessions are NOT flushed here -- the calling tool's ESYS layer flushes
 * its own auth session handle after the command response, and flushing it
 * early would cause TPM_RC_HANDLE (0x8B). Stale sessions from prior
 * commands are naturally invalidated when their auth values change... */

Recommendation: After building the response auth area and finalizing the response, iterate over cmdAuths and call FwFreeSession() for any session where the continueSession bit was NOT set in the command attributes. This should happen after FwRspFinalize at line 12805.

[aidangarske]

🔴 [High] Import inner integrity HMAC not verified after inner decryption
Category: Security

When Import processes an inner-wrapped key (symAlg != NULL and encryptionKey provided), it reads the innerIntegrity size from the decrypted buffer, AES-CFB decrypts the inner sensitive data, but never verifies the inner integrity HMAC. Per TPM 2.0 spec Part 1 Section 23.4, inner wrapping includes innerIntegrity = Hash(sensitive || objectName) that must be verified before accepting the decrypted sensitive. The code reads innerIntegSz at line 4385 but only uses it to compute the offset to skip past the integrity bytes. The actual integrity digest is never compared against a computed hash. This means a tampered inner-wrapped blob will be accepted as valid as long as the outer wrapping passes.

innerIntegSz = FwLoadU16BE(plainSens);
innerStart = 2 + innerIntegSz;
...
rc = TPM2_AesCfbDecrypt(encKeyBuf, encKeySz,
    NULL, encInner, (word32)encInnerSz);
...
XMEMMOVE(plainSens, plainSens + innerStart, encInnerSz);
plainSensSz = encInnerSz;
/* NO INTEGRITY VERIFICATION */

Recommendation: After AES-CFB decryption, compute Hash(decryptedSensitive || objectName) using the object's nameAlg and compare against the innerIntegrity digest at plainSens[2..2+innerIntegSz]. Reject with TPM_RC_INTEGRITY if they don't match.

[aidangarske]

🔴 [High] FwImportReconstructKey: No validation that sensType matches objectPublic.type (type confusion)
Category: Security

FwImportReconstructKey dispatches on sensType (from encrypted sensitive blob) but reads key parameters from objectPublic. If sensType == TPM_ALG_RSA but objectPublic->publicArea.type == TPM_ALG_ECC, the function accesses objectPublic->publicArea.parameters.eccDetail (via the ECC branch) while the union actually contains rsaDetail data, causing union type confusion. An attacker controlling the encrypted duplicate blob can set sensType to a mismatched algorithm, causing key material confusion or crashes.

if (sensType == TPM_ALG_ECC && primeSz > 0) {
    curveId = objectPublic->publicArea.parameters.eccDetail.curveID;
    // But what if objectPublic->publicArea.type == TPM_ALG_RSA?

Recommendation: Add at function start: if (sensType != objectPublic->publicArea.type) { return TPM_RC_TYPE; }

[aidangarske]

🔴 [High] FwDeriveEccPrimaryKey/FwDerivePrime: KDFa context counter starts at 0 instead of 1
Category: Security

Both FwDeriveEccPrimaryKey (line 552) and FwDerivePrime (line 658) initialize their iteration counter to 0. Per TPM 2.0 Part 1, the derivation counter passed as contextV should start at 1. Starting at 0 means the fwTPM derives different primary keys than a spec-compliant TPM given the same seed and template, breaking deterministic key derivation interoperability.

UINT32 counter = 0;
while (!valid && counter < 100) {
    FwStoreU32BE(counterBuf, counter);
    kdfRet = TPM2_KDFa_ex(nameAlg, seed, FWTPM_SEED_SIZE,
        "ECC", hashUnique, hashUniqueSz,
        counterBuf, sizeof(counterBuf), dBuf, keySz);

Recommendation: Initialize counter to 1: UINT32 counter = 1;

[aidangarske]

🟠 [Medium] TPM2_ParamEnc_CmdRequest silently succeeds without encrypting on unrecognized symmetric algorithm
Category: Security

After TPM2_BuildParamKey sets rc=0, if session->symmetric.algorithm is neither XOR nor AES-CFB, neither encryption branch executes and rc remains 0. The function returns TPM_RC_SUCCESS without encrypting, sending sensitive parameters in plaintext despite the caller requesting encryption.

rc = TPM2_BuildParamKey(...);
if (rc != 0) { return rc; }
if (session->symmetric.algorithm == TPM_ALG_XOR) { ... }
else if (...AES...CFB...) { ... }
// No else clause - returns 0

Recommendation: Add else clause: else { rc = TPM_RC_FAILURE; }

[aidangarske]

🟠 [Medium] PolicySigned skips signature verification for trial sessions
Category: Security

Signature verification is only performed for TPM_SE_POLICY sessions. For trial sessions (TPM_SE_TRIAL), any signature is accepted without verification. While trial sessions don't enforce policy, the spec does not exempt them from signature verification in PolicySigned.

if (rc == 0 && sess->sessionType == TPM_SE_POLICY) {
    // verify signature
    rc = FwVerifySignatureCore(authObj, aHash, dSz, &sig);
}
/* Trial sessions skip this block entirely */

Recommendation: Verify signatures for both TPM_SE_POLICY and TPM_SE_TRIAL sessions.

[aidangarske]

🟠 [Medium] FwComputeSessionHmac: sessionKey copy lacks bounds check
Category: Security

The sessionKey copy into hmacKey buffer at line 469 has no bounds check. Only the subsequent authValue append checks hmacKeySz + authValueSz <= sizeof(hmacKey). If sess->sessionKey.size is corrupted to exceed TPM_MAX_DIGEST_SIZE, it overflows the 128-byte hmacKey buffer.

if (sess->sessionKey.size > 0) {
    XMEMCPY(hmacKey, sess->sessionKey.buffer, sess->sessionKey.size);
    hmacKeySz = sess->sessionKey.size;
}

Recommendation: Add: if (sess->sessionKey.size > TPM_MAX_DIGEST_SIZE) { return TPM_RC_FAILURE; }

[aidangarske]

🟠 [Medium] FwImportParseSensitive: totalSensSize parsed but not validated
Category: Security

Reads totalSensSize from decrypted sensitive buffer but discards it with (void)totalSensSize. This field should be validated against plainSensSz to detect structural corruption.

totalSensSize = FwLoadU16BE(plainSens + sp);
sp += 2;
(void)totalSensSize;

Recommendation: Validate: if (totalSensSize + 2 > plainSensSz) { return TPM_RC_SIZE; }

[aidangarske]

🟠 [Medium] FwMarshalSensitive vs FwMarshalSensitiveStd: Incompatible formats risk silent key corruption
Category: Logic

Two different sensitive marshaling formats exist with no discriminator. FwMarshalSensitive stores DER-encoded private keys; FwMarshalSensitiveStd stores raw key components. Mixing them causes silent data corruption.

/* FwMarshalSensitive: type(2) + auth(TPM2B) + der(TPM2B) */
/* FwMarshalSensitiveStd: totalSize(2) + type(2) + auth(TPM2B) + seedValue(TPM2B) + sensitive(TPM2B) */

Recommendation: Add a format discriminator or unify the formats.

[aidangarske]

🟠 [Medium] FwEncryptSeed ECC: Seed not zeroed when encSeed packing fails
Category: Zeroization

If KDFe succeeds writing seed to seedBuf but encSeed buffer packing fails (TPM_RC_SIZE), seedBuf retains sensitive material without cleanup.

if ((int)(4 + ephemXSz + ephemYSz) > encSeedBufSz) {
    rc = TPM_RC_SIZE;
    /* seedBuf not zeroed here */
}

Recommendation: Add TPM2_ForceZero(seedBuf, seedBufSz) on the failure path.