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 = §ions[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;
