From be4566d669a8e3900526616f9df6ec0ee71856c2 Mon Sep 17 00:00:00 2001 From: Yuki Okushi Date: Fri, 17 Apr 2026 00:32:45 +0900 Subject: [PATCH] fix(multipart): do not parse with fixed index not to panic (#4024) --- actix-multipart/CHANGES.md | 1 + actix-multipart/src/multipart.rs | 202 ++++++++++++++++++++++++++----- 2 files changed, 171 insertions(+), 32 deletions(-) diff --git a/actix-multipart/CHANGES.md b/actix-multipart/CHANGES.md index 5e614a81e..4445bcd1e 100644 --- a/actix-multipart/CHANGES.md +++ b/actix-multipart/CHANGES.md @@ -4,6 +4,7 @@ - Add multi-field multipart payload builders to `actix_multipart::test`. [#3575] - Add `MultipartForm` support for `Option>` fields. [#3577] +- Fix user-triggerable panic when parsing multipart boundaries. - Minimum supported Rust version (MSRV) is now 1.88. - Update `rand` dependency to `0.10`. diff --git a/actix-multipart/src/multipart.rs b/actix-multipart/src/multipart.rs index e38fbde9e..be0bc59f0 100644 --- a/actix-multipart/src/multipart.rs +++ b/actix-multipart/src/multipart.rs @@ -84,6 +84,10 @@ impl Multipart { .as_str() .to_owned(); + if boundary.is_empty() { + return Err(Error::BoundaryMissing); + } + Ok((content_type, boundary)) } @@ -239,6 +243,10 @@ impl Inner { /// - `Ok(None)` - boundary not found, more data needs reading /// - `Err(BoundaryMissing)` - multipart boundary is missing fn read_boundary(payload: &mut PayloadBuffer, boundary: &str) -> Result, Error> { + if boundary.is_empty() { + return Err(Error::BoundaryMissing); + } + // TODO: need to read epilogue let chunk = match payload.readline_or_eof()? { // TODO: this might be okay as a let Some() else return Ok(None) @@ -249,34 +257,21 @@ impl Inner { const BOUNDARY_MARKER: &[u8] = b"--"; const LINE_BREAK: &[u8] = b"\r\n"; - let boundary_len = boundary.len(); - - if chunk.len() < boundary_len + 2 + 2 - || !chunk.starts_with(BOUNDARY_MARKER) - || &chunk[2..boundary_len + 2] != boundary.as_bytes() - { + let Some(chunk) = chunk.as_ref().strip_prefix(BOUNDARY_MARKER) else { return Err(Error::BoundaryMissing); - } + }; - // chunk facts: - // - long enough to contain boundary + 2 markers or 1 marker and line-break - // - starts with boundary marker - // - chunk contains correct boundary + let Some(chunk) = chunk.strip_prefix(boundary.as_bytes()) else { + return Err(Error::BoundaryMissing); + }; - if &chunk[boundary_len + 2..] == LINE_BREAK { + if chunk == LINE_BREAK { // boundary is followed by line-break, indicating more fields to come return Ok(Some(false)); } // boundary is followed by marker - if &chunk[boundary_len + 2..boundary_len + 4] == BOUNDARY_MARKER - && ( - // chunk is exactly boundary len + 2 markers - chunk.len() == boundary_len + 2 + 2 - // final boundary is allowed to end with a line-break - || &chunk[boundary_len + 4..] == LINE_BREAK - ) - { + if chunk == BOUNDARY_MARKER || chunk == b"--\r\n" { return Ok(Some(true)); } @@ -287,7 +282,12 @@ impl Inner { payload: &mut PayloadBuffer, boundary: &str, ) -> Result, Error> { + if boundary.is_empty() { + return Err(Error::BoundaryMissing); + } + let mut eof = false; + let boundary = boundary.as_bytes(); loop { match payload.readline()? { @@ -295,19 +295,17 @@ impl Inner { if chunk.is_empty() { return Err(Error::BoundaryMissing); } - if chunk.len() < boundary.len() { + + let Some(line) = chunk.as_ref().strip_suffix(b"\r\n") else { continue; - } - if &chunk[..2] == b"--" && &chunk[2..chunk.len() - 2] == boundary.as_bytes() { - break; - } else { - if chunk.len() < boundary.len() + 2 { - continue; + }; + + if let Some(line) = line.strip_prefix(b"--") { + if line == boundary { + break; } - let b: &[u8] = boundary.as_ref(); - if &chunk[..boundary.len()] == b - && &chunk[boundary.len()..boundary.len() + 2] == b"--" - { + + if line.strip_suffix(b"--") == Some(boundary) { eof = true; break; } @@ -589,11 +587,151 @@ mod tests { (bytes, headers) } + fn create_header(content_type: &'static str) -> HeaderMap { + let mut headers = HeaderMap::new(); + headers.insert( + header::CONTENT_TYPE, + header::HeaderValue::from_static(content_type), + ); + headers + } + + #[actix_rt::test] + async fn empty_boundary_does_not_panic() { + let payload = stream::once(async { Ok(Bytes::from_static(b"\n")) }); + let ct = "multipart/mixed; boundary=\"a\"".parse().unwrap(); + + let mut multipart = Multipart::from_ct_and_boundary(ct, String::new(), payload); + let res = multipart.next().await.unwrap(); + assert_matches!(res, Err(Error::BoundaryMissing)); + } + + #[actix_rt::test] + async fn short_line_with_one_byte_boundary_does_not_panic() { + let bytes = Bytes::from_static(b"\n"); + let mut headers = HeaderMap::new(); + headers.insert( + header::CONTENT_TYPE, + header::HeaderValue::from_static("multipart/mixed; boundary=\"a\""), + ); + let payload = stream::once(async { Ok(bytes) }); + + let mut multipart = Multipart::new(&headers, payload); + let res = multipart.next().await.unwrap(); + assert_matches!(res, Err(Error::Incomplete)); + } + + #[actix_rt::test] + async fn short_final_boundary_with_one_byte_boundary_does_not_panic() { + let bytes = Bytes::from_static(b"--\n"); + let mut headers = HeaderMap::new(); + headers.insert( + header::CONTENT_TYPE, + header::HeaderValue::from_static("multipart/mixed; boundary=\"a\""), + ); + let payload = stream::once(async { Ok(bytes) }); + + let mut multipart = Multipart::new(&headers, payload); + let res = multipart.next().await.unwrap(); + assert_matches!(res, Err(Error::Incomplete)); + } + + #[actix_rt::test] + async fn one_byte_boundary_parses_valid_body() { + let bytes = Bytes::from_static( + b"preamble\r\n\ + --a\r\n\ + Content-Type: text/plain\r\n\ + Content-Length: 3\r\n\ + \r\n\ + one\r\n\ + --a\r\n\ + Content-Type: text/plain\r\n\ + Content-Length: 3\r\n\ + \r\n\ + two\r\n\ + --a--\r\n", + ); + let headers = create_header("multipart/mixed; boundary=\"a\""); + let payload = stream::once(async { Ok(bytes) }); + + let mut multipart = Multipart::new(&headers, payload); + + let mut field = multipart.next().await.unwrap().unwrap(); + assert_eq!(get_whole_field(&mut field).await, "one"); + drop(field); + + let mut field = multipart.next().await.unwrap().unwrap(); + assert_eq!(get_whole_field(&mut field).await, "two"); + drop(field); + + assert!(multipart.next().await.is_none()); + } + + #[actix_rt::test] + async fn one_byte_boundary_parses_when_split_across_chunks() { + let bytes = Bytes::from_static( + b"x\r\n\ + --a\r\n\ + Content-Type: text/plain\r\n\ + Content-Length: 4\r\n\ + \r\n\ + data\r\n\ + --a--\r\n", + ); + let headers = create_header("multipart/mixed; boundary=\"a\""); + let payload = stream::iter(bytes) + .map(|byte| Ok(Bytes::copy_from_slice(&[byte]))) + .interleave_pending(); + + let mut multipart = Multipart::new(&headers, payload); + + let mut field = multipart.next().await.unwrap().unwrap(); + assert_eq!(get_whole_field(&mut field).await, "data"); + drop(field); + + assert!(multipart.next().await.is_none()); + } + + #[actix_rt::test] + async fn short_preamble_lines_before_boundary_are_skipped() { + let bytes = Bytes::from_static( + b"\n\ + -\r\n\ + --a\r\n\ + Content-Type: text/plain\r\n\ + Content-Length: 4\r\n\ + \r\n\ + data\r\n\ + --a--\r\n", + ); + let headers = create_header("multipart/mixed; boundary=\"a\""); + let payload = stream::once(async { Ok(bytes) }); + + let mut multipart = Multipart::new(&headers, payload); + + let mut field = multipart.next().await.unwrap().unwrap(); + assert_eq!(get_whole_field(&mut field).await, "data"); + drop(field); + + assert!(multipart.next().await.is_none()); + } + + #[actix_rt::test] + async fn first_boundary_can_be_final() { + let bytes = Bytes::from_static(b"--a--\r\n"); + let headers = create_header("multipart/mixed; boundary=\"a\""); + let payload = stream::once(async { Ok(bytes) }); + + let mut multipart = Multipart::new(&headers, payload); + assert!(multipart.next().await.is_none()); + } + #[actix_rt::test] async fn test_multipart_no_end_crlf() { let (sender, payload) = create_stream(); let (mut bytes, headers) = create_double_request_with_header(); - let bytes_stripped = bytes.split_to(bytes.len()); // strip crlf + let bytes_stripped = bytes.split_to(bytes.len() - 2); // strip crlf sender.send(Ok(bytes_stripped)).unwrap(); drop(sender); // eof