diff --git a/actix-http/CHANGES.md b/actix-http/CHANGES.md index ba4d0aa9d..115774f6a 100644 --- a/actix-http/CHANGES.md +++ b/actix-http/CHANGES.md @@ -2,6 +2,7 @@ ## Unreleased +- Reject HTTP/1 requests with ambiguous request framing from `Content-Length` and `Transfer-Encoding` headers to prevent request smuggling. - Encode the HTTP/1 `Connection: Upgrade` header in Camel-Case when camel-case header formatting is enabled.[#3953] - Fix `HeaderMap` iterators' `len()` and `size_hint()` implementations for multi-value headers. - Update `rand` dependency to `0.10`. diff --git a/actix-http/src/h1/codec.rs b/actix-http/src/h1/codec.rs index 2b452f8f8..13fc7e6c5 100644 --- a/actix-http/src/h1/codec.rs +++ b/actix-http/src/h1/codec.rs @@ -237,4 +237,18 @@ mod tests { assert_eq!(*req.method(), Method::POST); assert!(req.chunked().unwrap()); } + + #[actix_rt::test] + async fn test_http_request_rejects_content_length_and_chunked() { + let mut codec = Codec::default(); + let mut buf = BytesMut::from( + "POST /test HTTP/1.1\r\n\ + content-length: 11\r\n\ + transfer-encoding: chunked\r\n\r\n\ + 0\r\n\r\n\ + GET /test2 HTTP/1.1\r\n\r\n", + ); + + assert!(codec.decode(&mut buf).is_err()); + } } diff --git a/actix-http/src/h1/decoder.rs b/actix-http/src/h1/decoder.rs index af64e8802..5170ea4a1 100644 --- a/actix-http/src/h1/decoder.rs +++ b/actix-http/src/h1/decoder.rs @@ -275,6 +275,23 @@ impl MessageType for Request { // convert headers let mut length = msg.set_headers(&src.split_to(len).freeze(), &headers[..h_len], ver)?; + if msg.head().headers.contains_key(header::TRANSFER_ENCODING) { + if ver == Version::HTTP_10 { + debug!("Transfer-Encoding is not allowed in HTTP/1.0 requests"); + return Err(ParseError::Header); + } + + if !crate::HttpMessage::chunked(&msg)? { + debug!("request Transfer-Encoding must be chunked"); + return Err(ParseError::Header); + } + + if msg.head().headers.contains_key(header::CONTENT_LENGTH) { + debug!("both Content-Length and Transfer-Encoding are set"); + return Err(ParseError::Header); + } + } + // disallow HTTP/1.0 POST requests that do not contain a Content-Length headers // see https://datatracker.ietf.org/doc/html/rfc1945#section-7.2.2 if ver == Version::HTTP_10 && method == Method::POST && length.is_none() { @@ -1116,18 +1133,57 @@ mod tests { #[test] fn hrs_cl_and_te_http10() { - // in HTTP/1.0 transfer encoding is simply ignored so it's fine to have both - - let mut buf = BytesMut::from( + expect_parse_err!(&mut BytesMut::from( "GET / HTTP/1.0\r\n\ Host: example.com\r\n\ Content-Length: 3\r\n\ Transfer-Encoding: chunked\r\n\ \r\n\ 000", - ); + )); + } - parse_ready!(&mut buf); + #[test] + fn hrs_cl_and_chunked_te_http11() { + expect_parse_err!(&mut BytesMut::from( + "POST / HTTP/1.1\r\n\ + Host: example.com\r\n\ + Content-Length: 3\r\n\ + Transfer-Encoding: chunked\r\n\ + \r\n\ + 0\r\n\ + \r\n", + )); + + expect_parse_err!(&mut BytesMut::from( + "POST / HTTP/1.1\r\n\ + Host: example.com\r\n\ + Transfer-Encoding: chunked\r\n\ + Content-Length: 3\r\n\ + \r\n\ + 0\r\n\ + \r\n", + )); + } + + #[test] + fn hrs_identity_te_http11() { + expect_parse_err!(&mut BytesMut::from( + "POST / HTTP/1.1\r\n\ + Host: example.com\r\n\ + Transfer-Encoding: identity\r\n\ + \r\n\ + 0\r\n", + )); + + expect_parse_err!(&mut BytesMut::from( + "POST / HTTP/1.1\r\n\ + Host: example.com\r\n\ + Content-Length: 3\r\n\ + Transfer-Encoding: identity\r\n\ + \r\n\ + 0\r\n", + )); } #[test] @@ -1165,14 +1221,16 @@ mod tests { } #[test] - fn transfer_encoding_agrees() { + fn hrs_chunked_te_http11() { let mut buf = BytesMut::from( "GET /test HTTP/1.1\r\n\ Host: example.com\r\n\ - Content-Length: 3\r\n\ - Transfer-Encoding: identity\r\n\ + Transfer-Encoding: chunked\r\n\ \r\n\ - 0\r\n", + 1\r\n\ + a\r\n\ + 0\r\n\ + \r\n", ); let mut reader = MessageDecoder::::default(); @@ -1180,6 +1238,6 @@ mod tests { let mut pl = pl.unwrap(); let chunk = pl.decode(&mut buf).unwrap().unwrap(); - assert_eq!(chunk, PayloadItem::Chunk(Bytes::from_static(b"0\r\n"))); + assert_eq!(chunk, PayloadItem::Chunk(Bytes::from_static(b"a"))); } }