From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.120; helo=mga04.intel.com; envelope-from=hao.a.wu@intel.com; receiver=edk2-devel@lists.01.org Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) (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 052B8211D6747 for ; Thu, 7 Mar 2019 18:44:13 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 07 Mar 2019 18:44:12 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.58,454,1544515200"; d="scan'208";a="150368282" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga004.fm.intel.com with ESMTP; 07 Mar 2019 18:44:12 -0800 Received: from fmsmsx124.amr.corp.intel.com (10.18.125.39) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 7 Mar 2019 18:44:12 -0800 Received: from shsmsx154.ccr.corp.intel.com (10.239.6.54) by fmsmsx124.amr.corp.intel.com (10.18.125.39) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 7 Mar 2019 18:44:11 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.74]) by SHSMSX154.ccr.corp.intel.com ([169.254.7.223]) with mapi id 14.03.0415.000; Fri, 8 Mar 2019 10:44:09 +0800 From: "Wu, Hao A" To: "Ni, Ray" , "edk2-devel@lists.01.org" CC: "Bi, Dandan" Thread-Topic: [PATCH v2 1/2] MdeModulePkg/HiiDatabase: Fix potential integer overflow (CVE-2018-12181) Thread-Index: AQHU1VcvLmdrBN3txk+lZ8di8s4eQqYBBt/w Date: Fri, 8 Mar 2019 02:44:08 +0000 Message-ID: References: <20190308023514.103228-1-ray.ni@intel.com> <20190308023514.103228-2-ray.ni@intel.com> In-Reply-To: <20190308023514.103228-2-ray.ni@intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [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:44:14 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Hao Wu 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) >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1135 >=20 > 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(-) >=20 > 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. >=20 > #include "HiiDatabase.h" >=20 > +#define MAX_UINT24 0xFFFFFF >=20 > /** > Get the imageid of last image block: EFI_HII_IIBT_END_BLOCK when input > @@ -651,8 +652,16 @@ HiiNewImage ( >=20 > EfiAcquireLock (&mHiiDatabaseLock); >=20 > - NewBlockSize =3D 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 =3D (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 =3D NewBlockSize * 3 + (sizeof > (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - sizeof (EFI_HII_RGB_PIXEL)); >=20 > // > // 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 th= e > 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) <=3D MAX_UINT24 > + // > ImageBlocks =3D AllocatePool (ImagePackage->ImageBlockSize + > NewBlockSize); > if (ImageBlocks =3D=3D NULL) { > EfiReleaseLock (&mHiiDatabaseLock); > @@ -701,6 +722,13 @@ HiiNewImage ( > PackageListNode->PackageListHdr.PackageLength +=3D NewBlockSize; >=20 > } 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 structure= s is > the same. > // > CopyMem (&Iibt1bit, CurrentImageBlock, sizeof > (EFI_HII_IIBT_IMAGE_1BIT_BLOCK)); > - ImageLength =3D sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL) * > - ((UINT32) Iibt1bit.Bitmap.Width * Iibt1bit.Bitmap.Heig= ht); > + ImageLength =3D (UINTN) Iibt1bit.Bitmap.Width * Iibt1bit.Bitmap.Heig= ht; > + if (ImageLength > MAX_UINTN / sizeof > (EFI_GRAPHICS_OUTPUT_BLT_PIXEL)) { > + return EFI_OUT_OF_RESOURCES; > + } > + ImageLength *=3D sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL); > Image->Bitmap =3D AllocateZeroPool (ImageLength); > if (Image->Bitmap =3D=3D NULL) { > return EFI_OUT_OF_RESOURCES; > @@ -952,9 +983,13 @@ IGetImage ( > // fall through > // > case EFI_HII_IIBT_IMAGE_24BIT: > - Width =3D ReadUnaligned16 ((VOID *) > &((EFI_HII_IIBT_IMAGE_24BIT_BLOCK *) CurrentImageBlock)- > >Bitmap.Width); > + Width =3D ReadUnaligned16 ((VOID *) > &((EFI_HII_IIBT_IMAGE_24BIT_BLOCK *) CurrentImageBlock)- > >Bitmap.Width); > Height =3D ReadUnaligned16 ((VOID *) > &((EFI_HII_IIBT_IMAGE_24BIT_BLOCK *) CurrentImageBlock)- > >Bitmap.Height); > - ImageLength =3D sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL) * ((UINT32) > Width * Height); > + ImageLength =3D (UINTN)Width * Height; > + if (ImageLength > MAX_UINTN / sizeof > (EFI_GRAPHICS_OUTPUT_BLT_PIXEL)) { > + return EFI_OUT_OF_RESOURCES; > + } > + ImageLength *=3D sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL); > Image->Bitmap =3D AllocateZeroPool (ImageLength); > if (Image->Bitmap =3D=3D NULL) { > return EFI_OUT_OF_RESOURCES; > @@ -1124,8 +1159,23 @@ HiiSetImage ( > // > // Create the new image block according to input image. > // > - NewBlockSize =3D 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 =3D (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 =3D 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 !=3D NULL) { > + // > + // Make sure the BltX and BltY is inside the Blt area. > + // > + if ((BltX >=3D (*Blt)->Width) || (BltY >=3D (*Blt)->Height)) { > + return EFI_INVALID_PARAMETER; > + } > + > // > // Clip the image by (Width, Height) > // > @@ -1287,15 +1344,23 @@ HiiDrawImage ( > Width =3D Image->Width; > Height =3D Image->Height; >=20 > - if (Width > (*Blt)->Width - BltX) { > - Width =3D (*Blt)->Width - BltX; > + if (Width > (*Blt)->Width - (UINT16)BltX) { > + Width =3D (*Blt)->Width - (UINT16)BltX; > } > - if (Height > (*Blt)->Height - BltY) { > - Height =3D (*Blt)->Height - BltY; > + if (Height > (*Blt)->Height - (UINT16)BltY) { > + Height =3D (*Blt)->Height - (UINT16)BltY; > } >=20 > - BufferLen =3D Width * Height * sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL= ); > - BltBuffer =3D (EFI_GRAPHICS_OUTPUT_BLT_PIXEL *) AllocateZeroPool > (BufferLen); > + // > + // Prepare the buffer for the temporary image. > + // Make sure the buffer size doesn't overflow UINTN. > + // > + BufferLen =3D Width * Height; > + if (BufferLen > MAX_UINTN / sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL)) > { > + return EFI_OUT_OF_RESOURCES; > + } > + BufferLen *=3D sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL); > + BltBuffer =3D AllocateZeroPool (BufferLen); > if (BltBuffer =3D=3D NULL) { > return EFI_OUT_OF_RESOURCES; > } > @@ -1358,11 +1423,26 @@ HiiDrawImage ( > // > // Allocate a new bitmap to hold the incoming image. > // > - Width =3D Image->Width + BltX; > - Height =3D Image->Height + BltY; >=20 > - BufferLen =3D Width * Height * sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL= ); > - BltBuffer =3D (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 =3D Image->Width + (UINT16)BltX; > + Height =3D Image->Height + (UINT16)BltY; > + > + // > + // Make sure the output image size doesn't overflow UINTN. > + // > + BufferLen =3D Width * Height; > + if (BufferLen > MAX_UINTN / sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL)) > { > + return EFI_OUT_OF_RESOURCES; > + } > + BufferLen *=3D sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL); > + BltBuffer =3D AllocateZeroPool (BufferLen); > if (BltBuffer =3D=3D NULL) { > return EFI_OUT_OF_RESOURCES; > } > @@ -1372,8 +1452,8 @@ HiiDrawImage ( > FreePool (BltBuffer); > return EFI_OUT_OF_RESOURCES; > } > - ImageOut->Width =3D (UINT16) Width; > - ImageOut->Height =3D (UINT16) Height; > + ImageOut->Width =3D Width; > + ImageOut->Height =3D Height; > ImageOut->Image.Bitmap =3D BltBuffer; >=20 > // > @@ -1387,7 +1467,7 @@ HiiDrawImage ( > return Status; > } > ASSERT (FontInfo !=3D NULL); > - for (Index =3D 0; Index < Width * Height; Index++) { > + for (Index =3D 0; Index < (UINTN)Width * Height; Index++) { > BltBuffer[Index] =3D FontInfo->BackgroundColor; > } > FreePool (FontInfo); > -- > 2.20.1.windows.1