Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces functionality to decode TPM logs by adding tests and refactoring related code.
- Added unit tests for the TpmtHa functionality to validate digest sizes and error handling.
- Introduced tests for EfiTime binary decoding and added new methods (from_binary and get_datetime) in the EfiTime class.
- Refactored the EfiTime class for consistent naming and improved logging using f-string formatting.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests.unit/test_tcg_platform.py | Unit tests for TpmtHa initialization and reset functionality |
| tests.unit/test_authenticated_variables_structure_support.py | Added tests for EfiTime conversion from binary data |
| edk2toollib/uefi/authenticated_variables_structure_support.py | Refactored EfiTime: updated naming conventions, logging improvements, and enhanced binary conversion methods |
There was a problem hiding this comment.
I suspect I will need you to write more unit tests to improve code coverage. Seeing as the main logic is 1k lines, I suspect there will be some missing lines. Will be looking for 80% on the patch
| return valid_algs[alg] | ||
|
|
||
|
|
||
| class TpmtHa(object): |
| self.hash_alg = alg | ||
| self.hash_digest = b"\x00" * SIZE_FROM_ALG[alg] | ||
| else: | ||
| self.hash_alg = ALG_FROM_SIZE[len(digest)] |
There was a problem hiding this comment.
probably need to either first make sure the length of the digest is an expected length and return a well described exception if not
| """ | ||
| if digest is None: | ||
| self.hash_alg = alg | ||
| self.hash_digest = b"\x00" * SIZE_FROM_ALG[alg] |
There was a problem hiding this comment.
Same here but for the alg id number passed in
| """ | ||
| alg, *_ = struct.unpack_from(TpmtHa._hdr_struct_format, binary_data) | ||
| offset = TpmtHa._hdr_struct_size | ||
| digest = binary_data[offset: offset + SIZE_FROM_ALG[alg]] |
There was a problem hiding this comment.
And you will need to do the safety check here too.
| % (len(digest), SIZE_FROM_ALG[self.hash_alg]) | ||
| ) | ||
| hasher = _get_hasher_for_alg(self.hash_alg) | ||
| hasher.update(self.hash_digest + digest) |
There was a problem hiding this comment.
Is this right? I would have thought you need to set the hasher starting state to self.hash_digest, then hash the provided bytes.
Would this not hash re-hash the current digest?
e.g.
bytes1 = b"Hello There Testing This"
bytes2 = b"Hello Testing Again with something else"
hasher1 = hashlib.sha3_256()
hasher1.update(bytes1)
hasher1.update(bytes2)
o1 = hasher1.digest()
hasher2 = hashlib.sha3_256()
hasher2.update(bytes1)
o2 = hasher2.digest()
# Temp just to get the first hash
hasher3 = hashlib.sha3_256()
hasher3.update(o2 + bytes2)
o3 = hasher3.digest()
assert o1 == o3 # This would fail
| self.digests = digests | ||
| if algs is not None: | ||
| for alg in algs: | ||
| self.add_algorithm(alg) |
There was a problem hiding this comment.
probably need to do a safety check of supported algs
| """Class constructor method. | ||
|
|
||
| Args: | ||
| algs (Iterable, optional): Hash algorithms supported. Defaults to None. |
There was a problem hiding this comment.
missing the underlying iterable type here - is it the same int as above?
| digest.reset_with_locality(locality) | ||
| return self | ||
|
|
||
| def set_hash_data(self, data): |
There was a problem hiding this comment.
missing function description or a "_" to indicate its private
| for digest in self.digests.values(): | ||
| digest.set_hash_data(data) | ||
|
|
||
| def extend_data(self, data): |
There was a problem hiding this comment.
missing function description or a "_" to indicate its private
| ) | ||
| offset += digest_sizes[alg].get_size() | ||
|
|
||
| vendor_info_size, *_ = struct.unpack_from( |
There was a problem hiding this comment.
This is cool, I did not know you could do *_
Javagedes
left a comment
Javagedes
left a comment
There was a problem hiding this comment.
Overall looks good to me. Needs a few changes, and some more tests, but that is about it.
|
@Flickdm is this still a priority? |
This Pull Request provides basic decoding of the TPM Log
In windows this is found at
C:\Windows\Logs\MeasuredBoot. Decoding the TCG Log Tool allows for understanding things like why is a system bitlockering, if a system is compliant and so on.