Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PE: Use OFTs for resolving imports without FTs #430

Merged
merged 2 commits into from
Nov 18, 2024
Merged

PE: Use OFTs for resolving imports without FTs #430

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cfbb2c1
PE: Use OFTs for resolving imports without FTs
kkent030315 Oct 29, 2024
7ed559a
Simplify malformed expression
kkent030315 Nov 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 23 additions & 12 deletions src/pe/import.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,18 +263,29 @@ impl<'a> SyntheticImportDirectoryEntry<'a> {
}
};

let import_address_table_offset = &mut utils::find_offset(
import_directory_entry.import_address_table_rva as usize,
sections,
file_alignment,
opts,
)
.ok_or_else(|| {
error::Error::Malformed(format!(
"Cannot map import_address_table_rva {:#x} into offset for {}",
import_directory_entry.import_address_table_rva, name
))
})?;
let rva = match import_directory_entry.import_address_table_rva.is_zero() {
true => import_directory_entry.import_lookup_table_rva,
false => import_directory_entry.import_address_table_rva,
};
Comment on lines +266 to +269
Copy link
Contributor Author

@kkent030315 kkent030315 Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
    });

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor Author

@kkent030315 kkent030315 Nov 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +266 to +269
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


let import_address_table_offset =
&mut utils::find_offset(rva as usize, sections, file_alignment, opts).ok_or_else(
|| {
error::Error::Malformed(
if import_directory_entry.import_address_table_rva.is_zero() {
format!(
"Cannot map import_lookup_table_rva {:#x} into offset for {}",
rva, name
)
} else {
format!(
"Cannot map import_address_table_rva {:#x} into offset for {}",
rva, name
)
},
)
kkent030315 marked this conversation as resolved.
Show resolved Hide resolved
},
)?;
let mut import_address_table = Vec::new();
loop {
let import_address = bytes
Expand Down
Loading