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;