search

CasualX / pelite

Lightweight, memory-safe, zero-allocation library for reading and navigating PE binaries.
MIT License
280 stars 45 forks source link

Misaligned address when trying to access Import section of some PEs #246

Open roblabla opened 3 years ago

roblabla commented 3 years ago

First, thanks for this great crate, it's been a huge help!

While trying to look at the import table of C:\Program Files (x86)\Common Files\Microsoft Shared\Phone Tools\CoreCon\11.0\bin\IpOverUsbSvc.exe, I'm getting a misaligned address error. This appears to be caused by the IMAGE_IMPORT_DESCRIPTOR table being misaligned, which is confirmed when looking at the binary in a hex editor: image

The binary can be found here: IpOverUsbSvc.exe.zip

Here's some test code:

use pelite::PeFile;

static IP_OVER_USB_SVC: &[u8] = include_bytes!("IpOverUsbSvc.exe");

#[test]
fn test() {
    let pefile = PeFile::from_bytes(IP_OVER_USB_SVC).unwrap();
    let imports = pefile.imports().expect("Grabbing the import table to work.");
}
roblabla commented 3 years ago

I have locally fixed the issue by making IMAGE_IMPORT_DESCRIPTOR repr(packed). This is a breaking change since that type is publicly exported, but I think it's the correct way to go. Perhaps more generally, all currently repr(C) structs should be turned into repr(packed)?

CasualX commented 3 years ago

That's unfortunate. The design of pelite demands that everything is aligned so that I can give references to the underlying data instead of making copies. This makes pelite a 'zero allocation' parser which is one of my goals.

Unfortunately repr(packed) is completely broken in Rust (and tbh, in general) as it causes insta-UB when trying to take a reference to a field which is not the right alignment playground. Rust used to allow this and when the Rust developers tried to fix this they encountered a lot of broken crates (including pelite).

I'm not currently willing to go back to repr(packed). If parsing such files is important you can try parsers like goblin or manually decipher these imports.

roblabla commented 3 years ago

Huh, I'm aware of repr(packed) references being UB, but I thought this had been turned into hard errors already. That's kinda sad.

I don't see why you say repr(packed) is "completely broken" though? Once this issue is fixed, packed structs will be safe to use, you'd just have to never create references to any of its fields that have a bigger alignment than 1 (e.g. only create references to other nested packed structs), and the compiler will enforce it for you.

I understand reluctance to move to repr(packed) structs now when it's still a safety and stability hazard, especially as there are still some problems with the feature (in particular around auto-generated derives). But would you still be unwilling to move back to repr(packed) once its use is safe? I did a quick test, there are currently around 40 warnings being generated by turning all the repr(C) into repr(packed), and the vast majority are due to custom derives, or debug impls, both of which can be easily be fixed. I only found one function, CodeView::format, that intentionally create references that would probably need the raw ref operator to be UB-free.

In the meantime, I'll probably move to goblin or object. Shux because PELite's API and documentation are really nice!

Ben-Lichtman commented 9 months ago

Running ASAN over the tests reveals:


pelite 0.10.0

Using sanitizier `address`.
Preparing a careful sysroot (target: x86_64-unknown-linux-gnu, sanitizer: address)... done
warning: for loop over a `Result`. This is more readably written as an `if let` statement
   --> src/pe64/imports.rs:303:12
    |
303 |         for _ in desc.iat() {}
    |                  ^^^^^^^^^^
    |
    = note: `#[warn(for_loops_over_fallibles)]` on by default
help: to check pattern in a loop use `while let`
    |
303 |         while let Ok(_) = desc.iat() {}
    |         ~~~~~~~~~~~~~ ~~~
help: consider unwrapping the `Result` with `?` to iterate over its contents
    |
303 |         for _ in desc.iat()? {}
    |                            +
help: consider using `if let` to clear intent
    |
303 |         if let Ok(_) = desc.iat() {}
    |         ~~~~~~~~~~ ~~~

warning: for loop over a `Result`. This is more readably written as an `if let` statement
   --> src/pe64/imports.rs:304:12
    |
304 |         for _ in desc.int() {}
    |                  ^^^^^^^^^^
    |
help: to check pattern in a loop use `while let`
    |
304 |         while let Ok(_) = desc.int() {}
    |         ~~~~~~~~~~~~~ ~~~
