ababo / dtb Goto Github PK

The source of unsoundness

The first unsoundness lies in transmute_buf.

The second unsoundness lies in value_u32_list.

To reproduce the bug

To reproduce the bug in transmute_buf:

fn main() {
    let mut arr: [u32; 3] = [0; 3];
    let mut buf = unsafe {
        core::slice::from_raw_parts_mut::<u8>(
            arr.as_mut_ptr() as *mut u8,
            core::mem::size_of_val(&arr),
        )
    };

    println!("{:?}", prop.value_u32_list(&mut buf).unwrap())
}

run with miri,

error: Undefined Behavior: trying to retag from <2588> for Unique permission at alloc1308[0x0], but that tag only grants SharedReadOnly permission for this location
   --> /$HOME/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/raw.rs:145:9
    |
145 |         &mut *ptr::slice_from_raw_parts_mut(data, len)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |         |
    |         trying to retag from <2588> for Unique permission at alloc1308[0x0], but that tag only grants SharedReadOnly permission for this location
    |         this error occurs as part of retag at alloc1308[0x0..0xc]
    |

To reproduce the bug in value_u32_list, we just need to run cargo test:

running 38 tests
test reader::tests::test_bad_magic ... ok
thread 'struct_item::tests::test_value_u32_list' panicked at 'misaligned pointer dereference: address must be a multiple of 0x4 but is 0x563ee31717fb', src/struct_item.rs:130:17

Hope this could help you debug and fix the bugs:)

The reader my construct a reference of &Header even when the underlying blob is too small. This may lead to UB and the compiler would also not miscompile if it were to remove the check for header length that comes afterwards, even though it does not seem to exploit that fact at the moment.

Nice work!

Do you have any thoughts on adding some search functionality like this?

let reader = Reader::read_from_address(pdtb as usize).unwrap();
let (node, _) = reader.struct_items().path_struct_items("/interrupt-parent").next().unwrap();
let phandle = node.value().unwrap();

let intc_path = reader.struct_items().find_path_to("phandle", phandle).unwrap();

let (intc, _) = reader.struct_items().path_struct_items(intc_path).next().unwrap();

The current implementation trusts, to some extent, the offsets provided by the
file header to access its buffers. In particular, the order of checks for ensuring
that offsets and sizes are well defined on the whole blob is not enough to
avoid wrong all invalid indices.

For example reserved_mem_offset is used to read from the buffer first but
the order in Reader::read only checks the total_size last. Furthermore,
several conversion with as usize and additions may lead to overflows that
could confuse the conversion.

The impact for the struct and strings offsets is a simple panic, as they are
used as slice indices only which are fully checked against the buffer length.
However, reserved_mem_offset is used to unsafely construct a &[u8]
before any of the other checks have been performed. This allows an attacker
to create files with an invalid out-of-bounds reserved_mem_offset that is
improperly check in that portion of code.

It is then used to manipulate a pointer to the input buffer:

An invalid offset may thus lead to this pointer pointing out-of-bounds and
overflow in the length calculation will then iterate over a large portion of
invalid memory.

These lines read offsets from the input binary and trust them to only specify offsets in-bounds. This leads to panic! while reading from the StructItems iterator.

Reproduction case is attached (cargo run --example dump), I'll likely provide a fix PR this week.
issue-6.zip

Didn't have a time to finish DTB writer. Feel free to do that.