Missing import (IAT) information for Windows PE

[elast0ny]

In the struct

Struct goblin::pe::import::SyntheticImportDirectoryEntry { ... }

The field import_address_table does not point to the IAT but instead points to the Import Name Table.

[elast0ny]

The import_directory_entry.import_address_table_rva points to the correct offset of the first entry in the IAT but it somehow doesnt make it to the computed import_address_table.

Will dig into the code to see why this is hapenning

[m4b]

Was this ever resolved ? Is it still an issue ?

[elast0ny]

Hi, it appears that this is still an issue unless im computing these values wrong...

Here's an example program, it's output and a screenshot of the values parsed by Ghidra :

use goblin::pe::import::SyntheticImportLookupTableEntry;
use goblin::*;

use std::fs::File;
use std::io::Read;
use std::mem::size_of_val;

fn main() {
    let mut fd = File::open("target/debug/goblin_test.exe").unwrap();
    let mut buffer = Vec::new();
    fd.read_to_end(&mut buffer).unwrap();

    let win_pe = match Object::parse(&buffer).unwrap() {
        Object::PE(pe) => pe,
        _ => {
            println!("Unhandle bin type");
            return;
        }
    };

    let import_data = win_pe.import_data.unwrap().import_data;

    for cur_import in &import_data {
        if cur_import.name != "KERNEL32.dll" {
            continue;
        }

        let abs_ilt_base: usize =
            win_pe.image_base + cur_import.import_directory_entry.import_lookup_table_rva as usize;
        let abs_iat_base: usize =
            win_pe.image_base + cur_import.import_directory_entry.import_address_table_rva as usize;

        println!(
            "{:?} ILT : 0x{:X} IAT : 0x{:X}",
            cur_import.name, abs_ilt_base, abs_iat_base
        );

        for (i, lookup_table) in cur_import
            .import_lookup_table
            .as_ref()
            .unwrap()
            .iter()
            .enumerate()
        {
            match lookup_table {
                SyntheticImportLookupTableEntry::OrdinalNumber(ordinal) => {
                    print!("\tOrdinal {} ", ordinal);
                    continue;
                }
                SyntheticImportLookupTableEntry::HintNameTableRVA((_, v)) => {
                    print!("\t{} ", v.name)
                }
            };

            // This prints values that are in the import_lookup_table_rva
            println!(
                "0x{:X} (Expected 0x{:X})",
                cur_import.import_address_table[i] as usize + win_pe.image_base,
                abs_iat_base + i * size_of_val(&cur_import.import_address_table[i])
            );
        }
    }
}

[elast0ny]

It looks likes the offset is calculated differently in two locations : (offset here points to the address I expected) https://github.com/m4b/goblin/blob/cfa098a528409847c568216e04f764b792f8425a/src/pe/import.rs#L228-L231

