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

Fix parsing PE with zero raw_data_size of section #396

Merged
merged 7 commits into from
Oct 27, 2024
25 changes: 25 additions & 0 deletions src/pe/header.rs
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,7 @@ pub struct Header {
}

impl Header {
/// Parses PE header from the given bytes
Copy link
Owner

Choose a reason for hiding this comment

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

thank you for adding documentation here; can you add the part about how we will fail if dos stub or header is wrong?

e.g.:

Parses PE header from the given bytes; this will fail if the DosHeader or DosStub is malformed or missing in some way

pub fn parse(bytes: &[u8]) -> error::Result<Self> {
let dos_header = DosHeader::parse(&bytes)?;
let dos_stub = bytes.pread(DOS_STUB_OFFSET as usize).map_err(|_| {
Expand All @@ -569,6 +570,30 @@ impl Header {
optional_header,
})
}

/// Parses PE header from the given bytes that not contains DOS stub, default DosHeader will be used
Copy link
Owner

Choose a reason for hiding this comment

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

grammar: Parses PE header from the given bytes, a default DosHeader and DosStub are generated, and any malformed header or stub is ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for suggestion, fixed

pub fn parse_without_dos(bytes: &[u8]) -> error::Result<Self> {
Copy link
Owner

Choose a reason for hiding this comment

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

needs documentation if pub; i also want to understand what's different here and why this can't be the original function for parsing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@m4b Hello! This bug occurs when parsing file AmdCleanupUtility https://www.amd.com/en/resources/support-articles/faqs/GPU-601.html. Should I add .exe file to tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

need to understand why you need the new pub function

Do you mean error on DosHeader::parse(&bytes)? should be handled inside original Header::parse?
And try to parse without DosHeader in case of Error?

Copy link
Owner

Choose a reason for hiding this comment

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

What I'm asking is why is a new function being added, when does the user call it, and why can't we handle this transparently inside of the regular parse function? Ideally I'd prefer not to add another pub function if possible; maybe it is not possible, I don't know

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can handle this transparently inside of the regular parse function, but

  1. users may expect that parse returns error when DOS stub is missing
  2. there are existing tests that check invalid stub will not parse
  3. when it is known that we have not DOS stub in buffer, it makes sense to use parse_without_dos

Copy link
Contributor Author

@ideeockus ideeockus Jul 7, 2024

Choose a reason for hiding this comment

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

I just added docstrings for this two functions, is it ok now?

Copy link
Owner

Choose a reason for hiding this comment

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

ok thank you for clarifying when this happens, and why to use; i've suggested grammar for the documentation of parse_without_dos

let dos_header = DosHeader::default();
let mut offset = dos_header.pe_pointer as usize;
let signature = bytes.gread_with(&mut offset, scroll::LE).map_err(|_| {
error::Error::Malformed(format!("cannot parse PE signature (offset {:#x})", offset))
})?;
let coff_header = CoffHeader::parse(bytes, &mut offset)?;
let optional_header = if coff_header.size_of_optional_header > 0 {
let opt_hdr = bytes.pread::<optional_header::OptionalHeader>(offset)?;
Some(opt_hdr)
} else {
None
};

Ok(Header {
dos_header,
dos_stub: DosStub::default(),
signature,
coff_header,
optional_header,
})
Copy link
Owner

Choose a reason for hiding this comment

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

I should have said this before, but all of this is basically identical, it might be nice to make a private function:

fn parse_impl(bytes: &[u8]), header: DosHeader, stub: DosStub) -> Self {
  // highlighted section
}

and then the two pub functions call this respectively (your new function supply Default to the header and stub args)

}
}

impl ctx::TryIntoCtx<scroll::Endian> for Header {
Expand Down
2 changes: 2 additions & 0 deletions src/pe/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ fn section_read_size(section: &section_table::SectionTable, file_alignment: u32)

if virtual_size == 0 {
read_size
} else if size_of_raw_data == 0 {
Copy link
Owner

Choose a reason for hiding this comment

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

i'm a little worried about this; this function is used all over goblin, can you add a comment why this check is done, and why it returns the virtual_size in this case? also there are no tests added to verify anything here

Copy link
Contributor Author

@ideeockus ideeockus Jul 29, 2024

Choose a reason for hiding this comment

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

analogically to virtual_size == 0 -> used read_size, if read_size == 0 -> used virtual_size. i saw pefile does this and there are no errors in parsing the AMD sample, so i added this check too and it works

I think this is not normal, but can happen on different types of modified files, so parser should handle this.
(also found doc about PE malformations https://www.virusbulletin.com/uploads/pdf/conference_slides/2007/CaseySheehanVB2007.pdf)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also added test + sample for this case: took file from https://www.amd.com/en/resources/support-articles/faqs/GPU-601.html + packed with upx.

is it ok if i add this file to tests?

Copy link
Owner

Choose a reason for hiding this comment

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

I have no idea what the license of that binary is 😅 so probably let's not add it for now; also, it seems to parse just fine for me without this patch? e.g.:

cargo run --example rdr -- ~/Downloads/amdcleanuputility.exe

has no issue, neither does bingrep

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parse amdcleanuputility.exe itself is fine. But if binary will packed with upx, parsing goes broken, while binary can be run properly.

Ok, test removed from PR

virtual_size
} else {
cmp::min(read_size, round_size(virtual_size))
}
Expand Down
Loading