You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Keep slot-1 session in SignSequenceComplete - #642
wolfTPM2_SignSequenceComplete sets slot 1 with wolfTPM2_SetAuthHandle, which resets it to a password session. If the caller had an HMAC session there for parameter encryption, it is silently dropped. The next wolfTPM2_Decapsulate then runs without it, and the ML-KEM shared secret comes back in the clear.
Fix
Save slot 1 before the command and restore it after. If it holds an HMAC session and slot 2 is free, run that session from slot 2 during the sign so it still protects the command, then copy it back with its updated nonces.
Switching to wolfTPM2_SetAuthHandleName does not work: an unbound HMAC session does not carry the key's authValue, so signing with a password-protected key fails with TPM_RC_AUTH_FAIL.
Testing
test_wolfTPM2_MLDSA_SignSequence_NonEmptyAuth now signs with an HMAC session in slot 1, checks the session survives with a rolled nonce, and verifies the signature through it. Passes on fwTPM (--enable-v185). Fails before the fix, with the SetAuthHandleName variant (0x8e), and without the slot-2 move.
wolfTPM2_SignSequenceComplete authorizes keyHandle with
wolfTPM2_SetAuthHandle(dev, 1, ...), which resets session slot 1 to
password auth. A caller's parameter-encryption HMAC session in slot 1
was silently dropped, so the next command (e.g. wolfTPM2_Decapsulate)
returned its secret unencrypted and the session's rolling nonce was
lost.
Save slot 1 before the command and restore it afterwards. When the
saved session is an HMAC session and slot 2 is free, run it from slot
2 for this command, where the TPM takes extra encrypt and audit
sessions, so it also protects the sign. Copy it back with the nonces
the command advanced.
Authorizing the key through the slot-1 session instead would fail
with TPM_RC_AUTH_FAIL for keys with an authValue, since an unbound
HMAC session does not carry the key's authValue.
Extend the ML-DSA key-auth sign test to sign with an HMAC session in
slot 1, check the session is kept and its nonce rolled, and verify the
signature through the same session.
- With MAX_SESSION_NUM=1 (--enable-smallstack --disable-wolfcrypt)
the slot-1 save and restore read and wrote past dev->session.
Compile them only when a second slot exists.
- A policy-auth key keeps its policy session in slot 1 and the
command advances its nonce; restoring the snapshot rolled that
nonce back. Skip the save and restore for policyAuth keys.
- With MAX_SESSION_NUM=2 the session cannot move to slot 2, so its
nonce does not roll. Check the rolled nonce only when slot 2
exists.
The slot-1 session copied into slot 2 kept its Name. For a session
bound to that entity (name == bindName), TPM2_ParamEncBindKey() then
appended the bind authValue to the parameter-encryption key, but slot
2 authorizes no handle, so the TPM used the session key alone. The
command HMAC still validated, while the TPM decrypted the message to
other bytes and signed those.
Clear the Name in slot 2 and put the caller's Name back on restore,
unless the response retired the session. Add a bound-session
regression that verifies the signature over the original message.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The regression test can abort rather than skip unsupported sequence signing; cryptographic session handling also warrants human review.
No open issues 🎉
1 previously missed issue
In code that hasn't changed since last review
Avoid retrying ML-DSA signing when SignSequenceStart is unsupported
tests/unit_tests.c:10570
Medium (CWE-617): If the TPM supports the ML-DSA key but rejects SignSequenceStart as unsupported, the preceding helper sets sigSz to zero and returns. This block still starts a session and retries signing. That attempt also skips without advancing the TPM nonce, so the nonce assertion fails when MAX_SESSION_NUM > 2, aborting the suite instead of preserving the existing skip behavior. Unload the key and return before starting the session when the initial helper produces no signature.
When the TPM rejects SignSequenceStart, the sign helper sets sigSz to
zero and returns. The slot-1 session checks still ran, and with a
free slot 2 the nonce assertion failed because nothing advanced the
nonce, aborting the suite. Unload the key and return once the first
sign produces no signature.
The reason will be displayed to describe this comment to others. Learn more.
The added tests only cover policyAuth==0 keys, so this skip branch never runs. Can you add a policy-authorized ML-DSA sign with the policy session in slot 1, and assert the handle survives, its nonce advances, and a later command on that session still succeeds?
The reason will be displayed to describe this comment to others. Learn more.
Agreed, and added test_wolfTPM2_MLDSA_SignSequence_PolicyAuth in 3db79c0. It signs with a policy-only ML-DSA key using a policy session in slot 1, then checks that:
the session handle is still in slot 1
its nonceTPM advanced
a second PolicyAuthValue and sign on the same session succeeds
Removing the policyAuth guard makes the test fail. It also passes with MAX_SESSION_NUM=2 and compiles out with --disable-wolfcrypt.
NB, test uses a bound policy sesh. WIth an unbound, unsalted sesh, wolfTPM2_PolicyAuthValue computes the wrong HMAC and the TPM returns TPM_RC_AUTH_FAIL. Bug on master, so I can open a separate issue for it.
Cover the policyAuth branch: sign with a PolicyAuthValue-only ML-DSA
key whose policy session sits in slot 1, then assert the session
handle stays, its nonceTPM advances, and a second policy + sign on the
same session succeeds. The session is bound so the wrapper's
PolicyAuthValue HMAC key has a session key in front of the authValue.
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
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.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
aidangarske
left a comment
Problem
wolfTPM2_SignSequenceCompletesets slot 1 withwolfTPM2_SetAuthHandle, which resets it to a password session. If the caller had an HMAC session there for parameter encryption, it is silently dropped. The nextwolfTPM2_Decapsulatethen runs without it, and the ML-KEM shared secret comes back in the clear.Fix
Save slot 1 before the command and restore it after. If it holds an HMAC session and slot 2 is free, run that session from slot 2 during the sign so it still protects the command, then copy it back with its updated nonces.
Switching to
wolfTPM2_SetAuthHandleNamedoes not work: an unbound HMAC session does not carry the key's authValue, so signing with a password-protected key fails withTPM_RC_AUTH_FAIL.Testing
test_wolfTPM2_MLDSA_SignSequence_NonEmptyAuthnow signs with an HMAC session in slot 1, checks the session survives with a rolled nonce, and verifies the signature through it. Passes on fwTPM (--enable-v185). Fails before the fix, with theSetAuthHandleNamevariant (0x8e), and without the slot-2 move.