Hi Patrick, On Fri, 8 May 2020 at 12:56, Patrick Wildt <[email protected]> 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
