PQC parameter encryption for examples, plus bound-session key fixes#531
PQC parameter encryption for examples, plus bound-session key fixes#531dgarske wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates wolfTPM’s session and parameter-encryption behavior to support PQC-keyed parameter-encryption sessions in examples (ML-KEM as salt key, ML-DSA as bind key), adds create_primary ML-DSA support, and fixes two spec-compliance issues in session-key/param-key derivation that caused real TPMs to reject commands.
Changes:
- Fix sessionKey derivation for bound sessions with EmptyAuth (client and fwTPM): sessionKey is now computed even with zero-length bind authValue input, per TPM 2.0.
- Fix parameter-encryption key derivation for bound sessions by avoiding unconditional concatenation of the bind authValue.
- Add PQC parameter-encryption options (
-mlkem,-mldsa,-paramkey=...) across multiple examples; addcreate_primary -mldsa; extend docs andrun_examples.shcoverage.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/tpm2_wrap.c |
Adjusts client sessionKey derivation to compute sessionKey when bound even if bind authValue is empty. |
src/tpm2_param_enc.c |
Changes param-encryption key building to only append bind authValue in specific cases (instead of always). |
src/fwtpm/fwtpm_command.c |
Mirrors bound/EmptyAuth sessionKey derivation fix in fwTPM StartAuthSession implementation. |
examples/wrap/wrap_test.c |
Adds PQC param-encryption CLI options and uses shared helper to create PQC primary + start session. |
examples/wrap/include.am |
Links wrap_test against examples/tpm_test_keys.c for the new helper. |
examples/tpm_test_keys.h |
Declares new helper for PQC primary + param-encryption session setup. |
examples/tpm_test_keys.c |
Implements PQC primary creation and session start for ML-KEM (salt) / ML-DSA (bind). |
examples/run_examples.sh |
Adds v1.85 PQC runs for create_primary -mldsa and PQC-keyed parameter encryption coverage. |
examples/README.md |
Documents PQC-keyed parameter-encryption session options and usage. |
examples/pqc/README.md |
Adds PQC parameter-encryption documentation and create_primary -mldsa notes. |
examples/pcr/quote.c |
Adds PQC param-encryption CLI options and uses PQC helper. |
examples/nvram/store.c |
Adds PQC param-encryption CLI options and uses PQC helper. |
examples/nvram/counter.c |
Adds PQC param-encryption CLI options and uses PQC helper. |
examples/keygen/keygen.c |
Adds `-paramkey=mlkem |
examples/keygen/create_primary.c |
Adds `-mldsa[=44 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
examples/tpm_test_keys.c:1
- Invalid parameter-set values are silently coerced to the default (e.g.,
-mlkem=999becomes 768;-mldsa=1becomes 65). This makes CLI behavior non-obvious and can mask user errors. Recommend validating the numeric value and returning 0 (unrecognized) or a dedicated error path so callers can print a clear message andusage()when an unsupported parameter set is provided.
/* tpm_test_keys.c
aidangarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-securityOverall recommendation: COMMENT
Findings: 6 total — 4 posted, 2 skipped
2 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] [review+review-security] create_primary ML-DSA support guarded by WOLFTPM_PQC but links against the WOLFTPM_MLDSA-only symbol wolfTPM2_GetKeyTemplate_MLDSA —
examples/keygen/create_primary.c:90-102,142-154,262-270 - [Medium] [review] No automated regression test added for the bound-session / EmptyAuth-bind session-key fixes —
src/tpm2_param_enc.c:201-227, src/tpm2_wrap.c:2627, src/fwtpm/fwtpm_command.c:8306-8325 - [Low] [review] parsePqcParamSet treats a trailing '=' with empty suffix as an error instead of defaulting —
examples/tpm_test_keys.c:512-548 - [Low] [review] Selecting -mlkem/-mldsa (or -paramkey=) without -aes/-xor silently does nothing —
examples/nvram/counter.c:152-175, examples/nvram/store.c:172, examples/pcr/quote.c:218, examples/wrap/wrap_test.c:287, examples/keygen/keygen.c:386
Skipped findings
- [Info]
Parameter-encryption bind-key selection now depends on session-name ordering - [Info]
TPM2_AUTH_SESSION grows by a TPM2B_NAME (bindName) — ABI/size change
Review generated by Skoll
de85c26 to
b2faa40
Compare
aidangarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-security + bugsOverall recommendation: COMMENT
Findings: 4 total — 4 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] [review-security] ML-DSA bind parameter encryption is derived from a public authValue —
examples/tpm_test_keys.c:427-495 - [Medium] [review+review-security+bugs] Invalid PQC parameter-set parse leaves parser outputs armed —
examples/tpm_test_keys.c:506-548 - [Medium] [review+bugs] wrap_test PQC parameter-encryption session is overwritten before use —
examples/wrap/wrap_test.c:300-319 - [Medium] [review+review-security+bugs] Bound EmptyAuth regression test does not exercise the bound HMAC session —
tests/unit_tests.c:709-734
Review generated by Skoll
b2faa40 to
18fb248
Compare
18fb248 to
22a1fc6
Compare
aidangarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-security + bugsOverall recommendation: COMMENT
Findings: 4 total — 4 posted, 0 skipped
2 finding(s) posted as inline comments (see file-level comments below)
2 finding(s) not tied to a diff line (full detail below)
Posted findings
- [Medium] [review-security+bugs] fwTPM parameter encryption helpers can return an uninitialized status —
src/fwtpm/fwtpm_command.c:221-284 - [Medium] [review+bugs] create_primary ML-DSA parser treats explicit invalid suffixes as the default —
examples/keygen/create_primary.c:91-100
Findings not tied to a diff line
Invalid PQC option can be ignored after a previous valid option
File: examples/wrap/wrap_test.c:181-189; examples/nvram/counter.c:124-132; examples/nvram/store.c:138-146; examples/pcr/quote.c:143-151
Function: TPM2_Wrapper_TestArgs / TPM2_NVRAM_Counter_Example / TPM2_NVRAM_Store_Example / TPM2_PCR_Quote_Test
Severity: Medium
Category: bug
All three scan modes flagged this. The new callers use pqcParamEncAlg == TPM_ALG_NULL to decide whether parsePqcParamEncArg() reported an invalid PQC parameter set. That only works before any valid PQC option has been parsed. These examples parse argv from right to left, so a command like wrap_test -mldsa=999 -mlkem=768 first sets pqcParamEncAlg to ML-KEM, then parsePqcParamEncArg() returns -1 for -mldsa=999 without clearing the already-committed output. The invalid option is then silently accepted instead of being the fatal usage error this PR intends. Severity views differ (review: Medium; review-security and bugs: Low) — keeping the stricter Medium.
Recommendation: Store the parser return value and branch on it directly: int pqcRc = parsePqcParamEncArg(...); if (pqcRc < 0) { usage(); return -1; } else if (pqcRc > 0) { /* valid PQC option */ }. Add a regression test with one valid and one invalid PQC option in both argument orders.
Referenced code: examples/wrap/wrap_test.c:181-189; examples/nvram/counter.c:124-132; examples/nvram/store.c:138-146; examples/pcr/quote.c:143-149 (7 lines)
Zephyr wrap_test target misses the new helper source dependency
File: examples/wrap/wrap_test.c:38,181-189,311-312; examples/wrap/include.am:13-14
Function: TPM2_Wrapper_TestArgs
Severity: Medium
Category: Regression
The PR makes wrap_test.c include examples/tpm_test_keys.h and call parsePqcParamEncArg() / getPrimaryParamEncKey() under WOLFTPM_MLKEM || WOLFTPM_MLDSA. Automake and the top-level CMake helper were updated to compile/link examples/tpm_test_keys.c, but the Zephyr sample zephyr/samples/wolftpm_wrap_test/CMakeLists.txt still builds only examples/wrap/wrap_test.c. A PQC-enabled Zephyr wrap_test build will have undefined references to the new helper functions.
Recommendation: Update the Zephyr wrap_test sample to compile examples/tpm_test_keys.c when the same PQC macros can enable these calls, or move the helper linkage into shared Zephyr app sources so the sample stays aligned with the other build systems.
Referenced code: examples/wrap/wrap_test.c:38,181-189,311-312; examples/wrap/include.am:13-19 (7 lines)
Review generated by Skoll
| } | ||
|
|
||
| /* Decrypt first TPM2B parameter of incoming command (param encryption) */ | ||
| static int FwParamDecryptCmd(FWTPM_CTX* ctx, FWTPM_Session* sess, |
There was a problem hiding this comment.
🟠 [Medium] fwTPM parameter encryption helpers can return an uninitialized status · Uninitialized variable used before assignment
The PR changed both fwTPM parameter-encryption helpers from int rc = 0 to int rc while keeping the dispatch limited to XOR and AES-CFB and adding no final else. FwCmd_StartAuthSession stores any non-NULL symmetric definition, and FWTPM_ProcessCommand treats any non-NULL symmetric algorithm as an encryption session, so a caller can start a session with an unsupported non-NULL symmetric algorithm or AES mode (e.g. AES-CBC) and use it on a parameter-encryption-capable command. Before this change the path deterministically returned success; after it the helper returns indeterminate stack garbage, producing nondeterministic success/failure in the auth/parameter-encryption path.
Fix: Initialize rc to a failure code such as TPM_RC_FAILURE in both helpers and add an explicit unsupported-symmetric else (e.g. rc = TPM_RC_SYMMETRIC). Prefer also rejecting unsupported TPMT_SYM_DEF values in FwCmd_StartAuthSession so invalid sessions fail before command processing.
| } | ||
|
|
||
| #ifdef WOLFTPM_MLDSA | ||
| static int mldsaParamSet(const char* optVal, TPMI_MLDSA_PARAMETER_SET* ps) |
There was a problem hiding this comment.
🟠 [Medium] create_primary ML-DSA parser treats explicit invalid suffixes as the default · bug
The new create_primary -mldsa parser maps any suffix that XATOI() converts to zero to the default ML-DSA-65 set. XATOI is atoi, so -mldsa=0 and non-numeric typos like -mldsa=abc silently create an ML-DSA-65 primary instead of rejecting the unsupported parameter set, despite the comment saying only a missing or empty suffix should use the default. Severity views differ (review: Medium; bugs: Low) — keeping the stricter Medium.
Fix: Only use the default for an actually empty suffix, and reject non-digit input before calling XATOI. Add usage-error tests for -mldsa=0 and -mldsa=abc.
PQC parameter encryption for examples, plus parameter-encryption fixes
Adds post-quantum (ML-DSA / ML-KEM) parameter-encryption support to the examples, and fixes several parameter-encryption bugs in the client and the firmware TPM that were uncovered along the way.
Features
-mlkem[=512|768|1024](salt key) and-mldsa[=44|65|87](bind key), plus-paramkey=in keygen. Wired through keygen, wrap_test, pcr/quote, nvram/store and nvram/counter, with a shared parser in tpm_test_keys. ML-DSA is sign-only, so the helper adds a transient SRK salt for confidentiality while the ML-DSA key supplies the binding.create_primarygains-mldsa[=44|65|87]for an ML-DSA primary.-aes/-xornow defaults the session cipher to AES-CFB instead of silently doing nothing.Fixes
KDFa(bindAuth || salt)(TPM 2.0 Part 1 Sec.19.6.8) on both the client and the firmware TPM. The firmware TPM had the inputs reversed (salt || bindAuth), which broke salt+bind sessions, and a session bound to an EmptyAuth entity previously left the session key empty.sessionKey || authValueeven when the session is bound to the entity it authorizes (unlike the command HMAC, which excludes it). The firmware TPM omitted the authValue and silently corrupted parameter-encrypted data for bound-own-entity flows (e.g.examples/nvram/extend.c); now fixed on both sides (TPM2_ParamEncBindKey/FwBuildParamEncKey, withbindNameonTPM2_AUTH_SESSIONtracking the bind entity).TPM2_CreateLoaded: set the responseoutHandleCnt, which was mis-locating the response parameters (and rpHash) and rejecting the reply withTPM_RC_HMACwhenever it ran under a parameter-encryption session.-mlkem=999) is now a fatal usage error in every example instead of silently dropping parameter encryption.-mlkemand-mldsahelp lines are each guarded by their own family macro, so an ML-KEM-only or ML-DSA-only build only advertises what it supports.Tests / testing
unit.testregressions: bound-EmptyAuth param-enc;TPM2_CreateLoadedunder param-enc (fails pre-fix withTPM_RC_HMAC); and a bound-own-entity param-enc test that writes a POLICYWRITE NV index under a bound policy session and reads the data back, proving the param-enc key is correct (catches a wrong key on either the client or the firmware TPM).run_examples.shexercises the ML-KEM-salt and ML-DSA-bind param-enc paths across child-create, attestation and NV.