diff --git a/crates/e2e_test/src/snowball_auto_extract_test.rs b/crates/e2e_test/src/snowball_auto_extract_test.rs index 2b64a7953..40aa88d5c 100644 --- a/crates/e2e_test/src/snowball_auto_extract_test.rs +++ b/crates/e2e_test/src/snowball_auto_extract_test.rs @@ -70,6 +70,36 @@ mod tests { Ok(builder.into_inner().await?.into_inner()) } + fn build_archive_with_parent_dir_entry(victim_bucket: &str) -> Vec { + let path = format!("../{victim_bucket}/evil-injected.txt"); + let data = b"injected-body"; + let mut header = [0u8; 512]; + + header[..path.len()].copy_from_slice(path.as_bytes()); + header[100..108].copy_from_slice(b"0000644\0"); + header[108..116].copy_from_slice(b"0000000\0"); + header[116..124].copy_from_slice(b"0000000\0"); + let size = format!("{:011o}\0", data.len()); + header[124..136].copy_from_slice(size.as_bytes()); + header[136..148].copy_from_slice(b"00000000000\0"); + header[148..156].fill(b' '); + header[156] = b'0'; + header[257..263].copy_from_slice(b"ustar\0"); + header[263..265].copy_from_slice(b"00"); + + let checksum: u32 = header.iter().map(|byte| *byte as u32).sum(); + let checksum = format!("{:06o}\0 ", checksum); + header[148..156].copy_from_slice(checksum.as_bytes()); + + let mut archive = Vec::new(); + archive.extend_from_slice(&header); + archive.extend_from_slice(data); + let padding = (512 - (data.len() % 512)) % 512; + archive.extend(std::iter::repeat(0).take(padding)); + archive.extend_from_slice(&[0u8; 1024]); + archive + } + #[tokio::test] #[serial] async fn snowball_auto_extract_supports_minio_prefix_and_directory_markers() -> Result<(), Box> { @@ -273,6 +303,49 @@ mod tests { Ok(()) } + #[tokio::test] + #[serial] + async fn snowball_auto_extract_rejects_parent_dir_entry_without_cross_bucket_write() + -> Result<(), Box> { + init_logging(); + + let mut env = RustFSTestEnvironment::new().await?; + env.start_rustfs_server(vec![]).await?; + + let client = env.create_s3_client(); + let attacker_bucket = "snowball-traversal-source"; + let victim_bucket = "snowball-traversal-victim"; + let archive = build_archive_with_parent_dir_entry(victim_bucket); + + client.create_bucket().bucket(attacker_bucket).send().await?; + client.create_bucket().bucket(victim_bucket).send().await?; + + let err = client + .put_object() + .bucket(attacker_bucket) + .key("fixture.tar") + .metadata("Snowball-Auto-Extract", "true") + .body(ByteStream::from(archive)) + .send() + .await + .expect_err("parent directory archive entry should be rejected"); + let service_err = err.into_service_error(); + assert_eq!(service_err.code(), Some("InvalidArgument")); + + let victim_err = client + .head_object() + .bucket(victim_bucket) + .key("evil-injected.txt") + .send() + .await + .expect_err("rejected archive entry must not write into the victim bucket"); + let victim_service_err = victim_err.into_service_error(); + assert_eq!(victim_service_err.code(), Some("NotFound")); + + env.stop_server(); + Ok(()) + } + #[tokio::test] #[serial] async fn snowball_auto_extract_prefers_exact_minio_prefix_over_suffix_fallback() -> Result<(), Box> { diff --git a/crates/policy/src/policy/resource.rs b/crates/policy/src/policy/resource.rs index 12398c001..69fd61ccb 100644 --- a/crates/policy/src/policy/resource.rs +++ b/crates/policy/src/policy/resource.rs @@ -192,7 +192,7 @@ impl Resource { return true; } - if wildcard::is_match(resolved_pattern, resource) { + if cp != "." && wildcard::is_match(resolved_pattern, &cp) { return true; } } @@ -287,6 +287,8 @@ mod tests { #[test_case("arn:aws:s3:::mybucket/*","mybucket10/myobject" => false; "13")] #[test_case("arn:aws:s3:::mybucket?0/2010/photos/*","mybucket0/2010/photos/1.jpg" => false; "14")] #[test_case("arn:aws:s3:::mybucket","mybucket/myobject" => false; "15")] + #[test_case("arn:aws:s3:::attacker-bucket/*","attacker-bucket/../victim-bucket/evil.txt" => false; "16")] + #[test_case("arn:aws:s3:::attacker-bucket/*","attacker-bucket/safe/../../victim-bucket/evil.txt" => false; "17")] fn test_resource_is_match(resource: &str, object: &str) -> bool { let resource: Resource = resource.try_into().unwrap(); diff --git a/rustfs/src/app/object_usecase.rs b/rustfs/src/app/object_usecase.rs index 736395768..bbab2badc 100644 --- a/rustfs/src/app/object_usecase.rs +++ b/rustfs/src/app/object_usecase.rs @@ -110,7 +110,7 @@ use s3s::header::{X_AMZ_RESTORE, X_AMZ_RESTORE_OUTPUT_PATH}; use s3s::{S3Error, S3ErrorCode, S3Request, S3Response, S3Result, s3_error}; use std::collections::HashMap; use std::ops::Add; -use std::path::Path; +use std::path::{Component, Path}; use std::str::FromStr; use std::sync::atomic::{AtomicBool, Ordering}; @@ -775,16 +775,36 @@ fn snowball_meta_flag(headers: &HeaderMap, exact_keys: &[&str], suffix_lower: &s snowball_meta_value(headers, exact_keys, suffix_lower).is_some_and(|value| value.eq_ignore_ascii_case("true")) } -fn normalize_snowball_prefix(prefix: &str) -> Option { - let normalized = prefix.trim().trim_matches('/'); - if normalized.is_empty() { - return None; - } - - Some(normalized.to_string()) +fn contains_parent_dir_component(path: &str) -> bool { + path.split(['/', '\\']).any(|component| component == "..") } -fn normalize_extract_entry_key(path: &str, prefix: Option<&str>, is_dir: bool) -> String { +fn validate_extract_relative_path(path: &str) -> S3Result<()> { + let path = Path::new(path); + if path + .components() + .any(|component| matches!(component, Component::Prefix(_) | Component::RootDir | Component::ParentDir)) + || contains_parent_dir_component(path.to_string_lossy().as_ref()) + { + return Err(s3_error!(InvalidArgument, "archive entry path must stay within the target bucket")); + } + + Ok(()) +} + +fn normalize_snowball_prefix(prefix: &str) -> S3Result> { + let normalized = prefix.trim().trim_matches('/'); + if normalized.is_empty() { + return Ok(None); + } + + validate_extract_relative_path(normalized)?; + + Ok(Some(normalized.to_string())) +} + +fn normalize_extract_entry_key(path: &str, prefix: Option<&str>, is_dir: bool) -> S3Result { + validate_extract_relative_path(path)?; let path = path.trim_matches('/'); let mut key = match prefix { Some(prefix) if !path.is_empty() => format!("{prefix}/{path}"), @@ -796,7 +816,9 @@ fn normalize_extract_entry_key(path: &str, prefix: Option<&str>, is_dir: bool) - key.push('/'); } - key + validate_extract_relative_path(&key)?; + + Ok(key) } fn map_extract_archive_error(err: impl std::fmt::Display) -> S3Error { @@ -992,17 +1014,19 @@ fn delete_creates_delete_marker(opts: &ObjectOptions) -> bool { opts.version_id.is_none() && opts.versioned && !opts.version_suspended } -fn resolve_put_object_extract_options(headers: &HeaderMap) -> PutObjectExtractOptions { +fn resolve_put_object_extract_options(headers: &HeaderMap) -> S3Result { let prefix = snowball_meta_value(headers, SNOWBALL_PREFIX_HEADER_KEYS, SNOWBALL_PREFIX_SUFFIX_LOWER) - .and_then(|value| normalize_snowball_prefix(&value)); + .map(|value| normalize_snowball_prefix(&value)) + .transpose()? + .flatten(); let ignore_dirs = snowball_meta_flag(headers, SNOWBALL_IGNORE_DIRS_HEADER_KEYS, SNOWBALL_IGNORE_DIRS_SUFFIX_LOWER); let ignore_errors = snowball_meta_flag(headers, SNOWBALL_IGNORE_ERRORS_HEADER_KEYS, SNOWBALL_IGNORE_ERRORS_SUFFIX_LOWER); - PutObjectExtractOptions { + Ok(PutObjectExtractOptions { prefix, ignore_dirs, ignore_errors, - } + }) } fn is_sse_kms_requested(input: &PutObjectInput, headers: &HeaderMap) -> bool { @@ -4126,7 +4150,7 @@ impl DefaultObjectUsecase { return Err(S3Error::with_message(S3ErrorCode::InternalError, "Not init".to_string())); }; - let extract_options = resolve_put_object_extract_options(&req.headers); + let extract_options = resolve_put_object_extract_options(&req.headers)?; let version_id = match event_version_id { Some(v) => v.to_string(), None => String::new(), @@ -4167,7 +4191,16 @@ impl DefaultObjectUsecase { }; let is_dir = f.header().entry_type().is_dir(); - let fpath = normalize_extract_entry_key(&fpath.to_string_lossy(), extract_options.prefix.as_deref(), is_dir); + let fpath = match normalize_extract_entry_key(&fpath.to_string_lossy(), extract_options.prefix.as_deref(), is_dir) { + Ok(fpath) => fpath, + Err(err) => { + if extract_options.ignore_errors { + warn!("Skipping archive entry because path is unsafe and ignore-errors is enabled: {err}"); + continue; + } + return Err(err); + } + }; let mut auth_req = S3Request { input: PutObjectInput::default(), @@ -4464,18 +4497,39 @@ mod tests { #[test] fn normalize_snowball_prefix_trims_slashes_and_whitespace() { - assert_eq!(normalize_snowball_prefix(" /batch/incoming/ "), Some("batch/incoming".to_string())); - assert_eq!(normalize_snowball_prefix("///"), None); + assert_eq!( + normalize_snowball_prefix(" /batch/incoming/ ").unwrap(), + Some("batch/incoming".to_string()) + ); + assert_eq!(normalize_snowball_prefix("///").unwrap(), None); + } + + #[test] + fn normalize_snowball_prefix_rejects_parent_dir_components() { + assert!(normalize_snowball_prefix("../victim-bucket").is_err()); + assert!(normalize_snowball_prefix("safe/../../victim-bucket").is_err()); + assert!(normalize_snowball_prefix("safe\\..\\victim-bucket").is_err()); } #[test] fn normalize_extract_entry_key_applies_prefix_and_directory_suffix() { assert_eq!( - normalize_extract_entry_key("nested/path.txt", Some("imports"), false), + normalize_extract_entry_key("nested/path.txt", Some("imports"), false).unwrap(), "imports/nested/path.txt" ); - assert_eq!(normalize_extract_entry_key("nested/dir/", Some("imports"), true), "imports/nested/dir/"); - assert_eq!(normalize_extract_entry_key("top-level", None, false), "top-level"); + assert_eq!( + normalize_extract_entry_key("nested/dir/", Some("imports"), true).unwrap(), + "imports/nested/dir/" + ); + assert_eq!(normalize_extract_entry_key("top-level", None, false).unwrap(), "top-level"); + } + + #[test] + fn normalize_extract_entry_key_rejects_bucket_escape_paths() { + assert!(normalize_extract_entry_key("../victim-bucket/evil.txt", None, false).is_err()); + assert!(normalize_extract_entry_key("safe/../../victim-bucket/evil.txt", None, false).is_err()); + assert!(normalize_extract_entry_key("safe\\..\\victim-bucket\\evil.txt", None, false).is_err()); + assert!(normalize_extract_entry_key("evil.txt", Some("../victim-bucket"), false).is_err()); } #[test] @@ -4700,7 +4754,7 @@ mod tests { #[test] fn resolve_put_object_extract_options_defaults_when_headers_missing() { let headers = HeaderMap::new(); - let options = resolve_put_object_extract_options(&headers); + let options = resolve_put_object_extract_options(&headers).unwrap(); assert_eq!( options, PutObjectExtractOptions { @@ -4718,7 +4772,7 @@ mod tests { headers.insert(AMZ_SNOWBALL_IGNORE_DIRS_INTERNAL, HeaderValue::from_static("true")); headers.insert(AMZ_SNOWBALL_IGNORE_ERRORS_INTERNAL, HeaderValue::from_static("TRUE")); - let options = resolve_put_object_extract_options(&headers); + let options = resolve_put_object_extract_options(&headers).unwrap(); assert_eq!(options.prefix.as_deref(), Some("internal/prefix")); assert!(options.ignore_dirs); assert!(options.ignore_errors); @@ -4731,7 +4785,7 @@ mod tests { headers.insert(AMZ_SNOWBALL_IGNORE_DIRS, HeaderValue::from_static(" true ")); headers.insert(AMZ_SNOWBALL_IGNORE_ERRORS, HeaderValue::from_static("TRUE")); - let options = resolve_put_object_extract_options(&headers); + let options = resolve_put_object_extract_options(&headers).unwrap(); assert_eq!(options.prefix.as_deref(), Some("standard/prefix")); assert!(options.ignore_dirs); assert!(options.ignore_errors); @@ -4753,7 +4807,7 @@ mod tests { HeaderValue::from_static("TRUE"), ); - let options = resolve_put_object_extract_options(&headers); + let options = resolve_put_object_extract_options(&headers).unwrap(); assert_eq!(options.prefix.as_deref(), Some("partner/import")); assert!(options.ignore_dirs); assert!(options.ignore_errors); @@ -4767,7 +4821,7 @@ mod tests { headers.insert(AMZ_SNOWBALL_PREFIX, HeaderValue::from_static("/standard/prefix/")); headers.insert(AMZ_MINIO_SNOWBALL_PREFIX, HeaderValue::from_static("/minio/prefix/")); - let options = resolve_put_object_extract_options(&headers); + let options = resolve_put_object_extract_options(&headers).unwrap(); assert_eq!(options.prefix.as_deref(), Some("minio/prefix")); } @@ -4779,11 +4833,19 @@ mod tests { headers.insert(AMZ_RUSTFS_SNOWBALL_IGNORE_ERRORS, HeaderValue::from_static("false")); headers.insert("x-amz-meta-acme-snowball-ignore-errors", HeaderValue::from_static("true")); - let options = resolve_put_object_extract_options(&headers); + let options = resolve_put_object_extract_options(&headers).unwrap(); assert!(!options.ignore_dirs); assert!(!options.ignore_errors); } + #[test] + fn resolve_put_object_extract_options_rejects_unsafe_prefix_header() { + let mut headers = HeaderMap::new(); + headers.insert(AMZ_SNOWBALL_PREFIX, HeaderValue::from_static("../victim-bucket")); + + assert!(resolve_put_object_extract_options(&headers).is_err()); + } + #[tokio::test] async fn execute_put_object_rejects_post_object_sse_kms_from_input() { let input = PutObjectInput::builder()