On 2020/8/12 20:23, Laszlo Ersek wrote:
> On 08/12/20 09:04, wenyi,xie via groups.io wrote:
>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2215
>>
>> There is an integer overflow vulnerability in DxeImageVerificationHandler
>> function when parsing the PE files attribute certificate table. In cases
>> where WinCertificate->dwLength is sufficiently large, it's possible to
>> overflow Offset back to 0 causing an endless loop.
>>
>> Check offset inbetween VirtualAddress and VirtualAddress + Size.
>> Using SafeintLib to do offset addition with result check.
>>
>> Cc: Jiewen Yao <jiewen....@intel.com>
>> Cc: Jian J Wang <jian.j.w...@intel.com>
>> Cc: Chao Zhang <chao.b.zh...@intel.com>
>> Signed-off-by: Wenyi Xie <xiewen...@huawei.com>
>> ---
>> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf |
>> 1 +
>> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h |
>> 1 +
>> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c |
>> 21 +++++++++++++++-----
>> 3 files changed, 18 insertions(+), 5 deletions(-)
>>
>> diff --git
>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
>> index 1e1a639857e0..a7ac4830b3d4 100644
>> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
>> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
>> @@ -53,6 +53,7 @@ [LibraryClasses]
>> SecurityManagementLib
>> PeCoffLib
>> TpmMeasurementLib
>> + SafeIntLib
>>
>> [Protocols]
>> gEfiFirmwareVolume2ProtocolGuid ## SOMETIMES_CONSUMES
>> diff --git
>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h
>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h
>> index 17955ff9774c..060273917d5d 100644
>> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h
>> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h
>> @@ -23,6 +23,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>> #include <Library/DevicePathLib.h>
>> #include <Library/SecurityManagementLib.h>
>> #include <Library/PeCoffLib.h>
>> +#include <Library/SafeIntLib.h>
>> #include <Protocol/FirmwareVolume2.h>
>> #include <Protocol/DevicePath.h>
>> #include <Protocol/BlockIo.h>
>> diff --git
>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
>> index 36b87e16d53d..2b42d4595f2c 100644
>> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
>> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
>> @@ -1658,6 +1658,9 @@ DxeImageVerificationHandler (
>> EFI_STATUS HashStatus;
>> EFI_STATUS DbStatus;
>> BOOLEAN IsFound;
>> + UINT32 AlignedLength;
>> + UINT32 Result;
>> + EFI_STATUS AddStatus;
>>
>> SignatureList = NULL;
>> SignatureListSize = 0;
>> @@ -1667,6 +1670,7 @@ DxeImageVerificationHandler (
>> Action = EFI_IMAGE_EXECUTION_AUTH_UNTESTED;
>> IsVerified = FALSE;
>> IsFound = FALSE;
>> + Result = 0;
>>
>> //
>> // Check the image type and get policy setting.
>> @@ -1850,9 +1854,9 @@ DxeImageVerificationHandler (
>> // The first certificate starts at offset (SecDataDir->VirtualAddress)
>> from the start of the file.
>> //
>> for (OffSet = SecDataDir->VirtualAddress;
>> - OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size);
>> - OffSet += (WinCertificate->dwLength + ALIGN_SIZE
>> (WinCertificate->dwLength))) {
>> + (OffSet >= SecDataDir->VirtualAddress) && (OffSet <
>> (SecDataDir->VirtualAddress + SecDataDir->Size));) {
>> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
>> + AlignedLength = WinCertificate->dwLength + ALIGN_SIZE
>> (WinCertificate->dwLength);
>> if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <= sizeof
>> (WIN_CERTIFICATE) ||
>> (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <
>> WinCertificate->dwLength) {
>> break;
>> @@ -1881,7 +1885,7 @@ DxeImageVerificationHandler (
>> break;
>> }
>> if (!CompareGuid (&WinCertUefiGuid->CertType, &gEfiCertPkcs7Guid)) {
>> - continue;
>> + goto NEXT_LOOP;
>> }
>> AuthData = WinCertUefiGuid->CertData;
>> AuthDataSize = WinCertUefiGuid->Hdr.dwLength -
>> OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData);
>> @@ -1889,12 +1893,12 @@ DxeImageVerificationHandler (
>> if (WinCertificate->dwLength < sizeof (WIN_CERTIFICATE)) {
>> break;
>> }
>> - continue;
>> + goto NEXT_LOOP;
>> }
>>
>> HashStatus = HashPeImageByType (AuthData, AuthDataSize);
>> if (EFI_ERROR (HashStatus)) {
>> - continue;
>> + goto NEXT_LOOP;
>> }
>>
>> //
>> @@ -1946,6 +1950,13 @@ DxeImageVerificationHandler (
>> DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed but
>> signature is not allowed by DB and %s hash of image is not found in
>> DB/DBX.\n", mHashTypeStr));
>> }
>> }
>> +
>> +NEXT_LOOP:
>> + AddStatus = SafeUint32Add (OffSet, AlignedLength, &Result);
>> + if (EFI_ERROR (AddStatus)) {
>> + break;
>> + }
>> + OffSet = Result;
>> }
>>
>> if (OffSet != (SecDataDir->VirtualAddress + SecDataDir->Size)) {
>>
>
> (1) Please mark the patch URL
> <https://edk2.groups.io/g/devel/message/64059> on the bugzilla ticket,
> in a comment.
I saw you have marked URL on the bugzilla ticket, so I didn't mark it again.
>
> (2) We should not use "goto" statements for such control transfers, only
> for error handling. If necessary, please split the function in two, or
> reorganize the loop otherwise.
Ok, I removed "goto" and add a new flag IsAuthDataAssigned. When AuthData and
AuthDataSize are assigned, the flag is set true.
If either this flag or status of HashPeImageByType is false, then skip
signature verification, only increasing offset and step to next loop.
And thank you for your review.
>
> Thanks
> Laszlo
>
>
> .
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#64167): https://edk2.groups.io/g/devel/message/64167
Mute This Topic: https://groups.io/mt/76143922/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
