From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ma1-aaemail-dr-lapp01.apple.com (ma1-aaemail-dr-lapp01.apple.com [17.171.2.60]) by mx.groups.io with SMTP id smtpd.web09.3087.1616641216409459945 for ; Wed, 24 Mar 2021 20:00:16 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=KfTPQvV+; spf=pass (domain: apple.com, ip: 17.171.2.60, mailfrom: afish@apple.com) Received: from pps.filterd (ma1-aaemail-dr-lapp01.apple.com [127.0.0.1]) by ma1-aaemail-dr-lapp01.apple.com (8.16.0.42/8.16.0.42) with SMTP id 12P2rHhT044069; Wed, 24 Mar 2021 20:00:15 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=from : message-id : content-type : mime-version : subject : date : in-reply-to : cc : to : references; s=20180706; bh=2+dhZQgOx2jdHVMgryrLReNUKcnci1E//0sCNH8jvy4=; b=KfTPQvV+snURPI7SioLJN7PZUMrx76HVqtCJxylfvBL4g0PFPDg7Zs6lnCOKa6sAvYXJ DqTfZbauchxztUmkXyzxGuFpgqUXyFilrlAKM84+or3X2JvopO2wJimDVkgsOrE5mr0R Fd1p/5a84Op1EUlYva2jZS9xuD3gZ6E08zk8bfGNh/Pu6UfdPoNcPAggrvZKEeVVcRAz 2XmP+sBx54TseN2ZQC0A8mR1EAozokvs6oNmwD/wZp7uyjjTo4b8swrikxVYTVraA5WQ 4S4yeuwDzMBTpUBi040Bz7nxfUkWs8Vpudsk73FRrB/z/A10KNisz1lwrzeuBtI2tirF UQ== Received: from rn-mailsvcp-mta-lapp04.rno.apple.com (rn-mailsvcp-mta-lapp04.rno.apple.com [10.225.203.152]) by ma1-aaemail-dr-lapp01.apple.com with ESMTP id 37dfq32q62-2 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Wed, 24 Mar 2021 20:00:15 -0700 Received: from rn-mailsvcp-mmp-lapp03.rno.apple.com (rn-mailsvcp-mmp-lapp03.rno.apple.com [17.179.253.16]) by rn-mailsvcp-mta-lapp04.rno.apple.com (Oracle Communications Messaging Server 8.1.0.7.20201203 64bit (built Dec 3 2020)) with ESMTPS id <0QQI007JX8CF8930@rn-mailsvcp-mta-lapp04.rno.apple.com>; Wed, 24 Mar 2021 20:00:15 -0700 (PDT) Received: from process_milters-daemon.rn-mailsvcp-mmp-lapp03.rno.apple.com by rn-mailsvcp-mmp-lapp03.rno.apple.com (Oracle Communications Messaging Server 8.1.0.7.20201203 64bit (built Dec 3 2020)) id <0QQI00E007OATM00@rn-mailsvcp-mmp-lapp03.rno.apple.com>; Wed, 24 Mar 2021 20:00:15 -0700 (PDT) X-Va-A: X-Va-T-CD: fe60454d8cca2d204232efa0cd93203e X-Va-E-CD: 0fa34b0f1d4d4308fe82d341ec8cfe7e X-Va-R-CD: a05e86d08b2b22a8cb2cf7b3628ebee2 X-Va-CD: 0 X-Va-ID: c0be8ff3-adcc-4602-9173-ac3a64ffa6f4 X-V-A: X-V-T-CD: fe60454d8cca2d204232efa0cd93203e X-V-E-CD: 0fa34b0f1d4d4308fe82d341ec8cfe7e X-V-R-CD: a05e86d08b2b22a8cb2cf7b3628ebee2 X-V-CD: 0 X-V-ID: 4222cdc7-a382-49f5-97fc-8f2e3556b3ab X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.369,18.0.761 definitions=2021-03-24_14:2021-03-24,2021-03-24 signatures=0 Received: from [17.235.46.127] (unknown [17.235.46.127]) by rn-mailsvcp-mmp-lapp03.rno.apple.com (Oracle Communications Messaging Server 8.1.0.7.20201203 64bit (built Dec 3 2020)) with ESMTPSA id <0QQI00QFX8CAM400@rn-mailsvcp-mmp-lapp03.rno.apple.com>; Wed, 24 Mar 2021 20:00:11 -0700 (PDT) From: "Andrew Fish" Message-id: <4834AF3E-A929-4911-AB8E-0665AB2033FB@apple.com> MIME-version: 1.0 (Mac OS X Mail 14.0 \(3654.20.0.2.1\)) Subject: Re: [EXTERNAL] [edk2-devel] [PATCH 1/1] MdeModulePkg/BmpSupportLib: Allow BMP with extra data Date: Wed, 24 Mar 2021 20:00:10 -0700 In-reply-to: <7d5c8dd7-680c-98e6-b8a0-084704ca3021@redhat.com> Cc: Jeff Brasen , "bret.barkelew@microsoft.com" , "jian.j.wang@intel.com" , "ao.a.wu@intel.com" To: edk2-devel-groups-io , Laszlo Ersek References: <70c26f78d461d1b8021462d3c3fe6eb717b19193.1616520420.git.jbrasen@nvidia.com> <7d5c8dd7-680c-98e6-b8a0-084704ca3021@redhat.com> X-Mailer: Apple Mail (2.3654.20.0.2.1) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.369,18.0.761 definitions=2021-03-24_14:2021-03-24,2021-03-24 signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_0783D2AC-0D11-48A7-A79A-9308219494FC" --Apple-Mail=_0783D2AC-0D11-48A7-A79A-9308219494FC Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On Mar 24, 2021, at 11:26 AM, Laszlo Ersek wrote: >=20 > On 03/24/21 16:25, Jeff Brasen wrote: >> Some of the logo files we received for the group that makes our assets = like this (not sure what tool they were created with) look like they pad th= e BMP size to 8 bytes. >>=20 >> TranslateBmpToGopBlt: invalid BmpImage... >> BmpHeader->Size: 0xE1038 >> BmpHeader->ImageOffset: 0x36 >> BmpImageSize: 0xE1038 >> DataSize: 0xE1000 >> TranslateBmpToGopBlt: invalid BmpImage... >> BmpHeader->Size: 0x2A3038 >> BmpHeader->ImageOffset: 0x36 >> BmpImageSize: 0x2A3038 >> DataSize: 0x2A3000 >> TranslateBmpToGopBlt: invalid BmpImage... >> BmpHeader->Size: 0x5EEC38 >> BmpHeader->ImageOffset: 0x36 >> BmpImageSize: 0x5EEC38 >> DataSize: 0x5EEC00 >>=20 >> So, each of these has 2 bytes of padding at the end of the file. We cou= ld write a tool that would do the same size recalculation in order to updat= e the size in the header and remove the two bytes but it seems that this is= a valid BMP file and it doesn't seem correct that UEFI is rejecting it. I = can update the commit message with more context if needed as well. >=20 > If there's a spec describing the BMP format, Yes and there are various flavors as at some point I had some graphics giv= en to me in a format that did not work (I think it was BITMAPV4HEADER) :(.= =20 https://en.wikipedia.org/wiki/BMP_file_format#cite_note-DIBhelp-5 =20 edk2 supports =E2=80=98BM=E2=80=99 and the BITMAPINFOHEADER DIB. I seem to= remember DIBs are defined by the size. So =E2=80=98BM' is a Microsoft Spec= : https://docs.microsoft.com/en-us/previous-versions/ms969901(v=3Dmsdn.10)?r= edirectedfrom=3DMSDN The quote in that spec is: The file extension of a Windows DIB file is BMP. The file consists of a BI= TMAPFILEHEADER structure followed by the DIB itself. Unfortunately, because= the BITMAPFILEHEADER structure is never actually passed to the API, not ev= ery application that generates BMP files fills out the data structure caref= ully. To add to this confusion, the "proper" definition of the structure is= at odds with the documentation. Properly, the data structure contains the = following fields: The explanation of size field is: A DWORD that specifies the size of the file in bytes. The Microsoft Window= s Software Development Kit (SDK) documentation claims otherwise. To be on t= he safe side, many applications calculate their own sizes for reading in a = file. I would say that is not exactly a ringing endorsement from a spec point of= view on depending on that field. So it seems like that patch may be reason= able, but we should triple check it does not break any security related ass= umptions.=20 Thanks, Andrew Fish > and edk2 is needlessly > strict, and the check can be relaxed without security risks, then I > think a patch would be fair. >=20 > Thanks > Laszlo >=20 >=20 >=20 >=20 --Apple-Mail=_0783D2AC-0D11-48A7-A79A-9308219494FC Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

