From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.24; helo=mga09.intel.com; envelope-from=ray.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 6790E211D672F for ; Thu, 7 Mar 2019 18:32:27 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 07 Mar 2019 18:32:27 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.58,454,1544515200"; d="scan'208";a="140138303" Received: from ray-dev.ccr.corp.intel.com ([10.239.9.36]) by orsmga002.jf.intel.com with ESMTP; 07 Mar 2019 18:32:26 -0800 From: Ray Ni To: edk2-devel@lists.01.org Cc: Dandan Bi , Hao A Wu Date: Fri, 8 Mar 2019 10:35:13 +0800 Message-Id: <20190308023514.103228-2-ray.ni@intel.com> X-Mailer: git-send-email 2.20.1.windows.1 In-Reply-To: <20190308023514.103228-1-ray.ni@intel.com> References: <20190308023514.103228-1-ray.ni@intel.com> MIME-Version: 1.0 Subject: [PATCH v2 1/2] MdeModulePkg/HiiDatabase: Fix potential integer overflow (CVE-2018-12181) X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 08 Mar 2019 02:32:27 -0000 Content-Transfer-Encoding: 8bit REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1135 Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ray Ni Cc: Dandan Bi Cc: Hao A Wu --- 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