PE: unable to parse imports with a null directory size

[daladim]

Some valid PE files may declare that their import directory has a null size (but a valid location).
Such files are valid, and can run fine in Windows, which actually links to the required imports (declared at the valid location).
Such an example is the PE101 simple.exe (that is described here and that can be downloaded here via pe101.zip)

The declared size of the imports directory is ignored by the windows loader, since it is actually useless. The import data in the PE file (pointed to by the valid location) is a null-terminated list, that can be parsed without the knowledge of its size.

Expected behaviour

PeFile::imports() would return a valid vector of Imports.
For instance, that's what the Python pefile does.

Actual behaviour

PeFile::imports() returns Ok([]). This is due to self.data_directory(pe::IMAGE_DIRECTORY_ENTRY_IMPORT) being None (data_directory checks that the declared length of the directory is not 0).

How to solve it?

I'm not sure. We probably should forcibly parse the imports structure and ignore the declared size. On the other hand, this is a borderline case (the PE file is almost-but-not-totally valid), and I'm not sure what the philosophy of object is regarding these cases

[daladim]

I've made an MR because that was not a big change, but I'd like your opinion regarding how acceptable this is.

[philipc]

Handling malware and hand crafted files is a can of worms, and until now this crate has only cared about binaries from common toolchains. For some cases it may be better to return an error, rather than trying to duplicate the undocumented internals of the Windows loader. For this case, the change may be simple enough, but let me think about it. I assume for your use case you'd rather be able to parse the file fully, rather than getting an error?

[daladim]

Currently, we get Ok([]). I think it would make more sense to:

• at least get an Err
• get an Err, but provide a way to forcibly parse the import table anyway
• be able to choose between the two previous behaviours (e.g. have a imports(&self) that would return Err and a imports_forced(&self) that would return Ok([...]) (not sure this API would be very straightforward though)
• parse the list regardless of the declared size,

For my very use case, I'd prefer the latter (be able to parse the list of imports, without being even warned about an invalid size) because that would make my life easier :D

All the more as the logic in object already works (it walks the list of imports until it finds a null item).

[daladim]

But I understand that you could be reluctant to change object to try to mimic the undocumented behaviour of Windows loader, run-time, etc.

In that case, maybe we could create a cargo feature(say, --features=lenient or --feature=force), that would tweak many behaviours of object to be more, well, lenient, and that could be enabled when trying to parse potential malware.

This (ignoring invalid directory sizes when they have a valid pointer location) would be the first behaviour that would be tweaked, but maybe more would be added in the future

[philipc]

at least get an Err

I think that can be fixed with just this change from your PR?

-            .filter(|d| d.size.get(LE) != 0)
+            .filter(|d| d.virtual_address.get(LE) != 0)

get an Err, provide a way to forcibly parse the import table anyway

This can always be done using the lower level API. If it's missing functions to do this then we should add them. And this may be desirable for your use anyway (e.g. it was for another user in #324). The unified API makes it harder to support features that are unique to only one file format.

be able to choose between the two previous behaviours

This seems the same as the second option to me? i.e. the second option already includes the behaviour of the first option

parse the list regardless of the declared size

This is still an option, but I need to investigate further.