Introduce new ELF parser #348
Conversation
alessandrod
force-pushed
the
new-elf
branch
11 times, most recently
from
a9d1081
to
db3236e
Compare
|
I still want to add some structured fuzzing, but this is now ready for review! As it is now, the new parser is supposed to be a drop in replacement for the old goblin parser. It does not enforce stricter checks yet. I'm happy to add those as part of this PR or do them as a follow up, but my priority for now is to be 100% compatible with the old parser so we can drop goblin sooner rather than later. |
Codecov Report
@@ Coverage Diff @@
## main #348 +/- ##
==========================================
- Coverage 90.21% 89.88% -0.34%
==========================================
Files 21 24 +3
Lines 8312 9000 +688
==========================================
+ Hits 7499 8090 +591
- Misses 813 910 +97
Help us with your feedback. Take ten seconds to tell us how you rate us. |
alessandrod
force-pushed
the
new-elf
branch
3 times, most recently
from
fb6408f
to
ce0cdcd
Compare
| let mut offset = 0usize; | ||
| for section_header in section_header_table.iter() { | ||
| if section_header.sh_type == SHT_NOBITS { | ||
| continue; |
There was a problem hiding this comment.
Think we can restrict this further?
There was a problem hiding this comment.
Actually, it is the other way around. The continue statement here excludes sections which have no data in the file (SHT_NOBITS) from the restrictive checks below.
There was a problem hiding this comment.
Correct, but why do we enforce section header checks on sections we don't care about. Not saying we shouldn't but I suspect that there might be deployed elfs that don't adhere to these restrictions
There was a problem hiding this comment.
I'm "+0" on doing those checks - but keep in mind that it's checking that the sections don't overlap with the file header, program headers and the section table. Those are all things not allowed in ELF, and I can't think of anything that would generate such files (barring corruption or intentionally malicious inputs)
The new parser is a lot stricter (e.g. sections may not overlap, string length limits, etc.) do you plan on putting these restrictions behind a feature flag? Or just have them come with the general switch of the parser implementation feature gate? Also, additionally to fuzzing, can we have benchmarks for parsing speed and memory consumption? |
Check for alignment before casting pointers to references or reference slices.
When config.new_elf_parser=true, use the new dependency free ELF parser instead of the legacy goblin based one.
The dynamic table can be padded with invalid entries, and processing must stop at the first DT_NULL.
…ents At least until rust-bpf-sysroot v0.13, we used to generate invalid dynamic sections where the address of DT_REL was not contained in any program segment. When loading such files, fallback to relying on section headers to find out the offset of the relocations table. One notable program that exhibits this bug is https://github.com/solana-labs/solana-program-library/releases/tag/memo-v3.0.0
Its usage was removed in 2e6d571
unwrap() was actually safe since in a path only exercised from the cli, but just in case the code gets moved around in the future.
From the reference docs: "If it is not possible to align the pointer, the implementation returns usize::MAX. It is permissible for the implementation to always return usize::MAX. Only your algorithm’s performance can depend on getting a usable offset here, not its correctness."
… SHT_NULL Other data in ELF can link back to entry #0 to mean "undefined section".
When
config.new_elf_parser=true, parse ELF files using a new dependency free ELF parser instead of the old goblin based one.