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: fix address size incompatibility on 32-bit builds #441

Merged
merged 3 commits into from
Jan 5, 2025
Merged

PE: fix address size incompatibility on 32-bit builds #441

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
96d6cf8
PE: fix address size incompatibility on 32-bit builds
kkent030315 Dec 16, 2024
d12cc96
Fix doc that image base in PE actually a VA
kkent030315 Dec 16, 2024
01267a2
Merge remote-tracking branch 'upstream/master' into usize64
kkent030315 Dec 17, 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
57 changes: 51 additions & 6 deletions src/pe/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,10 @@ pub struct PE<'a> {
pub is_lib: bool,
/// Whether the binary is 64-bit (PE32+)
pub is_64: bool,
/// the entry point of the binary
pub entry: usize,
/// The binary's RVA, or image base - useful for computing virtual addreses
pub image_base: usize,
/// The entry point RVA of the binary
pub entry: u32,
/// The binary's VA, or image base - useful for computing virtual addreses
pub image_base: u64,
Copy link
Owner

Choose a reason for hiding this comment

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

is this the actual bug? (that we used a usize but on 32-bit systems, this can be u64?)

If yes, is it really required for entry to be fixed to u32? If there's a compelling reason (For consistency perhaps) then it seems fine, but this is a pretty big breaking change, but it may be worth it if it prevents future issues.

Copy link
Contributor Author

@kkent030315 kkent030315 Jan 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, this has been a serious hidden bug for long.

Fortunately, RVA calculations (i.e., TLS callbacks map) work fine even it overflows. (i.e., 0x140000000u64 overflows and trimmed into 0x40000000u32, then subtract the u32 RVA)

On the other hand, this is a significant correctness issue likewise whenever the consumer wants to use it for something like verification.

I guess this has been invisible for this long because no one really cares about 32-bit builds.

/// Data about any exported symbols in this binary (e.g., if it's a `dll`)
pub export_data: Option<export::ExportData<'a>>,
/// Data for any imported symbols, and from which `dll`, etc., in this binary
Expand Down Expand Up @@ -146,8 +146,8 @@ impl<'a> PE<'a> {
}
};

