From 6d6e5ece768b2e069bc654d7b170c4cc0f9b8686 Mon Sep 17 00:00:00 2001 From: Martin Thomson Date: Mon, 24 Nov 2025 16:02:08 +1100 Subject: [PATCH 1/3] Handle zero-length chunks on read This wasn't properly tested previously, so it was resulting in a short read. Also, because zero-length chunks are useless, put in basic protection against receiving too many. This protection isn't perfect, because it doesn't count across calls to `poll()` and it presently requires that two zero-length chunks are seen in the same call. That's OK, because the goal is to ensure that an attacker can't overload a reader with pointless work. If the reader is not overloaded, it might tolerate a few zero-length chunks, which is probably OK. --- ohttp/src/err.rs | 3 +++ ohttp/src/stream.rs | 60 ++++++++++++++++++++++++++++++++++++++++--- sync-async/src/lib.rs | 3 ++- 3 files changed, 62 insertions(+), 4 deletions(-) diff --git a/ohttp/src/err.rs b/ohttp/src/err.rs index ec7741e..dd3d11e 100644 --- a/ohttp/src/err.rs +++ b/ohttp/src/err.rs @@ -41,6 +41,9 @@ pub enum Error { #[cfg(feature = "stream")] #[error("writes are not supported after closing")] WriteAfterClose, + #[cfg(feature = "stream")] + #[error("read too many zero-length chunks")] + ZeroLengthRead, } impl From for Error { diff --git a/ohttp/src/stream.rs b/ohttp/src/stream.rs index 0c402ac..24f575d 100644 --- a/ohttp/src/stream.rs +++ b/ohttp/src/stream.rs @@ -285,6 +285,9 @@ enum ChunkReader { } impl ChunkReader { + /// The number of zero-length chunks we accept before aborting. + const ZERO_LENGTH_DOS_LIMIT: usize = 1; + fn length() -> Self { Self::Length { len: [0; 8], @@ -475,6 +478,8 @@ impl ChunkReader { return Poll::Ready(Ok(delivered)); } + let mut zero_length_chunks = 0; + while !matches!(self, Self::Done) { if let Some(res) = self.read_length(src.as_mut(), cx, cipher) { return res; @@ -482,6 +487,13 @@ impl ChunkReader { // Read data. if let Some(res) = self.read_into_output(src.as_mut(), cx, cipher, output) { + if matches!(res, Poll::Ready(Ok(0))) { + zero_length_chunks += 1; + if zero_length_chunks > Self::ZERO_LENGTH_DOS_LIMIT { + return ioerror(Error::ZeroLengthRead); + } + continue; + } return res; } @@ -544,6 +556,10 @@ impl ChunkReader { if pt.is_empty() { // We can't return zero length, as that means "end of stream". // So read the next chunk if this one was empty. + zero_length_chunks += 1; + if !last && zero_length_chunks > Self::ZERO_LENGTH_DOS_LIMIT { + return ioerror(Error::ZeroLengthRead); + } continue; } } @@ -814,18 +830,21 @@ impl AsyncRead for ClientResponse { mod test { use std::{ io::Result as IoResult, - pin::Pin, + pin::{pin, Pin}, task::{Context, Poll}, }; use futures::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt}; use log::trace; use pin_project::pin_project; - use sync_async::{Dribble, Pipe, SplitAt, Stutter, SyncRead, SyncResolve, Unadapt}; + use sync_async::{ + noop_context, Dribble, Pipe, SplitAt, Stutter, SyncRead, SyncResolve, Unadapt, + }; use crate::{ + stream::ChunkWriter, test::{init, make_config, REQUEST, RESPONSE}, - ClientRequest, Server, + ClientRequest, Error, Server, }; #[test] @@ -1106,4 +1125,39 @@ mod test { let mut server_request = server.decapsulate_stream(&enc_request[..]); assert_eq!(server_request.sync_read_to_end(), LONG_REQUEST); } + + /// Check that repeated zero-length chunks are treated as invalid. + #[test] + fn dos_zero_length() { + init(); + let server_config = make_config(); + let server = Server::new(server_config).unwrap(); + let encoded_config = server.config().encode().unwrap(); + let client = ClientRequest::from_encoded_config(&encoded_config).unwrap(); + let (request_read, request_write) = Pipe::new(); + let mut client_request = client.encapsulate_stream(request_write).unwrap(); + + let pin = Pin::new(&mut client_request.writer); + let mut projection = pin.project(); + let mut cx = noop_context(); + + // Write out two zero-length chunks before finalizing the request. + for _ in 0..2 { + let f = ChunkWriter::flush(&mut projection, &mut cx); + assert!(f.is_ready()); + assert!(projection.buf.is_empty()); + ChunkWriter::write_chunk(&mut projection, &mut cx, &[], false).unwrap(); + } + + client_request.write_all(REQUEST).sync_resolve().unwrap(); + + let mut buf = Vec::new(); + let mut server_request = server.decapsulate_stream(request_read); + let fut = server_request.read_to_end(&mut buf); + let err = pin!(fut).sync_resolve().unwrap_err(); + assert!(matches!( + err.get_ref().unwrap().downcast_ref().unwrap(), + Error::ZeroLengthRead + )); + } } diff --git a/sync-async/src/lib.rs b/sync-async/src/lib.rs index f4a9826..efd3a0a 100644 --- a/sync-async/src/lib.rs +++ b/sync-async/src/lib.rs @@ -12,7 +12,8 @@ use futures::{ }; use pin_project::pin_project; -fn noop_context() -> Context<'static> { +#[must_use] +pub fn noop_context() -> Context<'static> { use std::{ ptr::null, task::{RawWaker, RawWakerVTable, Waker}, From 747333b642c922dcb61e0ea5d30eb1b78a0825fe Mon Sep 17 00:00:00 2001 From: Martin Thomson Date: Thu, 27 Nov 2025 16:01:57 +1100 Subject: [PATCH 2/3] Simplify - zero-length is bad --- ohttp/src/err.rs | 2 +- ohttp/src/stream.rs | 41 ++++++++++++----------------------------- 2 files changed, 13 insertions(+), 30 deletions(-) diff --git a/ohttp/src/err.rs b/ohttp/src/err.rs index dd3d11e..212b234 100644 --- a/ohttp/src/err.rs +++ b/ohttp/src/err.rs @@ -42,7 +42,7 @@ pub enum Error { #[error("writes are not supported after closing")] WriteAfterClose, #[cfg(feature = "stream")] - #[error("read too many zero-length chunks")] + #[error("read a zero-length chunk")] ZeroLengthRead, } diff --git a/ohttp/src/stream.rs b/ohttp/src/stream.rs index 24f575d..f3a8c77 100644 --- a/ohttp/src/stream.rs +++ b/ohttp/src/stream.rs @@ -285,9 +285,6 @@ enum ChunkReader { } impl ChunkReader { - /// The number of zero-length chunks we accept before aborting. - const ZERO_LENGTH_DOS_LIMIT: usize = 1; - fn length() -> Self { Self::Length { len: [0; 8], @@ -425,7 +422,11 @@ impl ChunkReader { }; output[..pt.len()].copy_from_slice(&pt); *self = Self::length(); - Some(Poll::Ready(Ok(pt.len()))) + if pt.is_empty() { + Some(ioerror(Error::ZeroLengthRead)) + } else { + Some(Poll::Ready(Ok(pt.len()))) + } } else { buf.reserve_exact(*length); buf.extend_from_slice(&output[..r]); @@ -478,8 +479,6 @@ impl ChunkReader { return Poll::Ready(Ok(delivered)); } - let mut zero_length_chunks = 0; - while !matches!(self, Self::Done) { if let Some(res) = self.read_length(src.as_mut(), cx, cipher) { return res; @@ -487,13 +486,6 @@ impl ChunkReader { // Read data. if let Some(res) = self.read_into_output(src.as_mut(), cx, cipher, output) { - if matches!(res, Poll::Ready(Ok(0))) { - zero_length_chunks += 1; - if zero_length_chunks > Self::ZERO_LENGTH_DOS_LIMIT { - return ioerror(Error::ZeroLengthRead); - } - continue; - } return res; } @@ -552,16 +544,10 @@ impl ChunkReader { if last { *self = Self::Done; } else { - *self = Self::length(); if pt.is_empty() { - // We can't return zero length, as that means "end of stream". - // So read the next chunk if this one was empty. - zero_length_chunks += 1; - if !last && zero_length_chunks > Self::ZERO_LENGTH_DOS_LIMIT { - return ioerror(Error::ZeroLengthRead); - } - continue; + return ioerror(Error::ZeroLengthRead); } + *self = Self::length(); } pt.len() }; @@ -1141,14 +1127,11 @@ mod test { let mut projection = pin.project(); let mut cx = noop_context(); - // Write out two zero-length chunks before finalizing the request. - for _ in 0..2 { - let f = ChunkWriter::flush(&mut projection, &mut cx); - assert!(f.is_ready()); - assert!(projection.buf.is_empty()); - ChunkWriter::write_chunk(&mut projection, &mut cx, &[], false).unwrap(); - } - + // Write out a zero-length chunk before finalizing the request. + let f = ChunkWriter::flush(&mut projection, &mut cx); + assert!(f.is_ready()); + assert!(projection.buf.is_empty()); + ChunkWriter::write_chunk(&mut projection, &mut cx, &[], false).unwrap(); client_request.write_all(REQUEST).sync_resolve().unwrap(); let mut buf = Vec::new(); From 164a1114ae8fa102df8f270e224df5b340d99d9e Mon Sep 17 00:00:00 2001 From: Martin Thomson Date: Thu, 27 Nov 2025 16:02:34 +1100 Subject: [PATCH 3/3] Fix comment --- ohttp/src/stream.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ohttp/src/stream.rs b/ohttp/src/stream.rs index f3a8c77..6b2b92c 100644 --- a/ohttp/src/stream.rs +++ b/ohttp/src/stream.rs @@ -1112,7 +1112,7 @@ mod test { assert_eq!(server_request.sync_read_to_end(), LONG_REQUEST); } - /// Check that repeated zero-length chunks are treated as invalid. + /// Check that zero-length chunks are treated as invalid. #[test] fn dos_zero_length() { init();