diff --git a/Cargo.lock b/Cargo.lock index e50d073da..76d9c6f1b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9120,6 +9120,7 @@ dependencies = [ "percent-encoding", "pin-project-lite", "pprof-pyroscope-fork", + "proptest", "rand 0.10.1", "rcgen", "reqwest", diff --git a/crates/protocols/src/ftps/driver.rs b/crates/protocols/src/ftps/driver.rs index 7d6723f8e..2e00c58d7 100644 --- a/crates/protocols/src/ftps/driver.rs +++ b/crates/protocols/src/ftps/driver.rs @@ -39,6 +39,22 @@ const EVENT_FTPS_DIRECTORY_STATE: &str = "ftps_directory_state"; const EVENT_FTPS_CWD_FAILED: &str = "ftps_cwd_failed"; const EVENT_FTPS_RENAME_STATE: &str = "ftps_rename_state"; +fn parse_s3_path(path_input: &str) -> std::result::Result<(String, Option), String> { + if path_input.chars().any(char::is_control) { + return Err("control characters are not allowed in FTPS paths".to_string()); + } + + let cleaned_path = path::clean(path_input); + let (bucket, object) = path::path_to_bucket_object(&cleaned_path); + + if object.contains(path::GLOBAL_DIR_SUFFIX) { + return Err("internal directory marker is not allowed in FTPS paths".to_string()); + } + + let key = if object.is_empty() { None } else { Some(object) }; + Ok((bucket, key)) +} + /// FTPS metadata implementation #[derive(Debug, Clone)] pub struct FtpsMetadata { @@ -148,14 +164,6 @@ where } } - fn parse_s3_path(&self, path: &str) -> std::result::Result<(String, Option), String> { - let cleaned_path = path::clean(path); - let (bucket, object) = path::path_to_bucket_object(&cleaned_path); - let key = if object.is_empty() { None } else { Some(object) }; - - Ok((bucket, key)) - } - /// Recursively delete all objects in a bucket, then delete the bucket itself. async fn delete_bucket_recursively( &self, @@ -249,8 +257,7 @@ where let path_str = path.as_ref().to_string_lossy(); let session_context = &user.session_context; - let (bucket, key) = self - .parse_s3_path(&path_str) + let (bucket, key) = parse_s3_path(&path_str) .map_err(|e| Error::new(ErrorKind::PermanentFileNotAvailable, format!("{}: {}", "Invalid path", e)))?; if let Some(key) = key { @@ -353,8 +360,7 @@ where return self.list_buckets(session_context).await; } - let (bucket, prefix) = self - .parse_s3_path(&path_str) + let (bucket, prefix) = parse_s3_path(&path_str) .map_err(|e| Error::new(ErrorKind::PermanentFileNotAvailable, format!("{}: {}", "Invalid path", e)))?; // Authorize the operation @@ -485,8 +491,7 @@ where let session_context = &user.session_context; let masked_username = MaskedAccessKey(&user.username); - let (bucket, key) = self - .parse_s3_path(&path_str) + let (bucket, key) = parse_s3_path(&path_str) .map_err(|e| Error::new(ErrorKind::PermanentFileNotAvailable, format!("{}: {}", "Invalid path", e)))?; let key = key.ok_or_else(|| Error::new(ErrorKind::PermanentFileNotAvailable, "Cannot get directory"))?; @@ -566,8 +571,7 @@ where let session_context = &user.session_context; let masked_username = MaskedAccessKey(&user.username); - let (bucket, key) = self - .parse_s3_path(&path_str) + let (bucket, key) = parse_s3_path(&path_str) .map_err(|e| Error::new(ErrorKind::PermanentFileNotAvailable, format!("{}: {}", "Invalid path", e)))?; let key = key.ok_or_else(|| Error::new(ErrorKind::PermanentFileNotAvailable, "Cannot put to directory"))?; @@ -658,8 +662,7 @@ where "FTPS delete requested" ); - let (bucket, key) = self - .parse_s3_path(&path_str) + let (bucket, key) = parse_s3_path(&path_str) .map_err(|e| Error::new(ErrorKind::PermanentFileNotAvailable, format!("{}: {}", "Invalid path", e)))?; if let Some(key) = key { @@ -726,8 +729,7 @@ where "ftps directory state changed" ); - let (bucket, _key) = self - .parse_s3_path(&path_str) + let (bucket, _key) = parse_s3_path(&path_str) .map_err(|e| Error::new(ErrorKind::PermanentFileNotAvailable, format!("{}: {}", "Invalid path", e)))?; // Create bucket for directory @@ -774,8 +776,7 @@ where let path_str = path.as_ref().to_string_lossy(); let session_context = &user.session_context; - let (bucket, _key) = self - .parse_s3_path(&path_str) + let (bucket, _key) = parse_s3_path(&path_str) .map_err(|e| Error::new(ErrorKind::PermanentFileNotAvailable, format!("{}: {}", "Invalid path", e)))?; // Authorize delete bucket @@ -831,8 +832,7 @@ where let path_str = path.as_ref().to_string_lossy(); let session_context = &user.session_context; - let (bucket, _key) = self - .parse_s3_path(&path_str) + let (bucket, _key) = parse_s3_path(&path_str) .map_err(|e| Error::new(ErrorKind::PermanentFileNotAvailable, format!("{}: {}", "Invalid path", e)))?; // Authorize HeadBucket (CWD probes bucket existence) @@ -886,3 +886,43 @@ where )) } } + +#[cfg(test)] +mod tests { + use super::parse_s3_path; + use rustfs_utils::path; + + proptest::proptest! { + #[test] + fn parse_s3_path_never_leaks_control_bytes_or_traversal_in_ok_output( + input in proptest::prelude::any::(), + ) { + match parse_s3_path(&input) { + Err(_) => {} + Ok((bucket, key)) => { + proptest::prop_assert!(!bucket.contains('/')); + proptest::prop_assert!(!bucket.chars().any(char::is_control)); + + if let Some(k) = key.as_deref() { + proptest::prop_assert!(!k.chars().any(char::is_control)); + proptest::prop_assert!(!k.starts_with('/')); + proptest::prop_assert!(!k.split('/').any(|segment| segment == "..")); + proptest::prop_assert!(!k.contains(path::GLOBAL_DIR_SUFFIX)); + } + } + } + } + } + + #[test] + fn parse_s3_path_rejects_control_bytes() { + assert!(parse_s3_path("/bucket/line\rfeed").is_err()); + assert!(parse_s3_path("/bucket/line\nfeed").is_err()); + assert!(parse_s3_path("/bucket/tab\tname").is_err()); + } + + #[test] + fn parse_s3_path_rejects_internal_directory_marker() { + assert!(parse_s3_path("/bucket/__XLDIR__").is_err()); + } +} diff --git a/crates/protocols/src/swift/object.rs b/crates/protocols/src/swift/object.rs index 6f0478062..caaf2e5ff 100644 --- a/crates/protocols/src/swift/object.rs +++ b/crates/protocols/src/swift/object.rs @@ -111,6 +111,14 @@ impl ObjectKeyMapper { return Err(SwiftError::BadRequest("Object name cannot contain null bytes".to_string())); } + if object.chars().any(char::is_control) { + return Err(SwiftError::BadRequest("Object name cannot contain control characters".to_string())); + } + + if object.starts_with('/') { + return Err(SwiftError::BadRequest("Object name cannot start with '/'".to_string())); + } + // Check for directory traversal attempts if object.contains("..") { // Allow ".." as part of a filename, but not as a path segment @@ -1115,6 +1123,7 @@ pub fn format_content_range(start: i64, end: i64, total: i64) -> String { #[cfg(test)] mod tests { use super::*; + use proptest::prelude::*; #[test] fn test_validate_object_name_valid() { @@ -1162,6 +1171,30 @@ mod tests { } } + #[test] + fn test_validate_object_name_control_character() { + let result = ObjectKeyMapper::validate_object_name("file\nname.txt"); + assert!(result.is_err()); + match result { + Err(SwiftError::BadRequest(msg)) => { + assert!(msg.contains("control")); + } + _ => panic!("Expected BadRequest error"), + } + } + + #[test] + fn test_validate_object_name_leading_slash() { + let result = ObjectKeyMapper::validate_object_name("/file.txt"); + assert!(result.is_err()); + match result { + Err(SwiftError::BadRequest(msg)) => { + assert!(msg.contains("start with '/'")); + } + _ => panic!("Expected BadRequest error"), + } + } + #[test] fn test_validate_object_name_directory_traversal() { // ".." as a path segment should be rejected @@ -1240,6 +1273,41 @@ mod tests { assert_eq!(ObjectKeyMapper::normalize_path("path///to/file.txt"), "path/to/file.txt"); } + proptest! { + #[test] + fn validate_object_name_ok_outputs_are_safe(input in any::()) { + if let Ok(()) = ObjectKeyMapper::validate_object_name(&input) { + prop_assert!(!input.is_empty()); + prop_assert!(input.len() <= 1024); + prop_assert!(!input.starts_with('/')); + prop_assert!(!input.chars().any(char::is_control)); + prop_assert!(!input.split('/').any(|segment| segment == "..")); + } + } + + #[test] + fn decode_object_from_url_round_trips_valid_input(input in any::()) { + let encoded = ObjectKeyMapper::encode_object_for_url(&input); + + match ObjectKeyMapper::decode_object_from_url(&encoded) { + Ok(decoded) => { + prop_assert_eq!(decoded.as_str(), input.as_str()); + prop_assert!(ObjectKeyMapper::validate_object_name(&decoded).is_ok()); + } + Err(_) => { + prop_assert!(ObjectKeyMapper::validate_object_name(&input).is_err()); + } + } + } + + #[test] + fn normalize_path_is_idempotent(input in any::()) { + let once = ObjectKeyMapper::normalize_path(&input); + let twice = ObjectKeyMapper::normalize_path(&once); + prop_assert_eq!(twice, once); + } + } + #[test] fn test_parse_destination_header() { // Valid destination headers diff --git a/crates/protocols/src/webdav/driver.rs b/crates/protocols/src/webdav/driver.rs index 3b6b67891..99237c452 100644 --- a/crates/protocols/src/webdav/driver.rs +++ b/crates/protocols/src/webdav/driver.rs @@ -770,6 +770,11 @@ where let decoded_path = percent_decode_str(&path_str) .decode_utf8() .map_err(|_| FsError::GeneralFailure)?; + + if decoded_path.chars().any(char::is_control) { + return Err(FsError::GeneralFailure); + } + let cleaned_path = path::clean(&decoded_path); let (bucket, object) = path::path_to_bucket_object(&cleaned_path); @@ -777,6 +782,10 @@ where return Ok((String::new(), None)); } + if object.contains(path::GLOBAL_DIR_SUFFIX) { + return Err(FsError::GeneralFailure); + } + let key = if object.is_empty() { None } else { Some(object) }; Ok((bucket, key)) } @@ -1689,6 +1698,7 @@ mod tests { use dav_server::davpath::DavPath; use dav_server::fs::FsError; use futures_util::StreamExt; + use percent_encoding::{NON_ALPHANUMERIC, utf8_percent_encode}; use rustfs_credentials::Credentials; use rustfs_policy::auth::UserIdentity; use s3s::dto::*; @@ -2186,6 +2196,58 @@ mod tests { assert_eq!(key.as_deref(), Some("file with spaces.txt")); } + #[test] + fn parse_path_rejects_control_bytes_after_decode() { + let driver = driver(); + let path = DavPath::new("/bucket/report%0Aname.txt").expect("path should parse"); + + let err = driver.parse_path(&path).expect_err("control bytes should be rejected"); + + assert_eq!(err, FsError::GeneralFailure); + } + + #[test] + fn parse_path_rejects_internal_directory_marker() { + let driver = driver(); + let path = DavPath::new("/bucket/__XLDIR__").expect("path should parse"); + + let err = driver + .parse_path(&path) + .expect_err("internal directory marker should be rejected"); + + assert_eq!(err, FsError::GeneralFailure); + } + + proptest::proptest! { + #[test] + fn parse_path_never_leaks_control_bytes_or_traversal_in_ok_output( + input in proptest::prelude::any::(), + ) { + let driver = driver(); + let encoded = utf8_percent_encode(&input, NON_ALPHANUMERIC).to_string(); + let Ok(path) = DavPath::new(&format!("/{encoded}")) else { + return Ok(()); + }; + + match driver.parse_path(&path) { + Err(err) => { + proptest::prop_assert_eq!(err, FsError::GeneralFailure); + } + Ok((bucket, key)) => { + proptest::prop_assert!(!bucket.contains('/')); + proptest::prop_assert!(!bucket.chars().any(char::is_control)); + + if let Some(k) = key.as_deref() { + proptest::prop_assert!(!k.chars().any(char::is_control)); + proptest::prop_assert!(!k.starts_with('/')); + proptest::prop_assert!(!k.split('/').any(|segment| segment == "..")); + proptest::prop_assert!(!k.contains(rustfs_utils::path::GLOBAL_DIR_SUFFIX)); + } + } + } + } + } + #[tokio::test] async fn directory_rename_returns_error_when_delete_fails_after_successful_copy() { let (driver, storage) = recording_driver( diff --git a/rustfs/Cargo.toml b/rustfs/Cargo.toml index 0a6a3e439..371412896 100644 --- a/rustfs/Cargo.toml +++ b/rustfs/Cargo.toml @@ -203,6 +203,7 @@ serial_test = { workspace = true } tempfile = { workspace = true } aws-config = { workspace = true } anyhow = { workspace = true } +proptest = "1" tokio = { workspace = true, features = ["test-util"] } temp-env = { workspace = true, features = ["async_closure"] } tracing-subscriber = { workspace = true } diff --git a/rustfs/src/storage/options.rs b/rustfs/src/storage/options.rs index 86401771d..0203023af 100644 --- a/rustfs/src/storage/options.rs +++ b/rustfs/src/storage/options.rs @@ -617,9 +617,17 @@ pub fn parse_copy_source_range(range_str: &str) -> S3Result { .parse::() .map_err(|_| s3_error!(InvalidArgument, "Invalid range format"))?; + if length <= 0 { + return Err(s3_error!(InvalidArgument, "Invalid range format")); + } + + let start = length + .checked_neg() + .ok_or_else(|| s3_error!(InvalidArgument, "Invalid range format"))?; + Ok(HTTPRangeSpec { is_suffix_length: true, - start: -length, + start, end: -1, }) } else { @@ -777,6 +785,7 @@ fn get_content_sha256_cksum(headers: &HeaderMap, service_type: Serv #[cfg(test)] mod tests { + use proptest::prelude::*; use temp_env; use super::*; @@ -1560,5 +1569,28 @@ mod tests { assert!(parse_copy_source_range("bytes=").is_err()); assert!(parse_copy_source_range("bytes=abc-def").is_err()); assert!(parse_copy_source_range("bytes=100-50").is_err()); // start > end + assert!(parse_copy_source_range("bytes=-0").is_err()); + assert!(parse_copy_source_range("bytes=--9223372036854775808").is_err()); + } + + proptest! { + #[test] + fn parse_copy_source_range_never_panics_and_preserves_output_invariants( + input in any::(), + ) { + match std::panic::catch_unwind(|| parse_copy_source_range(&input)) { + Ok(Ok(spec)) => { + if spec.is_suffix_length { + prop_assert_eq!(spec.end, -1); + prop_assert!(spec.start < 0); + } else { + prop_assert!(spec.start >= 0); + prop_assert!(spec.end == -1 || spec.end >= spec.start); + } + } + Ok(Err(_)) => {} + Err(_) => prop_assert!(false, "parse_copy_source_range panicked for input {:?}", input), + } + } } }