help: consider unwrapping the `Result` with `?` to iterate over its contents
    |
304 |         for _ in desc.int()? {}
    |                            +
help: consider using `if let` to clear intent
    |
304 |         if let Ok(_) = desc.int() {}
    |         ~~~~~~~~~~ ~~~

warning: for loop over a `Result`. This is more readably written as an `if let` statement
   --> src/pe32/../pe64/imports.rs:303:12
    |
303 |         for _ in desc.iat() {}
    |                  ^^^^^^^^^^
    |
help: to check pattern in a loop use `while let`
    |
303 |         while let Ok(_) = desc.iat() {}
    |         ~~~~~~~~~~~~~ ~~~
help: consider unwrapping the `Result` with `?` to iterate over its contents
    |
303 |         for _ in desc.iat()? {}
    |                            +
help: consider using `if let` to clear intent
    |
303 |         if let Ok(_) = desc.iat() {}
    |         ~~~~~~~~~~ ~~~

warning: for loop over a `Result`. This is more readably written as an `if let` statement
   --> src/pe32/../pe64/imports.rs:304:12
    |
304 |         for _ in desc.int() {}
    |                  ^^^^^^^^^^
    |
help: to check pattern in a loop use `while let`
    |
304 |         while let Ok(_) = desc.int() {}
    |         ~~~~~~~~~~~~~ ~~~
help: consider unwrapping the `Result` with `?` to iterate over its contents
    |
304 |         for _ in desc.int()? {}
    |                            +
help: consider using `if let` to clear intent
    |
304 |         if let Ok(_) = desc.int() {}
    |         ~~~~~~~~~~ ~~~

warning: `pelite` (lib test) generated 4 warnings
    Finished test [unoptimized + debuginfo] target(s) in 0.01s
     Running unittests src/lib.rs (target/x86_64-unknown-linux-gnu/debug/deps/pelite-bb300847cff982c9)

running 19 tests
test pattern::tests::errors ... ok
test pattern::tests::patterns ... ok
test pe32::file::from_byte_slice ... ok
test pe32::scanner::exec_tests_parse_docs ... ok
test pe32::view::tests::from_byte_slice ... ok
test pe64::file::from_byte_slice ... ok
test pe64::scanner::exec_tests_parse_docs ... ok
test pe64::view::tests::from_byte_slice ... ok
test resources::version_info::test_parse_254 ... ok
test resources::version_info::test_parse_tlv_oob ... ok
test strings::testing ... ok
test tests::pocs ... 
96emptysections.exe
  base_relocs...    Err(Null)
  rich_structure... Err(BadMagic)
  exception...      Err(Null)
  exports...        Err(Null)
  imports...        Err(Null)
  debug...          Err(Null)
  load_config...    Err(Null)
  security...       Err(Null)
  tls...            Err(Null)
  resources...      Err(Null)
  scanner...        Ok(())

appendeddata.exe
  base_relocs...    Err(Null)
  rich_structure... Err(BadMagic)
  exception...      Err(Null)
  exports...        Err(Null)
  imports...        Err(Null)
  debug...          Err(Null)
  load_config...    Err(Null)
  security...       Err(Null)
  tls...            Err(Null)
  resources...      Err(Null)
  scanner...        Ok(())

appendedhdr.exe
  base_relocs...    Err(Null)
  rich_structure... Err(BadMagic)
  exception...      Err(Null)
  exports...        Err(Null)
  imports...        Err(Null)
  debug...          Err(Null)
  load_config...    Err(Null)
  security...       Err(Null)
  tls...            Err(Null)
  resources...      Err(Null)
  scanner...        Ok(())

appendedsecttbl.exe
  base_relocs...    Err(Null)
  rich_structure... Err(BadMagic)
  exception...      Err(Null)
  exports...        Err(Null)
  imports...        Err(Null)
  debug...          Err(Null)
  load_config...    Err(Null)
  security...       Err(Null)
  tls...            Err(Null)
  resources...      Err(Null)
  scanner...        Ok(())

apphdrW7.exe
  base_relocs...    Err(Null)
  rich_structure... Err(BadMagic)
  exception...      Err(Null)
  exports...        Err(Null)
  imports...        Err(Null)
  debug...          Err(Null)
  load_config...    Err(Null)
  security...       Err(Null)
  tls...            Err(Null)
  resources...      Err(Null)
  scanner...        Ok(())

