trunks: Add missing checks on TPM2B_'s size being non-zero |
||||
Issue descriptionSince https://crrev.com/c/906930 it's possible that the size field in the parsed complex TPM2B_* structures is zero, which indicates that the nested structure was not parsed and stays uninitialized. We should ensure that the code in Trunks handles this gracefully - i.e., in most cases, check |size| and fail-fast when it's zero, instead of reading from the (uninitialized) nested structure. Reading from an uninitialized memory is an Undefined Behavior with potentially bad consequences. As a particular example, take TpmUtilityImpl::GetNVSpacePublicArea(). It obtains a TPM2B_NV_PUBLIC struct from NV_ReadPublicSync() and later reads from the |nv_public| field of it (that is, reads the TPMS_NV_PUBLIC structure). While this only happens if NV_ReadPublicSync() returns TPM_RC_SUCCESS, we should be on the safe side and guard the accesses with the check of the |size|.
,
May 17 2018
So far found these potential issues: * TpmUtilityImpl::GetNVSpacePublicArea() - read of potentially uninitialized sub-structure from TPM2B_NV_PUBLIC. * TpmUtilityImpl::GetKeyPublicArea() - TPM2B_PUBLIC. * SessionManagerImpl::EncryptSalt() - TPM2B_PUBLIC. * cryptohome's Tpm2Impl::CreatePCRBoundKey() - TPM2B_PUBLIC. * cryptohome's Tpm2Impl::VerifyPCRBoundKey() - TPM2B_CREATION_DATA. * attestation's TpmUtilityV2::CreateRestrictedKey() - TPM2B_PUBLIC.
,
May 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/571677a83d9077ca4dcd56fec8fe7ddb7bc23137 commit 571677a83d9077ca4dcd56fec8fe7ddb7bc23137 Author: Maksim Ivanov <emaxx@google.com> Date: Tue May 22 04:17:53 2018 trunks: Avoid uninit read after NV_ReadPublic Make TpmUtilityImpl::GetNVSpacePublicArea() resistable to the case when Tpm::NV_ReadPublicSync() returns TPM_RC_SUCCESS, but the returned TPM2B_NV_PUBLIC structure has zero |size|. In that case the nested TPMS_NV_PUBLIC structure is uninitialized, and should not be read (while the old code could do this, leading to undefined behavior). Also fix many NVRAM related unit tests whose insufficient mocks were also leading to reads of uninitialized memory by callers of GetNVSpacePublicArea(). BUG=chromium:843662 TEST=new and renamed unit tests: NVTpmUtilityTest.* Change-Id: I30c8bbbb0f225858479a99fdfc4d13b31b0fadf9 Reviewed-on: https://chromium-review.googlesource.com/1064417 Commit-Ready: Maksim Ivanov <emaxx@chromium.org> Tested-by: Maksim Ivanov <emaxx@chromium.org> Reviewed-by: Andrey Pronin <apronin@chromium.org> [modify] https://crrev.com/571677a83d9077ca4dcd56fec8fe7ddb7bc23137/trunks/tpm_utility_test.cc [modify] https://crrev.com/571677a83d9077ca4dcd56fec8fe7ddb7bc23137/trunks/tpm_utility_impl.cc [modify] https://crrev.com/571677a83d9077ca4dcd56fec8fe7ddb7bc23137/trunks/tpm_utility_impl.h
,
Oct 12
,
Dec 10
|
||||
►
Sign in to add a comment |
||||
Comment 1 by emaxx@chromium.org
, May 17 2018