On Mar 24, 2= 021, at 11:26 AM, Laszlo Ersek <lersek@redhat.com> wrote:

On = 03/24/21 16:25, Jeff Brasen wrote:
Some of the logo files we received for the group= that makes our assets like this (not sure what tool they were created with= ) look like they pad the BMP size to 8 bytes.

= TranslateBmpToGopBlt: invalid BmpImage...
  BmpHead= er->Size: 0xE1038
  BmpHeader->ImageOffset: 0= x36
  BmpImageSize: 0xE1038
 &nb= sp;DataSize: 0xE1000
TranslateBmpToGopBlt: invalid BmpImage..= .
  BmpHeader->Size: 0x2A3038
&nbs= p; BmpHeader->ImageOffset: 0x36
  BmpImageS= ize: 0x2A3038
  DataSize: 0x2A3000
Tr= anslateBmpToGopBlt: invalid BmpImage...
  BmpHeader= ->Size: 0x5EEC38
  BmpHeader->ImageOffset: 0x= 36
  BmpImageSize: 0x5EEC38
 &nb= sp;DataSize: 0x5EEC00

So, each of these has 2 = bytes of padding at the end of the file. We could write a tool that would d= o the same size recalculation in order to update the size in the header and= remove the two bytes but it seems that this is a valid BMP file and it doe= sn't seem correct that UEFI is rejecting it. I can update the commit messag= e with more context if needed as well.

