fix(multipart): do not parse with fixed index not to panic (#4024)

actix-multipart/CHANGES.md

@@ -4,6 +4,7 @@
 
 - Add multi-field multipart payload builders to `actix_multipart::test`. [#3575]
 - Add `MultipartForm` support for `Option<Vec<T>>` 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`.
 

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<Option<bool>, 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<Option<bool>, 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