From b56d18c8d7b337aa19fe5b14c8af51b68db00c13 Mon Sep 17 00:00:00 2001 From: Harry Stern Date: Tue, 16 Apr 2024 17:52:40 -0400 Subject: [PATCH] Prohibit . and .. in bindmounts I'm not really sure if this could actually be exploited in any way, but it also prevents accidentally making bindmounts outside the root directory as well. Also update the docstring for add_bind_mount --- examples/isolate_test.rs | 44 ++++++++++++++++++++++++++++++++++++++ src/isolate/isolate_sys.rs | 13 ++++++++++- src/isolate/mod.rs | 8 +++++-- 3 files changed, 62 insertions(+), 3 deletions(-) diff --git a/examples/isolate_test.rs b/examples/isolate_test.rs index e93058d..5450405 100644 --- a/examples/isolate_test.rs +++ b/examples/isolate_test.rs @@ -188,6 +188,29 @@ fn test_safetycontext() { } +/// Test an `Isolate` will not bindmount outside of its root +fn test_isolate_bad_bindmount() { + // test /.. + let output = Isolate::run("isolate_bad_bindmount_absolute", &HashMap::new()) + .expect("running isolate failed"); + let stdout = String::from_utf8_lossy(&output.stdout).to_string(); + let stderr = String::from_utf8_lossy(&output.stderr).to_string(); + + assert!(!output.status.success(), "{:?}\nstdout {}\nstderr {}", output.status, stdout, stderr); + assert!(stderr.contains("dst directory must not contain .. paths:"), "{:?}\nstdout {}\nstderr {}", output.status, stdout, stderr); + + // test ./ + let output = Isolate::run("isolate_bad_bindmount_relative", &HashMap::new()) + .expect("running isolate failed"); + let stdout = String::from_utf8_lossy(&output.stdout).to_string(); + let stderr = String::from_utf8_lossy(&output.stderr).to_string(); + + assert!(!output.status.success(), "{:?}\nstdout {}\nstderr {}", output.status, stdout, stderr); + assert!(stderr.contains("dst directory must not contain . paths:"), "{:?}\nstdout {}\nstderr {}", output.status, stdout, stderr); + println!("isolate_bad_bindmount passed"); +} + + fn isolate_uid(name: &'static str) -> Isolate { fn uid() { let uid = unsafe { libc::getuid() }; @@ -321,6 +344,24 @@ fn isolate_no_network(name: &'static str) -> Isolate { .add_bind_mount("/", "/") } +fn isolate_bad_bindmount_absolute(name: &'static str) -> Isolate { + // we will not reach the actual isolate so it doesn't matter what function we call + fn hello() { + println!("hello"); + } + Isolate::new(name, hello) + .add_bind_mount("/", "/a/b/../../..") +} + +fn isolate_bad_bindmount_relative(name: &'static str) -> Isolate { + // we will not reach the actual isolate so it doesn't matter what function we call + fn hello() { + println!("hello"); + } + Isolate::new(name, hello) + .add_bind_mount("/", "./a/") +} + fn isolate_with_safetycontext(name: &'static str) -> Isolate { use extrasafe::SafetyContext; use extrasafe::builtins::*; @@ -357,6 +398,8 @@ fn main() { Isolate::main_hook("isolate_with_network", isolate_with_network); Isolate::main_hook("isolate_no_network", isolate_no_network); Isolate::main_hook("isolate_with_safetycontext", isolate_with_safetycontext); + Isolate::main_hook("isolate_bad_bindmount_absolute", isolate_bad_bindmount_absolute); + Isolate::main_hook("isolate_bad_bindmount_relative", isolate_bad_bindmount_relative); let argv0 = std::env::args().next().unwrap(); if argv0.contains("isolate_test") { @@ -378,6 +421,7 @@ fn main() { test_no_network(); test_tmpfs_size_limit(); test_isolate_fail(); + test_isolate_bad_bindmount(); } else { panic!("isolate didn't hit its hook: {}", argv0); diff --git a/src/isolate/isolate_sys.rs b/src/isolate/isolate_sys.rs index e7547b8..7190b26 100644 --- a/src/isolate/isolate_sys.rs +++ b/src/isolate/isolate_sys.rs @@ -177,8 +177,19 @@ fn mount_tmpfs(tempdir: &Path, max_size: u32) { fail_negative!(rc, "failed to make tmpfs private after mounting"); } -/// Set up a bindmount inside the new root +/// Set up a bindmount inside the new root. root is the root tmpfs dir, src is a directory from the +/// original filesystem, and dst is the location inside root in which to bindmount src. dst may be +/// absolute or relative - in both cases it is joined to root. Intermediate directories are created +/// automatically when mounting. fn do_bindmount(root: &Path, src: &Path, dst: &Path) { + // TODO (?) does this actually have any security implications? It seems like a good thing to do + // in general but I'm not sure if you could actually do anything "bad" with it that you + // couldn't do if an attacker otherwise controlled a dst path. + for a in dst.ancestors() { + assert!(!a.ends_with("."), "bindmount dst directory must not contain . paths: {}", dst.display()); + assert!(!a.ends_with(".."), "bindmount dst directory must not contain .. paths: {}", dst.display()); + } + let dst = if dst.is_absolute() { dst.strip_prefix("/").unwrap() } else { dst }; let dst = root.join(dst); diff --git a/src/isolate/mod.rs b/src/isolate/mod.rs index af6dcf6..2b81c8b 100644 --- a/src/isolate/mod.rs +++ b/src/isolate/mod.rs @@ -74,8 +74,12 @@ impl Isolate { /// Bind mount the file or path in src to the file or path in dst. If `dst` is relative, it is /// treated as relative to the root of the isolate's tmpfs. If `dst` is absolute, it will still - /// be absolute relative to the isolate's tmpfs root.`dst` will be created if it does not - /// exist. + /// be joined as if it were relative to the isolate's tmpfs root. `dst` will be created if it does not + /// exist, including all intermediate directories. + /// + /// Bindmounts to files and symlinks are allowed, but if a symlink points outside the current + /// filesystem it will not function. + /// /// Bind mounts are not created until `Isolate::main_hook` executes. pub fn add_bind_mount(mut self, src: impl AsRef, dst: impl AsRef) -> Isolate { let src = src.as_ref();