efi_loader: tighten PE verification code?

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

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;