Re: [edk2-devel] [PATCH EDK2 v1 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset(CVE-2019-14562)

["wenyi,xie" <xiewenyi2@huawei.com>]

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.yao@intel.com>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Chao Zhang <chao.b.zhang@intel.com>
>> Signed-off-by: Wenyi Xie <xiewenyi2@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
> 
> 
> .
>