[EXTERNAL] [edk2-devel] [PATCH 1/1] MdeModulePkg/BmpSupportLib: Allow BMP with extra data

Fri Apr 16 17:12:41 UTC 2021

Sorry I was out for a bit so didn't get back to this thread for a bit. Who should be on any additional security review?

In TranslateBmpToGopBlt the Size is only used in DEBUG prints and this verification check. Processing of the data uses the image structure data. In addition BMP that have extra data between the color map (if present right after bmp header) and the image data is allowed with an explicit comment.

    //
    // BMP file may has padding data between the bmp header section and the
    // bmp data section.
    //
    if (BmpHeader->ImageOffset - sizeof (BMP_IMAGE_HEADER) < sizeof (BMP_COLOR_MAP) * ColorMapNum) {
      return RETURN_UNSUPPORTED;
    }

Thanks,

Jeff

________________________________
From: Bret Barkelew <Bret.Barkelew at microsoft.com>
Sent: Thursday, April 1, 2021 11:37 AM
To: devel at edk2.groups.io <devel at edk2.groups.io>; zhichao.gao at intel.com <zhichao.gao at intel.com>; afish at apple.com <afish at apple.com>; Laszlo Ersek <lersek at redhat.com>
Cc: Jeff Brasen <jbrasen at nvidia.com>; Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A <hao.a.wu at intel.com>; Yao, Jiewen <jiewen.yao at intel.com>; Liming Gao <gaoliming at byosoft.com.cn>; Ni, Ray <ray.ni at intel.com>
Subject: RE: [EXTERNAL] [edk2-devel] [PATCH 1/1] MdeModulePkg/BmpSupportLib: Allow BMP with extra data

External email: Use caution opening links or attachments

I agree with the proposal for a deeper security review.

I also would suggest that we can provide tooling with BaseTools to check and/or correct the format of a BMP to match what the code expects (since there seems to be ambiguity in the spec/implementation). We’ve got a validator in Mu and would be happy to put together some patches to at least get this started for the community to hammer on.

- Bret

From: Gao, Zhichao via groups.io<mailto:zhichao.gao=intel.com at groups.io>
Sent: Monday, March 29, 2021 6:35 PM
To: devel at edk2.groups.io<mailto:devel at edk2.groups.io>; afish at apple.com<mailto:afish at apple.com>; Laszlo Ersek<mailto:lersek at redhat.com>
Cc: Jeff Brasen<mailto:jbrasen at nvidia.com>; Bret Barkelew<mailto:Bret.Barkelew at microsoft.com>; Wang, Jian J<mailto:jian.j.wang at intel.com>; Wu, Hao A<mailto:hao.a.wu at intel.com>; Yao, Jiewen<mailto:jiewen.yao at intel.com>; Liming Gao<mailto:gaoliming at byosoft.com.cn>; Ni, Ray<mailto:ray.ni at intel.com>
Subject: Re: [EXTERNAL] [edk2-devel] [PATCH 1/1] MdeModulePkg/BmpSupportLib: Allow BMP with extra data

The patch would let the BMP file that with a bunch of data pass the check, no matter the data is valid or not. Do we have other docs to descript which data is allowed and valid?

Correct the Cc mail address and invite more experts for security review.

Thanks,

Zhichao

From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Andrew Fish via groups.io
Sent: Thursday, March 25, 2021 11:00 AM
To: edk2-devel-groups-io <devel at edk2.groups.io>; Laszlo Ersek <lersek at redhat.com>
Cc: Jeff Brasen <jbrasen at nvidia.com>; bret.barkelew at microsoft.com; Wang, Jian J <jian.j.wang at intel.com>; ao.a.wu at intel.com
Subject: Re: [EXTERNAL] [edk2-devel] [PATCH 1/1] MdeModulePkg/BmpSupportLib: Allow BMP with extra data

On Mar 24, 2021, at 11:26 AM, Laszlo Ersek <lersek at redhat.com<mailto:lersek at 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...
  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

So, each of these has 2 bytes of padding at the end of the file. We could write a tool that would do 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 doesn't seem correct that UEFI is rejecting it. I can update the commit message with more context if needed as well.

If there's a spec describing the BMP format,

Yes and there 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) :(.

https://en.wikipedia.org/wiki/BMP_file_format#cite_note-DIBhelp-5<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen.wikipedia.org%2Fwiki%2FBMP_file_format%23cite_note-DIBhelp-5&data=04%7C01%7Cjbrasen%40nvidia.com%7C56c4c3c190144f8c01ad08d8f534ce80%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637528954453929169%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=7FTazyRzHFzdnpFv16knDCgMo3iX8RIxkZsHsW6yBRE%3D&reserved=0>

edk2 supports ‘BM’ and the BITMAPINFOHEADER DIB. I seem to remember DIBs are defined by the size. So ‘BM' is a Microsoft Spec:

https://docs.microsoft.com/en-us/previous-versions/ms969901(v=msdn.10)?redirectedfrom=MSDN<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.microsoft.com%2Fen-us%2Fprevious-versions%2Fms969901(v%3Dmsdn.10)%3Fredirectedfrom%3DMSDN&data=04%7C01%7Cjbrasen%40nvidia.com%7C56c4c3c190144f8c01ad08d8f534ce80%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637528954453939160%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=v%2BZ5Ft3AY60tXSO6PwAtx2yqMGMmfKwpwONI6r4rATQ%3D&reserved=0>

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 structure is never actually passed to the API, not every application that generates BMP files fills out the data structure carefully. 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 Windows Software Development Kit (SDK) documentation claims otherwise. To be on the 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 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 risks, then I
think a patch would be fair.

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74213): https://edk2.groups.io/g/devel/message/74213
Mute This Topic: https://groups.io/mt/81556871/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20210416/9467e964/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: F0EC9BD5722B471983D77D317B3A9240.png
Type: image/png
Size: 140 bytes
Desc: F0EC9BD5722B471983D77D317B3A9240.png
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20210416/9467e964/attachment.png>