and : (The values in import_address_table don't match the offset value above) https://github.com/m4b/goblin/blob/cfa098a528409847c568216e04f764b792f8425a/src/pe/import.rs#L167-L172

[m4b]

Can anyone help out elast0ny or clarify is this a bug we need to fix? @jan-auer, @willglynn @philipc maybe?

[Maimonator]

Hey! @m4b This still seems to be an issue, I suspect that it's because when parsing for PE the FirstThunk pointer is treated as the pointer for the IAT, even though the loader overrides this with the correct addresses. When the PE is on the disk the FirstThunk actually points to the ILT.

You can see that on CFF Explorer both entries for OFT and FT have the same value.

And as written here, and in some other sources that I found

[Maimonator]

Also I fixed this by using the IAT in the import directory entry as base and adding a counter, in the following code: https://github.com/m4b/goblin/blob/master/src/pe/import.rs#L279-L288

This is the code that solves it. Would love to PR this if it seems reasonable, tested it for both 64bit and 32bit

        let mut counter = 0;
        loop {
            let import_address = bytes
                .gread_with::<T>(import_address_table_offset, LE)?
                .into();
            if import_address == 0 {
                break;
            } else {
                import_address_table.push((import_directory_entry.import_address_table_rva as u64) + ((counter * std::mem::size_of::<T>()) as u64));
                counter += 1;
            }
        }

[m4b]

hi @Maimonator thank you for investigating! if you want to push your patch, we can review (a diff with the changes is easier to read); even better is if we add some tests that exhibit the failing problem, and how this fixes it/allows this new test to pass?

[philipc]

This is the code that solves it.

That's changing the code to return the addresses of the IAT entries instead of their values. I don't think that's the correct thing to do.

As I understand the problem, the values of the IAT entries are generally not useful because they are overwritten by the loader, but that doesn't mean that returning the values is incorrect; we're just returning the information that is in the file.

The solution is to also return the information that you do want. Your change gives the RVA of each entry, but that's a bit wasteful. Instead you simply need the RVA and length of the IAT. The length is the same as the length of SyntheticImportDirectoryEntry::import_address_table. So you just need the RVA, for which we should add a new field SyntheticImportDirectoryEntry::import_address_table_rva that is a copy of ImportDirectoryEntry::import_address_table_rva.

[Maimonator]

I actually agree with your statement, the values of IAT entries could be useful for other things, and this is an extra functionality. Though I do think that it should be quite common to return the RVA for each function. Exposing SyntheticImportDirectoryEntry::import_address_table_rva is a good start. But maybe it would be helpful to additionally expose a function that returns import names and their rva. WDYT @philipc?

[philipc]

But maybe it would be helpful to additionally expose a function that returns import names and their rva.

This isn't something I'm familiar enough with to be able to understand how it is useful, sorry. My understanding is that these values actually aren't useful until the loader has set them, so I don't see how knowing their rva is going to help you. Are you using this to inspect the memory of a program that is already running? Either way, if it is useful to you, please open a PR with your suggested change.

[Maimonator]

The RVA is useful when disassembling or patching binaries. For example IDA knows this is a jmp to the function CloseHandle because the operand is CloseHandle's RVA. The loader didn't set them yet, but we know it will set it there. So we can already draw conclusions about what the code does, without running the code. Hopefully that was clear!

Also thanks for replying so fast, I'll open a PR soon. Thanks for the help and your comments!

[philipc]

Thanks for that explanation. Also my suggestion to add SyntheticImportDirectoryEntry::import_address_table_rva is wrong because it already exists at SyntheticImportDirectoryEntry::import_directory_entry.import_address_table_rva .

[philipc]

I've also noticed now that the import_lookup_table can have holes in it: https://github.com/m4b/goblin/blob/b43b93ed2243b75043d6fb1021ad9aec227df1f5/src/pe/import.rs#L144-L145 so calculating the RVAs may be inaccurate. That is, we do need to store them.

Something that is unclear to me is what happens when the ILT and the IAT have different initial values? Which one does the loader use when setting the IAT?

[Maimonator]

Wow nice find, I guess I'll have to fix it in my PR.

About your question the loader uses the ILT, and overrides the IAT.

To be honest I'm not quite sure about the specifics, I've tried some tests with a sample code. I basically changed the values of the IAT and ran the program to see if it still loaded properly. It seems to me that the value under FirstThunk can have any value just not 0, so the loader does treat this information my guess is for enumerating the FirstThunk Also when changing the hint to an invalid value the correct symbol was loaded, I did not try to replace a hint with another valid hint. My guess is that the loader didn't find the hint so it defaulted to comparing indices.

[philipc]

I've also noticed now that the import_lookup_table can have holes in it:

This shouldn't actually happen for valid files. It did previously happen, but that was due to a bug in the parsing. Maybe we should simply stop parsing imports if this does happen for a corrupted file, instead of leaving a hole?

[Maimonator]

Do you have an example file that triggers this behavior?

[philipc]

No, there was an example file in the original report #28, but we now parse that successfully.

[Maimonator]

I see, sorry for the long time for the reply. How do you want to go forward with this? I think both options are legit, I rather leaving a hole, as even if the imports are corrupted it doesn't mean that other information is also corrupted. But that's just my opinion.

[philipc]

My opinion is to keep things simple, especially for something that probably prevents the file from being executed, but I don't maintain this project. This also has the advantage that then the original problem in this issue can be solved by the user doing a simple calculation, e.g.

    pe.image_base
        + import_dir.import_directory_entry.import_address_table_rva as usize
        + i * if pe.is_64 { 8 } else { 4 };