Goblin panics when reading certain kinds of UPXed binaries

[ssokolow]

I was investigating my options for parsing EXE files to determine what environment to auto-fill in my experimental game launcher (ie. DOSBox, Wine, Wine+qemu-user, Mono, etc.) and I managed to trigger some panics in goblin.

====================
TESTING WITH GOBLIN:
====================
unknown magic: ./hello_owatcom_com.com
Parse error: ./hello_owatcom_os2v2.exe => Invalid magic number: 0x1
pe: ./hello_pacific.exe
Parse error: ./hello_owatcom_dos.upx.exe => requested range [309100590..309100594) from object of len 6881
Parse error: ./hello_owatcom_dos4g.exe => Invalid magic number: 0x1
Parse error: ./hello_owatcom_windows.exe => Invalid magic number: 0x0
pe: ./hello_mingw32.exe
pe: ./hello_csharp_exe_itanium.exe
pe: ./hello_owatcom_win95.exe
Parse error: ./hello_owatcom_dos4g.upx.exe => Invalid magic number: 0x1
elf: ./hello_gcc.x86
unknown magic: ./hello_djgpp.upx.coff.exe
unknown magic: ./hello_owatcom_com.upx.com
unknown magic: ./hello_dev86.upx.com
unknown magic: ./hello_dev86.com
pe: ./hello_mingw64.exe
Parse error: ./hello_djgpp.exe => Invalid magic number: 0x0
PANICKED on hello_mingw32.upx.exe
Parse error: ./hello_owatcom_dos.exe => Invalid magic number: 0x20
PANICKED on hello_owatcom_win95.upx.exe
pe: ./hello_owatcom_nt.exe
Parse error: ./hello_owatcom_win386.exe => Invalid magic number: 0x0
Parse error: ./hello_djgpp.upx.exe => Invalid magic number: 0x0
Parse error: ./hello_owatcom_dos4gnz.exe => Invalid magic number: 0x1
PANICKED on hello_mingw64.upx.exe
Parse error: ./hello_owatcom_os2.exe => Invalid magic number: 0x0
pe: ./hello_csharp_exe_arm.exe
pe: ./hello_csharp_exe_x64.exe
pe: ./hello_csharp_exe_x86.exe
elf: ./hello_gcc.x86_64
PANICKED on hello_owatcom_nt.upx.exe
Parse error: ./hello_pacific.upx.exe => requested range [309100590..309100594) from object of len 4527

Here's a backtrace which appears to represent all of the panics:

ssokolow@monolith test_exes [rusty-core] % RUST_BACKTRACE=1 ./pe-test/target/debug/goblin-test hello_owatcom_nt.upx.exe
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', /checkout/src/libcore/option.rs:329
stack backtrace:
   0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace
             at /checkout/src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::_print
             at /checkout/src/libstd/sys_common/backtrace.rs:71
   2: std::panicking::default_hook::{{closure}}
             at /checkout/src/libstd/sys_common/backtrace.rs:60
             at /checkout/src/libstd/panicking.rs:355
   3: std::panicking::default_hook
             at /checkout/src/libstd/panicking.rs:371
   4: std::panicking::rust_panic_with_hook
             at /checkout/src/libstd/panicking.rs:549
   5: std::panicking::begin_panic
             at /checkout/src/libstd/panicking.rs:511
   6: std::panicking::begin_panic_fmt
             at /checkout/src/libstd/panicking.rs:495
   7: rust_begin_unwind
             at /checkout/src/libstd/panicking.rs:471
   8: core::panicking::panic_fmt
             at /checkout/src/libcore/panicking.rs:69
   9: core::panicking::panic
             at /checkout/src/libcore/panicking.rs:49
  10: <core::option::Option<T>>::unwrap
             at /checkout/src/libcore/macros.rs:21
  11: goblin::pe::import::SyntheticImportDirectoryEntry::parse
             at /home/ssokolow/.cargo/registry/src/github.com-1ecc6299db9ec823/goblin-0.0.10/src/pe/import.rs:125
  12: goblin::pe::import::ImportData::parse
             at /home/ssokolow/.cargo/registry/src/github.com-1ecc6299db9ec823/goblin-0.0.10/src/pe/import.rs:158
  13: goblin::pe::PE::parse
             at /home/ssokolow/.cargo/registry/src/github.com-1ecc6299db9ec823/goblin-0.0.10/src/pe/mod.rs:80
  14: goblin::parse
             at /home/ssokolow/.cargo/registry/src/github.com-1ecc6299db9ec823/goblin-0.0.10/src/lib.rs:276
  15: goblin_test::run
             at ./pe-test/src/goblin.rs:17
  16: goblin_test::main
             at ./pe-test/src/goblin.rs:36
  17: __rust_maybe_catch_panic
             at /checkout/src/libpanic_unwind/lib.rs:98
  18: std::rt::lang_start
             at /checkout/src/libstd/panicking.rs:433
             at /checkout/src/libstd/panic.rs:361
             at /checkout/src/libstd/rt.rs:57
  19: main
  20: __libc_start_main
  21: <unknown>

