3Hren / 3Hren/msgpack-rust

`rmp_serde::Raw` considered harmful

Open
#305 2 comments 2 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
1.4k
Forks
162
PR merge metrics
No merged PRs in 30d

Description

The `unsafe` behavior of `Raw` is trivially shown to be unsound. `str` is *required* to be valid UTF-8, and its methods assume that it contains valid UTF-8 for the purpose of, for example, scanning for code points. I'm able to produce UB by creating a simple Serializer that serializes strings as an array of chars, and then feeding it malformed UTF-8 data:

```rust
/// Serializer that serializes strings as a list of char
struct CharListSerializer {
output: Vec,
}

impl ser::Serializer for &mut CharListSerializer {
fn serialize_str(self, s: &str) -> Result {
self.output.extend(s.chars());
Ok(())
}

// Other methods simply panic
}

fn main() {
let mut ser = CharListSerializer { output: Vec::new() };
let data = "ABC";
data.serialize(&mut ser).unwrap();
assert_eq!(ser.output, ['A', 'B', 'C']);
ser.output.clear();

// An emoji: 😣
let data = [0xf0, 0x9f, 0x98, 0xa3];
let data = Raw::from_utf8(data.as_slice().to_owned());
data.serialize(&mut ser).unwrap();
assert_eq!(ser.output, ['😣']);
ser.output.clear();

// Uh oh. 0xf0 signals a 4-byte UTF-8 code point,
// but there are only two bytes in this buffer.
let data = [0xf0, 0x9f];
let data = Raw::from_utf8(data.as_slice().to_owned());
data.serialize(&mut ser).unwrap();
}
```

When I run this normally, I get a crash:

```
error: process didn't exit successfully: `target\debug\rust-playground.exe` (exit code: 0xc000001d, STATUS_ILLEGAL_INSTRUCTION)
```

And when I run with `cargo miri run`, it immediately detects the undefined behavior:

```
error: Undefined Behavior: entering unreachable code
--> C:\Users\Lucre\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\hint.rs:51:14
|
51 | unsafe { intrinsics::unreachable() }
| ^^^^^^^^^^^^^^^^^^^^^^^^^ entering unreachable code
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information

= note: inside `std::hint::unreachable_unchecked` at C:\Users\Lucre\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\hint.rs:51:14
= note: inside `std::option::Option::<&u8>::unwrap_unchecked` at C:\Users\Lucre\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\option.rs:877:30
= note: inside `core::str::validations::next_code_point::>` at C:\Users\Lucre\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\str\validations.rs:56:27
= note: inside `::next` at C:\Users\Lucre\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\str\iter.rs:44:18
= note: inside `std::vec::Vec::::extend_desugared::` at C:\Users\Lucre\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\alloc\src\vec\mod.rs:2696:35
= note: inside ` as std::vec::spec_extend::SpecExtend>::spec_extend` at C:\Users\Lucre\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\alloc\src\vec\spec_extend.rs:18:9
= note: inside ` as std::iter::Extend>::extend::` at C:\Users\Lucre\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\alloc\src\vec\mod.rs:2670:9
note: inside `<&mut CharListSerializer as serde::Serializer>::serialize_str` at src\main.rs:86:9
--> src\main.rs:86:9
|
86 | self.output.extend(s.chars());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= note: inside `::serialize::<&mut CharListSerializer>` at C:\Users\Lucre\.cargo\registry\src\github.com-1ecc6299db9ec823\rmp-serde-1.0.0\src\lib.rs:190:9
note: inside `main` at src\main.rs:211:5
--> src\main.rs:211:5
|
211 | data.serialize(&mut ser).unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to previous error

error: process didn't exit successfully: `C:\Users\Lucre\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\bin\cargo-miri.exe target\miri\x86_64-pc-windows-msvc\debug\rust-playground.exe` (exit code: 1)
```

This type should be either:

- Removed,
- Redesigned to use `serialize_bytes`
- Marked as `unsafe` and changed to *require* valid utf-8.

To be frank, it's not at all clear to me why this type exists in the first place? It isn't used by RMP anywhere, and I don't really understand why a user would ever be allowed to call `serialize_str` on non-UTF8 data, when the entire point of that method is that valid UTF-8 can be assumed. It appears to exist for no other purpose than to create this kind of soundness hole.

My full reproduction code is available as a [gist](https://gist.github.com/Lucretiel/5deaf285f06a85056aa76276abf9bd77).

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.