Re: efi_loader: tighten PE verification code?

Patrick,

On Fri, May 08, 2020 at 08:56:37PM +0200, Patrick Wildt wrote:

Hi,

even though this mail has a diff, it's not supposed to be a patch. I have started adjusting my fuzzer to the upstreamed EFI Secure Boot code and I hit a few issues right away. Two of those I have already sent and were reviewed. I have seen more, but since I needed to reschedule some things I couldn't continue and unfortunately have to postpone fuzzing the code. I can assure you if someone starts fuzzing that code, we will find a few more bugs.

Thank you for examining the code.

Especially since this is "Secure Boot", this part should definitely be air tight.

Yeah, we definitely need more eyes on the code.

One thing I saw is that the virt_size is smaller then the memcpy below at the "Copy PE headers" line then actually copies.

Another thing I saw is SizeOfRawData can be bigger then the VirtualSize, and PointerToRawData + SizeOfRawData bigger then the allocated size.

I'm not sure if this is the right place to do the checks. Maybe they need to be at the place where we are adding the image regions. I guess the fields should be properly verified in view of "does it make sense?".

Also I'm questioning whether it's a good idea to continue parsing the file if it's already clear that the signature can't be verified against the "db". I can understand that you'd like to finish loading the file into an object, and some other instance decides whether or not it's fine to start that image, but you also open yourself to issues when you're parsing a file that obviously is against the current security policy.

One comment: I have changed the logic in a past version when Heinrich pointed out that we should return EFI_SECURITY_VIOLATION and that image execution information table must also be supported.

"Status Codes Returned" by LoadImage API in UEFI specification says,

EFI_ACCESS_DENIED:Image was not loaded because the platform policy prohibits the image from being loaded. NULL is returned in *ImageHandle. EFI_SECURITY_VIOLATION:Image was loaded and an ImageHandle was created with a valid EFI_LOADED_IMAGE_PROTOCOL. However, the current platform policy specifies that the image should not be started.

Now the code returns EFI_SECURITY_VIOLATION, instead of EFI_ACCESS_DENIED, when image's signature can not be verified but yet the image itself will be loaded. When invoking the binary with StartImage API, it fails and put a record in image execution information table. (I have not yet submitted a patch for table support though.)

As UEFI specification doesn't say anything about "policy," I don't know which error code should be returned here.

-Takahiro Akashi

If you keep on parsing a file that obviously (because tested against the "db") cannot be allowed to boot anyway, the checks for the headers need to be even tighter.

I'd be unhappy to see U-Boot CVEs come up regarding PE parsing issues.

Best regards, Patrick

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c index 43a53d3dd1..d134b748be 100644 --- a/lib/efi_loader/efi_image_loader.c +++ b/lib/efi_loader/efi_image_loader.c @@ -681,10 +681,11 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, }

/* Authenticate an image */

• if (efi_image_authenticate(efi, efi_size))

• handle->auth_status = EFI_IMAGE_AUTH_PASSED;
  

• else

• if (!efi_image_authenticate(efi, efi_size)) { handle->auth_status = EFI_IMAGE_AUTH_FAILED;

• ret = EFI_SECURITY_VIOLATION;
  

• goto err;
  

• }

  /* Calculate upper virtual address boundary */ for (i = num_sections - 1; i >= 0; i--) {

@@ -736,6 +737,15 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, goto err; }

• if (virt_size < sizeof(*dos) + sizeof(*nt) +

•    nt->FileHeader.SizeOfOptionalHeader +
  

•    num_sections * sizeof(IMAGE_SECTION_HEADER)) {
  

• efi_free_pages((uintptr_t) efi_reloc,
  

•        (virt_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT);
  

• ret = EFI_LOAD_ERROR;
  

• goto err;
  

• }
• /* Copy PE headers */ memcpy(efi_reloc, efi, sizeof(*dos)

@@ -746,6 +756,13 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, /* Load sections into RAM */ for (i = num_sections - 1; i >= 0; i--) { IMAGE_SECTION_HEADER *sec = &sections[i];

• if (sec->SizeOfRawData > sec->Misc.VirtualSize ||
  

•     sec->PointerToRawData + sec->SizeOfRawData > efi_size) {
  

• 	efi_free_pages((uintptr_t) efi_reloc,
  

• 	       (virt_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT);
  

• 	ret = EFI_LOAD_ERROR;
  

• 	goto err;
  

• }
  

  memset(efi_reloc + sec->VirtualAddress, 0, sec->Misc.VirtualSize); memcpy(efi_reloc + sec->VirtualAddress,

@@ -771,10 +788,8 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, loaded_image_info->image_base = efi_reloc; loaded_image_info->image_size = virt_size;

• if (handle->auth_status == EFI_IMAGE_AUTH_PASSED)

• return EFI_SUCCESS;
  

• else

• return EFI_SECURITY_VIOLATION;
  

• handle->auth_status = EFI_IMAGE_AUTH_PASSED;
• return EFI_SUCCESS;

err: return ret;