Merge #463

463: Prevent parent directory from being opened without being preopened wasi r=MarkMcCaskey a=MarkMcCaskey resolves #462 Because the logic of opening a directory and traversing the tree are separate, we allowed one level of `..` to be opened beyond what was preopened The diff on github isn't clear, but this adds an if and then puts the previous logic in an else block Co-authored-by: Mark McCaskey <[email protected]>
wasmerio · May 22, 2019 · dcf0a7c · dcf0a7c
2 parents 5ff9e74 + dc3ac15
commit dcf0a7c
Show file tree

Hide file tree

Showing 8 changed files with 122 additions and 80 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,7 @@ Blocks of changes will separated by version increments.
 
 ## **[Unreleased]**
 
+- [#463](https://github.com/wasmerio/wasmer/pull/463) Fix bug in WASI path_open allowing one level above preopened dir to be accessed
 - [#461](https://github.com/wasmerio/wasmer/pull/461) Prevent passing negative lengths in various places in the runtime C API
 - [#459](https://github.com/wasmerio/wasmer/pull/459) Add monotonic and real time clocks for wasi on windows
 - [#447](https://github.com/wasmerio/wasmer/pull/447) Add trace macro (`--features trace`) for more verbose debug statements

diff --git a/lib/wasi/src/syscalls/mod.rs b/lib/wasi/src/syscalls/mod.rs
@@ -1463,97 +1463,117 @@ pub fn path_open(
         "Looking for file {} in directory {:#?}",
         file_name, cumulative_path
     );
+
     cumulative_path.push(file_name);
     let file_path = cumulative_path;
 
-    let out_fd = if let Kind::Dir { entries, .. } = &mut state.fs.inodes[cur_dir_inode].kind {
-        if let Some(child) = entries.get(file_name).cloned() {
-            let child_inode_val = &state.fs.inodes[child];
-            // early return based on flags
-            if o_flags & __WASI_O_EXCL != 0 {
-                return __WASI_EEXIST;
-            }
-            if o_flags & __WASI_O_DIRECTORY != 0 {
-                match &child_inode_val.kind {
-                    Kind::Dir { .. } => (),
-                    Kind::Symlink { .. } => {
-                        unimplemented!("Symlinks not yet supported in path_open")
-                    }
-                    _ => return __WASI_ENOTDIR,
-                }
+    let out_fd = if let Kind::Dir {
+        entries, parent, ..
+    } = &mut state.fs.inodes[cur_dir_inode].kind
+    {
+        // short circuit logic if attempting to get parent
+        if file_name == ".." {
+            if let Some(p) = parent {
+                let parent_inode = *p;
+                wasi_try!(state.fs.create_fd(
+                    fs_rights_base,
+                    fs_rights_inheriting,
+                    fs_flags,
+                    parent_inode
+                ))
+            } else {
+                return __WASI_EACCES;
             }
-            // do logic on child
-            wasi_try!(state
-                .fs
-                .create_fd(fs_rights_base, fs_rights_inheriting, fs_flags, child))
         } else {
-            debug!("Attempting to load file from host system");
-            let file_metadata = file_path.metadata();
-            // if entry does not exist in parent directory, try to lazily
-            // load it; possibly creating or truncating it if flags set
-            let kind = if file_metadata.is_ok() && file_metadata.unwrap().is_dir() {
-                // special dir logic
-                Kind::Dir {
-                    parent: Some(cur_dir_inode),
-                    path: file_path.clone(),
-                    entries: Default::default(),
+            if let Some(child) = entries.get(file_name).cloned() {
+                let child_inode_val = &state.fs.inodes[child];
+                // early return based on flags
+                if o_flags & __WASI_O_EXCL != 0 {
+                    return __WASI_EEXIST;
+                }
+                if o_flags & __WASI_O_DIRECTORY != 0 {
+                    match &child_inode_val.kind {
+                        Kind::Dir { .. } => (),
+                        Kind::Symlink { .. } => {
+                            unimplemented!("Symlinks not yet supported in path_open")
+                        }
+                        _ => return __WASI_ENOTDIR,
+                    }
                 }
+                // do logic on child
+                wasi_try!(state
+                    .fs
+                    .create_fd(fs_rights_base, fs_rights_inheriting, fs_flags, child))
             } else {
-                // file is not a dir
-                let real_opened_file = {
-                    let mut open_options = std::fs::OpenOptions::new();
-                    let open_options = open_options.read(true);
-                    let open_options = if fs_rights_base & __WASI_RIGHT_FD_WRITE != 0 {
-                        open_options.write(true)
-                    } else {
-                        open_options
-                    };
-                    let open_options = if o_flags & __WASI_O_CREAT != 0 {
-                        debug!(
-                            "File {:?} may be created when opened if it does not exist",
-                            &file_path
-                        );
-                        open_options.create(true)
-                    } else {
-                        open_options
-                    };
-                    let open_options = if o_flags & __WASI_O_TRUNC != 0 {
-                        debug!("File {:?} will be truncated when opened", &file_path);
-                        open_options.truncate(true)
-                    } else {
-                        open_options
-                    };
-                    debug!("Opening host file {:?}", &file_path);
-                    let real_open_file =
-                        wasi_try!(open_options.open(&file_path).map_err(|_| __WASI_EIO));
+                debug!("Attempting to load file from host system");
+
+                let file_metadata = file_path.metadata();
+                // if entry does not exist in parent directory, try to lazily
+                // load it; possibly creating or truncating it if flags set
+                let kind = if file_metadata.is_ok() && file_metadata.unwrap().is_dir() {
+                    // special dir logic
+                    Kind::Dir {
+                        parent: Some(cur_dir_inode),
+                        path: file_path.clone(),
+                        entries: Default::default(),
+                    }
+                } else {
+                    // file is not a dir
+                    let real_opened_file = {
+                        let mut open_options = std::fs::OpenOptions::new();
+                        let open_options = open_options.read(true);
+                        let open_options = if fs_rights_base & __WASI_RIGHT_FD_WRITE != 0 {
+                            open_options.write(true)
+                        } else {
+                            open_options
+                        };
+                        let open_options = if o_flags & __WASI_O_CREAT != 0 {
+                            debug!(
+                                "File {:?} may be created when opened if it does not exist",
+                                &file_path
+                            );
+                            open_options.create(true)
+                        } else {
+                            open_options
+                        };
+                        let open_options = if o_flags & __WASI_O_TRUNC != 0 {
+                            debug!("File {:?} will be truncated when opened", &file_path);
+                            open_options.truncate(true)
+                        } else {
+                            open_options
+                        };
+                        debug!("Opening host file {:?}", &file_path);
+                        let real_open_file =
+                            wasi_try!(open_options.open(&file_path).map_err(|_| __WASI_EIO));
 
-                    real_open_file
+                        real_open_file
+                    };
+                    Kind::File {
+                        handle: WasiFile::HostFile(real_opened_file),
+                    }
                 };
-                Kind::File {
-                    handle: WasiFile::HostFile(real_opened_file),
-                }
-            };
 
-            // record lazily loaded or newly created fd
-            let new_inode = state.fs.inodes.insert(InodeVal {
-                stat: wasi_try!(get_stat_for_kind(&kind).ok_or(__WASI_EIO)),
-                is_preopened: false,
-                name: file_name.clone(),
-                kind,
-            });
-
-            // reborrow to insert entry
-            if let Kind::Dir { entries, .. } = &mut state.fs.inodes[working_dir.inode].kind {
-                entries.insert(file_name.clone(), new_inode);
+                // record lazily loaded or newly created fd
+                let new_inode = state.fs.inodes.insert(InodeVal {
+                    stat: wasi_try!(get_stat_for_kind(&kind).ok_or(__WASI_EIO)),
+                    is_preopened: false,
+                    name: file_name.clone(),
+                    kind,
+                });
+
+                // reborrow to insert entry
+                if let Kind::Dir { entries, .. } = &mut state.fs.inodes[working_dir.inode].kind {
+                    entries.insert(file_name.clone(), new_inode);
+                }
+                let new_fd = wasi_try!(state.fs.create_fd(
+                    fs_rights_base,
+                    fs_rights_inheriting,
+                    fs_flags,
+                    new_inode,
+                ));
+
+                new_fd
             }
-            let new_fd = wasi_try!(state.fs.create_fd(
-                fs_rights_base,
-                fs_rights_inheriting,
-                fs_flags,
-                new_inode,
-            ));
-
-            new_fd
         }
     } else {
         // working_dir did not match on Kind::Dir

diff --git a/lib/wasi/tests/wasitests/fs_sandbox_test.rs b/lib/wasi/tests/wasitests/fs_sandbox_test.rs
@@ -0,0 +1,9 @@
+#[test]
+fn test_fs_sandbox_test() {
+    assert_wasi_output!(
+        "../../wasitests/fs_sandbox_test.wasm",
+        "fs_sandbox_test",
+        vec![],
+        "../../wasitests/fs_sandbox_test.out"
+    );
+}
diff --git a/lib/wasi/tests/wasitests/mod.rs b/lib/wasi/tests/wasitests/mod.rs
@@ -7,6 +7,7 @@
 mod _common;
 mod create_dir;
 mod file_metadata;
+mod fs_sandbox_test;
 mod hello;
 mod mapdir;
 mod quine;
diff --git a/lib/wasi/wasitests/fs_sandbox_test b/lib/wasi/wasitests/fs_sandbox_test
diff --git a/lib/wasi/wasitests/fs_sandbox_test.out b/lib/wasi/wasitests/fs_sandbox_test.out
@@ -0,0 +1 @@
+Reading the parent directory was okay? false
diff --git a/lib/wasi/wasitests/fs_sandbox_test.rs b/lib/wasi/wasitests/fs_sandbox_test.rs
@@ -0,0 +1,10 @@
+fn main() {
+    #[cfg(target = "wasi")]
+    let result = std::fs::read_dir("..");
+    #[cfg(not(target = "wasi"))]
+    let result: Result<(), String> = Err("placeholder".to_string());
+    println!(
+        "Reading the parent directory was okay? {:?}",
+        result.is_ok()
+    );
+}
diff --git a/lib/wasi/wasitests/fs_sandbox_test.wasm b/lib/wasi/wasitests/fs_sandbox_test.wasm