* [PATCH 1/1] MdeModulePkg/BmpSupportLib: Allow BMP with extra data @ 2021-03-23 17:29 Jeff Brasen 2021-03-23 17:41 ` [EXTERNAL] [edk2-devel] " Bret Barkelew 0 siblings, 1 reply; 9+ messages in thread From: Jeff Brasen @ 2021-03-23 17:29 UTC (permalink / raw) To: devel; +Cc: jian.j.wang, ao.a.wu, Jeff Brasen Add support for processing BMP data that contains extra data after the image array, this data will not be parsed in anyway in the library but images that contain this will not be rejected from processing. --- MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c b/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c index 3ac31f6723d0..944d01fe7cdf 100644 --- a/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c +++ b/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c @@ -213,7 +213,7 @@ TranslateBmpToGopBlt ( if ((BmpHeader->Size != BmpImageSize) || (BmpHeader->Size < BmpHeader->ImageOffset) || - (BmpHeader->Size - BmpHeader->ImageOffset != DataSize)) { + (BmpHeader->Size - BmpHeader->ImageOffset < DataSize)) { DEBUG ((DEBUG_ERROR, "TranslateBmpToGopBlt: invalid BmpImage... \n")); DEBUG ((DEBUG_ERROR, " BmpHeader->Size: 0x%x\n", BmpHeader->Size)); -- 2.25.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [EXTERNAL] [edk2-devel] [PATCH 1/1] MdeModulePkg/BmpSupportLib: Allow BMP with extra data 2021-03-23 17:29 [PATCH 1/1] MdeModulePkg/BmpSupportLib: Allow BMP with extra data Jeff Brasen @ 2021-03-23 17:41 ` Bret Barkelew 2021-03-24 11:31 ` Laszlo Ersek 0 siblings, 1 reply; 9+ messages in thread From: Bret Barkelew @ 2021-03-23 17:41 UTC (permalink / raw) To: devel@edk2.groups.io, jbrasen@nvidia.com Cc: jian.j.wang@intel.com, ao.a.wu@intel.com, Jeff Brasen [-- Attachment #1: Type: text/plain, Size: 1681 bytes --] Is this a *good* idea? What is considered valid extra data? If it’s immaterial to the FW displaying the image, our policy has been to strip it off BEFORE adding it to the FW image. - Bret From: Jeff Brasen via groups.io<mailto:jbrasen=nvidia.com@groups.io> Sent: Tuesday, March 23, 2021 10:29 AM To: devel@edk2.groups.io<mailto:devel@edk2.groups.io> Cc: jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>; ao.a.wu@intel.com<mailto:ao.a.wu@intel.com>; Jeff Brasen<mailto:jbrasen@nvidia.com> Subject: [EXTERNAL] [edk2-devel] [PATCH 1/1] MdeModulePkg/BmpSupportLib: Allow BMP with extra data Add support for processing BMP data that contains extra data after the image array, this data will not be parsed in anyway in the library but images that contain this will not be rejected from processing. --- MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c b/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c index 3ac31f6723d0..944d01fe7cdf 100644 --- a/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c +++ b/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c @@ -213,7 +213,7 @@ TranslateBmpToGopBlt ( if ((BmpHeader->Size != BmpImageSize) || (BmpHeader->Size < BmpHeader->ImageOffset) || - (BmpHeader->Size - BmpHeader->ImageOffset != DataSize)) { + (BmpHeader->Size - BmpHeader->ImageOffset < DataSize)) { DEBUG ((DEBUG_ERROR, "TranslateBmpToGopBlt: invalid BmpImage... \n")); DEBUG ((DEBUG_ERROR, " BmpHeader->Size: 0x%x\n", BmpHeader->Size)); -- 2.25.1 [-- Attachment #2: Type: text/html, Size: 3918 bytes --] ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [EXTERNAL] [edk2-devel] [PATCH 1/1] MdeModulePkg/BmpSupportLib: Allow BMP with extra data 2021-03-23 17:41 ` [EXTERNAL] [edk2-devel] " Bret Barkelew @ 2021-03-24 11:31 ` Laszlo Ersek 2021-03-24 15:25 ` Jeff Brasen 0 siblings, 1 reply; 9+ messages in thread From: Laszlo Ersek @ 2021-03-24 11:31 UTC (permalink / raw) To: devel, bret.barkelew, jbrasen@nvidia.com Cc: jian.j.wang@intel.com, ao.a.wu@intel.com On 03/23/21 18:41, Bret Barkelew via groups.io wrote: > Is this a *good* idea? > > What is considered valid extra data? If it’s immaterial to the FW displaying the image, our policy has been to strip it off BEFORE adding it to the FW image. Not counting any potential security aspects, stripping out undisplayed portions helps with flash usage too (I think?), so at least some concrete justification in the commit message would be nice... Thanks Laszlo > > - Bret > > From: Jeff Brasen via groups.io<mailto:jbrasen=nvidia.com@groups.io> > Sent: Tuesday, March 23, 2021 10:29 AM > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io> > Cc: jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>; ao.a.wu@intel.com<mailto:ao.a.wu@intel.com>; Jeff Brasen<mailto:jbrasen@nvidia.com> > Subject: [EXTERNAL] [edk2-devel] [PATCH 1/1] MdeModulePkg/BmpSupportLib: Allow BMP with extra data > > Add support for processing BMP data that contains extra data after the > image array, this data will not be parsed in anyway in the library but > images that contain this will not be rejected from processing. > > --- > MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c b/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c > index 3ac31f6723d0..944d01fe7cdf 100644 > --- a/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c > +++ b/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c > @@ -213,7 +213,7 @@ TranslateBmpToGopBlt ( > > if ((BmpHeader->Size != BmpImageSize) || > (BmpHeader->Size < BmpHeader->ImageOffset) || > - (BmpHeader->Size - BmpHeader->ImageOffset != DataSize)) { > + (BmpHeader->Size - BmpHeader->ImageOffset < DataSize)) { > > DEBUG ((DEBUG_ERROR, "TranslateBmpToGopBlt: invalid BmpImage... \n")); > DEBUG ((DEBUG_ERROR, " BmpHeader->Size: 0x%x\n", BmpHeader->Size)); > -- > 2.25.1 > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [EXTERNAL] [edk2-devel] [PATCH 1/1] MdeModulePkg/BmpSupportLib: Allow BMP with extra data 2021-03-24 11:31 ` Laszlo Ersek @ 2021-03-24 15:25 ` Jeff Brasen 2021-03-24 18:26 ` Laszlo Ersek 0 siblings, 1 reply; 9+ messages in thread From: Jeff Brasen @ 2021-03-24 15:25 UTC (permalink / raw) To: Laszlo Ersek, devel@edk2.groups.io, bret.barkelew@microsoft.com Cc: jian.j.wang@intel.com, ao.a.wu@intel.com [-- Attachment #1: Type: text/plain, Size: 3589 bytes --] 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. Thanks, Jeff ________________________________ From: Laszlo Ersek <lersek@redhat.com> Sent: Wednesday, March 24, 2021 5:31 AM To: devel@edk2.groups.io <devel@edk2.groups.io>; bret.barkelew@microsoft.com <bret.barkelew@microsoft.com>; Jeff Brasen <jbrasen@nvidia.com> Cc: jian.j.wang@intel.com <jian.j.wang@intel.com>; ao.a.wu@intel.com <ao.a.wu@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 On 03/23/21 18:41, Bret Barkelew via groups.io wrote: > Is this a *good* idea? > > What is considered valid extra data? If it’s immaterial to the FW displaying the image, our policy has been to strip it off BEFORE adding it to the FW image. Not counting any potential security aspects, stripping out undisplayed portions helps with flash usage too (I think?), so at least some concrete justification in the commit message would be nice... Thanks Laszlo > > - Bret > > From: Jeff Brasen via groups.io<mailto:jbrasen=nvidia.com@groups.io> > Sent: Tuesday, March 23, 2021 10:29 AM > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io> > Cc: jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>; ao.a.wu@intel.com<mailto:ao.a.wu@intel.com>; Jeff Brasen<mailto:jbrasen@nvidia.com> > Subject: [EXTERNAL] [edk2-devel] [PATCH 1/1] MdeModulePkg/BmpSupportLib: Allow BMP with extra data > > Add support for processing BMP data that contains extra data after the > image array, this data will not be parsed in anyway in the library but > images that contain this will not be rejected from processing. > > --- > MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c b/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c > index 3ac31f6723d0..944d01fe7cdf 100644 > --- a/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c > +++ b/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c > @@ -213,7 +213,7 @@ TranslateBmpToGopBlt ( > > if ((BmpHeader->Size != BmpImageSize) || > (BmpHeader->Size < BmpHeader->ImageOffset) || > - (BmpHeader->Size - BmpHeader->ImageOffset != DataSize)) { > + (BmpHeader->Size - BmpHeader->ImageOffset < DataSize)) { > > DEBUG ((DEBUG_ERROR, "TranslateBmpToGopBlt: invalid BmpImage... \n")); > DEBUG ((DEBUG_ERROR, " BmpHeader->Size: 0x%x\n", BmpHeader->Size)); > -- > 2.25.1 > > > > > > > > > > > > [-- Attachment #2: Type: text/html, Size: 6027 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [EXTERNAL] [edk2-devel] [PATCH 1/1] MdeModulePkg/BmpSupportLib: Allow BMP with extra data 2021-03-24 15:25 ` Jeff Brasen @ 2021-03-24 18:26 ` Laszlo Ersek 2021-03-25 3:00 ` Andrew Fish 0 siblings, 1 reply; 9+ messages in thread From: Laszlo Ersek @ 2021-03-24 18:26 UTC (permalink / raw) To: Jeff Brasen, devel@edk2.groups.io, bret.barkelew@microsoft.com Cc: jian.j.wang@intel.com, ao.a.wu@intel.com 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, and edk2 is needlessly strict, and the check can be relaxed without security risks, then I think a patch would be fair. Thanks Laszlo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [EXTERNAL] [edk2-devel] [PATCH 1/1] MdeModulePkg/BmpSupportLib: Allow BMP with extra data 2021-03-24 18:26 ` Laszlo Ersek @ 2021-03-25 3:00 ` Andrew Fish 2021-03-30 1:35 ` Gao, Zhichao 0 siblings, 1 reply; 9+ messages in thread From: Andrew Fish @ 2021-03-25 3:00 UTC (permalink / raw) To: edk2-devel-groups-io, Laszlo Ersek Cc: Jeff Brasen, bret.barkelew@microsoft.com, jian.j.wang@intel.com, ao.a.wu@intel.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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [EXTERNAL] [edk2-devel] [PATCH 1/1] MdeModulePkg/BmpSupportLib: Allow BMP with extra data 2021-03-25 3:00 ` Andrew Fish @ 2021-03-30 1:35 ` Gao, Zhichao 2021-04-01 17:37 ` Bret Barkelew 0 siblings, 1 reply; 9+ messages in thread From: Gao, Zhichao @ 2021-03-30 1:35 UTC (permalink / raw) To: devel@edk2.groups.io, afish@apple.com, Laszlo Ersek Cc: Jeff Brasen, bret.barkelew@microsoft.com, Wang, Jian J, Wu, Hao A, Yao, Jiewen, Liming Gao, Ni, Ray [-- Attachment #1: Type: text/plain, Size: 3545 bytes --] 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@edk2.groups.io <devel@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@edk2.groups.io>; Laszlo Ersek <lersek@redhat.com> Cc: Jeff Brasen <jbrasen@nvidia.com>; bret.barkelew@microsoft.com; Wang, Jian J <jian.j.wang@intel.com>; ao.a.wu@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@redhat.com<mailto: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 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 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: 9968 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [EXTERNAL] [edk2-devel] [PATCH 1/1] MdeModulePkg/BmpSupportLib: Allow BMP with extra data 2021-03-30 1:35 ` Gao, Zhichao @ 2021-04-01 17:37 ` Bret Barkelew 2021-04-16 17:12 ` Jeff Brasen 0 siblings, 1 reply; 9+ messages in thread From: Bret Barkelew @ 2021-04-01 17:37 UTC (permalink / raw) To: devel@edk2.groups.io, zhichao.gao@intel.com, afish@apple.com, Laszlo Ersek Cc: Jeff Brasen, Wang, Jian J, Wu, Hao A, Yao, Jiewen, Liming Gao, Ni, Ray [-- Attachment #1.1: Type: text/plain, Size: 5502 bytes --] 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@groups.io> Sent: Monday, March 29, 2021 6:35 PM To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; afish@apple.com<mailto:afish@apple.com>; Laszlo Ersek<mailto:lersek@redhat.com> Cc: Jeff Brasen<mailto:jbrasen@nvidia.com>; Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>; Wang, Jian J<mailto:jian.j.wang@intel.com>; Wu, Hao A<mailto:hao.a.wu@intel.com>; Yao, Jiewen<mailto:jiewen.yao@intel.com>; Liming Gao<mailto:gaoliming@byosoft.com.cn>; Ni, Ray<mailto:ray.ni@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@edk2.groups.io <devel@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@edk2.groups.io>; Laszlo Ersek <lersek@redhat.com> Cc: Jeff Brasen <jbrasen@nvidia.com>; bret.barkelew@microsoft.com; Wang, Jian J <jian.j.wang@intel.com>; ao.a.wu@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@redhat.com<mailto: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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen.wikipedia.org%2Fwiki%2FBMP_file_format%23cite_note-DIBhelp-5&data=04%7C01%7CBret.Barkelew%40microsoft.com%7C962ec4d4113f436d738f08d8f31c267c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637526649524198979%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=M0IaRFkFKzmJKGZs9fYioFQHuV4hssMUXD7qLdB1lV0%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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.microsoft.com%2Fen-us%2Fprevious-versions%2Fms969901(v%3Dmsdn.10)%3Fredirectedfrom%3DMSDN&data=04%7C01%7CBret.Barkelew%40microsoft.com%7C962ec4d4113f436d738f08d8f31c267c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637526649524198979%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=mywuMJCCBcROvw3o89VeLWYI9VWjiMsqZabFg8jYM%2B0%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 [-- Attachment #1.2: Type: text/html, Size: 11990 bytes --] [-- Attachment #2: F0EC9BD5722B471983D77D317B3A9240.png --] [-- Type: image/png, Size: 140 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [EXTERNAL] [edk2-devel] [PATCH 1/1] MdeModulePkg/BmpSupportLib: Allow BMP with extra data 2021-04-01 17:37 ` Bret Barkelew @ 2021-04-16 17:12 ` Jeff Brasen 0 siblings, 0 replies; 9+ messages in thread From: Jeff Brasen @ 2021-04-16 17:12 UTC (permalink / raw) To: Bret Barkelew, devel@edk2.groups.io, zhichao.gao@intel.com, afish@apple.com, Laszlo Ersek Cc: Wang, Jian J, Wu, Hao A, Yao, Jiewen, Liming Gao, Ni, Ray [-- Attachment #1.1: Type: text/plain, Size: 6954 bytes --] 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@microsoft.com> Sent: Thursday, April 1, 2021 11:37 AM To: devel@edk2.groups.io <devel@edk2.groups.io>; zhichao.gao@intel.com <zhichao.gao@intel.com>; afish@apple.com <afish@apple.com>; Laszlo Ersek <lersek@redhat.com> Cc: Jeff Brasen <jbrasen@nvidia.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Ni, Ray <ray.ni@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@groups.io> Sent: Monday, March 29, 2021 6:35 PM To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; afish@apple.com<mailto:afish@apple.com>; Laszlo Ersek<mailto:lersek@redhat.com> Cc: Jeff Brasen<mailto:jbrasen@nvidia.com>; Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>; Wang, Jian J<mailto:jian.j.wang@intel.com>; Wu, Hao A<mailto:hao.a.wu@intel.com>; Yao, Jiewen<mailto:jiewen.yao@intel.com>; Liming Gao<mailto:gaoliming@byosoft.com.cn>; Ni, Ray<mailto:ray.ni@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@edk2.groups.io <devel@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@edk2.groups.io>; Laszlo Ersek <lersek@redhat.com> Cc: Jeff Brasen <jbrasen@nvidia.com>; bret.barkelew@microsoft.com; Wang, Jian J <jian.j.wang@intel.com>; ao.a.wu@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@redhat.com<mailto: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://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 [-- Attachment #1.2: Type: text/html, Size: 14141 bytes --] [-- Attachment #2: F0EC9BD5722B471983D77D317B3A9240.png --] [-- Type: image/png, Size: 140 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-04-16 17:12 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-03-23 17:29 [PATCH 1/1] MdeModulePkg/BmpSupportLib: Allow BMP with extra data Jeff Brasen 2021-03-23 17:41 ` [EXTERNAL] [edk2-devel] " Bret Barkelew 2021-03-24 11:31 ` Laszlo Ersek 2021-03-24 15:25 ` Jeff Brasen 2021-03-24 18:26 ` Laszlo Ersek 2021-03-25 3:00 ` Andrew Fish 2021-03-30 1:35 ` Gao, Zhichao 2021-04-01 17:37 ` Bret Barkelew 2021-04-16 17:12 ` Jeff Brasen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox