Re: [PATCH v2 1/2] MdeModulePkg/HiiDatabase: Fix potential integer overflow (CVE-2018-12181)

["Wu, Hao A" <hao.a.wu@intel.com>]

Reviewed-by: Hao Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> -----Original Message-----
> From: Ni, Ray
> Sent: Friday, March 08, 2019 10:35 AM
> To: edk2-devel@lists.01.org
> Cc: Bi, Dandan; Wu, Hao A
> Subject: [PATCH v2 1/2] MdeModulePkg/HiiDatabase: Fix potential integer
> overflow (CVE-2018-12181)
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1135
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> ---
>  MdeModulePkg/Universal/HiiDatabaseDxe/Image.c | 126
> ++++++++++++++----
>  1 file changed, 103 insertions(+), 23 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
> b/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
> index 71ebc559c0..80a4ec1114 100644
> --- a/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
> +++ b/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
> @@ -16,6 +16,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
> 
>  #include "HiiDatabase.h"
> 
> +#define MAX_UINT24    0xFFFFFF
> 
>  /**
>    Get the imageid of last image block: EFI_HII_IIBT_END_BLOCK when input
> @@ -651,8 +652,16 @@ HiiNewImage (
> 
>    EfiAcquireLock (&mHiiDatabaseLock);
> 
> -  NewBlockSize = sizeof (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - sizeof
> (EFI_HII_RGB_PIXEL) +
> -                 BITMAP_LEN_24_BIT ((UINT32) Image->Width, Image->Height);
> +  //
> +  // Calcuate the size of new image.
> +  // Make sure the size doesn't overflow UINT32.
> +  // Note: 24Bit BMP occpuies 3 bytes per pixel.
> +  //
> +  NewBlockSize = (UINT32)Image->Width * Image->Height;
> +  if (NewBlockSize > (MAX_UINT32 - (sizeof
> (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - sizeof (EFI_HII_RGB_PIXEL))) / 3) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +  NewBlockSize = NewBlockSize * 3 + (sizeof
> (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - sizeof (EFI_HII_RGB_PIXEL));
> 
>    //
>    // Get the image package in the package list,
> @@ -671,6 +680,18 @@ HiiNewImage (
>      //
>      // Update the package's image block by appending the new block to the
> end.
>      //
> +
> +    //
> +    // Make sure the final package length doesn't overflow.
> +    // Length of the package header is represented using 24 bits. So MAX
> length is MAX_UINT24.
> +    //
> +    if (NewBlockSize > MAX_UINT24 - ImagePackage-
> >ImagePkgHdr.Header.Length) {
> +      return EFI_OUT_OF_RESOURCES;
> +    }
> +    //
> +    // Because ImagePackage->ImageBlockSize < ImagePackage-
> >ImagePkgHdr.Header.Length,
> +    // So (ImagePackage->ImageBlockSize + NewBlockSize) <= MAX_UINT24
> +    //
>      ImageBlocks = AllocatePool (ImagePackage->ImageBlockSize +
> NewBlockSize);
>      if (ImageBlocks == NULL) {
>        EfiReleaseLock (&mHiiDatabaseLock);
> @@ -701,6 +722,13 @@ HiiNewImage (
>      PackageListNode->PackageListHdr.PackageLength += NewBlockSize;
> 
>    } else {
> +    //
> +    // Make sure the final package length doesn't overflow.
> +    // Length of the package header is represented using 24 bits. So MAX
> length is MAX_UINT24.
> +    //
> +    if (NewBlockSize > MAX_UINT24 - (sizeof
> (EFI_HII_IMAGE_PACKAGE_HDR) + sizeof (EFI_HII_IIBT_END_BLOCK))) {
> +      return EFI_OUT_OF_RESOURCES;
> +    }
>      //
>      // The specified package list does not contain image package.
>      // Create one to add this image block.
> @@ -902,8 +930,11 @@ IGetImage (
>      // Use the common block code since the definition of these structures is
> the same.
>      //
>      CopyMem (&Iibt1bit, CurrentImageBlock, sizeof
> (EFI_HII_IIBT_IMAGE_1BIT_BLOCK));
> -    ImageLength = sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL) *
> -                  ((UINT32) Iibt1bit.Bitmap.Width * Iibt1bit.Bitmap.Height);
> +    ImageLength = (UINTN) Iibt1bit.Bitmap.Width * Iibt1bit.Bitmap.Height;
> +    if (ImageLength > MAX_UINTN / sizeof
> (EFI_GRAPHICS_OUTPUT_BLT_PIXEL)) {
> +      return EFI_OUT_OF_RESOURCES;
> +    }
> +    ImageLength  *= sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL);
>      Image->Bitmap = AllocateZeroPool (ImageLength);
>      if (Image->Bitmap == NULL) {
>        return EFI_OUT_OF_RESOURCES;
> @@ -952,9 +983,13 @@ IGetImage (
>      // fall through
>      //
>    case EFI_HII_IIBT_IMAGE_24BIT:
> -    Width = ReadUnaligned16 ((VOID *)
> &((EFI_HII_IIBT_IMAGE_24BIT_BLOCK *) CurrentImageBlock)-
> >Bitmap.Width);
> +    Width  = ReadUnaligned16 ((VOID *)
> &((EFI_HII_IIBT_IMAGE_24BIT_BLOCK *) CurrentImageBlock)-
> >Bitmap.Width);
>      Height = ReadUnaligned16 ((VOID *)
> &((EFI_HII_IIBT_IMAGE_24BIT_BLOCK *) CurrentImageBlock)-
> >Bitmap.Height);
> -    ImageLength = sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL) * ((UINT32)
> Width * Height);
> +    ImageLength = (UINTN)Width * Height;
> +    if (ImageLength > MAX_UINTN / sizeof
> (EFI_GRAPHICS_OUTPUT_BLT_PIXEL)) {
> +      return EFI_OUT_OF_RESOURCES;
> +    }
> +    ImageLength  *= sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL);
>      Image->Bitmap = AllocateZeroPool (ImageLength);
>      if (Image->Bitmap == NULL) {
>        return EFI_OUT_OF_RESOURCES;
> @@ -1124,8 +1159,23 @@ HiiSetImage (
>    //
>    // Create the new image block according to input image.
>    //
> -  NewBlockSize = sizeof (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - sizeof
> (EFI_HII_RGB_PIXEL) +
> -                 BITMAP_LEN_24_BIT ((UINT32) Image->Width, Image->Height);
> +
> +  //
> +  // Make sure the final package length doesn't overflow.
> +  // Length of the package header is represented using 24 bits. So MAX
> length is MAX_UINT24.
> +  // 24Bit BMP occpuies 3 bytes per pixel.
> +  //
> +  NewBlockSize = (UINT32)Image->Width * Image->Height;
> +  if (NewBlockSize > (MAX_UINT32 - (sizeof
> (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - sizeof (EFI_HII_RGB_PIXEL))) / 3) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +  NewBlockSize = NewBlockSize * 3 + (sizeof
> (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - sizeof (EFI_HII_RGB_PIXEL));
> +  if ((NewBlockSize > OldBlockSize) &&
> +      (NewBlockSize - OldBlockSize > MAX_UINT24 - ImagePackage-
> >ImagePkgHdr.Header.Length)
> +      ) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
>    //
>    // Adjust the image package to remove the original block firstly then add
> the new block.
>    //
> @@ -1219,8 +1269,8 @@ HiiDrawImage (
>    EFI_IMAGE_OUTPUT                    *ImageOut;
>    EFI_GRAPHICS_OUTPUT_BLT_PIXEL       *BltBuffer;
>    UINTN                               BufferLen;
> -  UINTN                               Width;
> -  UINTN                               Height;
> +  UINT16                              Width;
> +  UINT16                              Height;
>    UINTN                               Xpos;
>    UINTN                               Ypos;
>    UINTN                               OffsetY1;
> @@ -1280,6 +1330,13 @@ HiiDrawImage (
>    // Otherwise a new bitmap will be allocated to hold this image.
>    //
>    if (*Blt != NULL) {
> +    //
> +    // Make sure the BltX and BltY is inside the Blt area.
> +    //
> +    if ((BltX >= (*Blt)->Width) || (BltY >= (*Blt)->Height)) {
> +      return EFI_INVALID_PARAMETER;
> +    }
> +
>      //
>      // Clip the image by (Width, Height)
>      //
> @@ -1287,15 +1344,23 @@ HiiDrawImage (
>      Width  = Image->Width;
>      Height = Image->Height;
> 
> -    if (Width > (*Blt)->Width - BltX) {
> -      Width = (*Blt)->Width - BltX;
> +    if (Width > (*Blt)->Width - (UINT16)BltX) {
> +      Width = (*Blt)->Width - (UINT16)BltX;
>      }
> -    if (Height > (*Blt)->Height - BltY) {
> -      Height = (*Blt)->Height - BltY;
> +    if (Height > (*Blt)->Height - (UINT16)BltY) {
> +      Height = (*Blt)->Height - (UINT16)BltY;
>      }
> 
> -    BufferLen = Width * Height * sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL);
> -    BltBuffer = (EFI_GRAPHICS_OUTPUT_BLT_PIXEL *) AllocateZeroPool
> (BufferLen);
> +    //
> +    // Prepare the buffer for the temporary image.
> +    // Make sure the buffer size doesn't overflow UINTN.
> +    //
> +    BufferLen = Width * Height;
> +    if (BufferLen > MAX_UINTN / sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL))
> {
> +      return EFI_OUT_OF_RESOURCES;
> +    }
> +    BufferLen *= sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL);
> +    BltBuffer  = AllocateZeroPool (BufferLen);
>      if (BltBuffer == NULL) {
>        return EFI_OUT_OF_RESOURCES;
>      }
> @@ -1358,11 +1423,26 @@ HiiDrawImage (
>      //
>      // Allocate a new bitmap to hold the incoming image.
>      //
> -    Width  = Image->Width  + BltX;
> -    Height = Image->Height + BltY;
> 
> -    BufferLen = Width * Height * sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL);
> -    BltBuffer = (EFI_GRAPHICS_OUTPUT_BLT_PIXEL *) AllocateZeroPool
> (BufferLen);
> +    //
> +    // Make sure the final width and height doesn't overflow UINT16.
> +    //
> +    if ((BltX > (UINTN)MAX_UINT16 - Image->Width) || (BltY >
> (UINTN)MAX_UINT16 - Image->Height)) {
> +      return EFI_INVALID_PARAMETER;
> +    }
> +
> +    Width  = Image->Width  + (UINT16)BltX;
> +    Height = Image->Height + (UINT16)BltY;
> +
> +    //
> +    // Make sure the output image size doesn't overflow UINTN.
> +    //
> +    BufferLen = Width * Height;
> +    if (BufferLen > MAX_UINTN / sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL))
> {
> +      return EFI_OUT_OF_RESOURCES;
> +    }
> +    BufferLen *= sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL);
> +    BltBuffer  = AllocateZeroPool (BufferLen);
>      if (BltBuffer == NULL) {
>        return EFI_OUT_OF_RESOURCES;
>      }
> @@ -1372,8 +1452,8 @@ HiiDrawImage (
>        FreePool (BltBuffer);
>        return EFI_OUT_OF_RESOURCES;
>      }
> -    ImageOut->Width        = (UINT16) Width;
> -    ImageOut->Height       = (UINT16) Height;
> +    ImageOut->Width        = Width;
> +    ImageOut->Height       = Height;
>      ImageOut->Image.Bitmap = BltBuffer;
> 
>      //
> @@ -1387,7 +1467,7 @@ HiiDrawImage (
>        return Status;
>      }
>      ASSERT (FontInfo != NULL);
> -    for (Index = 0; Index < Width * Height; Index++) {
> +    for (Index = 0; Index < (UINTN)Width * Height; Index++) {
>        BltBuffer[Index] = FontInfo->BackgroundColor;
>      }
>      FreePool (FontInfo);
> --
> 2.20.1.windows.1