carbonblack / binee

Binee: binary emulation environment
GNU General Public License v2.0
503 stars 73 forks source link

Potential PE Parsing Code Improvements #4

Closed recvfrom closed 5 years ago

recvfrom commented 5 years ago

I was looking at the PE parsing code and wanted to note some areas that could potentially be improved upon

https://github.com/carbonblack/binee/blob/d9419fd56a0c8b3094f684f4608d8026d0eb115f/pefile/pefile.go#L260-L273

The OptionalHeader32 and OptionalHeader32P structures include space for 16 DataDirectories, but it's not guaranteed that there will be 16. In these cases, the SizeOfOptionalHeader might be less than the checks expect, which would cause issues (or could populate the DataDirectories with invalid data). I can share a few example cases if you'd like.

https://github.com/carbonblack/binee/blob/d9419fd56a0c8b3094f684f4608d8026d0eb115f/pefile/pefile.go#L823-L827

It might be worth checking that index < min(16, NumberOfRvaAndSizes) and return nil otherwise. A similar check might also be good to have in all of the functions that have similar code using DataDirectories[0] or DataDirectories[1]. Also, the 'Certificate Table' (index 4) RVA stores a file offset instead of an actual RVA, so it might be worth skipping that one.

kgwinnup commented 5 years ago

Will work on implementing changes. Thank you and please do share a hash/sample for testing

recvfrom commented 5 years ago

This collection of PEs is great for testing (although supporting all of the edge cases in those executables might be overkill): https://github.com/corkami/pocs/tree/master/PE

Specifically, checkout https://github.com/corkami/pocs/blob/master/PE/bin/no_dd.exe and https://github.com/corkami/pocs/blob/master/PE/bin/no_dd64.exe for examples of executables with no DataDirectories.

If you want "real" samples for testing, I'll have to dig a bit more for really strange ones. From a quick search of some samples I had sitting around, though:

0af77fc8eb19d3a3d98940229b48513d67d47d61709a2a0be7373b571c8bc8ab
SizeOfOptionalHeader: 224
NumberOfRvaAndSizes: 65037
SizeOfHeaders: 4096

d853c5fb025d4601d00ed8c463aecc259aba02d094bafd2d29131dc25209fc17
SizeOfOptionalHeader: 224
NumberOfRvaAndSizes: 15
SizeOfHeaders: 1024