appsectableW7.exe
  base_relocs...    Err(Null)
  rich_structure... Err(BadMagic)
  exception...      Err(Null)
  exports...        Err(Null)
  imports...        Err(Null)
  debug...          Err(Null)
  load_config...    Err(Null)
  security...       Err(Null)
  tls...            Err(Null)
  resources...      Err(Null)
  scanner...        Ok(())

aslr-ld.exe
  base_relocs...    Err(Null)
  rich_structure... Err(BadMagic)
  exception...      Err(Null)
  exports...        Err(Null)
  imports...        Err(Null)
  debug...          Err(Null)
  load_config...    Err(Null)
  security...       Err(Null)
  tls...            Err(Null)
  resources...      Err(Null)
  scanner...        Ok(())

aslr.dll
  base_relocs...    Err(Null)
  rich_structure... Err(BadMagic)
  exception...      Err(Null)
  exports...        Ok(())
  imports...        Err(Null)
  debug...          Err(Null)
  load_config...    Err(Null)
  security...       Err(Null)
  tls...            Err(Null)
  resources...      Err(Null)
  scanner...        Ok(())

bigib.exe
  base_relocs...    Err(Null)
  rich_structure... Err(BadMagic)
  exception...      Err(Null)
  exports...        Err(Null)
  imports...        Err(Null)
  debug...          Err(Null)
  load_config...    Err(Null)
  security...       Err(Null)
  tls...            Err(Null)
  resources...      Err(Null)
  scanner...        Ok(())

bigsec.exe
  base_relocs...    Err(Null)
  rich_structure... Err(BadMagic)
  exception...      Err(Null)
  exports...        Err(Null)
  imports...        Err(Null)
  debug...          Err(Null)
  load_config...    Err(Null)
  security...       Err(Null)
  tls...            Err(Null)
  resources...      Err(Null)
  scanner...        Ok(())

bigSoRD.exe
  base_relocs...    Err(Null)
  rich_structure... Err(BadMagic)
  exception...      Err(Null)
  exports...        Err(Null)
  imports...        Err(Invalid)
  debug...          Err(Null)
  load_config...    Err(Null)
  security...       Err(Null)
  tls...            Err(Null)
  resources...      Err(Null)
  scanner...        Ok(())

bottomsecttbl.exe
  base_relocs...    Err(Null)
  rich_structure... Err(BadMagic)
  exception...      Err(Null)
  exports...        Err(Null)
  imports...        Err(Null)
  debug...          Err(Null)
  load_config...    Err(Null)
  security...       Err(Null)
  tls...            Err(Null)
  resources...      Err(Null)
  scanner...        Ok(())

cfgbogus.exe
  base_relocs...    Err(Null)
  rich_structure... Err(BadMagic)
  exception...      Err(Null)
  exports...        Err(Null)
thread 'tests::pocs' panicked at src/pe32/../pe64/pe.rs:344:22:
misaligned pointer dereference: address must be a multiple of 0x4 but is 0x557d846a8005
stack backtrace:
   0: rust_begin_unwind
             at /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:597:5
   1: core::panicking::panic_nounwind_fmt
             at /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/panicking.rs:106:14
   2: core::panicking::panic_misaligned_pointer_dereference
             at /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/panicking.rs:203:5
   3: pelite::pe32::pe::Pe::derva_slice_f
             at ./src/pe32/../pe64/pe.rs:344:10
   4: pelite::pe32::imports::Imports<P>::try_from
             at ./src/pe32/../pe64/imports.rs:86:15
   5: pelite::pe32::pe::Pe::imports
             at ./src/pe32/../pe64/pe.rs:481:3
   6: pelite::pe32::imports::test
             at ./src/pe32/../pe64/imports.rs:297:16
   7: pelite::tests::pocs
             at ./src/tests.rs:29:40
   8: pelite::tests::pocs::{{closure}}
             at ./src/tests.rs:21:10
   9: core::ops::function::FnOnce::call_once
             at /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
  10: core::ops::function::FnOnce::call_once
             at /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
thread caused non-unwinding panic. aborting.
error: test failed, to rerun pass `--lib`

