efi_loader: tighten PE verification code?

[Patrick Wildt <patrick@blueri.se>]

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@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