Add/Extend ELF/PE permissive parsing mode to better handle packed, broken, or malware samples #479
Conversation
…ved or does not exist
- Resolved conflict in src/pe/load_config.rs by keeping the debug log statement - Brings in latest changes from master including: * TLS raw data parser fix * LoadConfig cb size fix * ELF dynamic string fixes * Note type_to_str support for coredump constants * PE offset computation fixes * Documentation updates
chf0x
changed the title
kkent030315
left a comment
kkent030315
left a comment
There was a problem hiding this comment.
Thank you for the PR! I reviewed it partly. I'll do more review at the rest of time.
|
Thank you for the review! I’ll work on the fixes on Monday |
|
@chf0x by the way what IDE are you on? If you're under VSCode or Zed you might install rust-analyzer extension (if you don't have one yet). It looks like you're having slightly different formatting rule so. :) |
kkent030315
left a comment
kkent030315
left a comment
There was a problem hiding this comment.
Thanks very much, the patch looks very clean!
m4b
left a comment
m4b
left a comment
There was a problem hiding this comment.
ok the major thing is the non-exhaustive which I realized must be removed to make this backwards compat, moving the (pub(crate)) helper trait into options for now, and some other minor nits, mostly reducing the api surface from pub to pub(crate).
thanks for your patience, this should be the last review :)
|
Thank you for the review. I’ll make the changes. I’ve recently started a new job, so my time is a bit limited, but I’ll return to this asap |
|
Ok congrats on the new job! I hope we didn't scare you off with too much review, definitely would like to see this landed at some point, it's a great improvement to goblin. Thanks for your time! |
…reduce public API, and lazily format error messages
|
Hi team, apologies for the delay. It’s been a busy few weeks, but I believe I’ve addressed all the requested changes. Could you please review? Thank you! |
kkent030315
left a comment
kkent030315
left a comment
There was a problem hiding this comment.
LGTM. With the permissive trait the codebase looks very clean now! (maybe you can fold them down the comments in options.rs?)
m4b
left a comment
m4b
left a comment
There was a problem hiding this comment.
So I'd prefer the pub(crate) and non_exhaustive to go in with this patch, but if you don't feel like it, i'll wait until tomorrow, and then merge and force push the changes since thy're important before a point release.
regardless thank you so much for your work on this, this is really great stuff! thank you for pulling through to the final stage :)
src/pe/header.rs
Outdated
| Self::parse_with_opts(bytes, &crate::pe::options::ParseOptions::default()) | ||
| } | ||
|
|
||
| pub fn parse_with_opts( |
…RichHeader::parse_with_opts pub(crate), and remove unnecessary comment
|
Awesome news, thanks for reviewing @m4b @kkent030315 ! The last requested changes are now in |
|
thanks for getting this across the finish line @chf0x ! great stuff! |
|
NB: backwards compatible |
|
NB: non-breaking, in 0.10.2 |
3 participants
@m4b I’ve been using Goblin to work with malware, and over time I’ve encountered a number of cases where parsing fails. In the
strstabissue, you mentioned that you might be open to adding a permissive mode to address such pain points. I’d like to reopen this pull request with the changes I’ve made over time to enable parsing of broken binaries.There are still a few places where the permissive flag is missing, for example, in strtab. I’d be happy to add those if you think this is worth merging into the main project.
I also probably would need to update tests.
Thank you!