From 0d7fa1d82281f3a54d4652445107c47cefba12a4 Mon Sep 17 00:00:00 2001 From: overtrue Date: Fri, 9 Oct 2026 21:47:24 +0800 Subject: [PATCH] fix(s3s-fs)!: reject path aliases in object keys Reject dot segments, repeated separators and absolute or platform paths before filesystem normalization. Batch deletion reports invalid keys individually and continues with valid entries. Assisted-by: GPT-6 --- crates/s3s-fs/src/fs.rs | 62 +++++++++++++++++++++++++-- crates/s3s-fs/src/s3.rs | 95 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 153 insertions(+), 4 deletions(-) diff --git a/crates/s3s-fs/src/fs.rs b/crates/s3s-fs/src/fs.rs index 543ffcef2..0079a56fb 100644 --- a/crates/s3s-fs/src/fs.rs +++ b/crates/s3s-fs/src/fs.rs @@ -9,10 +9,11 @@ use s3s::crypto::Checksum; use s3s::crypto::Md5; use s3s::dto; use s3s::dto::PartNumber; +use s3s::{S3Result, s3_error}; use std::env; use std::ops::Not; -use std::path::{Path, PathBuf}; +use std::path::{Component, Path, PathBuf}; use std::sync::atomic::{AtomicU64, Ordering}; use std::time::SystemTime; @@ -150,10 +151,23 @@ impl FileSystem { } /// resolve object path under the virtual root - pub(crate) fn get_object_path(&self, bucket: &str, key: &str) -> Result { + pub(crate) fn get_object_path(&self, bucket: &str, key: &str) -> S3Result { let dir = Path::new(&bucket); let file_path = Path::new(&key); - self.resolve_abs_path(dir.join(file_path)) + // Filesystem normalization would make these keys refer to other objects. + let path_key = key.strip_suffix('/').unwrap_or(key); + if path_key + .split(std::path::is_separator) + .any(|part| matches!(part, "" | "." | "..")) + || (cfg!(windows) && key.contains('\\')) + || file_path.components().any(|part| !matches!(part, Component::Normal(_))) + { + return Err(s3_error!( + InvalidArgument, + "Object key cannot be represented as a literal filesystem path" + )); + } + self.resolve_abs_path(dir.join(file_path)).map_err(Into::into) } /// resolve bucket path under the virtual root @@ -514,6 +528,48 @@ mod tests { } } + #[test] + fn object_paths_reject_normalization_aliases() { + let fs = FileSystem { + root: env::temp_dir(), + tmp_file_counter: AtomicU64::new(0), + }; + for key in [ + "", + ".", + "..", + "../victim", + "allowed/../victim", + "allowed/./victim", + "allowed//victim", + "allowed//", + "/victim", + ] { + let error = fs.get_object_path("bucket", key).unwrap_err(); + assert_eq!(error.code(), &s3s::S3ErrorCode::InvalidArgument, "key {key:?}"); + } + #[cfg(windows)] + for key in [ + r"allowed\victim", + r"allowed\..\victim", + r"allowed\.\victim", + r"C:\victim", + r"C:victim", + r"\victim", + ] { + let error = fs.get_object_path("bucket", key).unwrap_err(); + assert_eq!(error.code(), &s3s::S3ErrorCode::InvalidArgument, "key {key:?}"); + } + for key in ["allowed/ok", "allowed/.config", "allowed/report..txt", "allowed/directory/"] { + assert_eq!(fs.get_object_path("bucket", key).unwrap(), fs.root.join("bucket").join(key)); + } + #[cfg(not(windows))] + assert_eq!( + fs.get_object_path("bucket", r"allowed\literal").unwrap(), + fs.root.join("bucket").join(r"allowed\literal") + ); + } + #[test] fn concurrent_upload_directory_deletion_is_idempotent() -> Result<()> { let runtime = tokio::runtime::Builder::new_current_thread() diff --git a/crates/s3s-fs/src/s3.rs b/crates/s3s-fs/src/s3.rs index 976d1db6c..15aea931a 100644 --- a/crates/s3s-fs/src/s3.rs +++ b/crates/s3s-fs/src/s3.rs @@ -496,8 +496,20 @@ impl S3 for FileSystem { let input = req.input; let mut deleted_objects: Vec = Vec::new(); + let mut errors = Vec::new(); for object in input.delete.objects { - let path = self.get_object_path(&input.bucket, &object.key)?; + let path = match self.get_object_path(&input.bucket, &object.key) { + Ok(path) => path, + Err(error) => { + errors.push(s3s::dto::Error { + key: Some(object.key), + version_id: object.version_id, + code: Some(error.code().as_str().to_owned()), + message: error.message().map(str::to_owned), + }); + continue; + } + }; if object.key.ends_with('/') { match fs::read_dir(&path).await { Ok(mut dir) => { @@ -532,6 +544,7 @@ impl S3 for FileSystem { let output = DeleteObjectsOutput { deleted: Some(deleted_objects), + errors: (!errors.is_empty()).then_some(errors), ..Default::default() }; Ok(S3Response::new(output)) @@ -2198,6 +2211,86 @@ mod tests { } } + #[tokio::test] + async fn batch_delete_rejects_path_aliases_and_preserves_other_objects() { + let root = env::temp_dir().join(format!("s3s-fs-delete-path-{}", Uuid::new_v4())); + std::fs::create_dir_all(root.join("bucket/allowed")).unwrap(); + std::fs::create_dir_all(root.join("other-bucket")).unwrap(); + let _root = TestRoot(root.clone()); + let fs = FileSystem::new(&root).unwrap(); + for (key, content) in [ + ("bucket/allowed/ok", "delete me"), + ("bucket/allowed/after", "delete me too"), + ("bucket/victim", "keep victim"), + ("bucket/allowed/sibling", "keep sibling"), + ("bucket/allowed/repeated", "keep repeated-separator victim"), + ("other-bucket/secret", "keep other bucket"), + ] { + std::fs::write(root.join(key), content).unwrap(); + } + + let invalid_keys = [ + "allowed//repeated", + "allowed/../victim", + "allowed/./sibling", + "../other-bucket/secret", + "/other-bucket/secret", + ]; + let objects = std::iter::once("allowed/ok") + .chain(invalid_keys) + .chain(std::iter::once("allowed/after")) + .map(|key| ObjectIdentifier { + key: key.to_owned(), + ..Default::default() + }) + .collect(); + let input = DeleteObjectsInput { + bucket: "bucket".to_owned(), + delete: Delete { + objects, + ..Default::default() + }, + bypass_governance_retention: None, + checksum_algorithm: None, + expected_bucket_owner: None, + mfa: None, + request_payer: None, + }; + let request = S3Request { + input, + method: http::Method::POST, + uri: http::Uri::from_static("/bucket?delete"), + headers: http::HeaderMap::new(), + extensions: http::Extensions::new(), + credentials: None, + region: None, + service: None, + trailing_headers: None, + }; + let result = fs.delete_objects(request).await; + + assert_eq!(std::fs::read(root.join("bucket/victim")).unwrap(), b"keep victim"); + assert_eq!(std::fs::read(root.join("bucket/allowed/sibling")).unwrap(), b"keep sibling"); + assert_eq!( + std::fs::read(root.join("bucket/allowed/repeated")).unwrap(), + b"keep repeated-separator victim" + ); + assert_eq!(std::fs::read(root.join("other-bucket/secret")).unwrap(), b"keep other bucket"); + assert!(!root.join("bucket/allowed/ok").exists()); + assert!(!root.join("bucket/allowed/after").exists()); + let output = result.unwrap().output; + let deleted = output.deleted.unwrap(); + assert_eq!(deleted.len(), 2); + assert_eq!(deleted[0].key.as_deref(), Some("allowed/ok")); + assert_eq!(deleted[1].key.as_deref(), Some("allowed/after")); + let errors = output.errors.unwrap(); + assert_eq!(errors.len(), invalid_keys.len()); + for (error, key) in errors.iter().zip(invalid_keys) { + assert_eq!(error.key.as_deref(), Some(key)); + assert_eq!(error.code.as_deref(), Some("InvalidArgument")); + } + } + #[tokio::test] async fn a_stat_failure_is_skipped_only_when_the_entry_is_gone() { let root = env::temp_dir().join(format!("s3s-fs-vanished-{}", Uuid::new_v4()));