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();