carllerche
commented
1 year ago Thanks for the report.
For context, @zachs18 initially reported this via our security process. We decided that it was not a security issue and asked for the report to be filed as an issue. The reasoning is that the specific set of conditions needed to trigger an issue (a !Unpin type in ReadHalf) is unusual, combined with the difficulty of making any arbitrary use-after-free exploitable in Rust without doing a lot of careful alignment of data types in the surrounding code.
We will address the issue here and backport fixes to the LTS branches.
taiki-e
Version From what I gather on docs.rs,
tokio::io::ReadHalf::unsplitwas introduced in tokio0.1.12, so all versions since0.1.12are probably affected, but the example is tested with tokio1.24.1. Requires theio-utilfeature.Platform Not platform-specific.
Background
tokio::io::splitallows splitting aT: AsyncRead + AsyncWriteinto separateReadHalf<T>: AsyncReadandWriteHalf<T>: AsyncWritehalves, each of which can be used separately. This is done by havingReadHalf<T>'spoll_readmethod callT'spoll_readmethod, andWriteHalf<T>'spoll_writecallT'spoll_writemethod (with appropriate mutual exclusion). Note that both of these methods takeself: Pin<&mut Self>, which is a guarantee that theTwill not be moved before it is dropped (unlessT: Unpin) (https://doc.rust-lang.org/std/pin/index.html#drop-guarantee). This is fine in absence ofunsplit, since as a safety comment says (https://github.com/tokio-rs/tokio/blob/master/tokio/src/io/split.rs#L154), theTis stored in anArcthat is shared between theReadhalf<T>andWriteHalf<T>, and which is not deallocated or moved from until both theReadHalfandWriteHalfare dropped (and thus theTis dropped).Description However, in the presence of
ReadHalf::unsplit, this safety comment is incorrect: theTis not pinned, since theTmay be moved out of the sharedArcby callingReadHalf::unsplit(https://github.com/tokio-rs/tokio/blob/master/tokio/src/io/split.rs#L82), possibly after aPin<&mut T>has been created by callingReadHalf::poll_readorWriteHalf::poll_write, violating thePincontract (ifT: !Unpin).A simple (but breaking-change) fix would be to add a
where T: Unpinbound toReadHalf::unsplit. (and perhaps to hold aPin<Arc<Inner<T>>>instead of anArc<Inner<T>>inReadHalfandWriteHalfto communicate the requirement).Note that there are not (to my knowledge) any concrete types in
tokioby itself which areAsyncRead + AsyncWrite + !Unpin(File, TcpStream, and UnixStream are allUnpin, and the adapters all areUnpinif their underlying type isUnpin).Following is (what I think is) an (admittedly somewhat dubious) minimal example of undefined behaviour (use-after-free) resulting from this violation of the
Pincontract. Here is a playground link to the same example.Example code
`Cargo.toml`: ```toml [package] name = "tokio-mwe" version = "0.1.0" edition = "2021" [dependencies] tokio = { version = "1.24.1", features = ["io-util", "rt", "macros"] } ``` `src/main.rs` ```rs use std::{ cell::Cell, marker::{PhantomData, PhantomPinned}, pin::Pin, ptr::NonNull, sync::Arc, task::{Context, Poll}, }; use tokio::io::AsyncReadExt; // align(2048) to make allocator collsions happen more easily #[repr(align(2048))] pub struct BadRW { value: CellIn
async fn example_1,BadRWallows writing to deallocated memory. Inasync fn example_2,BadRWallows writing to a different object than the one that was pinned (if the allocator re-uses the address of a particular previous allocation).Following are the output of
cargo runandMIRIFLAGS=-Zmiri-backtrace=full cargo +nightly miri runon my computer (only the first example gets run under miri, because it aborts. The second example isn't interesting under miri, since miri doesn't re-use the previous allocation anyway).`cargo run` output
```sh $ cargo run Compiling tokio-mwe v0.1.0 (/home/zachary/Programming/rusttesting/tokio-crimes) Finished dev [unoptimized + debuginfo] target(s) in 0.72s Running `target/debug/tokio-mwe` Example 1 0x556871452000 (pinned) Setting *BAD_PTR to 42 [src/main.rs:91] bad.value.get() = 0 0x7ffd1d452000 (dropped) Example 2 0x556871454000 (pinned) 0x556871454000 (unrelated, non-pinned) Setting *BAD_PTR to 42 [src/main.rs:119] bad.value.get() = 0 [src/main.rs:120] unrelated.value.get() = 42 0x556871454000 (dropped) 0x7ffd1d451000 (dropped) ```miri output
```sh $ MIRIFLAGS=-Zmiri-backtrace=full cargo +nightly miri run Preparing a sysroot for Miri (target: x86_64-unknown-linux-gnu)... done Finished dev [unoptimized + debuginfo] target(s) in 0.01s Running `/home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo-miri runner target/miri/x86_64-unknown-linux-gnu/debug/tokio-mwe` Example 1 0x4e800 (pinned) Setting *BAD_PTR to 42 error: Undefined Behavior: pointer to alloc5497 was dereferenced after this allocation got freed --> /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/non_null.rs:385:18 | 385 | unsafe { &*self.as_ptr() } | ^^^^^^^^^^^^^^^ pointer to alloc5497 was dereferenced after this allocation got freed | = 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: BACKTRACE: = note: inside `std::ptr::NonNull::