Repository navigation
tpm2: reject unknown hash state type in HASH_STATE_TYPE_Unmarshal - #644
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: rejecting unknown hash state types during HASH_STATE_TYPE_Unmarshal. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
| * HASH_STATE_HMAC therefore leaves state.smac uninitialized, and | ||
| * CryptDigestUpdate()/CryptMacEnd() would later call through the | ||
| * function pointers in that union. Reject it here. */ | ||
| if (data->type > HASH_STATE_HMAC) { |
There was a problem hiding this comment.
I'd rather have a switch statement in HASH_STATE_TYPE_Unmarshal rejecting bad input there.
There was a problem hiding this comment.
done, moved it into HASH_STATE_TYPE_Unmarshal as a switch that only accepts EMPTY/HASH/HMAC and returns TPM_RC_BAD_PARAMETER for anything else. HASH_STATE_Unmarshal itself is untouched now.
| if (rc == TPM_RC_SUCCESS) { | ||
| /* ANY_HASH_STATE_{Marshal,Unmarshal} only handle the plain hash | ||
| * contexts (EMPTY/HASH/HMAC); they never serialize the SMAC union | ||
| * (smacMethods function pointers + CMAC state). A type beyond |
There was a problem hiding this comment.
CrypySmacStart is the only place that sets the HASH_STATE_SMAC value. And you are saying that no internal hash object of type HASH_STATE_SMAC will ever have to be marshalled?
There was a problem hiding this comment.
My concern here would be a sequence like TPM2_MAC_Start() or TPM2_HMAC_Start() create this SMAC and then a request for marshalling the state occurs.
There was a problem hiding this comment.
... it's also quite possible that the support for SMAC marshalling is simply missing at the moment...
There was a problem hiding this comment.
you're right that TPM2_MAC_Start creates one: ObjectCreateHMACSequence goes through CryptMacStart, and with a CMAC symcipher key that lands in CryptSmacStart which sets type = HASH_STATE_SMAC on a live sequence object, so it does get marshalled by ContextSave and by the volatile state dump. TPM2_HMAC_Start is fine, CryptMacStart routes hash algs to CryptHmacStart and that's type HMAC.
and yes, SMAC marshalling is simply missing today. CryptMacStart zeroes the state so hashAlg stays 0, HASH_STATE_Marshal writes type=3 and hashAlg=0, and ANY_HASH_STATE_Marshal falls into its default case and writes nothing, so neither the smacMethods pointers nor the tpmCmacState (symKey, iv, bcount) ever make it into the blob. on the way back in, CryptGetHashDef(0) returns NULL_Def so the unmarshal succeeds, and the object comes back as type SMAC with whatever was left in that slot for smacMethods (NULL in the usual case). the next SequenceUpdate or SequenceComplete calls through that pointer. so a saved and restored CMAC sequence is already broken, this just turns it into TPM_RC_BAD_PARAMETER at load instead of a crash later. the one behaviour change worth noting is that a volatile state blob with an in-flight CMAC sequence now fails to load as a whole, rather than loading and only blowing up if that sequence is continued.
if you'd rather have it actually work, the fix is to marshal tpmCmacState for type SMAC and re-bind smacMethods to CryptCmacData/CryptCmacEnd on unmarshal (needs a HASH_STATE version bump or an append in the skip block). happy to do that here or as a follow-up, your call given it touches the state format.
There was a problem hiding this comment.
if you'd rather have it actually work, the fix is to marshal tpmCmacState for type SMAC and re-bind smacMethods to CryptCmacData/CryptCmacEnd on unmarshal (needs a HASH_STATE version bump or an append in the skip block). happy to do that here or as a follow-up, your call given it touches the state format.
I'd rather have it work as well... I wasn't able to create a CMAC/SMAC yesterday. Do you have a sample program to create it and where we can do a state suspend/resume on it?
There was a problem hiding this comment.
I implemented the support for marshalling and unmarshalling (PR #645 ) of the state in case we wanted to backport support for this, which I am not sure whether we should do this or rather go with your blocker. I was not able to create an SMAC so far...
I also don't want to merge my PR because there will be lots of changes coming to the marshalling and unmarshalling functions so that rebasing these changes that I already have elsewhere would painful and a source of potential errors.
There was a problem hiding this comment.
re creating one: the catch is probably that TPM_CC_MAC_Start and TPM_CC_HMAC_Start share the same command code (0x15B), so it's the key type that picks the path, not the command. the sequence i used:
- create an unrestricted SYMCIPHER key (AES-128) with
signset anddecrypt/restrictedclear, then load it - send 0x15B on that key handle with inScheme = TPM_ALG_CMAC (0x3F). CryptSelectMac accepts it, ObjectCreateHMACSequence -> CryptMacStart -> CryptSmacStart, and the sequence object now has type HASH_STATE_SMAC
- optionally a TPM2_SequenceUpdate with some data
- suspend/resume: TPMLIB_GetState(TPMLIB_STATE_VOLATILE) then TPMLIB_SetState with that blob on a fresh instance (or ContextSave/ContextLoad on the sequence handle)
- TPM2_SequenceUpdate or TPM2_SequenceComplete on the sequence handle
before this change step 5 calls through a NULL smacMethods pointer, with it the SetState/ContextLoad in step 4 fails with TPM_RC_BAD_PARAMETER. with #645 step 5 should succeed and the CMAC should match one computed without the suspend/resume, which would be the test for it.
on the backport, the blocker seems fine for stable to me since a CMAC sequence never survived a save/restore anyway, and #645 can come with the marshalling rework. thanks for merging this one.
88570d0 to
d9cfe73
Compare
HASH_STATE_TYPE_Unmarshal restored the type byte from the state blob without checking it. ANY_HASH_STATE_Marshal/Unmarshal never serialize the SMAC state, so a blob with type HASH_STATE_SMAC produces a hash state whose smacMethods function pointers are whatever the hash context unmarshal left in the union, and CryptDigestUpdate() / CryptMacEnd() later call through them. Only accept EMPTY, HASH and HMAC. Signed-off-by: sameera <sameera@research.itecz.au>
sam-kota
changed the title
d9cfe73 to
02ecb97
Compare
mask off the trust issue in HASH_STATE_Unmarshal: it restores the HASH_STATE_TYPE byte straight from the state blob without a range check, but ANY_HASH_STATE_Marshal/Unmarshal never serialize the SMAC union, so a restored type==HASH_STATE_SMAC leaves state.smac (the smacMethods function pointers) uninitialized/attacker-controlled and a later SequenceUpdate/SequenceComplete calls through it via CryptDigestUpdate()/CryptMacEnd(), so reject any type past HASH_STATE_HMAC the same way the array_size guards here reject bad sizes.
reachable through the same TPMLIB_SetState(volatile)/ContextLoad path as the num/md_len bounds fix; builds clean with --with-openssl --with-tpm2 --with-tpm1 under asan/ubsan and make check still passes (9/9 runnable, base64decode.sh fails only due to macOS base64 cli syntax, pre-existing).
Summary by CodeRabbit