Repository navigation
tpm12: validate SHA1 context num on load in TPM_Sha1Context_Load - #647
Conversation
TPM_Sha1Context_Load() (OpenSSL backend, src/tpm12/tpm_crypto.c)
deserializes the OpenSSL SHA_CTX from a TPMLIB_SetState(VOLATILE) blob
and restores sha_ctx->num with TPM_Load32() without any range check.
num is the number of bytes buffered in sha_ctx->data and OpenSSL uses
it as the write offset in SHA1_Update()/SHA1_Final(): the update path
runs memcpy(p + num, data, SHA_CBLOCK - num). A restored num >=
SHA_CBLOCK makes SHA_CBLOCK - num wrap to a huge size_t, so the next
update writes far past the heap-allocated context, and SHA1_Final()
writes the 0x80 pad byte at p[num] well outside it.
The restored sha1_context / sha1_context_tis are later driven by the
TPM_SHA1Update command and by the host-side TPM_IO_Hash_Data(), both of
which reach TPM_SHA1UpdateCmd() -> SHA1_Update(). The volatile-state
blob is only protected by an unkeyed SHA1 integrity digest computed
after parsing, so num is attacker-controlled across a state restore.
Reproduced by loading a SHA1 context with num = 256 and then issuing
one update; AddressSanitizer reports a wrapped copy:
ERROR: AddressSanitizer: negative-size-param: (size=-192)
#0 memcpy
stefanberger#1 SHA1_Update
stefanberger#2 TPM_SHA1UpdateCmd tpm_crypto.c:2028
Reject num >= SHA_CBLOCK with TPM_BAD_PARAMETER and free the
partially loaded context, the same num bound the TPM 2.0 hash-state
unmarshalers already enforce. The FreeBL backend loads into a
fixed-layout struct guarded by a flatten-size check and does not
restore a write offset, so only the OpenSSL path is affected.
Signed-off-by: sameera <sameera@research.itecz.au>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Validate and clean up loaded contexts src/tpm12/tpm_crypto.c |
The loader rejects a buffered-byte count greater than or equal to SHA_CBLOCK. On load failure, it frees the allocated context and sets the pointer to NULL. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Suggested reviewers: stefanberger
Merge Risk: 
6b865
-
The change addresses the malformed SHA-1 state without an established remaining merge-blocking risk.
Pre-merge checks: 
-
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: validating the SHA-1 context buffer count during loading in TPM_Sha1Context_Load. Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. 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.
|
Thanks a lot! |
TPM_Sha1Context_Load (the OpenSSL backend in src/tpm12/tpm_crypto.c) restores the SHA_CTX from a TPMLIB_SetState volatile-state blob and reads sha_ctx->num with TPM_Load32 without range-checking it. num is the in-buffer write offset OpenSSL uses in SHA1_Update (memcpy(p + num, data, SHA_CBLOCK - num)), so a restored num >= SHA_CBLOCK makes the length wrap and the next update walks off the heap-allocated context. The restored sha1_context / sha1_context_tis get there through the TPM_SHA1Update command and the host-side TPM_IO_Hash_Data, and the blob is only covered by an unkeyed SHA1 digest checked after parsing, so num is attacker-controlled across a restore.
fix rejects num >= SHA_CBLOCK and frees the partial context, the same bound the tpm2 hash-state unmarshalers already enforce. keeping it in the loader means every restore path gets the check without each caller repeating it. reproduced with a context loaded with num=256 plus one update; asan reports a negative-size memcpy in SHA1_Update via TPM_SHA1UpdateCmd. make check still passes (fuzz.sh state round-trip included), so valid contexts still load.
Summary by CodeRabbit