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

["Laszlo Ersek" <lersek@redhat.com>]

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.

(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.

Thanks
Laszlo