File ovmf-bsc1128503-fix-stack-overflow-in-HiiImage-... of Package ovmf

Overview Repositories Revisions Requests Users Attributes Meta

File ovmf-bsc1128503-fix-stack-overflow-in-HiiImage-and-HiiDatabase.patch of Package ovmf (Revision cccc10741e6011ba7d49c13ab8b2c77d)

Currently displaying revision cccc10741e6011ba7d49c13ab8b2c77d , Show latest

From d3743fad8d9e3d3877849fa4d099a7d20a0ca256 Mon Sep 17 00:00:00 2001
From: Ray Ni <ray.ni@intel.com>
Date: Thu, 7 Mar 2019 18:35:13 +0800
Subject: [PATCH 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>
Reviewed-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
(cherry picked from commit ffe5f7a6b4e978dffbe1df228963adc914451106)
---
 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 431a5b84543b..dc9566b736cb 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
@@ -649,8 +650,16 @@ HiiNewImage (
     return EFI_NOT_FOUND;
   }
 
-  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,
@@ -669,6 +678,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) {
       return EFI_OUT_OF_RESOURCES;
@@ -698,6 +719,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.
@@ -895,8 +923,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;
@@ -945,9 +976,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;
@@ -1114,8 +1149,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.
   //
@@ -1207,8 +1257,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;
@@ -1268,6 +1318,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)
     //
@@ -1275,15 +1332,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;
     }
@@ -1346,11 +1411,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;
     }
@@ -1360,8 +1440,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;
 
     //
@@ -1375,7 +1455,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

From 00fe2260ceb106e2bfe69312c9a17b7ab1658fe5 Mon Sep 17 00:00:00 2001
From: Ray Ni <ray.ni@intel.com>
Date: Thu, 7 Mar 2019 18:35:14 +0800
Subject: [PATCH 2/2] MdeModulePkg/HiiImage: Fix stack overflow when corrupted
 BMP is parsed (CVE-2018-12181)

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1135

For 4bit BMP, there are only 2^4 = 16 colors in the palette.
But when a corrupted BMP contains more than 16 colors in the palette,
today's implementation wrongly copies all colors to the local
PaletteValue[16] array which causes stack overflow.

The similar issue also exists in the logic to handle 8bit BMP.

The patch fixes the issue by only copies the first 16 or 256 colors
in the palette depending on the BMP type.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
(cherry picked from commit 89910a39dcfd788057caa5d88b7e76e112d187b5)
---
 MdeModulePkg/Universal/HiiDatabaseDxe/Image.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c b/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
index dc9566b736cb..9829bddaa804 100644
--- a/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
+++ b/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
@@ -370,7 +370,7 @@ Output4bitPixel (
   PaletteNum = (UINT16)(Palette->PaletteSize / sizeof (EFI_HII_RGB_PIXEL));
 
   ZeroMem (PaletteValue, sizeof (PaletteValue));
-  CopyRgbToGopPixel (PaletteValue, Palette->PaletteValue, PaletteNum);
+  CopyRgbToGopPixel (PaletteValue, Palette->PaletteValue, MIN (PaletteNum, ARRAY_SIZE (PaletteValue)));
   FreePool (Palette);
 
   //
@@ -447,7 +447,7 @@ Output8bitPixel (
   CopyMem (Palette, PaletteInfo, PaletteSize);
   PaletteNum = (UINT16)(Palette->PaletteSize / sizeof (EFI_HII_RGB_PIXEL));
   ZeroMem (PaletteValue, sizeof (PaletteValue));
-  CopyRgbToGopPixel (PaletteValue, Palette->PaletteValue, PaletteNum);
+  CopyRgbToGopPixel (PaletteValue, Palette->PaletteValue, MIN (PaletteNum, ARRAY_SIZE (PaletteValue)));
   FreePool (Palette);
 
   //
-- 
2.20.1

Places

File ovmf-bsc1128503-fix-stack-overflow-in-HiiImage-and-HiiDatabase.patch of Package ovmf (Revision cccc10741e6011ba7d49c13ab8b2c77d)

Places