Skip to content

fix(services): confine compfs/monoiofs object keys to root by rejecting parent-dir traversal #7702

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tonghuaroot wants to merge 1 commit into
base: main
Choose a base branch
from
+151 −23
Open

fix(services): confine compfs/monoiofs object keys to root by rejecting parent-dir traversal #7702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions core/services/compfs/src/backend.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ impl Access for CompfsBackend {
}

async fn create_dir(&self, path: &str, _: OpCreateDir) -> Result<RpCreateDir> {
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 })
Expand All @@ -186,7 +186,7 @@ impl Access for CompfsBackend {
}

async fn stat(&self, path: &str, _: OpStat) -> Result<RpStat> {
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 })
Expand Down Expand Up @@ -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 {
Expand All @@ -247,8 +247,8 @@ impl Access for CompfsBackend {
}

async fn rename(&self, from: &str, to: &str, _: OpRename) -> Result<RpRename> {
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 {
Expand All @@ -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 {
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
73 changes: 71 additions & 2 deletions core/services/compfs/src/core.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
// under the License.

use std::future::Future;
use std::path::Path;
use std::path::PathBuf;
use std::sync::Arc;

Expand Down Expand Up @@ -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<PathBuf> {
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<Fn, Fut, R>(&self, f: Fn) -> opendal_core::Result<R>
Expand Down Expand Up @@ -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")
);
}
}
4 changes: 2 additions & 2 deletions core/services/compfs/src/deleter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 5 additions & 5 deletions core/services/monoiofs/src/backend.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ impl Access for MonoiofsBackend {
}

async fn stat(&self, path: &str, _args: OpStat) -> Result<RpStat> {
let path = self.core.prepare_path(path);
let path = self.core.prepare_path(path)?;
let meta = self
.core
.dispatch(move || monoio::fs::metadata(path))
Expand All @@ -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)))
}
Expand All @@ -144,7 +144,7 @@ impl Access for MonoiofsBackend {
}

async fn rename(&self, from: &str, to: &str, _args: OpRename) -> Result<RpRename> {
let from = self.core.prepare_path(from);
let from = self.core.prepare_path(from)?;
// ensure file exists
self.core
.dispatch({
Expand All @@ -162,7 +162,7 @@ impl Access for MonoiofsBackend {
}

async fn create_dir(&self, path: &str, _args: OpCreateDir) -> Result<RpCreateDir> {
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
Expand All @@ -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({
Expand Down
67 changes: 63 additions & 4 deletions core/services/monoiofs/src/core.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<PathBuf> {
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<PathBuf> {
let path = self.prepare_path(path);
let path = self.prepare_path(path)?;
let parent = path
.parent()
.ok_or_else(|| {
Expand Down Expand Up @@ -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")
);
}
}
2 changes: 1 addition & 1 deletion core/services/monoiofs/src/deleter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down
Loading