Harden fwTPM key derivation and command validation - #596
Draft
aidangarske wants to merge 8 commits into
Draft
Conversation
aidangarske
commented
Sep 9, 2026
aidangarske
commented
Member
There was a problem hiding this comment.
🟡 Changes recommended
The new RNG parameters are used without defined NULL-handling in hardened ECC paths and the public header prototype changes are API-breaking without an explicit compatibility/versioning plan.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens fwTPM cryptographic operations and command validation paths, primarily by adding RNG-backed blinding to ECC operations and tightening command behavior around PCR capability reporting, hierarchy selection, and physical presence.
Changes:
- Plumbs
WC_RNG*into ECC primary key derivation and ECC shared-point computation to enable RNG-backed timing/blinding hardening where supported. - Improves safety/robustness in crypto helpers (OOM handling in
FwComputePublicName, guarded sensitive marshaling inFwWrapPrivate, and safer RSA CRT cleanup on init failures). - Tightens command behavior (PCR bank capability output reflects allocated banks; invalid hierarchy selectors return
TPM_RC_VALUEvsTPM_RC_HIERARCHY; platform PP latch disabled unless explicitly opted-in).
File summaries
| File | Description |
|---|---|
| wolftpm/fwtpm/fwtpm_crypto.h | Updates public prototypes to accept WC_RNG* for ECC derivation/shared-point routines. |
| src/fwtpm/fwtpm_crypto.c | Implements RNG-backed ECC hardening and improves error/cleanup handling in several crypto helpers. |
| src/fwtpm/fwtpm_command.c | Updates PCR capability response generation, refines hierarchy-handle validation, and gates platform PP latch behind an opt-in macro. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
751
to
755
| #ifdef ECC_TIMING_RESISTANT | ||
| rc = wc_ecc_make_pub_ex(eccKey, NULL, NULL); | ||
| rc = wc_ecc_make_pub_ex(eccKey, NULL, rng); | ||
| #else | ||
| (void)rng; | ||
| rc = wc_ecc_make_pub(eccKey, NULL); |
Comment on lines
+3637
to
+3646
| #ifdef WOLFSSL_PUBLIC_ECC_ADD_DBL | ||
| /* RNG-blinded scalar multiply bound to the curve order */ | ||
| rc = wc_ecc_mulmod_ex2(ecc_get_k(priv), &peer->pubkey, R, &a, | ||
| &prime, &order, rng, 1, NULL); | ||
| #else | ||
| /* wc_ecc_mulmod_ex2 is public only with WOLFSSL_PUBLIC_ECC_ADD_DBL; | ||
| * fall back to the base multiply when it is unavailable */ | ||
| (void)rng; | ||
| rc = wc_ecc_mulmod(ecc_get_k(priv), &peer->pubkey, R, &a, &prime, 1); | ||
| #endif |
Comment on lines
127
to
130
| TPM_RC FwDeriveEccPrimaryKey(TPMI_ALG_HASH nameAlg, | ||
| const byte* seed, const byte* hashUnique, int hashUniqueSz, | ||
| UINT16 curveId, TPMS_ECC_POINT* pubOut, | ||
| UINT16 curveId, WC_RNG* rng, TPMS_ECC_POINT* pubOut, | ||
| byte* privKeyDer, int privKeyDerBufSz, int* privKeyDerSz); |
Comment on lines
333
to
337
| int FwImportEccKeyFromDer(const FWTPM_Object* obj, ecc_key* key); | ||
| int FwImportEccPubFromPublic(const TPMT_PUBLIC* pub, ecc_key* key); | ||
| int FwImportEccKey(const FWTPM_Object* obj, ecc_key* key); | ||
| int FwEccSharedPoint(ecc_key* priv, ecc_key* peer, | ||
| int FwEccSharedPoint(ecc_key* priv, ecc_key* peer, WC_RNG* rng, | ||
| byte* xBuf, word32* xSz, byte* yBuf, word32* ySz); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.