PE: parser hangs while processing corkami's `manyimportsW7.exe`

[prettyroseslover]

Current version of goblin hangs on parsing corkami's manyimportsW7.exe , which is described as:

W7-only binary use the TLS AddressOfIndex trick to clean its imports. On disk, the import table is full of bogus descriptors, which will be ignored on loading

Snippet to reproduce this behavior:

use std::fs;

use goblin::pe::options::ParseOptions;
use goblin::pe::options::ParseMode;
use goblin::pe;

fn main() {
    let path = "manyimportsW7.exe";
    let content = fs::read(path).unwrap();
    let mut parse_options = ParseOptions::default();
    parse_options.parse_mode = ParseMode::Permissive;

    let pe_file = pe::PE::parse_with_opts(&content, &parse_options).unwrap();
}

Other PE parsers, for example pedump catch this trick:

[!] catched the 'imports terminator in TLS trick'

Taking into account, this sample is valid and somewhat popular with the community (as it is supported by other PE parsers, etc), it would be nice to catch fake imports usage in order not to load them all.

[kkent030315]

Hi! Thank you for bringing that neat technique to light. Yea, I can see what's happening and the appropriate fix for this real quick. I actually didn't know this trick.

Indeed, the pedump patch seems too tricky and inappropriate for me. We can do better here. The appropriate fix is to cap the size of iteration to the size of the data directory.

In other words, we should always only feed the bytes of the data directory into the parser.

[prettyroseslover]

@kkent030315 wow! thanks a lot for such a quick response!

[philipc]

I'm not sure that capping to the size of the data directory is the correct fix for this. The Windows loader doesn't use that size. So for corkami's manyimportsW7.exe, the Windows loader will actually parse a few imports (up until the TLS address), but with #524, goblin will return 0.

Additionally, I haven't tested but I think it would be possible to change manyimportsW7.exe to specify the size, and then we'll be back to the same problem: it's not that the import table is overflowing the data referenced by the data directory entry, it's that the data the entry references is very large, and Windows ignores most of it due to the TLS address zeroing.

It's a bit of a design question: does goblin intend to support malware analysis? If so, then you need to support parsing things that the Windows loader accepts, rather than going by the PE spec.

[kkent030315]

Hey @philipc, thanks for the comment. Yes I think you are right here, I was able to reproduce that a size cap alone is not sufficient.

Here is the binary to reproduce:
manyimportsW7_bigsize.zip

cargo run --example rdr -- "path/to/manyimportsW7_bigsize.exe"

Besides the goal of the patch is not to align with the behavior of Windows loader (goblin is static binary parser, not the one that mapped really, there is an option to not parse rva though), I do agree with the design question. goblin is not really well designed for malware analysis (i.e. it is not failure tolerant).

This is my thoughts:

• We can technically check that TLS.AddressOfIndex falls within the import directory range but I do not really like that approach.
• We can also introduce (configurable) hard cap on how many import entries we can parse at a time. This would be more ideally.

I do like neither of one, it should be more of an approach that addresses the design.

@philipc Is there anything you'd think of appropriate?

[philipc]

Deciding the design direction is more suitable for those who use goblin and want to parse these binaries. At the top of this issue:

Taking into account, this sample is valid and somewhat popular with the community (as it is supported by other PE parsers, etc), it would be nice to catch fake imports usage in order not to load them all.

I'm uncertain that the sample is valid: yes, it loads, but if I had my way Windows would stop loading it, security scanners would automatically quarantine it, and binaries like this would become curiosities kept in a museum. But that's not how the world is currently, and there probably are reasons to want to parse binaries like this.

As for practical things you can do: if you want to support this binary, you have to match what the Windows loader does with the TLS address.

If you don't want to support this binary, then it becomes a question of mitigating denial of service attacks, and a configurable hard cap in the parse options sounds appropriate for that. Another option would be to palm the problem off onto the caller by returning an iterator instead of parsing it all up front, and then they can decide when to stop iterating, but that's probably too much of a breaking change, and doesn't really solve the problem. Maybe there's some cheap validation we can use to determine when the imports are garbage and stop parsing, but I haven't thought about it much. I guess they all need unique valid names, but checking that probably isn't cheap.

[prettyroseslover]

I dare say this sample should be considered valid (for Win7), as corkami's collection is

a set of handmade files showing the various possibilities of the Portable Executable format, under Windows.
All these files are clean and working. However, they are hand-made and push the PE file format to its limits, so they might be detected as malicious or as corrupted files.

Even though some vendors on Virus Total consider this file to be malicious, it is (in fact) not. However, the bare minimum support will be mitigating the denial of service of goblin.

However, aforementioned pedump will show all the valid imports:

=== IMPORTS ===

MODULE_NAME      HINT   ORD  FUNCTION_NAME
kernel32.dll        0        ExitProcess
msvcrt.dll          0        printf

pefile will produce something similar, though throwing a couple of errors along the way:

kernel32.dll.ExitProcess Hint[0]
msvcrt.dll.printf Hint[0]

The hanging of PE-bear on this sample was also fixed.

[philipc]

You need to distinguish between valid, accepted by the Windows loader, and malicious. corkami is using a definition of clean = not malicious, working = accepted by the Windows loader. Clean and working still does not mean valid, and while determining what is valid is subjective, I think TLS address zeroing tricks are clearly not valid.

[kkent030315]

@philipc Be respectful to everyone.

[philipc]

Apologies. I know I am blunt and opinionated but I meant no disrespect. We may disagree on validity semantics, but the denial of service is a real issue, and thank you @prettyroseslover for reporting this issue.

Investigating further, the reason this results in a denial of service is because the descriptor table and thunk tables overlap, resulting in runtime that is quadratic in the size of the table. So another option is to check that the tables are not overlapping. (This is the fix I have implemented for another crate gimli-rs/object#871 Edit: this implementation is flawed)

[philipc]

While checking for overlapping tables is enough for manyimportsW7.exe, a related problem is shared IMAGE_IMPORT_BY_NAME entries: many imports sharing a long name will also result in quadratic resource usage. I don't have a good solution for that yet. Might need to add a limit (e.g max sum of name lengths), possibly automatically scaled based on the size of the file.

[kkent030315]

@philipc thanks ❤️ We are here to make it better. Let's be more constructive. I always appreciate your feedback, which covers my flaws (as always!). We just need to be respectful each other, we don't need to disagree on each comments.

[kkent030315]

Honestly I do not know what is the appropriate (aka generic) fix for this. The maximum iteration cap might work, but I don't really like that. But yeah, as well as the max sum of name lengths, it might be the solution. Alternatively we can maybe... assume the PE as mapped, so we never miss those gaps?

[philipc]

Alternatively we can maybe... assume the PE as mapped, so we never miss those gaps?

Can you explain more what you mean?

This is a hard problem space. There's lots of possible denial of service attacks of this nature across all of the file formats, and I don't think goblin handles many of them yet. It does have some handling for overlap in string tables (#275) and PE resource cycles (#499), both of which are variants of the problems we are seeing here.

The generic fixes that I know of to handle this type of problem are either limit the amount of work done, or use a visited-set to detect overlap or cycles.