efi_loader: tighten PE verification code?

[Simon Glass]

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