PE: Use OFTs for resolving imports without FTs

[kkent030315]

• Addresses issue PE: handle empty import address table #371.

Sample: sample.zip

OFTs (Original First Thunk, aka lookup table) are kept as raw when mapped into virtual memory;
FTs (First Thunk, aka address table) are rewritten to the absolute address where the import function is located when mapped into virtual memory.

Static Import parsing would work fine on both cases, so use OFTs if FTs are zero, but keep FTs preffered.

[kkent030315]

I am pretty sure I should check if opts.resolve_rva is true in match-false bracket (otherwise raises malformed as well as how it does before) in order to use address table which is replaced with absolute address of import address when the image is mapped.

let rva = match import_directory_entry.import_address_table_rva.is_zero() {
    true => import_directory_entry.import_lookup_table_rva,
    false => match opts.resolve_rva {
        true => import_directory_entry.import_address_table_rva,
        false => import_directory_entry.import_lookup_table_rva,
    },
};

aka

let rva = import_directory_entry
    .import_address_table_rva
    .is_zero()
    .then_some(import_directory_entry.import_lookup_table_rva)
    .unwrap_or_else(|| {
        opts.resolve_rva
            .then_some(import_directory_entry.import_address_table_rva)
            .unwrap_or(import_directory_entry.import_lookup_table_rva)
    });

[m4b]

the second is somewhat harder to read at first, but i think I like it better :)

[m4b]

when you say you're pretty sure, you mean that this PR needs that amendment? And are you worried about some kind of regression in doing this (i.e., less strict, problems further down the parsing pipeline or how come you didn't add it to this PR?)

[kkent030315]

If I am correct that resolve_rva opt is meant to be solely used for the mapped image (i.e., raw memory dumps, minidumps etc):

Yes, but what I am not sure is that we already make use of address table right now which is rewritten to the 64/32-bit absolute address when mapped into the memory. So If this is not to be the problem for this long in goblin, then I see no problems there to use lookup table.

I guess this is some kind of non-well implementation.. we should look for lookup tables rather than address tables.

• AddressTable/FTs: may not be valid in memory dumps.
  • Unlike PLT/GOT in ELF, In x86-64 arch and PE64, rel32 RVA of import calls like call cs:__imp_MessageBoxA are actually points to the FTs.
• LookupTable/OFTs: always correct regardless of mapped images (i.e., for dependency walks)

I came to the conclusion that we do not need this optimization at this time.

[m4b]

I would alter with the suggestion, otherwise LGTM, assuming you want to make that additional change you mentioned

[m4b]

when you say you're pretty sure, you mean that this PR needs that amendment? And are you worried about some kind of regression in doing this (i.e., less strict, problems further down the parsing pipeline or how come you didn't add it to this PR?)

[kkent030315]

@m4b Thank you for the review! This PR is ready to go.

[m4b]

just fyi matching on true/false in this case is somewhat unidiomatic, generally if condition { } else { } is preferred

[m4b]

Thanks for this! I would probably return the target_rva name as a tuple in the is_zero lookup for slightly less code/redundancy.

I don't fully understand semantically why we'd use the import_lookup_table when the address_table_rva is 0 for resolving the import_address_table offset but:

1. if it fixes an open bug
2. it doesn't regress any existing resolution for users then this seems ok (i don't think it can regress at the moment because if that value is zero it wouldn't get resolved iiuc, which would mean we'd return an error "cannot map import address table" in that case, whereas now we'll try the lookup table)

so the only real question i'd have is whether we'd return a kind of false positive in the case it's 0, and we return incorrectly an import_address_table_offset where we shouldn't?

[kkent030315]

@m4b I'm the specialist in PE and I'd confident that it's totally fine to use lookup table in static binary parsers like goblin.

Yet the code behaves semantically what it was before, but only make exceptions such as the issue describes.

As I originally posted my request to provide the original binary in question, however that sample is 99%―mostly be specially crafted (packed) with private PE packers that no one else knows. First thunk (address table) is zero, means that it semantically does nothing other than Windows loader loads that dependency at execution but does not resolves the symbol. That is what makes me believe it is specially crafted and not for the usual cases where proprietary linkers do.

Anyways, thanks for the heads up!