Caused by:
  process didn't exit successfully: `/build/target/x86_64-unknown-linux-gnu/debug/deps/pelite-bb300847cff982c9` (signal: 6, SIGABRT: process abort signal)
     Running unittests src/bin/findsig.rs (target/x86_64-unknown-linux-gnu/debug/deps/findsig-a77a2d9a4de7802e)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running unittests src/bin/module-def.rs (target/x86_64-unknown-linux-gnu/debug/deps/module_def-c3929c6915e5247a)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running unittests src/bin/msrtti.rs (target/x86_64-unknown-linux-gnu/debug/deps/msrtti-82a8aef45a3b9758)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running unittests src/bin/pedump.rs (target/x86_64-unknown-linux-gnu/debug/deps/pedump-572230a3682c4f29)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running unittests src/bin/strings.rs (target/x86_64-unknown-linux-gnu/debug/deps/strings-17dd063b70a7f2bf)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/demo64.rs (target/x86_64-unknown-linux-gnu/debug/deps/demo64-4526f6f4579baa54)

running 11 tests
test base_relocs ... ok
test debug ... ok
test exception ... ok
test exports ... ok
test find_data ... ok
test imports ... ok
test rich_structure ... ok
test scanner ... ok
test security ... ok
test slice_edges ... ok
test tls ... ok

test result: ok. 11 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.03s

     Running tests/edges.rs (target/x86_64-unknown-linux-gnu/debug/deps/edges-94e610d59f8f2a41)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/patterns.rs (target/x86_64-unknown-linux-gnu/debug/deps/patterns-483a19a13b63e2b3)

running 1 test
test main ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/tiny.rs (target/x86_64-unknown-linux-gnu/debug/deps/tiny-86d21cc94ef3a357)

running 11 tests
test tiny_128 ... ok
test tiny_168 ... ok
test tiny_296 ... ok
test tiny_356 ... ok
test tiny_97 ... ok
test tiny_c_1024 ... ok
test tiny_c_468 ... ok
test tiny_import_133 ... ok
test tiny_import_161 ... ok
test tiny_import_209 ... ok
test tiny_webdav_133 ... ok

test result: ok. 11 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.03s

   Doc-tests pelite

running 30 tests
test src/base_relocs.rs - base_relocs (line 10) ... ok
test src/error.rs - error::Error::is_null (line 77) ... ok
test src/pe32/../pe64/debug.rs - pe32::debug (line 5) ... ok
test src/pe32/../pe64/exports.rs - pe32::exports (line 10) ... ok
test src/pe32/../pe64/imports.rs - pe32::imports (line 10) ... ok
test src/pe32/../pe64/load_config.rs - pe32::load_config (line 5) ... ok
test src/pe32/../pe64/ptr.rs - pe32::ptr::Ptr<T>::offset (line 47) ... ok
test src/pe32/../pe64/resources.rs - pe32::resources (line 7) ... ok
test src/pe32/../pe64/scanner.rs - pe32::scanner (line 7) ... ok
test src/pe32/../pe64/tls.rs - pe32::tls (line 5) ... ok
test src/pe32/mod.rs - pe32 (line 29) ... ok
test src/pe32/mod.rs - pe32 (line 61) ... ok
test src/pe32/mod.rs - pe32 (line 97) ... ok
test src/pe64/debug.rs - pe64::debug (line 5) ... ok
test src/pe64/exports.rs - pe64::exports (line 10) ... ok
test src/pe64/imports.rs - pe64::imports (line 10) ... ok
test src/pe64/load_config.rs - pe64::load_config (line 5) ... ok
test src/pe64/mod.rs - pe64 (line 29) ... ok
test src/pe64/mod.rs - pe64 (line 61) ... ok
test src/pe64/mod.rs - pe64 (line 97) ... ok
test src/pe64/ptr.rs - pe64::ptr::Ptr<T>::offset (line 47) ... ok
test src/pe64/resources.rs - pe64::resources (line 7) ... ok
test src/pe64/scanner.rs - pe64::scanner (line 7) ... ok
test src/pe64/tls.rs - pe64::tls (line 5) ... ok
test src/resources/group.rs - resources::group (line 13) ... ok
test src/resources/version_info.rs - resources::version_info (line 9) ... ok
test src/security.rs - security (line 8) ... ok
test src/util/c_str.rs - util::c_str::CStr::from_bytes (line 28) ... ok
test src/util/mod.rs - util::strn (line 50) ... ok
test src/util/mod.rs - util::wstrn (line 89) ... ok

test result: ok. 30 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 46.08s

error: 1 target failed: