From a86bab42e0906e1b2f804563928dece3d00cb66b Mon Sep 17 00:00:00 2001 From: tonghuaroot <23011166+tonghuaroot@users.noreply.github.com> Date: Sat, 6 Jun 2026 14:04:00 +0800 Subject: [PATCH] fix(services): confine compfs/monoiofs object keys to root by rejecting parent-dir traversal --- core/services/compfs/src/backend.rs | 18 +++---- core/services/compfs/src/core.rs | 73 ++++++++++++++++++++++++++- core/services/compfs/src/deleter.rs | 4 +- core/services/monoiofs/src/backend.rs | 10 ++-- core/services/monoiofs/src/core.rs | 67 ++++++++++++++++++++++-- core/services/monoiofs/src/deleter.rs | 2 +- 6 files changed, 151 insertions(+), 23 deletions(-) diff --git a/core/services/compfs/src/backend.rs b/core/services/compfs/src/backend.rs index d5ce70796842..6b75fb2494ff 100644 --- a/core/services/compfs/src/backend.rs +++ b/core/services/compfs/src/backend.rs @@ -176,7 +176,7 @@ impl Access for CompfsBackend { } async fn create_dir(&self, path: &str, _: OpCreateDir) -> Result { - let path = self.core.prepare_path(path); + let path = self.core.prepare_path(path)?; self.core .exec(move || async move { compio::fs::create_dir_all(path).await }) @@ -186,7 +186,7 @@ impl Access for CompfsBackend { } async fn stat(&self, path: &str, _: OpStat) -> Result { - let path = self.core.prepare_path(path); + let path = self.core.prepare_path(path)?; let meta = self .core .exec(move || async move { compio::fs::metadata(path).await }) @@ -220,8 +220,8 @@ impl Access for CompfsBackend { _: OpCopy, _opts: OpCopier, ) -> Result<(RpCopy, Self::Copier)> { - let from = self.core.prepare_path(from); - let to = self.core.prepare_path(to); + let from = self.core.prepare_path(from)?; + let to = self.core.prepare_path(to)?; self.core .exec(move || async move { @@ -247,8 +247,8 @@ impl Access for CompfsBackend { } async fn rename(&self, from: &str, to: &str, _: OpRename) -> Result { - let from = self.core.prepare_path(from); - let to = self.core.prepare_path(to); + let from = self.core.prepare_path(from)?; + let to = self.core.prepare_path(to)?; self.core .exec(move || async move { @@ -262,7 +262,7 @@ impl Access for CompfsBackend { Ok(RpRename::default()) } async fn read(&self, path: &str, _: OpRead) -> Result<(RpRead, Self::Reader)> { - let path = self.core.prepare_path(path); + let path = self.core.prepare_path(path)?; let file = self .core .exec(move || async move { @@ -281,7 +281,7 @@ impl Access for CompfsBackend { } async fn write(&self, path: &str, args: OpWrite) -> Result<(RpWrite, Self::Writer)> { - let path = self.core.prepare_path(path); + let path = self.core.prepare_path(path)?; let append = args.append(); let file = self .core @@ -309,7 +309,7 @@ impl Access for CompfsBackend { } async fn list(&self, path: &str, _: OpList) -> Result<(RpList, Self::Lister)> { - let path = self.core.prepare_path(path); + let path = self.core.prepare_path(path)?; let read_dir = match self .core diff --git a/core/services/compfs/src/core.rs b/core/services/compfs/src/core.rs index 1fcb13d568cb..05997c574cf8 100644 --- a/core/services/compfs/src/core.rs +++ b/core/services/compfs/src/core.rs @@ -16,6 +16,7 @@ // under the License. use std::future::Future; +use std::path::Path; use std::path::PathBuf; use std::sync::Arc; @@ -63,8 +64,31 @@ pub(super) struct CompfsCore { } impl CompfsCore { - pub fn prepare_path(&self, path: &str) -> PathBuf { - self.root.join(path.trim_end_matches('/')) + /// Join a caller-supplied key onto `self.root` while keeping the result + /// confined to that root. + /// + /// `normalize_path` (opendal-core) strips leading `/` and empty segments but + /// intentionally does NOT resolve `.`/`..`, and `PathBuf::join` is purely + /// lexical, so a key such as `../../etc/passwd` would otherwise escape the + /// configured `root` at syscall time. compfs is a local (compio-backed) + /// filesystem, so the host kernel resolves `..` when the path is used; we + /// reject any key whose components include a `..` (parent-dir) traversal. + /// + /// This mirrors the confinement added to the `fs` backend in #7684. + pub fn prepare_path(&self, path: &str) -> Result { + use std::path::Component; + let trimmed = path.trim_end_matches('/'); + if Path::new(trimmed) + .components() + .any(|c| matches!(c, Component::ParentDir)) + { + return Err(Error::new( + ErrorKind::NotFound, + "path escapes the configured root via `..`", + ) + .with_context("path", path)); + } + Ok(self.root.join(trimmed)) } pub async fn exec(&self, f: Fn) -> opendal_core::Result @@ -136,4 +160,49 @@ mod tests { assert_eq!(buf.total_len(), len); assert_eq!(collected, bytes); } + + fn new_test_core() -> CompfsCore { + CompfsCore { + info: Arc::new(AccessorInfo::default()), + root: PathBuf::from("/data/root"), + dispatcher: Dispatcher::new().unwrap(), + buf_pool: oio::PooledBuf::new(16), + } + } + + #[test] + fn test_prepare_path_rejects_parent_dir() { + let core = new_test_core(); + for key in ["../etc/passwd", "../../etc/passwd", "a/../../b", "a/.."] { + let err = core.prepare_path(key).unwrap_err(); + assert_eq!( + err.kind(), + ErrorKind::NotFound, + "key should be rejected: {key}" + ); + } + } + + #[test] + fn test_prepare_path_allows_normal_keys() { + let core = new_test_core(); + // Normal keys, `.` (CurDir), and trailing slashes resolve unchanged. + assert_eq!( + core.prepare_path("a/b.txt").unwrap(), + PathBuf::from("/data/root/a/b.txt") + ); + assert_eq!( + core.prepare_path("a/b/").unwrap(), + PathBuf::from("/data/root/a/b") + ); + assert_eq!( + core.prepare_path("./a/b").unwrap(), + PathBuf::from("/data/root/a/b") + ); + // A key containing `..` only as a substring of a name is not a traversal. + assert_eq!( + core.prepare_path("a..b").unwrap(), + PathBuf::from("/data/root/a..b") + ); + } } diff --git a/core/services/compfs/src/deleter.rs b/core/services/compfs/src/deleter.rs index 7be9b396c349..90c0c716d108 100644 --- a/core/services/compfs/src/deleter.rs +++ b/core/services/compfs/src/deleter.rs @@ -34,12 +34,12 @@ impl CompfsDeleter { impl oio::OneShotDelete for CompfsDeleter { async fn delete_once(&self, path: String, _: OpDelete) -> Result<()> { let res = if path.ends_with('/') { - let path = self.core.prepare_path(&path); + let path = self.core.prepare_path(&path)?; self.core .exec(move || async move { compio::fs::remove_dir(path).await }) .await } else { - let path = self.core.prepare_path(&path); + let path = self.core.prepare_path(&path)?; self.core .exec(move || async move { compio::fs::remove_file(path).await }) .await diff --git a/core/services/monoiofs/src/backend.rs b/core/services/monoiofs/src/backend.rs index 55624e9eaecc..5fa16a9715d0 100644 --- a/core/services/monoiofs/src/backend.rs +++ b/core/services/monoiofs/src/backend.rs @@ -104,7 +104,7 @@ impl Access for MonoiofsBackend { } async fn stat(&self, path: &str, _args: OpStat) -> Result { - let path = self.core.prepare_path(path); + let path = self.core.prepare_path(path)?; let meta = self .core .dispatch(move || monoio::fs::metadata(path)) @@ -125,7 +125,7 @@ impl Access for MonoiofsBackend { Ok(RpStat::new(m)) } async fn read(&self, path: &str, _args: OpRead) -> Result<(RpRead, Self::Reader)> { - let path = self.core.prepare_path(path); + let path = self.core.prepare_path(path)?; let reader = MonoiofsReader::new(self.core.clone(), path).await?; Ok((RpRead::default(), oio::PositionReader::new(reader))) } @@ -144,7 +144,7 @@ impl Access for MonoiofsBackend { } async fn rename(&self, from: &str, to: &str, _args: OpRename) -> Result { - let from = self.core.prepare_path(from); + let from = self.core.prepare_path(from)?; // ensure file exists self.core .dispatch({ @@ -162,7 +162,7 @@ impl Access for MonoiofsBackend { } async fn create_dir(&self, path: &str, _args: OpCreateDir) -> Result { - let path = self.core.prepare_path(path); + let path = self.core.prepare_path(path)?; self.core .dispatch(move || monoio::fs::create_dir_all(path)) .await @@ -177,7 +177,7 @@ impl Access for MonoiofsBackend { _args: OpCopy, _opts: OpCopier, ) -> Result<(RpCopy, Self::Copier)> { - let from = self.core.prepare_path(from); + let from = self.core.prepare_path(from)?; // ensure file exists self.core .dispatch({ diff --git a/core/services/monoiofs/src/core.rs b/core/services/monoiofs/src/core.rs index 676566253cfc..a74cec336ab8 100644 --- a/core/services/monoiofs/src/core.rs +++ b/core/services/monoiofs/src/core.rs @@ -16,6 +16,7 @@ // under the License. use std::mem; +use std::path::Path; use std::path::PathBuf; use std::sync::Arc; use std::sync::Mutex; @@ -95,14 +96,36 @@ impl MonoiofsCore { } } - /// join root and path - pub fn prepare_path(&self, path: &str) -> PathBuf { - self.root.join(path.trim_end_matches('/')) + /// Join a caller-supplied key onto `self.root` while keeping the result + /// confined to that root. + /// + /// `normalize_path` (opendal-core) strips leading `/` and empty segments but + /// intentionally does NOT resolve `.`/`..`, and `PathBuf::join` is purely + /// lexical, so a key such as `../../etc/passwd` would otherwise escape the + /// configured `root` at syscall time. monoiofs is a local (monoio/io_uring) + /// filesystem, so the host kernel resolves `..` when the path is used; we + /// reject any key whose components include a `..` (parent-dir) traversal. + /// + /// This mirrors the confinement added to the `fs` backend in #7684. + pub fn prepare_path(&self, path: &str) -> Result { + use std::path::Component; + let trimmed = path.trim_end_matches('/'); + if Path::new(trimmed) + .components() + .any(|c| matches!(c, Component::ParentDir)) + { + return Err(Error::new( + ErrorKind::NotFound, + "path escapes the configured root via `..`", + ) + .with_context("path", path)); + } + Ok(self.root.join(trimmed)) } /// join root and path, create parent dirs pub async fn prepare_write_path(&self, path: &str) -> Result { - let path = self.prepare_path(path); + let path = self.prepare_path(path)?; let parent = path .parent() .ok_or_else(|| { @@ -309,4 +332,40 @@ mod tests { let core = new_core(4); dispatch_panic(core).await; } + + #[tokio::test] + async fn test_prepare_path_rejects_parent_dir() { + let core = Arc::new(MonoiofsCore::new(PathBuf::from("/data/root"), 1, 1024)); + for key in ["../etc/passwd", "../../etc/passwd", "a/../../b", "a/.."] { + let err = core.prepare_path(key).unwrap_err(); + assert_eq!( + err.kind(), + ErrorKind::NotFound, + "key should be rejected: {key}" + ); + } + } + + #[tokio::test] + async fn test_prepare_path_allows_normal_keys() { + let core = Arc::new(MonoiofsCore::new(PathBuf::from("/data/root"), 1, 1024)); + // Normal keys, `.` (CurDir), and trailing slashes resolve unchanged. + assert_eq!( + core.prepare_path("a/b.txt").unwrap(), + PathBuf::from("/data/root/a/b.txt") + ); + assert_eq!( + core.prepare_path("a/b/").unwrap(), + PathBuf::from("/data/root/a/b") + ); + assert_eq!( + core.prepare_path("./a/b").unwrap(), + PathBuf::from("/data/root/a/b") + ); + // A key containing `..` only as a substring of a name is not a traversal. + assert_eq!( + core.prepare_path("a..b").unwrap(), + PathBuf::from("/data/root/a..b") + ); + } } diff --git a/core/services/monoiofs/src/deleter.rs b/core/services/monoiofs/src/deleter.rs index e11b4e30a3cf..a040219087ab 100644 --- a/core/services/monoiofs/src/deleter.rs +++ b/core/services/monoiofs/src/deleter.rs @@ -34,7 +34,7 @@ impl MonoiofsDeleter { impl oio::OneShotDelete for MonoiofsDeleter { async fn delete_once(&self, path: String, _: OpDelete) -> Result<()> { - let path = self.core.prepare_path(&path); + let path = self.core.prepare_path(&path)?; let meta = self .core .dispatch({