If there's a spec describing the BMP = format,

Yes and the= re are various flavors as at some point I had some graphics given to me in = a format that did not work (I think it was BITMAPV4HEADER) :(. 
<= div>

edk2 supports =E2=80=98BM=E2=80=99 and the BITMAPINFOHEADER DIB.= I seem to remember DIBs are defined by the size. So =E2=80=98BM' is a Micr= osoft Spec:

The quote in that spec is= :

The file extension of a = Windows DIB file is BMP. The file consists of a BITMAPFILEHEADER structure = followed by the DIB itself. Unfortunately, because the BITMAPFILEHEADER str= ucture is never actually passed to the API, not every application that gene= rates BMP files fills out the data structure carefully. To add to this conf= usion, the "proper" definition of the structure is at odds with the documen= tation. Properly, the data structure contains the following fields:<= /div>

The explanation of size field is:
A DWORD that specifies the size of the file in bytes. The Microsof= t Windows Software Development Kit (SDK) documentation claims otherwise. To= be on the safe side, many applications calculate their own sizes for readi= ng in a file.

I would say that i= s not exactly a ringing endorsement from a spec point of view on depending = on that field. So it seems like that patch may be reasonable, but we should= triple check it does not break any security related assumptions. 

Thanks,

Andrew Fish

and edk2 is needlessly
strict, and the check can be relaxed without security r= isks, then I
think a pa= tch would be fair.

Thanks
Laszlo
<= br style=3D"caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 1= 2px; font-style: normal; font-variant-caps: normal; font-weight: normal; le= tter-spacing: normal; text-align: start; text-indent: 0px; text-transform: = none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0p= x; text-decoration: none;" class=3D"">

<= /div>
--Apple-Mail=_0783D2AC-0D11-48A7-A79A-9308219494FC--