While this renders it unsuitable for my project (the mere fact that Goblin is capable of dying at an unwrap (when the other PE parser I've tried so far simply used Result to indicate a parse failure) indicates that using it in my project would cause me more worry than simply writing my own MZ/NE/PE parser with Nom), I thought you'd want to know so you can fix the problem for others.

If you want to re-create my test binaries, the source materials are in the test_exes folder of ssokolow/game_launcher and build.sh contains instructions for the simplest, easiest way to install the requisite packages on a *buntu Linux 14.04 LTS machine like mine.

To reiterate what build.sh says, all compilers are optional, so producing just the binaries which caused panics here should only require apt-get install upx-ucl mingw-w64 and then downloading and unpacking OpenWatcom.

[m4b]

Hello and thanks for the bug report! Sorry this crate didn't work out for you

Pretty sure this is a dup of #28

Should be relatively straightforward to fix, just been busy. The panic is just because it unwraps an option, I believe it's one of the last remaining places that unwraps in a parsing scenario.

Unfortunately I definitely don't have time to generate the binaries.

From the log you posted though, I'm not sure what's going on with them, but it looks like the magic numbers are bad, or offsets in the binary are bad, which makes me wary that the binaries are bad; would have to see actual stuff tho

[m4b]

Oh, you're trying to parse binaries compressed with this? https://en.wikipedia.org/wiki/UPX

Yea, from what I understand, that modifies the on disk image, which is completely unsupported by goblin, probably forever. I would have to essentially write a UPX decompressor, which isn't really goblin's job

[ssokolow]

Oh, you're trying to parse binaries compressed with this? https://en.wikipedia.org/wiki/UPX

Yea, from what I understand, that modifies the on disk image, which is completely unsupported by goblin, probably forever. I would have to essentially write a UPX decompressor, which isn't really goblin's job

A UPX-compressed binary still has to present a certain minimum file structure to the OS to remain executable and leave the first icon resource uncompressed so Windows can display it.

My goal is simply to heuristically generate launcher menu entries from a recursive filesystem traversal. That means that I only need best-effort support of the following functionality:

• Assign DOS (MZ), Windows 3.1 (NE), and the various flavours of PE, to user-customizable default handler profiles (eg. DOSBox, DOSBox+Win311, 32-bit Wine, 64-bit Wine, Wine+qemu-user, Mono, etc.)
• Extract and display the first icon resource from NE and PE files (I'm expecting to have to write the NE-parsing code myself)
• Reading fields like ProductName, ProductVersion and FileDescription from the VERSIONINFO resource, if available.

(And given that I don't remember MZ having space for an icon resource while the extra PE structures are wasted bloat in a DOS-only binary, I'm assuming that the DOS (MZ) vs. Windows (PE) distinction should still be externally visible without having to speak UPX.)

Unfortunately I definitely don't have time to generate the binaries.

If you've got a place I can upload them to, I can offer you a zip of them.

From the log you posted though, I'm not sure what's going on with them, but it looks like the magic numbers are bad, or offsets in the binary are bad, which makes me wary that the binaries are bad; would have to see actual stuff tho

I don't have the time to boot up my Windows XP retro-PC to test them on genuine Windows right now, but Wine doesn't have a problem running all of the panicking Windows ones except the 64-bit one (which only fails because my default Wine prefix is 32-bit for greater compatibility with old games).

ssokolow@monolith test_exes [rusty-core] % wine hello_owatcom_windows.exe

ssokolow@monolith test_exes [rusty-core] % wine hello_mingw32.upx.exe
Hello World
ssokolow@monolith test_exes [rusty-core] % wine hello_owatcom_win95.upx.exe
Hello World
ssokolow@monolith test_exes [rusty-core] % wine hello_owatcom_nt.upx.exe 
Hello World

I also took the liberty of re-testing the DOS ones that gave requested range [309100590..309100594) from object of len 4527 errors:

ssokolow@monolith test_exes [rusty-core] % cp hello_owatcom_dos.upx.exe ~/.dosbox/owdosupx.exe          
ssokolow@monolith test_exes [rusty-core] % cp hello_pacific.upx.exe ~/.dosbox/pacupx.exe
ssokolow@monolith test_exes [rusty-core] % dosbox

(Every single binary except the hello_csharp_exe_... ones is built from the same hello.c file... and the C# ones are all built from the same hello.cs.)

[m4b]

I very much would like to see and test on these binaries, please.

If you've got a place I can upload them to, I can offer you a zip of them.

Is emailing a possibility - m4b dot github dot io at gmail dot com ? Also firefox added some 1GB upload thing. Otherwise I don't know how to internet very well :/

A UPX-compressed binary still has to present a certain minimum file structure to the OS to remain executable and leave the first icon resource uncompressed so Windows can display it.

Interesting; definitely need to see these binaries.

(And given that I don't remember MZ having space for an icon resource while the extra PE structures are wasted bloat in a DOS-only binary, I'm assuming that the DOS (MZ) vs. Windows (PE) distinction should still be externally visible without having to speak UPX.

IIRC, I thought PE binaries were dos binaries; every "pe" binary has a dos header, which alternatively has a pointer to the PE header if it's PE, and not, if just DOS. This is the entire optional header part of the parsing, so should be easy to parse a dos and recognize just a dos file, iuc

[m4b]

Actually a just checked code, and it assumes it's a PE binary by always assuming the PE pointer is valid; porting to parse DOS files should work without much effort

[ssokolow]

Is emailing a possibility - [email address redacted] ?

I'm doubtful. When I had to send a project to my professor, I wound up getting fed up trying to outwit GMail's "nothing which might be virus-like" detector and just resorting to the release upload functionality on a private BitBucket repo.

Give me a sec and I'll see if I can remember how to get Dropbox to generate a share URL. (I haven't touched it since they killed off the Public folder functionality which meant I didn't need to either use the Nautilus file manager or log into their website to construct a valid share URL for something new.)

IIRC, I thought PE binaries were dos binaries; every "pe" binary has a dos header, which alternatively has a pointer to the PE header if it's PE, and not, if just DOS. This is the entire optional header part of the parsing, so should be easy to parse a dos and recognize just a dos file, iuc

Yes. All EXEs must have the MZ header, even if the application can't run under DOS. Thus the "You've gotta run me under Windows" stub application.

(Though the more elegant applications like the QEMM97 installer would have both a fully-functional DOS application and a fully-functional Windows application within the same EXE file, similar to how ISO9660 images can contain both an ISO9660 filesystem for DOS and an HFS filesystem for MacOS and they can share as many or as few files as desired.)

This delphiDabbler article has probably the best explanation I've seen for how the different Microsoft EXE formats relate to each other and how to tell them apart and parse them (including a flowchart).

[ssokolow]

Here. I can't guarantee this'll be up until the end of time, but it's only half a megabyte, so I'll leave it in my Dropbox until some point in the indefinite future when I've forgotten why it was there and delete it.

https://www.dropbox.com/s/cxrgj4eyfkb69n8/test_exes.zip?dl=0

It contains hello.c, hello.cs, build.sh, and the 30 different binaries built from that combination of input files (2 ELF, 2 .com, and 26 .exe), each representing a different compiler output configuration that /usr/bin/file produces distinct output for, plus its UPXed form, if UPX supports it.

hello_pacific.exe:            MS-DOS executable
hello_pacific.upx.exe:        MS-DOS executable, MZ for MS-DOS

hello_djgpp.exe:              MS-DOS executable, COFF for MS-DOS, DJGPP go32 DOS extender
hello_djgpp.upx.exe:          MS-DOS executable, COFF for MS-DOS, DJGPP go32 DOS extender, UPX compressed
hello_djgpp.upx.coff.exe:     80386 COFF executable

hello_owatcom_com.com:        data
hello_owatcom_com.upx.com:    FREE-DOS executable (COM), UPX compressed
hello_owatcom_dos.exe:        MS-DOS executable, MZ for MS-DOS
hello_owatcom_dos.upx.exe:    MS-DOS executable, MZ for MS-DOS
hello_owatcom_dos4g.exe:      MS-DOS executable, LE executable
hello_owatcom_dos4g.upx.exe:  MS-DOS executable, LE executable, UPX compressed
hello_owatcom_dos4gnz.exe:    MS-DOS executable, LX for OS/2 (console) i80386
hello_owatcom_os2.exe:        MS-DOS executable, NE for OS/2 1.x
hello_owatcom_os2v2.exe:      MS-DOS executable, LX for OS/2 (console) i80386
hello_owatcom_windows.exe:    MS-DOS executable, NE for MS Windows 3.x
hello_owatcom_win386.exe:     MS-DOS executable, NE for MS Windows 3.x
hello_owatcom_win95.exe:      PE32 executable (GUI) Intel 80386, for MS Windows
hello_owatcom_win95.upx.exe:  PE32 executable (GUI) Intel 80386, for MS Windows, UPX compressed
hello_owatcom_nt.exe:         PE32 executable (console) Intel 80386, for MS Windows
hello_owatcom_nt.upx.exe:     PE32 executable (console) Intel 80386, for MS Windows, UPX compressed

hello_mingw32.exe:            PE32 executable (console) Intel 80386, for MS Windows
hello_mingw32.upx.exe:        PE32 executable (console) Intel 80386, for MS Windows, UPX compressed
hello_mingw64.exe:            PE32+ executable (console) x86-64, for MS Windows
hello_mingw64.upx.exe:        PE32+ executable (console) x86-64, for MS Windows

hello_csharp_exe_x86.exe:     PE32 executable (console) Intel 80386 Mono/.Net assembly, for MS Windows
hello_csharp_exe_x64.exe:     PE32+ executable (console) x86-64 Mono/.Net assembly, for MS Windows
hello_csharp_exe_itanium.exe: PE32+ executable (console) Intel Itanium Mono/.Net assembly, for MS Windows
hello_csharp_exe_arm.exe:     PE32 executable (console) ARMv7 Thumb Mono/.Net assembly, for MS Windows

hello_gcc.x86:                ELF 32-bit LSB  executable, Intel 80386, version 1 (SYSV), dynamically linked (uses shared libs), for GNU/Linux 2.6.24, BuildID[sha1]=1a576ed0b6645df210b4c62a3baa684223e68d52, not stripped
hello_gcc.x86_64:             ELF 64-bit LSB  executable, x86-64, version 1 (SYSV), dynamically linked (uses shared libs), for GNU/Linux 2.6.24, BuildID[sha1]=56511d90294ea3d2632771c3f9a758c3085009b6, not stripped

[m4b]

@ssokolow Awesome, got it! Thanks for much for uploading it; investigating the binaries now, in particular: hello_mingw64.upx.exe

Seems the KERNEL32 import entry rva can't be transformed into an offset into the binary:

DEBUG:goblin::pe::import: ImportDirectoryEntry {
    import_lookup_table_rva: 0,
    time_date_stamp: 0,
    forwarder_chain: 0,
    name_rva: 151684,
    import_address_table_rva: 151612
}
Malformed entity: Cannot map import_lookup_table_rva 0x2503c into offset for KERNEL32.DLL

Now I need to figure out why

[ssokolow]

Be aware that hello_mingw64.upx.exe is the one EXE that I didn't reconfirm just before sending that, because of the aforementioned detail that my default Wine prefix is 32-bit.

Give me a few minutes to figure out which of my PlayOnLinux prefixes are 64-bit and I'll pop open a console in one of them and confirm it there.

[ssokolow]

OK, confirmed. Both hello_mingw64.exe and hello_mingw64.upx.exe work perfectly in a 64-bit WINEPREFIX.

(Correction, one of the two EXEs.)

[m4b]

It's ok, I think I fixed it. Going to push now, onto master, and test some more.

The KERNEL32 doesn't have an import lookup table, only an address table. I believe this means it requests an import from an absolute address from kernel32 iirc; since iirc windows builds all system binaries with absolute values
of functions precomputed (i.e., basically nothing is PIC, everything is absolutely relocated for system binaries), this is technically legal.

And this is not a dup of #28

[m4b]

I think this commit fixes the issue, which was that import lookup tables are essentially optional. @ssokolow would you mind verifying on your end? Or if not that's cool too, otherwise will close, thanks so much for the info and files.

I believe the magic errors are due to not properly parsing just a DOS file, which would be a simple PR iiuc.

I'm not sure about the offset errors coming from the other binaries though, that's unusual

[ssokolow]

I've never looked into how to reference a git repo from Cargo.toml and I'm getting sleepy, so I'll have to verify tomorrow.

[m4b]

NP, no rush, I do think it's been resolved though.

(And for the record it would be:

[dependencies]
goblin = { git = "https://github.com/m4b/goblin" }

and then you might need to do cargo update -p goblin if their are troubles, not sure what other deps you have.

I need to remove map the unwraps to ok_or in exports, and then i believe PE is completely panic free, fwiw.

[ssokolow]

I think this commit fixes the issue, which was that import lookup tables are essentially optional. @ssokolow would you mind verifying on your end?

OK, I've just tested commit bd68660 and here's what I found:

1. It did fix the panics.

2. Something broke the ELF support since 0.0.10:

   Parse error: ./hello_gcc.x86 => Malformed entity: Strtable size (594) + offset (6708) is out of bounds
   Parse error: ./hello_gcc.x86_64 => Malformed entity: Strtable size (569) + offset (7952) is out of bounds
   

   Speaking of which, feel free to incorporate those files I sent you into your test suite if you want.

   I don't think a "Hello World" implementation is novel enough to qualify for copyright, they're all built using compilers that impose no legal limitations on the resulting binaries (Namely, a bunch of GCC variants, Open Watcom C/C++, and the Freeware release of Pacific C that Hi-Tech Software released after it became obsolete), and, to be thorough:

   I hereby give you permission to use the files I sent to you within text_exes.zip under whatever license you wish. (If you like, I can be more formal or enumerate each individual file.)

I need to remove map the unwraps to ok_or in exports, and then i believe PE is completely panic free, fwiw.

That line was a bit difficult to parse, but I assume you meant goblin/src/pe/export.rs and goblin/src/mach/exports.rs?

I ran a quick ripgrep search...

rg -g '!tests' -g '!examples' 'unwrap|expect[_\(]|panic|unimplemented!|unreachable!|\[\d+\]'

...to look for low-hanging fruit and then tossed out the lines which were in #[test] functions or commented out.

There are still a few other places where only human perfection during refactoring is protecting against denial-of-service attacks via intentionally malformed EXEs:

goblin/src/strtab.rs
50:            let string = self.get(i).unwrap()?;
82:    /// **NB**: this will panic if the underlying bytes are not valid utf8, or the offset is invalid
85:        get_str(offset, &self.bytes, self.delim).unwrap()

goblin/src/pe/mod.rs
4:// TODO: panics with unwrap on None for apisetschema.dll, fhuxgraphics.dll and some others

goblin/src/pe/export.rs
70:        let export_offset = utils::find_offset(export_rva, sections).unwrap();
76:        let mut name_pointer_table_offset = &mut utils::find_offset(export_directory_table.name_pointer_rva as usize, sections).unwrap();
82:        let mut export_ordinal_table_offset = &mut utils::find_offset(export_directory_table.ordinal_table_rva as usize, sections).unwrap();
88:        let export_address_table_offset = utils::find_offset(export_directory_table.export_address_table_rva as usize, sections).unwrap();
101:        let name_offset = utils::find_offset(export_directory_table.name_rva as usize, sections).unwrap();
143:                    match rest[0] {
198:        let name_offset = utils::find_offset(ptr as usize, sections).unwrap();
210:                    let offset = utils::find_offset(rva, sections).unwrap();
216:                    let offset = utils::find_offset(rva, sections).unwrap();

goblin/src/archive/mod.rs
331:                Some(self.extended_names.get(name).unwrap())

goblin/src/elf/program_header.rs
307:                phdrs.copy_from_bytes(bytes).expect("buffer is too short for given number of entries");

goblin/src/elf/sym.rs
404:                    bytes.pwrite_with(sym, 0, le).unwrap();
408:                    bytes.pwrite_with(sym, 0, le).unwrap();

goblin/src/elf/reloc.rs
379:            bytes.pwrite_with(self, 0, ctx).unwrap();

goblin/src/elf/header.rs
48:                // but it can still panic due to invalid alignment.
49:                plain::from_bytes(bytes).unwrap()
337:                    bytes.pwrite_with(header32::Header::from(self), 0, ctx.le).unwrap()
340:                    bytes.pwrite_with(header64::Header::from(self), 0, ctx.le).unwrap()
539:                let header: Header = crt1.pread(0).unwrap();
543:                bytes.pwrite(header, 0).unwrap();
544:                let header2: Header = bytes.pread(0).unwrap();
555:                let header: Header = crt1.pread(0).unwrap();
560:                bytes.pwrite(header_, 0).unwrap();
561:                let header2: Header = bytes.pread(0).unwrap();
567:                bytes.pwrite(header, 0).unwrap();

goblin/src/elf/section_header.rs
322:                shdrs.copy_from_bytes(bytes).expect("buffer is too short for given number of entries");
522:                    bytes.pwrite_with(shdr, 0, le).unwrap();
526:                    bytes.pwrite_with(shdr, 0, le).unwrap();

goblin/src/mach/mod.rs
285:            .field("arches",  &self.arches().unwrap())

goblin/src/mach/load_command.rs
1390:            .field("sectname", &self.name().unwrap())
1391:            .field("segname",  &self.segname().unwrap())
1535:            .field("segname", &self.segname.pread::<&str>(0).unwrap())
1545:            .field("sections", &self.sections().unwrap())

goblin/src/mach/header.rs
205:        plain::from_bytes(bytes).unwrap()
257:        plain::from_bytes(bytes).unwrap()

goblin/src/mach/fat.rs
38:        let magic = bytes.gread_with(&mut offset, scroll::BE).unwrap();
39:        let nfat_arch = bytes.gread_with(&mut offset, scroll::BE).unwrap();

goblin/src/mach/exports.rs
272:        let exports = trie.exports(&libs).unwrap();

[m4b]

Thanks for the list!

So I need to check but most of those are acceptable unwraps, e.g. in debug printers or implementations that state they are allowed to panic.

The used in PE export are not, specifically unwrapping the option. Those will be easy to fix.

As for binaries, thank you! I don't like committing binaries into goblin but I might make a goblin_binaries repo and have it as a submodule for optional tests

[ssokolow]

Thanks for the list!

No problem. :)

I just hope that some day, someone with more time than I have decides to implement my idea for a "find all non-whitelisted panics" static analyzer to do that sort of thing properly.

in debug printers

Not ideal, but OK. (It's nice to be able to use such things in your logging or error reporting facilities without having to wrap them in a panic-catcher. Having your handler for unexpected errors unexpectedly error is not nice.)

or implementations that state they are allowed to panic.

I hope you mean impls in core or std like core::ops::Index and that there are non-panicking alternatives implemented.

Introducing new "never use this on untrusted input" API surface without good reason never sits well with me.

As for binaries, thank you! I don't like committing binaries into goblin but I might make a goblin_binaries repo and have it as a submodule for optional tests

Again, happy to help. Eventually, when I have time to write a GUI for easily breaking apart Git repos with full history preserved, I'm thinking I might offer up a repo for everyone which just contains the most generally useful collection of test binaries I can think to generate.

[ssokolow]

Thanks. Once I have time to go through and make a list of the functions to avoid calling to ensure panic-safety, including in my debug logging, I think this is what I'll wind up using in my project.

[ssokolow]

Just for the record, I hadn't realized how much unsafe code is in Goblin. Given that I doubt my ability to audit it, I'm back to considering writing my own PE/NE/LE/MZ parser using Nom.

[m4b]

The parser uses no unsafe code except in PE array accesses which are easily verified to be safe