Skip to content

Commit dd9cee1

Browse files
committed
fix: don't truncate compressed packets that outgrow the output reserve
Compress::compress_into reserves input.len() + 10 bytes and grew that buffer only on flate2::Status::BufError, which deflate returns when it cannot make progress at all, not when it filled the buffer after consuming input - that comes back as Status::Ok. The loop broke there with output still pending inside zlib. The packet went out short and the leftover bytes desynced the stream for every later packet. Incompressible payloads are what outgrow the reserve: deflate has to store them, so past 32 KiB a packet needs more room than input.len() + 10 leaves it. Peers with compression on lose bulk binary transfers - git pack data is the easy way to hit it - which OpenSSH reports as channel 0: get data: incomplete message. Break out of the loop on the output having room to spare instead, which is what deflate signals completion with, and grow whenever it comes back full. compress now delegates to compress_into so the two loops cannot drift apart again. The regression tests round-trip packets that outgrow the reserve, including through compress_into at a non-zero start_len - the shape the packet writer uses, which the existing compressible under-4096-byte tests don't cover.
1 parent dbe2234 commit dd9cee1

1 file changed

Lines changed: 86 additions & 26 deletions

File tree

russh/src/compression.rs

Lines changed: 86 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,77 @@ mod tests {
236236
assert_eq!(out, payload.as_slice(), "payload of {len} bytes came back wrong");
237237
}
238238
}
239+
240+
/// Deterministic bytes with no redundancy for deflate to exploit, so the
241+
/// packet comes back out larger than it went in.
242+
fn incompressible(seed: u64, len: usize) -> Vec<u8> {
243+
let mut state = seed
244+
.wrapping_mul(6364136223846793005)
245+
.wrapping_add(1442695040888963407);
246+
(0..len)
247+
.map(|_| {
248+
state ^= state << 13;
249+
state ^= state >> 7;
250+
state ^= state << 17;
251+
(state >> 24) as u8
252+
})
253+
.collect()
254+
}
255+
256+
#[test]
257+
fn compressed_packets_larger_than_the_reserve_round_trip() {
258+
// Deflate grows incompressible input rather than shrinking it, so a
259+
// large enough packet needs more output than the reserve holds.
260+
for len in [1024usize, 32768, 32769, 40000, 65536, 131072, 262144] {
261+
let mut comp = Compress::None;
262+
let mut decomp = Decompress::None;
263+
Compression::Zlib.init_compress(&mut comp);
264+
Compression::Zlib.init_decompress(&mut decomp);
265+
266+
let payload = incompressible(len as u64, len);
267+
let mut cbuf = Vec::new();
268+
let compressed = comp.compress(&payload, &mut cbuf).unwrap().to_vec();
269+
let mut dbuf = Vec::new();
270+
let out = decomp.decompress(&compressed, &mut dbuf).unwrap();
271+
assert!(
272+
out == payload.as_slice(),
273+
"payload of {len} bytes came back as {} bytes",
274+
out.len()
275+
);
276+
}
277+
}
278+
279+
#[test]
280+
fn compress_into_round_trips_oversized_packets_after_a_prefix() {
281+
// The packet writer compresses into a buffer that already holds the
282+
// packet prefix, so the payload lands partway into the output.
283+
const PREFIX: &[u8] = b"packet prefix";
284+
285+
let mut comp = Compress::None;
286+
let mut decomp = Decompress::None;
287+
Compression::Zlib.init_compress(&mut comp);
288+
Compression::Zlib.init_decompress(&mut decomp);
289+
290+
for packet in 0..4u64 {
291+
let payload = incompressible(packet, 65536);
292+
let mut buf = PREFIX.to_vec();
293+
let n = comp
294+
.compress_into(&payload, &mut buf, PREFIX.len())
295+
.unwrap();
296+
297+
assert_eq!(&buf[..PREFIX.len()], PREFIX, "prefix was clobbered");
298+
assert_eq!(buf.len(), PREFIX.len() + n, "output length disagrees");
299+
300+
let mut dbuf = Vec::new();
301+
let out = decomp.decompress(&buf[PREFIX.len()..], &mut dbuf).unwrap();
302+
assert!(
303+
out == payload.as_slice(),
304+
"packet {packet} came back as {} of {} bytes",
305+
out.len(),
306+
payload.len()
307+
);
308+
}
309+
}
239310
}
240311

241312
#[cfg(feature = "flate2")]
@@ -249,31 +320,12 @@ impl Compress {
249320
input: &'a [u8],
250321
output: &'a mut Vec<u8>,
251322
) -> Result<&'a [u8], crate::Error> {
252-
match *self {
253-
Compress::None => Ok(input),
254-
Compress::Zlib(ref mut z) => {
255-
output.clear();
256-
let n_in = z.total_in() as usize;
257-
let n_out = z.total_out() as usize;
258-
output.resize(input.len() + 10, 0);
259-
let flush = flate2::FlushCompress::Partial;
260-
loop {
261-
let n_in_ = z.total_in() as usize - n_in;
262-
let n_out_ = z.total_out() as usize - n_out;
263-
#[allow(clippy::indexing_slicing)] // length checked
264-
let c = z.compress(&input[n_in_..], &mut output[n_out_..], flush)?;
265-
match c {
266-
flate2::Status::BufError => {
267-
output.resize(output.len() * 2, 0);
268-
}
269-
_ => break,
270-
}
271-
}
272-
let n_out_ = z.total_out() as usize - n_out;
273-
#[allow(clippy::indexing_slicing)] // length checked
274-
Ok(&output[..n_out_])
275-
}
323+
if let Compress::None = *self {
324+
return Ok(input);
276325
}
326+
let n_out_ = self.compress_into(input, output, 0)?;
327+
#[allow(clippy::indexing_slicing)] // length checked
328+
Ok(&output[..n_out_])
277329
}
278330

279331
pub fn compress_into(
@@ -300,9 +352,17 @@ impl Compress {
300352
let n_out_ = z.total_out() as usize - n_out;
301353
#[allow(clippy::indexing_slicing)] // length checked
302354
let c = z.compress(&input[n_in_..], &mut output[start_len + n_out_..], flush)?;
355+
356+
// deflate leaves room to spare only once it has emitted
357+
// everything it holds, so a buffer filled to the brim may
358+
// still have more to come.
359+
let room = output.len().saturating_sub(start_len);
360+
if z.total_out() as usize - n_out < room {
361+
break;
362+
}
303363
match c {
304-
flate2::Status::BufError => {
305-
let growth = output.len().saturating_sub(start_len).max(1);
364+
flate2::Status::Ok | flate2::Status::BufError => {
365+
let growth = room.max(1);
306366
output.resize(output.len() + growth, 0);
307367
}
308368
_ => break,

0 commit comments

Comments
 (0)