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

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

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

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<Path>, dst: impl AsRef<Path>) -> Isolate {
         let src = src.as_ref();