Audit libgoblin

[Shnatsel]

https://crates.io/crates/goblin

Parser for binary formats: ELF, Mach-O, PE. 750 downloads/day. High-risk due to being a binary parser and potentially exposed to untrusted input.

At a glance the ELF module contains unnecessary unsafe code. It should be possible to rewrite it safely without losing performance.

[nico-abram]

There seems to be quite a bit of unsafe in here.

Some low hanging fruit seems to be in https://github.com/m4b/goblin/blob/master/src/pe/data_directories.rs#L38-L97 . I believe these unsafe get_unchecked's to be completely unnecessary. In my experience (At least in small samples in godbolt), rust seems to elide the bounds checks when indexing a constant when using arrays with a length longer than that constant (Example: https://godbolt.org/z/tHdEs4 ). Anecdotically, someone was talking about get_unchecked being slower on debug than the regular indexing because it didn't inline the call or some such the other day in the rust community discord iirc.

[danielhenrymantilla]

To be honest, I don't think debug performance matters that much (when it does, one can adjust the opt-level for the [profile.dev]).

On the other hand, the code you just linked could be far more resiliant (e.g., to code refactoring) if the lets became const, which would allow appending static assertions for the bound checks.

---

On the other hand, all these unsafe blocks without // # Safety annotations are indeed worrisome: the least thing to do is to add such annotations justifying their soundness (if any).

[icefoxen]

Sheesh, that's some weird Rust code. Very C-ish in style, though I guess they really don't want to copy things if they can avoid it. Looking through the ELF code, at least, most of the unsafety is hazardous but reasonable -- A lot of it is unsafe impl plain::Plain for Whatever {}, which provides methods via the plain crate allowing casting to and from structs that have the correct layout -- these impl's seem to always be on structs with #[repr(C)] that match ELF header structure layouts. There's also a number of implementations of from_raw_parts(), which on its own are (mostly?) harmless and just call slice::from_raw_parts().

More dubious things in src/elf/*.rs:

• this guy looks okay, I think; it's casting data that gets read from a file, but the lengths are basically fixed. There's a FIXME comment related to it but while an error there might provide invalid values I don't think it can cause an overflow, at least not in that function, though it'll produce garbage values that may cause overflows elsewhere. Is there a better way to read bytes from a file directly into a vec of a type? This comes up a few times.
• these look... okay-ish as long as input is good. Which it may not be. I'd probably just assume those are guilty by default.
• this one needs rewriting; it's probably both simpler and safer using slices. As it is it looks like a direct port from C code.
• this is basically the same idiom as the first item with the FIXME comment, and subject to the same caveats. Same here. And here. And here.