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

["Andrew Fish" <afish@apple.com>]

[-- Attachment #1: Type: text/plain, Size: 3039 bytes --]



> On Mar 24, 2021, 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...
>>   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://en.wikipedia.org/wiki/BMP_file_format#cite_note-DIBhelp-5> 

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://docs.microsoft.com/en-us/previous-versions/ms969901(v=msdn.10)?redirectedfrom=MSDN>

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
> 
> 
> 
> 


[-- Attachment #2: Type: text/html, Size: 13856 bytes --]