diff --git a/actix-files/CHANGES.md b/actix-files/CHANGES.md index 3e1590991..501181d92 100644 --- a/actix-files/CHANGES.md +++ b/actix-files/CHANGES.md @@ -1,7 +1,7 @@ # Changes ## Unreleased - 2021-xx-xx -- `Files`: `%2F` in request URL path is now decoded to `/` and thus functions as a path separator. [#2398] +- `Files`: request URL paths with `%2F` are now rejected. [#2398] - `Files`: Fixed a regression where `%25` in the URL path is not decoded to `%` in the file path. [#2398] - Minimum supported Rust version (MSRV) is now 1.54. diff --git a/actix-files/src/error.rs b/actix-files/src/error.rs index 242eda4be..d28889e73 100644 --- a/actix-files/src/error.rs +++ b/actix-files/src/error.rs @@ -23,16 +23,20 @@ impl ResponseError for FilesError { #[allow(clippy::enum_variant_names)] #[derive(Display, Debug, PartialEq)] +#[non_exhaustive] pub enum UriSegmentError { /// The segment started with the wrapped invalid character. #[display(fmt = "The segment started with the wrapped invalid character")] BadStart(char), + /// The segment contained the wrapped invalid character. #[display(fmt = "The segment contained the wrapped invalid character")] BadChar(char), + /// The segment ended with the wrapped invalid character. #[display(fmt = "The segment ended with the wrapped invalid character")] BadEnd(char), + /// The path is not a valid UTF-8 string after doing percent decoding. #[display(fmt = "The path is not a valif UTF-8 string after percent-decoding")] NotValidUtf8, diff --git a/actix-files/src/files.rs b/actix-files/src/files.rs index 9d35472bc..adfb93232 100644 --- a/actix-files/src/files.rs +++ b/actix-files/src/files.rs @@ -28,16 +28,6 @@ use crate::{ /// /// `Files` service must be registered with `App::service()` method. /// -/// # Percent-Encoding and Security Considerations -/// -/// When converting the request URL path into the target [file path](std::path::Path), -/// `Files` service *does* decode *all* percent-encoded characters in the path string. -/// One implication is that the resulting file path may have more components than the URL path -/// as a result of decoding `%2F` into `/`. -/// -/// Any middleware that is responsible for validating the paths managed under `Files` -/// should be aware of this behavior. -/// /// # Examples /// ``` /// use actix_web::App; diff --git a/actix-files/src/lib.rs b/actix-files/src/lib.rs index 5d141d6ea..a11aa32c7 100644 --- a/actix-files/src/lib.rs +++ b/actix-files/src/lib.rs @@ -805,11 +805,9 @@ mod tests { assert_eq!(res.status(), StatusCode::OK); // `%2F` == `/` - let req = TestRequest::get() - .uri("/test/%2F..%2F..%2Ftests%2Ftest.binary") - .to_request(); + let req = TestRequest::get().uri("/test%2Ftest.binary").to_request(); let res = test::call_service(&srv, req).await; - assert_eq!(res.status(), StatusCode::OK); + assert_eq!(res.status(), StatusCode::NOT_FOUND); let req = TestRequest::get().uri("/test/Cargo.toml%00").to_request(); let res = test::call_service(&srv, req).await; diff --git a/actix-files/src/path_buf.rs b/actix-files/src/path_buf.rs index e46d3f232..fdd69c10c 100644 --- a/actix-files/src/path_buf.rs +++ b/actix-files/src/path_buf.rs @@ -1,5 +1,5 @@ use std::{ - path::{Path, PathBuf}, + path::{Component, Path, PathBuf}, str::FromStr, }; @@ -26,12 +26,21 @@ impl PathBufWrap { pub fn parse_path(path: &str, hidden_files: bool) -> Result { let mut buf = PathBuf::new(); + // equivalent to `path.split('/').count()` + let mut segment_count = path.matches('/').count() + 1; + let path = percent_encoding::percent_decode_str(path) .decode_utf8() .map_err(|_| UriSegmentError::NotValidUtf8)?; + // disallow decoding `%2F` into `/` + if segment_count != path.matches('/').count() + 1 { + return Err(UriSegmentError::BadChar('/')); + } + for segment in path.split('/') { if segment == ".." { + segment_count -= 1; buf.pop(); } else if !hidden_files && segment.starts_with('.') { return Err(UriSegmentError::BadStart('.')); @@ -44,6 +53,7 @@ impl PathBufWrap { } else if segment.ends_with('<') { return Err(UriSegmentError::BadEnd('<')); } else if segment.is_empty() { + segment_count -= 1; continue; } else if cfg!(windows) && segment.contains('\\') { return Err(UriSegmentError::BadChar('\\')); @@ -52,6 +62,12 @@ impl PathBufWrap { } } + // make sure we agree with stdlib parser + for (i, component) in buf.components().enumerate() { + assert!(matches!(component, Component::Normal(_))); + assert!(i < segment_count); + } + Ok(PathBufWrap(buf)) } }