entry = optional_header.standard_fields.address_of_entry_point as usize;
image_base = optional_header.windows_fields.image_base as usize;
entry = optional_header.standard_fields.address_of_entry_point;
image_base = optional_header.windows_fields.image_base;
is_64 = optional_header.container()? == container::Container::Big;
debug!(
"entry {:#x} image_base {:#x} is_64: {}",
Expand Down Expand Up @@ -734,6 +734,44 @@ mod tests {
0x0, 0x0, 0x0, 0x45, 0x78, 0x69, 0x74, 0x50, 0x72, 0x6f, 0x63, 0x65, 0x73, 0x73, 0x0,
];

static HEADERONLY_EMPTY_PE64: &[u8; 512] = &[
0x4D, 0x5A, 0x78, 0x00, 0x01, 0x00, 0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x40, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x78, 0x00, 0x00, 0x00, 0x0E, 0x1F, 0xBA, 0x0E, 0x00, 0xB4, 0x09, 0xCD, 0x21, 0xB8, 0x01,
0x4C, 0xCD, 0x21, 0x54, 0x68, 0x69, 0x73, 0x20, 0x70, 0x72, 0x6F, 0x67, 0x72, 0x61, 0x6D,
0x20, 0x63, 0x61, 0x6E, 0x6E, 0x6F, 0x74, 0x20, 0x62, 0x65, 0x20, 0x72, 0x75, 0x6E, 0x20,
0x69, 0x6E, 0x20, 0x44, 0x4F, 0x53, 0x20, 0x6D, 0x6F, 0x64, 0x65, 0x2E, 0x24, 0x00, 0x00,
0x50, 0x45, 0x00, 0x00, 0x64, 0x86, 0x00, 0x00, 0x5D, 0x3F, 0xC8, 0x19, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0xF0, 0x00, 0x22, 0x00, 0x0B, 0x02, 0x0E, 0x00, 0x00, 0x02,
0x00, 0x00, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x20, 0x10, 0x00, 0x00, 0x00,
0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x40, 0x01, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, 0x00,
0x00, 0x02, 0x00, 0x00, 0x06, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x06, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x20, 0x00, 0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x03, 0x00, 0x60, 0x81, 0x00, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00,
];

#[test]
fn string_table_excludes_length() {
let coff = Coff::parse(&&COFF_FILE_SINGLE_STRING_IN_STRING_TABLE[..]).unwrap();
Expand Down Expand Up @@ -769,4 +807,11 @@ mod tests {
panic!("must not parse PE with invalid PE header");
}
}

#[test]
fn pe64_image_base_and_entry_nonoverflows() {
let binary = PE::parse(HEADERONLY_EMPTY_PE64).unwrap();
assert_eq!(binary.entry, 0x1020u32);
assert_eq!(binary.image_base, 0x140000000u64);
}
}
6 changes: 3 additions & 3 deletions src/pe/optional_header.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ pub struct StandardFields {
/// * For device drivers, this is the address of the initialization function.
///
/// The entry point function is optional for DLLs. When no entry point is present, this member is zero.
pub address_of_entry_point: u64,
pub address_of_entry_point: u32,
Copy link
Owner

Choose a reason for hiding this comment

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

this seems like the only actually required fix, but even that is somewhat unclear to me? everything else seems to push the usize <-> u64/u32 casting down into the pe::utils::find_offset, which I admit does seem more elegant.

However, there was a certain clarity of passing a usize into these parse functions since they are ultimately used to compute 32/64-bit addresses/offsets.

Could you elaborate more on why this gets downgraded to a u32 though?

Copy link
Contributor Author

@kkent030315 kkent030315 Jan 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Theoretically, this u64 is fine. However,

  1. Its 4 bytes half is always wasted because this is defined as u32 as per PE specification1.

  2. And as you mentioned, the AddressOfEntryPoint is always an RVA (u32) and consumers are eventually forced to make unnecessary downcast if it is defined as u64, one way or another.

Footnotes

  1. https://learn.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-image_optional_header64 ↩

/// A pointer to the beginning of the code section (.text), relative to the image base.
pub base_of_code: u64,
/// A pointer to the beginning of the data section (.data), relative to the image base. Absent in 64-bit PE32+.
Expand All @@ -139,7 +139,7 @@ impl From<StandardFields32> for StandardFields {
size_of_code: u64::from(fields.size_of_code),
size_of_initialized_data: u64::from(fields.size_of_initialized_data),
size_of_uninitialized_data: u64::from(fields.size_of_uninitialized_data),
address_of_entry_point: u64::from(fields.address_of_entry_point),
address_of_entry_point: fields.address_of_entry_point,
base_of_code: u64::from(fields.base_of_code),
base_of_data: fields.base_of_data,
}
Expand Down Expand Up @@ -171,7 +171,7 @@ impl From<StandardFields64> for StandardFields {
size_of_code: u64::from(fields.size_of_code),
size_of_initialized_data: u64::from(fields.size_of_initialized_data),
size_of_uninitialized_data: u64::from(fields.size_of_uninitialized_data),
address_of_entry_point: u64::from(fields.address_of_entry_point),
address_of_entry_point: fields.address_of_entry_point,
base_of_code: u64::from(fields.base_of_code),
base_of_data: 0,
}
Expand Down
34 changes: 17 additions & 17 deletions src/pe/tls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ impl ImageTlsDirectory {
impl<'a> TlsData<'a> {
pub fn parse<T: Sized>(
bytes: &'a [u8],
image_base: usize,
image_base: u64,
dd: &data_directories::DataDirectory,
sections: &[section_table::SectionTable],
file_alignment: u32,
Expand All @@ -183,7 +183,7 @@ impl<'a> TlsData<'a> {

pub fn parse_with_opts<T: Sized>(
bytes: &'a [u8],
image_base: usize,
image_base: u64,
dd: &data_directories::DataDirectory,
sections: &[section_table::SectionTable],
file_alignment: u32,
Expand All @@ -208,18 +208,18 @@ impl<'a> TlsData<'a> {
)));
}

if (itd.start_address_of_raw_data as usize) < image_base {
if itd.start_address_of_raw_data < image_base {
return Err(error::Error::Malformed(format!(
"tls start_address_of_raw_data ({:#x}) is less than image base ({:#x})",
itd.start_address_of_raw_data, image_base
)));
}

// VA to RVA
let rva = itd.start_address_of_raw_data as usize - image_base;
let rva = itd.start_address_of_raw_data - image_base;
let size = itd.end_address_of_raw_data - itd.start_address_of_raw_data;
let offset =
utils::find_offset(rva, sections, file_alignment, opts).ok_or_else(|| {
let offset = utils::find_offset(rva as usize, sections, file_alignment, opts)
.ok_or_else(|| {
error::Error::Malformed(format!(
"cannot map tls start_address_of_raw_data rva ({:#x}) into offset",
rva
Expand All @@ -230,32 +230,32 @@ impl<'a> TlsData<'a> {

// Parse the index if any
if itd.address_of_index != 0 {
if (itd.address_of_index as usize) < image_base {
if itd.address_of_index < image_base {
return Err(error::Error::Malformed(format!(
"tls address_of_index ({:#x}) is less than image base ({:#x})",
itd.address_of_index, image_base
)));
}

// VA to RVA
let rva = itd.address_of_index as usize - image_base;
let offset = utils::find_offset(rva, sections, file_alignment, opts);
let rva = itd.address_of_index - image_base;
let offset = utils::find_offset(rva as usize, sections, file_alignment, opts);
slot = offset.and_then(|x| bytes.pread_with::<u32>(x, scroll::LE).ok());
}

// Parse the callbacks if any
if itd.address_of_callbacks != 0 {
if (itd.address_of_callbacks as usize) < image_base {
if itd.address_of_callbacks < image_base {
return Err(error::Error::Malformed(format!(
"tls address_of_callbacks ({:#x}) is less than image base ({:#x})",
itd.address_of_callbacks, image_base
)));
}

// VA to RVA
let rva = itd.address_of_callbacks as usize - image_base;
let offset =
utils::find_offset(rva, sections, file_alignment, opts).ok_or_else(|| {
let rva = itd.address_of_callbacks - image_base;
let offset = utils::find_offset(rva as usize, sections, file_alignment, opts)
.ok_or_else(|| {
error::Error::Malformed(format!(
"cannot map tls address_of_callbacks rva ({:#x}) into offset",
rva
Expand All @@ -279,11 +279,11 @@ impl<'a> TlsData<'a> {
)));
}
// Each callback is an VA so convert it to RVA
// For x86 compatibility, `usize` is 32-bit, `u64` is 64-bit.
// Therefore upcast to u64 first, then downcast the whole var to u32.
let callback_rva = (callback - image_base as u64) as usize;
let callback_rva = callback - image_base;
// Check if the callback is in the image
if utils::find_offset(callback_rva, sections, file_alignment, opts).is_none() {
if utils::find_offset(callback_rva as usize, sections, file_alignment, opts)
Copy link
Owner

Choose a reason for hiding this comment

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

isn't there technically still an issue on 32-bit systems if callback_rva is > u32, and then it gets casted to a usize (hence truncating a potentially 64 bit number to 32 bits (sizeof pointer))?

tbh i'm not sure we ever supported analyzing 64-bit binaries on on 32-bit systems, simply due to rust &[u8] requiring usize to index, which means if that slice is larger than 32-bits, you cannot access the upper bits in rust.

Copy link
Contributor Author

@kkent030315 kkent030315 Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Oh yes, where the case the image must be broken. We shouldn't assume any of TLS callback addresses to be a valid VA/RVA. We can maybe put the check there.

.is_none()
{
return Err(error::Error::Malformed(format!(
"cannot map tls callback ({:#x})",
callback
Expand Down
Loading