pe: only process RelocDir->Size of reloc section #562

mikebeaton · 2023-04-10T07:36:08Z

Previously processing full padding-aligned Section->Misc.VirtualSize relied on padding reloc entries being inserted by GenFw, which is not required by spec.

Addresses #561

Previously processing full padding-aligned Section->Misc.VirtualSize relied on padding reloc entries being inserted by GenFw, which is not required by spec. Signed-off-by: Mike Beaton <mjsbeaton@gmail.com>

mikebeaton · 2023-04-10T07:40:33Z

pe.c

@@ -1277,8 +1277,11 @@ handle_image (void *data, unsigned int datasize,
 					Section->Misc.VirtualSize &&
 					base && end &&
 					RelocBase == base &&
-					RelocBaseEnd == end) {
+					RelocBaseEnd <= end) {


Without this change, the file loads but crashes (since shim will attempt to use the file as if there is no reloc section).

To avoid crash, may I suggest we return error code to caller if RelocSection is not found finally. And let restore_loaded_image() to recover system.

Hi,

Thanks for this input.

Is BS->FreePages correct, here? The various other (pre-existing) return EFI_UNSUPPORTED nearby (and in the same allocation context, I believe), do not do it.

Then on to the general idea, yes, in the current version of this PR the code continues if no reloc section is finally found (because this is what the code previously did), but the correct place to decide not to do so (which I tend to agree would be an improvement) would be around line 1331:

shim/pe.c

Line 1331 in 0640e13

if (context.RelocDir->Size && RelocSection) {

If you (possibly) meant the suggestion as an alternative fix - i.e. to avoid my crash - then I think that misunderstands the nature of the PR - the file should load and run (not abort, and also - ofc - not crash) and this PR allows it to load.

mikebeaton · 2023-04-10T07:43:32Z

pe.c

+				perror(L"Relocation section is invalid \n");
+				return EFI_UNSUPPORTED;


Suggest in the case where a reloc section is found but rejected, aborting with an error such as this is helpful (unless there are known cases where ignore and continue behaviour is required?).

mikebeaton · 2023-04-10T07:44:10Z

pe.c

@@ -87,7 +87,7 @@ relocate_coff (PE_COFF_LOADER_IMAGE_CONTEXT *context,
 	/* RelocBaseEnd here is the address of the first entry /past/ the
 	 * table.  */
 	RelocBaseEnd = ImageAddress(orig, size, Section->PointerToRawData +
-						Section->Misc.VirtualSize);
+						context->RelocDir->Size);


Once the test is modifed to accept the .reloc section, we also have to process only the used size.

mikebeaton · 2023-04-10T07:45:49Z

pe.c

@@ -741,7 +741,7 @@ read_header(void *data, unsigned int datasize,
 	context->NumberOfSections = PEHdr->Pe32.FileHeader.NumberOfSections;

 	if (EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES < context->NumberOfRvaAndSizes) {
-		perror(L"Image header too small\n");
+		perror(L"Image header too large\n");


Unrelated really - apologies if I'm misreading this? (Could make separate tiny PR; or remove, if the wording change does not seem right. Tyvm!)

vathpela · 2023-06-21T18:11:12Z

Pushed as a8b0b60.

@vathpela

What's changed * Various CVE fixes: CVE-2023-40546 mok: fix LogError() invocation CVE-2023-40547 - avoid incorrectly trusting HTTP headers CVE-2023-40548 Fix integer overflow on SBAT section size on 32-bit system CVE-2023-40549 Authenticode: verify that the signature header is in bounds. CVE-2023-40550 pe: Fix an out-of-bound read in verify_buffer_sbat() CVE-2023-40551: pe-relocate: Fix bounds check for MZ binaries * Add make infrastructure to set the NX_COMPAT flag by @vathpela in #530 * Make sbat_var.S parse right with buggy gcc/binutils by @vathpela in #535 * Drop invalid calls to CRYPTO_set_mem_functions by @nicholasbishop in #537 * pe: Align section size up to page size for mem attrs by @nicholasbishop in #539 * test-sbat: Fix exit code by @vathpela in #540 * pe: Add IS_PAGE_ALIGNED macro by @nicholasbishop in #541 * CryptoPkg/BaseCryptLib: Fix buffer overflow issue in realloc wrapper by @nicholasbishop in #546 * Don't loop forever in load_certs() with buggy firmware by @rmetrich in #547 * Block Debian grub binaries with SBAT < 4 by @steve-mcintyre in #550 * Shim unable to locate grubx64 in PXE boot mode when grubx64 is stored in a different file path by @Alberto-Perez-Guevara in #551 * Further improve load_certs() for non-compliant drivers/firmwares by @pbatard in #560 * pe: only process RelocDir->Size of reloc section by @mikebeaton in #562 * Rename 'msecs' to 'usecs' to avoid potential confusion by @aronowski in #563 * Optionally allow to keep shim protocol installed by @bluca in #565 * SBAT-related documents formatting and spelling by @aronowski in #566 * Add SbatLevel_Variable.txt to document the various revocations by @jsetje in #569 * Add a security contact email address in README.md by @vathpela in #572 * Use -Wno-unused-but-set-variable for Cryptlib and OpenSSL by @vathpela in #576 * mok: fix LogError() invocation by @vathpela in #577 * Minor housekeeping by @vathpela in #578 * Test ImageAddress() by @vathpela in #579 * FreePages() is used to return memory allocated by AllocatePages() by @dennis-tseng99 in #580 * Size should minus 1 when calculating 'RelocBaseEnd' by @jsetje in #581 * Verify signature before verifying sbat levels by @jsetje in #583 * Add libFuzzer support for csv.c and sbat.c by @vathpela in #584 * mok: Avoid underflow in maximum variable size calculation by @alpernebbi in #587 * Housekeeping by @vathpela in #605 Signed-off-by: Peter Jones <pjones@redhat.com>

@vathpela

What's changed * Various CVE fixes: CVE-2023-40546 mok: fix LogError() invocation CVE-2023-40547 - avoid incorrectly trusting HTTP headers CVE-2023-40548 Fix integer overflow on SBAT section size on 32-bit system CVE-2023-40549 Authenticode: verify that the signature header is in bounds. CVE-2023-40550 pe: Fix an out-of-bound read in verify_buffer_sbat() CVE-2023-40551: pe-relocate: Fix bounds check for MZ binaries * Add make infrastructure to set the NX_COMPAT flag by @vathpela in rhboot#530 * Make sbat_var.S parse right with buggy gcc/binutils by @vathpela in rhboot#535 * Drop invalid calls to CRYPTO_set_mem_functions by @nicholasbishop in rhboot#537 * pe: Align section size up to page size for mem attrs by @nicholasbishop in rhboot#539 * test-sbat: Fix exit code by @vathpela in rhboot#540 * pe: Add IS_PAGE_ALIGNED macro by @nicholasbishop in rhboot#541 * CryptoPkg/BaseCryptLib: Fix buffer overflow issue in realloc wrapper by @nicholasbishop in rhboot#546 * Don't loop forever in load_certs() with buggy firmware by @rmetrich in rhboot#547 * Block Debian grub binaries with SBAT < 4 by @steve-mcintyre in rhboot#550 * Shim unable to locate grubx64 in PXE boot mode when grubx64 is stored in a different file path by @Alberto-Perez-Guevara in rhboot#551 * Further improve load_certs() for non-compliant drivers/firmwares by @pbatard in rhboot#560 * pe: only process RelocDir->Size of reloc section by @mikebeaton in rhboot#562 * Rename 'msecs' to 'usecs' to avoid potential confusion by @aronowski in rhboot#563 * Optionally allow to keep shim protocol installed by @bluca in rhboot#565 * SBAT-related documents formatting and spelling by @aronowski in rhboot#566 * Add SbatLevel_Variable.txt to document the various revocations by @jsetje in rhboot#569 * Add a security contact email address in README.md by @vathpela in rhboot#572 * Use -Wno-unused-but-set-variable for Cryptlib and OpenSSL by @vathpela in rhboot#576 * mok: fix LogError() invocation by @vathpela in rhboot#577 * Minor housekeeping by @vathpela in rhboot#578 * Test ImageAddress() by @vathpela in rhboot#579 * FreePages() is used to return memory allocated by AllocatePages() by @dennis-tseng99 in rhboot#580 * Size should minus 1 when calculating 'RelocBaseEnd' by @jsetje in rhboot#581 * Verify signature before verifying sbat levels by @jsetje in rhboot#583 * Add libFuzzer support for csv.c and sbat.c by @vathpela in rhboot#584 * mok: Avoid underflow in maximum variable size calculation by @alpernebbi in rhboot#587 * Housekeeping by @vathpela in rhboot#605 Signed-off-by: Peter Jones <pjones@redhat.com>

pe: only process RelocDir->Size of reloc section

c92dc95

Previously processing full padding-aligned Section->Misc.VirtualSize relied on padding reloc entries being inserted by GenFw, which is not required by spec. Signed-off-by: Mike Beaton <mjsbeaton@gmail.com>

mikebeaton commented Apr 10, 2023

View reviewed changes

vathpela closed this Jun 21, 2023

mikebeaton mentioned this pull request Jun 21, 2023

shim image loader assumes padding relocs are added by GenFw, but not all efi files have this #561

Closed

mikebeaton mentioned this pull request Jul 2, 2023

rhboot/shim cannot directly load OpenCore.efi after recent build tooling changes acidanthera/bugtracker#2239

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pe: only process RelocDir->Size of reloc section #562

pe: only process RelocDir->Size of reloc section #562

mikebeaton commented Apr 10, 2023

mikebeaton Apr 10, 2023 •

edited

Loading

dennis-tseng99 Jun 13, 2023

mikebeaton Jun 18, 2023 •

edited

Loading

mikebeaton Apr 10, 2023 •

edited

Loading

mikebeaton Apr 10, 2023 •

edited

Loading

mikebeaton Apr 10, 2023

vathpela commented Jun 21, 2023

		perror(L"Relocation section is invalid \n");
		return EFI_UNSUPPORTED;

pe: only process RelocDir->Size of reloc section #562

pe: only process RelocDir->Size of reloc section #562

Conversation

mikebeaton commented Apr 10, 2023

mikebeaton Apr 10, 2023 • edited Loading

Choose a reason for hiding this comment

dennis-tseng99 Jun 13, 2023

Choose a reason for hiding this comment

mikebeaton Jun 18, 2023 • edited Loading

Choose a reason for hiding this comment

mikebeaton Apr 10, 2023 • edited Loading

Choose a reason for hiding this comment

mikebeaton Apr 10, 2023 • edited Loading

Choose a reason for hiding this comment

mikebeaton Apr 10, 2023

Choose a reason for hiding this comment

vathpela commented Jun 21, 2023

mikebeaton Apr 10, 2023 •

edited

Loading

mikebeaton Jun 18, 2023 •

edited

Loading

mikebeaton Apr 10, 2023 •

edited

Loading

mikebeaton Apr 10, 2023 •

edited

Loading