Skip to content

Commit

Permalink
Prohibit . and .. in bindmounts
Browse files Browse the repository at this point in the history
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
  • Loading branch information
boustrophedon committed Apr 18, 2024
1 parent 5ad0ece commit b56d18c
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 3 deletions.
44 changes: 44 additions & 0 deletions examples/isolate_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() };
Expand Down Expand Up @@ -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::*;
Expand Down Expand Up @@ -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") {
Expand All @@ -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);
Expand Down
13 changes: 12 additions & 1 deletion src/isolate/isolate_sys.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
8 changes: 6 additions & 2 deletions src/isolate/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Path>, dst: impl AsRef<Path>) -> Isolate {
let src = src.as_ref();
Expand Down

0 comments on commit b56d18c

Please sign in to comment.