Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PE: add overlay detection #344

Merged
merged 3 commits into from
Aug 10, 2021
Merged

PE: add overlay detection #344

merged 3 commits into from
Aug 10, 2021

Conversation

daladim
Copy link
Contributor

@daladim daladim commented Aug 6, 2021

This change adds supports for overlay detection in object.

A data overlay is defined as the part of a PE file that is not declared in its headers, i.e. the part of the file past all declared sections.
That's a place where e.g. authenticode certificates are embedded (and where some binaries may also use to conceal data)

@daladim
Copy link
Contributor Author

daladim commented Aug 6, 2021

Final offset computation is not very expensive, but tell me if you want it not to be computed until the user actually calls pub fn overlay_offset(&self)

Copy link
Contributor

@philipc philipc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't calculate this in PeFile::parse. We try to make things lazily calculated as much as possible, for two reasons:

  • it avoids unnecessary work and code bloat for users that don't need it
  • it avoids potential parsing errors for parts of the file that users may not care about

Don't add methods to the unified API unless they are needed for its implementation. Instead, add things like this to the lower level API.

I'm not keen on adding concepts like "overlay" data that are not officially defined. This would better belong in a malware analysis crate built on top of this crate. And only looking at the end of the file seems wrong to me. That may be the easiest place to add "overlay" data, but it's not the only place.

So instead, I would like to see a method such as SectionTable::maximum_file_offset that returns the maximum section end file offset, and then you can do whatever you want with that (such as comparing it with the file size or data directories).

src/read/pe/file.rs Outdated Show resolved Hide resolved
@daladim
Copy link
Contributor Author

daladim commented Aug 9, 2021

Thanks for your review. I've made changes to implement only a SectionTable::max_section_file_offset.

src/read/coff/section.rs Outdated Show resolved Hide resolved
src/read/coff/section.rs Show resolved Hide resolved
Copy link
Contributor

@philipc philipc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@philipc philipc merged commit 024b288 into gimli-rs:master Aug 10, 2021
@daladim daladim deleted the pe_overlay branch September 10, 2021 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants