From 5cb8ec8ed72b2cb964aa303e25830dfe298589ff Mon Sep 17 00:00:00 2001 From: Amirmahdi Date: Tue, 9 Jun 2026 10:41:24 +0330 Subject: [PATCH 1/2] fix: prevent path traversal vulnerability in object handlers --- src/handlers/object.rs | 66 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 61 insertions(+), 5 deletions(-) diff --git a/src/handlers/object.rs b/src/handlers/object.rs index 95c51a7..a3dae36 100644 --- a/src/handlers/object.rs +++ b/src/handlers/object.rs @@ -85,7 +85,10 @@ pub async fn get_object( None => return S3ErrorType::NoSuchBucket.to_response(Some(bucket)), }; - let path = storage.join(key.trim_start_matches('/')); + let path = match safe_join(storage, &key) { + Some(p) => p, + None => return S3ErrorType::AccessDenied.to_response(Some(key)), + }; let mut file = match File::open(&path).await { Ok(f) => f, Err(_) => return S3ErrorType::NoSuchKey.to_response(Some(key)), @@ -161,7 +164,10 @@ pub async fn head_object( None => return S3ErrorType::NoSuchBucket.to_response(Some(bucket)), }; - let path = storage.join(key.trim_start_matches('/')); + let path = match safe_join(storage, &key) { + Some(p) => p, + None => return S3ErrorType::AccessDenied.to_response(Some(key)), + }; let metadata = match fs::metadata(&path).await { Ok(m) => m, Err(_) => return S3ErrorType::NoSuchKey.to_response(Some(key)), @@ -205,7 +211,10 @@ pub async fn put_object( None => return S3ErrorType::NoSuchBucket.to_response(Some(bucket)), }; - let path = storage.join(key.trim_start_matches('/')); + let path = match safe_join(storage, &key) { + Some(p) => p, + None => return S3ErrorType::AccessDenied.to_response(Some(key)), + }; if let Some(copy_source) = headers .get("x-amz-copy-source") @@ -261,7 +270,10 @@ async fn copy_object( None => return S3ErrorType::NoSuchBucket.to_response(Some(source_bucket)), }; - let source_path = source_storage.join(source_key.trim_start_matches('/')); + let source_path = match safe_join(source_storage, &source_key) { + Some(p) => p, + None => return S3ErrorType::AccessDenied.to_response(Some(source_key)), + }; let source_metadata = match fs::metadata(&source_path).await { Ok(metadata) if !metadata.is_dir() => metadata, Ok(_) => return S3ErrorType::NoSuchKey.to_response(Some(source_key)), @@ -318,7 +330,10 @@ pub async fn delete_object( None => return S3ErrorType::NoSuchBucket.to_response(Some(bucket)), }; - let path = storage.join(key.trim_start_matches('/')); + let path = match safe_join(storage, &key) { + Some(p) => p, + None => return S3ErrorType::AccessDenied.to_response(Some(key)), + }; if let Err(_) = fs::remove_file(&path).await { // S3 returns 204 even if file doesn't exist during DELETE return StatusCode::NO_CONTENT.into_response(); @@ -391,3 +406,44 @@ fn parse_copy_source(copy_source: &str) -> Option<(String, String)> { fn owner_id() -> String { "75aa57f09aa0c8caeab4f8c24e99d10f8e7faeebf76c078efc7c6caea54ba06a".to_string() } + +fn safe_join(storage: &std::path::Path, key: &str) -> Option { + use std::path::Component; + let mut resolved = storage.to_path_buf(); + for component in std::path::Path::new(key).components() { + match component { + Component::Normal(c) => resolved.push(c), + Component::RootDir | Component::CurDir => continue, + Component::Prefix(_) | Component::ParentDir => return None, + } + } + Some(resolved) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::path::Path; + + #[test] + fn test_safe_join() { + let storage = Path::new("/var/data"); + + // Normal keys + assert_eq!(safe_join(storage, "my_file.txt").unwrap(), Path::new("/var/data/my_file.txt")); + assert_eq!(safe_join(storage, "folder/file.txt").unwrap(), Path::new("/var/data/folder/file.txt")); + + // Leading slashes are ignored (RootDir) + assert_eq!(safe_join(storage, "/folder/file.txt").unwrap(), Path::new("/var/data/folder/file.txt")); + + // Current dir dots are ignored + assert_eq!(safe_join(storage, "./folder/./file.txt").unwrap(), Path::new("/var/data/folder/file.txt")); + + // ParentDir traversal is rejected + assert!(safe_join(storage, "../etc/passwd").is_none()); + assert!(safe_join(storage, "folder/../../etc/passwd").is_none()); + + // Windows prefixes are rejected + assert!(safe_join(storage, "C:/Windows/System32").is_none()); + } +} From b72121e50acf4b3104c1cc4e98fe5745ff44abf1 Mon Sep 17 00:00:00 2001 From: Amirmahdi Date: Tue, 9 Jun 2026 10:49:30 +0330 Subject: [PATCH 2/2] fix: correctly handle suffix and unsatisfiable range requests Co-authored-by: Cursor --- src/handlers/object.rs | 176 +++++++++++++++++++++++++++++------------ 1 file changed, 125 insertions(+), 51 deletions(-) diff --git a/src/handlers/object.rs b/src/handlers/object.rs index a3dae36..ed44e41 100644 --- a/src/handlers/object.rs +++ b/src/handlers/object.rs @@ -109,22 +109,32 @@ pub async fn get_object( // Handle Range Header if let Some(range_header) = headers.get(header::RANGE).and_then(|h| h.to_str().ok()) { - if let Some(range) = parse_range(range_header, size) { - let (start, end) = range; - let range_size = end - start + 1; - - if file.seek(io::SeekFrom::Start(start)).await.is_ok() { - let stream = ReaderStream::new(file.take(range_size)); + match parse_range(range_header, size) { + RangeRequest::Satisfiable(start, end) => { + let range_size = end - start + 1; + if file.seek(io::SeekFrom::Start(start)).await.is_ok() { + let stream = ReaderStream::new(file.take(range_size)); + return Response::builder() + .status(StatusCode::PARTIAL_CONTENT) + .header(header::CONTENT_TYPE, "application/octet-stream") + .header(header::CONTENT_LENGTH, range_size) + .header(header::CONTENT_RANGE, format!("bytes {}-{}/{}", start, end, size)) + .header(header::ETAG, etag) + .header("Last-Modified", mod_time.to_rfc2822()) + .body(Body::from_stream(stream)) + .unwrap(); + } + } + RangeRequest::Unsatisfiable => { return Response::builder() - .status(StatusCode::PARTIAL_CONTENT) - .header(header::CONTENT_TYPE, "application/octet-stream") - .header(header::CONTENT_LENGTH, range_size) - .header(header::CONTENT_RANGE, format!("bytes {}-{}/{}", start, end, size)) - .header(header::ETAG, etag) - .header("Last-Modified", mod_time.to_rfc2822()) - .body(Body::from_stream(stream)) + .status(StatusCode::RANGE_NOT_SATISFIABLE) + .header(header::CONTENT_RANGE, format!("bytes */{}", size)) + .body(Body::empty()) .unwrap(); } + RangeRequest::Invalid => { + // Fallthrough to standard 200 OK + } } } @@ -343,23 +353,115 @@ pub async fn delete_object( StatusCode::NO_CONTENT.into_response() } -fn parse_range(range_header: &str, file_size: u64) -> Option<(u64, u64)> { - if !range_header.starts_with("bytes=") { return None; } +enum RangeRequest { + Satisfiable(u64, u64), + Unsatisfiable, + Invalid, +} + +fn parse_range(range_header: &str, file_size: u64) -> RangeRequest { + if !range_header.starts_with("bytes=") { return RangeRequest::Invalid; } let range_str = &range_header[6..]; let parts: Vec<&str> = range_str.split('-').collect(); - if parts.len() != 2 { return None; } + if parts.len() != 2 { return RangeRequest::Invalid; } + + let start_str = parts[0].trim(); + let end_str = parts[1].trim(); + + if start_str.is_empty() && end_str.is_empty() { + return RangeRequest::Invalid; + } + + if start_str.is_empty() { + // Suffix range + let Ok(suffix_len) = end_str.parse::() else { + return RangeRequest::Invalid; + }; + if suffix_len == 0 { + return RangeRequest::Invalid; + } + if file_size == 0 { + return RangeRequest::Unsatisfiable; + } + let start = file_size.saturating_sub(suffix_len); + let end = file_size - 1; + return RangeRequest::Satisfiable(start, end); + } + + let Ok(start) = start_str.parse::() else { + return RangeRequest::Invalid; + }; + + if start >= file_size { + return RangeRequest::Unsatisfiable; + } - let start = parts[0].parse::().ok()?; - let end = if parts[1].is_empty() { - file_size - 1 + let end = if end_str.is_empty() { + file_size.saturating_sub(1) } else { - parts[1].parse::().ok()? + let Ok(parsed_end) = end_str.parse::() else { + return RangeRequest::Invalid; + }; + std::cmp::min(parsed_end, file_size.saturating_sub(1)) }; - if start <= end && end < file_size { - Some((start, end)) + if start <= end { + RangeRequest::Satisfiable(start, end) } else { - None + RangeRequest::Invalid + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::path::Path; + + #[test] + fn test_parse_range() { + // Normal ranges + assert!(matches!(parse_range("bytes=0-499", 1000), RangeRequest::Satisfiable(0, 499))); + assert!(matches!(parse_range("bytes=500-", 1000), RangeRequest::Satisfiable(500, 999))); + + // Suffix ranges + assert!(matches!(parse_range("bytes=-500", 1000), RangeRequest::Satisfiable(500, 999))); + assert!(matches!(parse_range("bytes=-1500", 1000), RangeRequest::Satisfiable(0, 999))); + + // Unsatisfiable + assert!(matches!(parse_range("bytes=1000-", 1000), RangeRequest::Unsatisfiable)); + assert!(matches!(parse_range("bytes=9999-", 1000), RangeRequest::Unsatisfiable)); + + // Zero length files + assert!(matches!(parse_range("bytes=-500", 0), RangeRequest::Unsatisfiable)); + assert!(matches!(parse_range("bytes=0-", 0), RangeRequest::Unsatisfiable)); + + // Invalid ranges + assert!(matches!(parse_range("bytes=abc-def", 1000), RangeRequest::Invalid)); + assert!(matches!(parse_range("bytes=-", 1000), RangeRequest::Invalid)); + assert!(matches!(parse_range("bytes=500-499", 1000), RangeRequest::Invalid)); + assert!(matches!(parse_range("wrong=0-100", 1000), RangeRequest::Invalid)); + } + + #[test] + fn test_safe_join() { + let storage = Path::new("/var/data"); + + // Normal keys + assert_eq!(safe_join(storage, "my_file.txt").unwrap(), Path::new("/var/data/my_file.txt")); + assert_eq!(safe_join(storage, "folder/file.txt").unwrap(), Path::new("/var/data/folder/file.txt")); + + // Leading slashes are ignored (RootDir) + assert_eq!(safe_join(storage, "/folder/file.txt").unwrap(), Path::new("/var/data/folder/file.txt")); + + // Current dir dots are ignored + assert_eq!(safe_join(storage, "./folder/./file.txt").unwrap(), Path::new("/var/data/folder/file.txt")); + + // ParentDir traversal is rejected + assert!(safe_join(storage, "../etc/passwd").is_none()); + assert!(safe_join(storage, "folder/../../etc/passwd").is_none()); + + // Windows prefixes are rejected + assert!(safe_join(storage, "C:/Windows/System32").is_none()); } } @@ -419,31 +521,3 @@ fn safe_join(storage: &std::path::Path, key: &str) -> Option } Some(resolved) } - -#[cfg(test)] -mod tests { - use super::*; - use std::path::Path; - - #[test] - fn test_safe_join() { - let storage = Path::new("/var/data"); - - // Normal keys - assert_eq!(safe_join(storage, "my_file.txt").unwrap(), Path::new("/var/data/my_file.txt")); - assert_eq!(safe_join(storage, "folder/file.txt").unwrap(), Path::new("/var/data/folder/file.txt")); - - // Leading slashes are ignored (RootDir) - assert_eq!(safe_join(storage, "/folder/file.txt").unwrap(), Path::new("/var/data/folder/file.txt")); - - // Current dir dots are ignored - assert_eq!(safe_join(storage, "./folder/./file.txt").unwrap(), Path::new("/var/data/folder/file.txt")); - - // ParentDir traversal is rejected - assert!(safe_join(storage, "../etc/passwd").is_none()); - assert!(safe_join(storage, "folder/../../etc/passwd").is_none()); - - // Windows prefixes are rejected - assert!(safe_join(storage, "C:/Windows/System32").is_none()); - } -}