Fix parsing PE with zero raw_data_size of section by ideeockus · Pull Request #396 · m4b/goblin

ideeockus · 2024-03-13T19:30:27Z

Sometimes I need to parse PE header without parsing DOS header + DOS stub
Seems that finding offset by rva is not implemented correctly and breaks down on some PE files. See 'cannot map exception_rva (0x5e000) into offset' when parsing PE32+ library #307

m4b

it seems like there are two changes, one adding a new public function, and the other fixing a bug in the rva helper function, yes? need to understand why you need the new pub function, but the other fix probably is ok, though i would prefer a test showing how it fixes a bug

m4b · 2024-03-18T03:01:13Z

        })
    }
+
+    pub fn parse_without_dos(bytes: &[u8]) -> error::Result<Self> {


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

@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?

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?

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

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

users may expect that parse returns error when DOS stub is missing

there are existing tests that check invalid stub will not parse

when it is known that we have not DOS stub in buffer, it makes sense to use parse_without_dos

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

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

…out dos

m4b

thanks for your patience! this is the last round, to summarize:

documentation grammar fixes I suggested
making the private impl function would be nice

and then this is ready to go, thank you!

m4b · 2024-07-07T18:38:07Z

        })
    }
+
+    /// Parses PE header from the given bytes that not contains DOS stub, default DosHeader will be used


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

thanks for suggestion, fixed

m4b · 2024-07-07T18:39:05Z


    if virtual_size == 0 {
        read_size
+    } else if size_of_raw_data == 0 {


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

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)

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?

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

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

m4b · 2024-07-07T18:43:04Z

        })
    }
+
+    pub fn parse_without_dos(bytes: &[u8]) -> error::Result<Self> {


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

m4b · 2024-07-07T18:43:50Z

 }

 impl Header {
+    /// Parses PE header from the given bytes


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

m4b · 2024-07-07T18:49:25Z

+        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,
+        })


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)

m4b

Ok this looks great, sorry for the delay! If you just delete the binary (and test), we can merge this; we can even put it on a minor release of 0.9 if you want, since it has no breaking changes. There is another change upcoming that will need to rebase against this, or you rebase against them, i'd prefer the first, since we can minor release it if want, and also your PR has been outstanding longer :)

m4b · 2024-10-27T00:37:15Z

@ideeockus just fyi, in the next breaking release 0.10, the pe::Header will have a lifetime parameter, just curious, will this be ok for your usecases? If people need owned versions I was thinking of some ways to allow this, but if it's not readily needed, I won't think too hard about it

m4b · 2024-10-27T00:46:26Z

ok published in 0.9.2, thanks for your contribution!

pe.utils: handle case when section raw_data is zero

06cf965

m4b requested changes Mar 18, 2024

View reviewed changes

ideeockus force-pushed the master branch from 1a6abe6 to 43278d5 Compare July 7, 2024 18:05

pe.header: added ability to parse part of pe header without dos

088b6d6

ideeockus force-pushed the master branch from 43278d5 to 088b6d6 Compare July 7, 2024 18:06

pe.header: added docstring for new pub method to parse pe header with…

ac439f7

…out dos

m4b requested changes Jul 7, 2024

View reviewed changes

ideeockus and others added 3 commits July 29, 2024 14:11

Merge branch 'm4b:master' into master

2a5534c

pe.header: fixed docstrings & added parse_impl

802d4cf

pe.utils: added test for 'cannot map exception_rva into offset' case

f504af9

ideeockus requested a review from m4b September 2, 2024 07:28

m4b requested changes Oct 26, 2024

View reviewed changes

pe.utils: removed test for 'cannot map exception_rva into offset' case

0874939

m4b approved these changes Oct 27, 2024

View reviewed changes

m4b merged commit f401d18 into m4b:master Oct 27, 2024

philipc mentioned this pull request Apr 20, 2026

fix(pe): only map file-backed sections to offsets #527

Merged

Conversation

ideeockus commented Mar 13, 2024

Uh oh!

m4b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ideeockus Jul 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m4b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ideeockus Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m4b left a comment

Choose a reason for hiding this comment

Uh oh!

m4b commented Oct 27, 2024

Uh oh!

m4b commented Oct 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ideeockus Jul 7, 2024 •

edited

Loading

ideeockus Jul 29, 2024 •

edited

Loading