Repository navigation
Backport a few patches for v0.8.11 - #623
Open
stefanberger wants to merge 3 commits into
Open
stefanberger wants to merge 3 commits into
stefanberger wants to merge 3 commits into
Conversation
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
| Layer / File(s) | Summary |
|---|---|
Reject negative remaining-buffer sizes src/tpm2/Unmarshal.c |
UINT8_Unmarshal, UINT16_Unmarshal, UINT32_Unmarshal, UINT64_Unmarshal, and Array_Unmarshal return TPM_RC_INSUFFICIENT before reading or copying data when *size is negative. |
Estimated code review effort: 1 (Trivial) | ~5 minutes
Merge Risk: 
4ab38
-
TPM unmarshaling now rejects invalid negative buffer sizes before processing data, preventing unsafe length conversion and memory access. The change is ready to merge.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
-
❌ Failed checks (1 warning)
Check name Status Explanation Resolution Docstring Coverage ⚠️ WarningDocstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold. ✅ Passed checks (4 passed)
-
Check name Status Explanation Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled. Title check ✅ Passed The title accurately identifies the primary change as backporting patches for the v0.8.11 release. It is concise and relevant, although it does not describe the negative-size validation details. Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request. Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
-
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
-
- Create PR with unit tests
- Commit unit tests in branch
stable-0.8.next
-
- 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.
…rypt
TPM_SymmetricKeyData_Decrypt (AES path in src/tpm12/tpm_crypto.c,
OpenSSL backend; and the NSS/FreeBL backend in
src/tpm12/tpm_crypto_freebl.c) only checked
encrypt_length < TPM_AES_BLOCK_SIZE before calling
AES_cbc_encrypt(..., AES_DECRYPT) on a buffer allocated to exactly
encrypt_length bytes. CBC decryption always consumes a whole final
block, so a length that is not a multiple of 16 makes OpenSSL read up
to 15 bytes past the end of the input buffer.
That length is TPM_CONTEXT_BLOB.sensitiveData.size from the command
stream. TPM_Process_LoadKeyContext and TPM_Process_LoadAuthContext run
under TPM_CHECK_ALLOW_NO_OWNER and decrypt the blob with the permanent
contextKey before the integrity HMAC is checked, so the over-read is
reachable with a single unauthenticated command.
Reproduced with a TPM_LoadKeyContext command whose sensitiveData.size
is 17; AddressSanitizer reports a heap over-read:
READ of size 4 ...
#0 AES_decrypt aes_core.c
#1 CRYPTO_cbc128_decrypt cbc128.c
#2 TPM_SymmetricKeyData_Decrypt tpm_crypto.c
#3 TPM_Process_LoadKeyContext tpm_session.c
The fix rejects a length that is not a whole number of blocks, the same
requirement the DES backend already enforces. The command then returns
TPM_DECRYPT_ERROR. A zero length is still rejected by TPM_Malloc().
Signed-off-by: sameera <sameera@research.itecz.au>
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
#1 SHA1_Update
#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>
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.
This PR backports a few patches for v0.8.11
Summary by CodeRabbit