PE loader should zero out dest buffer on allocation (Bugzilla Bug 1999)

[tianocore-issues]

This issue was created automatically with bugzilla2github

Bugzilla Bug 1999

Date: 2019-07-19T05:10:10+00:00
From: Wang, Jian J <<jian.j.wang>>
To: @LiuZhiguang001
CC: @jkmathews, mhaeuser, @mdkinney, nobody, siyuan.fu, @vincent-j-zimmer

Last updated: 2021-12-03T12:53:23+00:00

[tianocore-issues]

Comment 8510

Date: 2019-07-19 05:10:10 +0000
From: Wang, Jian J <<jian.j.wang>>

• Industry Specification: ---
• Release Observed: EDK II Master
• Releases to Fix: EDK II Master
• Target OS: ---
• Bugzilla Assignee(s): @LiuZhiguang001

PE loader should zero out dest buffer on allocation. In Image.c check the CoreLoadPeImage() to see if after CoreAllocatePages() is run, the memory is simply used and not zeroed anywhere.

[tianocore-issues]

Comment 8511

Date: 2019-07-19 05:10:52 +0000
From: Wang, Jian J <<jian.j.wang>>

CVSS score: Low: 2.6 https://www.first.org/cvss/calculator/3.0#CVSS:3.0/AV:A/AC:H/PR:L/UI:N/S:U/C:H/I:L/A:L

[tianocore-issues]

Comment 8661

Date: 2019-07-31 12:25:15 +0000
From: @vincent-j-zimmer

not worthy of embargo. move to normal bz per 7.31.19 infosec meeting

[tianocore-issues]

Comment 8762

Date: 2019-08-02 09:35:07 +0000
From: @lersek

Opening up the BZ to the public, per comment 2.

[tianocore-issues]

Comment 9564

Date: 2019-09-09 22:49:00 +0000
From: Hao Wu <<hao.a.wu>>

For this reported issue, I am not sure what is the impact if there is
uninitialized memory after loading the image.

My thought is that in function PeCoffLoaderLoadImage(), the header and sections
of the PE/TE image gets loaded into memory with specific offset and size (the
offset and size of every section is got from parsing the header). Hence, these
memory ranges will be filled with valid contents. But also, some holes in
memory can be left uninitialized.

The offset and size information is managed by the data structure specified by
'ImageContext' until the unload of the image. Later on, CoreStartImage() will
consume the content in 'ImageContext' for the image execution.

I think these uninitialized memory holes will not be consumed since the image
execution is based on the contents managed by 'ImageContext'. In my opinion,
there should be no impact for the reported issue.

Add MdePkg maintainers to see if they have comments on this report.

[tianocore-issues]

Comment 10861

Date: 2020-01-13 04:04:08 +0000
From: Marvin Häuser <>

tl;dr: Please zero the entire buffer from either its start or from right past SizeOfHeaders to the allocated size (not just SizeOfImage).

First off: The proposed patch is dangerous and not sufficient because there may be further uninitialized data between sections (there is no verification that they are consecutive) and in the trailer (the top RVA is not checked against SizeOfImage). To be honest, I think the current PE loader is very dangerous entirely and I have been working on a rewrite for a while, which I will eventually submit for review in a few months. In my local fork, I have the caller pass the destination's size in (because it may be bigger than SizeOfImage for any sort of reason), copy the header and then zero the subsequent bytes till the buffer's end to ensure no unexpected gaps in loaded regions result in uninitialized data.

Hao,

Zeroing of ranges not filled by the image loader is common security practice and not to ensure functionality. Basically it's done to account for bugs in the code that may access data at unexpected offsets (i.e. outside of sections). An attacker knowing about such a bug could somehow abuse any sort of access (even a lucky expansion bug in the loader itself maybe) to fill intentionally malicious contents into this uninitialized range without any effect on the binary's signature, which may or may not be a severe vulnerability. Hence, it's a best practice convention to never load any byte from a binary which is not part of the signature hash, and to also not allow any uninitialized data within the load range, so the entire load buffer can be considered deterministic and authenticated.
While such an offset access bug could of course also lead to accesses outside of the image region, that is much more likely caught by sanitizers (I guess implementing this in UEFI would be tough, however the images may be tested and fuzzed in any OS userland to ensure functionality). This is also the reason why it is only acceptable to zero the entire buffer and not just SizeOfImage bytes.

[tianocore-issues]

Comment 10865

Date: 2020-01-13 12:57:24 +0000
From: @lersek

Patch on the list, from "Liu, Zhiguang" <zhiguang.liu@intel.com>:

[edk2-devel] [PATCH]
MdePkg: PE loader should zero out dest buffer on allocation

https://edk2.groups.io/g/devel/message/53185
http://mid.mail-archive.com/20200113081854.9732-1-zhiguang.liu@intel.com