EncryptedWriter encrypts through a buffer pointer that `put_u16`/`put_slice` already invalidated

[kbhetrr]

EncryptedWriter::poll_write_encrypted derives a raw slice from self.buffer, then calls self.buffer.put_, and finally writes through the earlier slice. The put_ call takes a fresh &mut self.buffer, which invalidates the earlier derivation, so the encryption writes through a pointer that is no longer valid under Rust's aliasing rules. The same mistake appears twice in the same block.

crates/shadowsocks/src/relay/tcprelay/aead.rs (master, lines 386-390 and 397-401; same code in the published shadowsocks 1.25.0 at lines 381-385 and 392-396):

// Step 1. Append Length
let length_size = 2 + self.cipher.tag_len();
self.buffer.reserve(length_size);

let mbuf = &mut self.buffer.chunk_mut()[..length_size];
let mbuf = unsafe { slice::from_raw_parts_mut(mbuf.as_mut_ptr(), mbuf.len()) };

self.buffer.put_u16(buf.len() as u16);   // takes &mut self.buffer -> invalidates `mbuf`
self.cipher.encrypt_packet(mbuf);        // writes through the invalidated pointer
unsafe { self.buffer.advance_mut(self.cipher.tag_len()) };

and again a few lines below:

// Step 2. Append data
let mbuf = &mut self.buffer.chunk_mut()[..data_size];
let mbuf = unsafe { slice::from_raw_parts_mut(mbuf.as_mut_ptr(), mbuf.len()) };

self.buffer.put_slice(buf);              // invalidates `mbuf`
self.cipher.encrypt_packet(mbuf);        // writes through the invalidated pointer
unsafe { self.buffer.advance_mut(self.cipher.tag_len()) };

poll_write_encrypted is a safe public method (also reachable through CryptoWrite::poll_write_encrypted on CryptoStream), so this is reachable from safe code and is a soundness issue rather than an API misuse.

I have not observed a miscompilation from this; current rustc keeps the write. Reporting it as unsoundness, in the same sense as RUSTSEC-2020-0073.

Reproduction

This uses the real bytes crate and mirrors step 1 exactly; only the cipher is stubbed (it encrypts the two length bytes in place and writes a tag, so it does not read uninitialized memory):

// Cargo.toml: bytes = "1"
use bytes::{BufMut, BytesMut};
use std::slice;

fn encrypt_packet(b: &mut [u8]) {
    for i in 0..2 { b[i] ^= 0xAA; }          // encrypt length in place
    for i in 2..b.len() { b[i] = 0x5A; }     // write tag
}

fn main() {
    let tag_len = 16usize;
    let mut buffer = BytesMut::with_capacity(64);
    let length_size = 2 + tag_len;
    buffer.reserve(length_size);

    let mbuf = &mut buffer.chunk_mut()[..length_size];
    let mbuf = unsafe { slice::from_raw_parts_mut(mbuf.as_mut_ptr(), mbuf.len()) };

    buffer.put_u16(7);
    encrypt_packet(mbuf);
    unsafe { buffer.advance_mut(tag_len) };
    println!("len={} head={:?}", buffer.len(), &buffer[..2]);
}

Stacked Borrows (Miri default):

error: Undefined Behavior: trying to retag from <752> for SharedReadWrite permission
       at alloc309[0x0], but that tag does not exist in the borrow stack for this location
  --> src/main.rs:18:20
help: <752> was created by a Unique retag at offsets [0x0..0x12]
  --> src/main.rs:15:25
help: <752> was later invalidated at offsets [0x0..0x40] by a Unique retag
  --> src/main.rs:17:5

Tree Borrows (MIRIFLAGS=-Zmiri-tree-borrows):

error: Undefined Behavior: reborrow through <720> at alloc309[0x0] is forbidden
  --> src/main.rs:18:20
   = help: the accessed tag <720> has state Disabled which forbids this reborrow
help: the accessed tag <720> was created here, in the initial state Reserved
  --> src/main.rs:15:25
help: the accessed tag <720> later transitioned to Disabled due to a foreign write access
       at offsets [0x0..0x2]
  --> src/main.rs:17:5

Both of Miri's aliasing models reject it, and both point at the put_u16 line as the invalidating access.

Suggested fix

The slice has to stay valid from derivation to use, so the put_* call cannot sit in between. Writing the length through the same slice keeps one derivation and preserves the byte layout (BufMut::put_u16 is big-endian):

let length_size = 2 + self.cipher.tag_len();
self.buffer.reserve(length_size);

let mbuf = &mut self.buffer.chunk_mut()[..length_size];
let mbuf = unsafe { slice::from_raw_parts_mut(mbuf.as_mut_ptr(), length_size) };
mbuf[..2].copy_from_slice(&(buf.len() as u16).to_be_bytes());
self.cipher.encrypt_packet(mbuf);
unsafe { self.buffer.advance_mut(length_size) };   // 2 length bytes + tag

Step 2 can be fixed the same way by copying buf into mbuf[..buf.len()] instead of calling put_slice, and then advancing by data_size.

I checked this variant under Miri: both Stacked Borrows and Tree Borrows accept it, and the resulting buffer contents are unchanged.

Note that re-deriving from chunk_mut() after put_u16 does not work: at that point chunk_mut() starts two bytes further along, so the slice would no longer cover the length field.

Context

Found while developing a static checker for Rust's aliasing rules; I verified this one by hand with Miri before reporting. Happy to open a PR if the suggested shape looks right to you, and happy to coordinate before anything is published.