diff --git a/actix-files/CHANGES.md b/actix-files/CHANGES.md index 7c4d008d8..307f5852d 100644 --- a/actix-files/CHANGES.md +++ b/actix-files/CHANGES.md @@ -3,6 +3,9 @@ ## Unreleased - Minimum supported Rust version (MSRV) is now 1.88. +- `PathBufWrap` & `UriSegmentError` made public. [#3694] + +[#3694]: https://github.com/actix/actix-web/pull/3694 ## 0.6.9 diff --git a/actix-files/src/error.rs b/actix-files/src/error.rs index e762116e6..1ba4ce67e 100644 --- a/actix-files/src/error.rs +++ b/actix-files/src/error.rs @@ -21,6 +21,7 @@ impl ResponseError for FilesError { } } +/// Error which can occur with parsing/validating a request-uri path #[derive(Debug, PartialEq, Eq, Display)] #[non_exhaustive] pub enum UriSegmentError { diff --git a/actix-files/src/lib.rs b/actix-files/src/lib.rs index 9859db456..b0bb76ace 100644 --- a/actix-files/src/lib.rs +++ b/actix-files/src/lib.rs @@ -37,13 +37,12 @@ mod range; mod service; pub use self::{ - chunked::ChunkedReadFile, directory::Directory, files::Files, named::NamedFile, - range::HttpRange, service::FilesService, + chunked::ChunkedReadFile, directory::Directory, error::UriSegmentError, files::Files, + named::NamedFile, path_buf::PathBufWrap, range::HttpRange, service::FilesService, }; use self::{ directory::{directory_listing, DirectoryRenderer}, error::FilesError, - path_buf::PathBufWrap, }; type HttpService = BoxService; diff --git a/actix-files/src/path_buf.rs b/actix-files/src/path_buf.rs index c1983279b..0fe8493bf 100644 --- a/actix-files/src/path_buf.rs +++ b/actix-files/src/path_buf.rs @@ -8,8 +8,11 @@ use actix_web::{dev::Payload, FromRequest, HttpRequest}; use crate::error::UriSegmentError; +/// Secure Path Traversal Guard +/// +/// This struct parses a request-uri [`PathBuf`](std::path::PathBuf) #[derive(Debug, PartialEq, Eq)] -pub(crate) struct PathBufWrap(PathBuf); +pub struct PathBufWrap(PathBuf); impl FromStr for PathBufWrap { type Err = UriSegmentError; @@ -20,6 +23,37 @@ impl FromStr for PathBufWrap { } impl PathBufWrap { + /// Parse a safe path from the unprocessed tail of a supplied + /// [`HttpRequest`](actix_web::HttpRequest), given the choice of allowing hidden files to be + /// considered valid segments. + /// + /// This uses [`HttpRequest::match_info`](actix_web::HttpRequest::match_info) and + /// [`Path::unprocessed`](actix_web::dev::Path::unprocessed), which returns the part of the + /// path not matched by route patterns. This is useful for mounted services (eg. `Files`), + /// where only the tail should be parsed. + /// + /// Path traversal is guarded by this method. + #[inline] + pub fn parse_unprocessed_req( + req: &HttpRequest, + hidden_files: bool, + ) -> Result { + Self::parse_path(req.match_info().unprocessed(), hidden_files) + } + + /// Parse a safe path from the full request path of a supplied + /// [`HttpRequest`](actix_web::HttpRequest), given the choice of allowing hidden files to be + /// considered valid segments. + /// + /// This uses [`HttpRequest::path`](actix_web::HttpRequest::path), and is more appropriate + /// for non-mounted handlers that want the entire request path. + /// + /// Path traversal is guarded by this method. + #[inline] + pub fn parse_req_path(req: &HttpRequest, hidden_files: bool) -> Result { + Self::parse_path(req.path(), hidden_files) + } + /// Parse a path, giving the choice of allowing hidden files to be considered valid segments. /// /// Path traversal is guarded by this method. @@ -91,6 +125,7 @@ impl FromRequest for PathBufWrap { type Future = Ready>; fn from_request(req: &HttpRequest, _: &mut Payload) -> Self::Future { + // Uses the unprocessed tail of the request path and disallows hidden files. ready(req.match_info().unprocessed().parse()) } } diff --git a/actix-http/CHANGES.md b/actix-http/CHANGES.md index 197370bdf..f052c6d22 100644 --- a/actix-http/CHANGES.md +++ b/actix-http/CHANGES.md @@ -3,6 +3,9 @@ ## Unreleased - Minimum supported Rust version (MSRV) is now 1.88. +- Fix truncated body ending without error when connection closed abnormally. [#3067] + +[#3067]: https://github.com/actix/actix-web/pull/3067 ## 3.11.2 diff --git a/actix-http/src/h1/dispatcher.rs b/actix-http/src/h1/dispatcher.rs index 03851d0fb..c59be2d50 100644 --- a/actix-http/src/h1/dispatcher.rs +++ b/actix-http/src/h1/dispatcher.rs @@ -1156,6 +1156,7 @@ where let inner = inner.as_mut().project(); inner.flags.insert(Flags::READ_DISCONNECT); if let Some(mut payload) = inner.payload.take() { + payload.set_error(PayloadError::Incomplete(None)); payload.feed_eof(); } }; diff --git a/actix-http/tests/test_rustls.rs b/actix-http/tests/test_rustls.rs index 29e559666..b58c1138d 100644 --- a/actix-http/tests/test_rustls.rs +++ b/actix-http/tests/test_rustls.rs @@ -51,7 +51,7 @@ where Ok(buf) } -fn tls_config() -> RustlsServerConfig { +fn tls_config_with_alpn(protocols: &[&[u8]]) -> RustlsServerConfig { let rcgen::CertifiedKey { cert, key_pair } = rcgen::generate_simple_self_signed(["localhost".to_owned()]).unwrap(); let cert_chain = vec![cert.der().clone()]; @@ -62,12 +62,23 @@ fn tls_config() -> RustlsServerConfig { .with_single_cert(cert_chain, key_der) .unwrap(); - config.alpn_protocols.push(HTTP1_1_ALPN_PROTOCOL.to_vec()); - config.alpn_protocols.push(H2_ALPN_PROTOCOL.to_vec()); + config.alpn_protocols = protocols.iter().map(|proto| proto.to_vec()).collect(); config } +fn tls_config() -> RustlsServerConfig { + tls_config_with_alpn(&[HTTP1_1_ALPN_PROTOCOL, H2_ALPN_PROTOCOL]) +} + +fn tls_config_h1() -> RustlsServerConfig { + tls_config_with_alpn(&[HTTP1_1_ALPN_PROTOCOL]) +} + +fn tls_config_h2() -> RustlsServerConfig { + tls_config_with_alpn(&[H2_ALPN_PROTOCOL]) +} + pub fn get_negotiated_alpn_protocol( addr: SocketAddr, client_alpn_protocol: &[u8], @@ -98,7 +109,7 @@ async fn h1() -> io::Result<()> { let srv = test_server(move || { HttpService::build() .h1(|_| ok::<_, Error>(Response::ok())) - .rustls_0_23(tls_config()) + .rustls_0_23(tls_config_h1()) }) .await; @@ -112,7 +123,7 @@ async fn h2() -> io::Result<()> { let srv = test_server(move || { HttpService::build() .h2(|_| ok::<_, Error>(Response::ok())) - .rustls_0_23(tls_config()) + .rustls_0_23(tls_config_h2()) }) .await; @@ -130,7 +141,7 @@ async fn h1_1() -> io::Result<()> { assert_eq!(req.version(), Version::HTTP_11); ok::<_, Error>(Response::ok()) }) - .rustls_0_23(tls_config()) + .rustls_0_23(tls_config_h1()) }) .await; @@ -149,7 +160,7 @@ async fn h2_1() -> io::Result<()> { ok::<_, Error>(Response::ok()) }) .rustls_0_23_with_config( - tls_config(), + tls_config_h2(), TlsAcceptorConfig::default().handshake_timeout(Duration::from_secs(5)), ) }) @@ -169,7 +180,7 @@ async fn h2_body1() -> io::Result<()> { let body = load_body(req.take_payload()).await?; Ok::<_, Error>(Response::ok().set_body(body)) }) - .rustls_0_23(tls_config()) + .rustls_0_23(tls_config_h2()) }) .await; @@ -195,7 +206,7 @@ async fn h2_content_length() { ]; ok::<_, Infallible>(Response::new(statuses[indx])) }) - .rustls_0_23(tls_config()) + .rustls_0_23(tls_config_h2()) }) .await; @@ -267,7 +278,7 @@ async fn h2_headers() { } ok::<_, Infallible>(config.body(data.clone())) }) - .rustls_0_23(tls_config()) + .rustls_0_23(tls_config_h2()) }) .await; @@ -306,7 +317,7 @@ async fn h2_body2() { let mut srv = test_server(move || { HttpService::build() .h2(|_| ok::<_, Infallible>(Response::ok().set_body(STR))) - .rustls_0_23(tls_config()) + .rustls_0_23(tls_config_h2()) }) .await; @@ -323,7 +334,7 @@ async fn h2_head_empty() { let mut srv = test_server(move || { HttpService::build() .finish(|_| ok::<_, Infallible>(Response::ok().set_body(STR))) - .rustls_0_23(tls_config()) + .rustls_0_23(tls_config_h2()) }) .await; @@ -349,7 +360,7 @@ async fn h2_head_binary() { let mut srv = test_server(move || { HttpService::build() .h2(|_| ok::<_, Infallible>(Response::ok().set_body(STR))) - .rustls_0_23(tls_config()) + .rustls_0_23(tls_config_h2()) }) .await; @@ -374,7 +385,7 @@ async fn h2_head_binary2() { let srv = test_server(move || { HttpService::build() .h2(|_| ok::<_, Infallible>(Response::ok().set_body(STR))) - .rustls_0_23(tls_config()) + .rustls_0_23(tls_config_h2()) }) .await; @@ -400,7 +411,7 @@ async fn h2_body_length() { Response::ok().set_body(SizedStream::new(STR.len() as u64, body)), ) }) - .rustls_0_23(tls_config()) + .rustls_0_23(tls_config_h2()) }) .await; @@ -424,7 +435,7 @@ async fn h2_body_chunked_explicit() { .body(BodyStream::new(body)), ) }) - .rustls_0_23(tls_config()) + .rustls_0_23(tls_config_h2()) }) .await; @@ -453,7 +464,7 @@ async fn h2_response_http_error_handling() { ) })) })) - .rustls_0_23(tls_config()) + .rustls_0_23(tls_config_h2()) }) .await; @@ -483,7 +494,7 @@ async fn h2_service_error() { let mut srv = test_server(move || { HttpService::build() .h2(|_| err::, _>(BadRequest)) - .rustls_0_23(tls_config()) + .rustls_0_23(tls_config_h2()) }) .await; @@ -500,7 +511,7 @@ async fn h1_service_error() { let mut srv = test_server(move || { HttpService::build() .h1(|_| err::, _>(BadRequest)) - .rustls_0_23(tls_config()) + .rustls_0_23(tls_config_h1()) }) .await; @@ -519,7 +530,7 @@ const CUSTOM_ALPN_PROTOCOL: &[u8] = b"custom"; #[actix_rt::test] async fn alpn_h1() -> io::Result<()> { let srv = test_server(move || { - let mut config = tls_config(); + let mut config = tls_config_h1(); config.alpn_protocols.push(CUSTOM_ALPN_PROTOCOL.to_vec()); HttpService::build() .h1(|_| ok::<_, Error>(Response::ok())) @@ -541,7 +552,7 @@ async fn alpn_h1() -> io::Result<()> { #[actix_rt::test] async fn alpn_h2() -> io::Result<()> { let srv = test_server(move || { - let mut config = tls_config(); + let mut config = tls_config_h2(); config.alpn_protocols.push(CUSTOM_ALPN_PROTOCOL.to_vec()); HttpService::build() .h2(|_| ok::<_, Error>(Response::ok())) diff --git a/actix-http/tests/test_server.rs b/actix-http/tests/test_server.rs index aafcde19a..688fc9d0b 100644 --- a/actix-http/tests/test_server.rs +++ b/actix-http/tests/test_server.rs @@ -443,6 +443,60 @@ async fn content_length() { srv.stop().await; } +#[actix_rt::test] +async fn content_length_truncated() { + use tokio::io::{AsyncReadExt, AsyncWriteExt}; + + let mut srv = test_server(|| { + HttpService::build() + .h1(|mut req: Request| async move { + let expected_length: usize = req.uri().path()[1..].parse().unwrap(); + let mut payload = req.take_payload(); + + let mut length = 0; + let mut seen_error = false; + while let Some(chunk) = payload.next().await { + match chunk { + Ok(b) => length += b.len(), + Err(_) => { + seen_error = true; + break; + } + } + } + if seen_error { + return Result::<_, Infallible>::Ok(Response::bad_request()); + } + + assert_eq!(length, expected_length, "length must match when no error"); + Result::<_, Infallible>::Ok(Response::ok()) + }) + .tcp() + }) + .await; + + let addr = srv.addr(); + let mut buf = [0; 12]; + + let mut conn = TcpStream::connect(&addr).await.unwrap(); + conn.write_all(b"POST /10000 HTTP/1.1\r\nContent-Length: 10000\r\n\r\ndata_truncated") + .await + .unwrap(); + conn.shutdown().await.unwrap(); + conn.read_exact(&mut buf).await.unwrap(); + assert_eq!(&buf, b"HTTP/1.1 400"); + + let mut conn = TcpStream::connect(&addr).await.unwrap(); + conn.write_all(b"POST /4 HTTP/1.1\r\nContent-Length: 4\r\n\r\ndata") + .await + .unwrap(); + conn.shutdown().await.unwrap(); + conn.read_exact(&mut buf).await.unwrap(); + assert_eq!(&buf, b"HTTP/1.1 200"); + + srv.stop().await; +} + #[actix_rt::test] async fn h1_headers() { let data = STR.repeat(10);