rosrust icon indicating copy to clipboard operation
rosrust copied to clipboard

Unsoundness in `(decode/encode)_variable_primitive_vec`

Open shinmao opened this issue 5 months ago • 0 comments

Bug and PoC

Hi, we consider the following function has the unsound implementation: https://github.com/adnanademovic/rosrust/blob/c46a7e6742e5e01e0271d144ae2a05be9a035539/rosrust/src/rosmsg.rs#L217-L235 The function first defines the type of buffer it wants to fill up; then read from the input data. This could break the validity in Rust. For example, we use bool as the generic type T, then this function allows us to write some invalid bits into the buffer while Rust only allows 0x00 or 0x01 to be boolean type.

Following is the PoC:

use rosrust::rosmsg::decode_variable_primitive_vec;
use rosrust::RosMsg;
use std::io::{self, Cursor, Read, Write};

fn main() -> io::Result<()> {
    let invalid_data = vec![0x02, 0x03, 0xFF, 0x00, 0x01];

    // Encode the number of elements
    let mut encoded_data = Vec::new();
    (invalid_data.len() as u32).encode(&mut encoded_data)?;
    encoded_data.extend_from_slice(&invalid_data);

    // Use a Cursor to simulate reading from a stream
    let mut cursor = Cursor::new(encoded_data);

    let decoded_vec = decode_variable_primitive_vec::<_, bool>(&mut cursor)?;
    println!("Decoded vector: {:?}", decoded_vec);

    for (i, &b) in decoded_vec.iter().enumerate() {
        println!("Boolean at index {}: {:?}", i, b);
    }

    Ok(())
}

run with miri then you can get,

error: Undefined Behavior: constructing invalid value: encountered 0x02, but expected a boolean
    --> /{user}/.rustup/toolchains/nightly-2023-06-01-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:2307:25
     |
2307 |         Display::fmt(if *self { "true" } else { "false" }, f)
     |                         ^^^^^ constructing invalid value: encountered 0x02, but expected a boolean

At the same time, we also consider the both functions should inherit to the supertrait Pod rather than Sized. The reason is that slice::from_raw_parts requires the raw pointer to be initialized. However, we can implement our own repr(Rust) struct, which could contains padding bytes, and cast to u8 pointer at line 228. It could break the safety requirement of from_raw_parts.

Please consider to use Pod for scalar types!!

shinmao avatar Sep 04 '24 06:09 shinmao