efi_loader: tighten PE verification code?

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.

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

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.

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

Hi Patrick,

On Fri, 8 May 2020 at 12:56, Patrick Wildt <patrick at blueri.se> 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.
>
> Especially since this is "Secure Boot", this part should definitely be
> air tight.
>
> 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.
>
> 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

Can I suggest adding some more unit tests?

Regards,
Simon

On Sun, May 10, 2020 at 02:36:51PM -0600, Simon Glass wrote:
> Hi Patrick,
> 
> On Fri, 8 May 2020 at 12:56, Patrick Wildt <patrick at blueri.se> 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.
> >
> > Especially since this is "Secure Boot", this part should definitely be
> > air tight.
> >
> > 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.
> >
> > 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
> 
> Can I suggest adding some more unit tests?
> 
> Regards,
> Simon

Hi,

It's a good suggestion.  Now it only needs someone with enough time to
actually do it. :-)  I'll come back to this in a few weeks when my
current project becomes quieter.  I wish I could right now.

Regards,
Patrick

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;