m4b
PE: fix base relocation parser panic
- Fixes base relocation parser panicking on malformed binaries.
- Added new
parse_baserelocoption inParseOptionsto allow parsing the rest while ignoring the edge case.
Found the issue on the same binary in PR below.
- https://github.com/m4b/goblin/pull/458#issuecomment-2800461527
Malformed entity: tls raw data offset (0xae400) and size (0x10) greater than byte slice len (0x415eb)
The base relocation directory 0xB1000..size(0xADAC) does not exists in the binary.
1, .nsp0, 0x400, 0x400, 0, 0x1000, 0xc7000, 792 kB, Code, Initialized data, Executable, Readable, Writeable (0xe0000060), , , ,
2, .nsp1, 0x400, 0x4155a, 260.34 kB, 0xc7000, 0x109000, 264 kB, Code, Initialized data, Executable, Readable, Writeable (0xe0000060), 6e162fbb8f934b424d0f8c91d9a7d78b, 7.98, 6144:3J4KYvxupv2mlkmJhJP7JC9AnruTag4lsFItEnYFupq4gI0:54K+xusn9ArCag9FItEnmupq4gI0, T1E84423B7321D5F00E44C74FBFADD2733B6E7C9F3158AA34A1A612488BE759801DA1167
3, .nsp2, 0x400, 0x400, 0, 0x109000, 0x109562, 1.35 kB, Code, Initialized data, Executable, Readable, Writeable (0xe0000060), , , ,
N.B. This is the super "special" case where only supposed to happen in this sort of badly "packed" binaries. Proprietary linkers would usually not produce such a completely broken binaries.
One worry j have is proliferation of these flags so that it overwhelms the user or has weird hard to debug affects if multiple settings are changed and they affect parsing in various hard to understand ways, but I don’t think we’re there yet (or ever will be?)
I would like to understand why the te has this set to false otherwise seems reasonable !
IMO returning an empty basereloc table in the case of the malformation would be an alternate option, but frankly, I don't like such a fail-silence design that makes debugging harder. However while I don't think basereloc malformation error is ever "useful."
Should we stand by parse_basereloc flag?
i.e., most PE parsers ignore basereloc malformation and are returning empty info
How about we just get .reloc size from the SectionTable when reported size in DIRECTORY is invalid? Something like this, works well for me: https://github.com/chf0x/goblin/commit/3b46c38c9c002b5b8a9a4a014566fc80e10ded5c
How about we just get .reloc size from the SectionTable when reported size in DIRECTORY is invalid? Something like this, works well for me: chf0x@3b46c38
Hi there. Thank you for your proposal. If I get your solution correct, you're ignoring the invalid range of bytes while parsing the other. That would be an option too but my concern still stands by. The concern is that whether ignoring malformations in such a way isn't a traditional way in goblin. As I mentioned previously, yet it is totally doable particularly this case however.
And specifically for the binary in question, the base relocation directory is completely wrong even if some partial bytes point to the correct range of bytes. This is also my concern if this is confusing to consumers.
@m4b I reverted the change that introduces parse_basereloc: bool option in ParseOptions. Instead, it should store an empty bytes that implies no-op (so that consumers can parse the rest of binary). Is this acceptable solution?
The CI failure is not relevant to this change it seems (it is in ELF parser)
