Add a test and a hotfix for path_open read rights

crates/test-programs/wasi-tests/src/bin/path_open_read_without_rights.rs

@@ -0,0 +1,96 @@
+use std::{env, process};
+use wasi_old::wasi_unstable;
+use wasi_tests::open_scratch_directory;
+use wasi_tests::utils::{cleanup_file, close_fd, drop_rights, fd_get_rights};
+use wasi_tests::wasi_wrappers::{wasi_fd_read, wasi_path_open};
+
+const TEST_FILENAME: &'static str = "file";
+
+unsafe fn create_testfile(dir_fd: wasi_unstable::Fd) {
+    let mut fd: wasi_unstable::Fd = wasi_unstable::Fd::max_value() - 1;
+    let status = wasi_path_open(
+        dir_fd,
+        0,
+        TEST_FILENAME,
+        wasi_unstable::O_CREAT | wasi_unstable::O_EXCL,
+        wasi_unstable::RIGHT_FD_READ | wasi_unstable::RIGHT_FD_WRITE,
+        0,
+        0,
+        &mut fd,
+    );
+    assert_eq!(
+        status,
+        wasi_unstable::raw::__WASI_ESUCCESS,
+        "creating a file"
+    );
+    close_fd(fd);
+}
+
+unsafe fn try_read_file(dir_fd: wasi_unstable::Fd) {
+    let mut fd: wasi_unstable::Fd = wasi_unstable::Fd::max_value() - 1;
+    let status = wasi_path_open(dir_fd, 0, TEST_FILENAME, 0, 0, 0, 0, &mut fd);
+    assert_eq!(
+        status,
+        wasi_unstable::raw::__WASI_ESUCCESS,
+        "opening the test file"
+    );
+
+    // Check that we don't have the right to exeucute fd_read
+    let (rbase, rinher) = fd_get_rights(fd);
+    assert_eq!(rbase & wasi_unstable::RIGHT_FD_READ, 0, "should not have base RIGHT_FD_READ");
+    assert_eq!(rinher & wasi_unstable::RIGHT_FD_READ, 0, "should not have inheriting RIGHT_FD_READ");
+
+    let contents = &mut [0u8; 1];
+    let iovec = wasi_unstable::IoVec {
+        buf: contents.as_mut_ptr() as *mut libc::c_void,
+        buf_len: contents.len(),
+    };
+    let mut nread = 0;
+    // Since we no longer have the right to fd_read, trying to read a file
+    // should be an error.
+    let status = wasi_fd_read(fd, &[iovec], &mut nread);
+    assert_ne!(
+        status,
+        wasi_unstable::raw::__WASI_ESUCCESS,
+        "reading bytes from file"
+    );
+}
+
+unsafe fn test_read_rights(dir_fd: wasi_unstable::Fd) {
+    create_testfile(dir_fd);
+    drop_rights(
+        dir_fd,
+        wasi_unstable::RIGHT_FD_READ,
+        wasi_unstable::RIGHT_FD_READ,
+    );
+
+    let (rbase, rinher) = fd_get_rights(dir_fd);
+    assert_eq!(rbase & wasi_unstable::RIGHT_FD_READ, 0, "dir should not have base RIGHT_FD_READ");
+    assert_eq!(rinher & wasi_unstable::RIGHT_FD_READ, 0, "dir should not have inheriting RIGHT_FD_READ");
+
+    try_read_file(dir_fd);
+    cleanup_file(dir_fd, TEST_FILENAME);
+}
+
+fn main() {
+    let mut args = env::args();
+    let prog = args.next().unwrap();
+    let arg = if let Some(arg) = args.next() {
+        arg
+    } else {
+        eprintln!("usage: {} <scratch directory>", prog);
+        process::exit(1);
+    };
+
+    // Open scratch directory
+    let dir_fd = match open_scratch_directory(&arg) {
+        Ok(dir_fd) => dir_fd,
+        Err(err) => {
+            eprintln!("{}", err);
+            process::exit(1)
+        }
+    };
+
+    // Run the tests.
+    unsafe { test_read_rights(dir_fd) }
+}

crates/test-programs/wasi-tests/src/utils.rs

@@ -52,3 +52,32 @@ pub unsafe fn cleanup_file(dir_fd: wasi_unstable::Fd, file_name: &str) {
 pub unsafe fn close_fd(fd: wasi_unstable::Fd) {
     assert!(wasi_unstable::fd_close(fd).is_ok(), "closing a file");
 }
+
+// Returns: (rights_base, rights_inheriting)
+pub unsafe fn fd_get_rights(fd: wasi_unstable::Fd) -> (wasi_unstable::Rights, wasi_unstable::Rights) {
+    let mut fdstat: wasi_unstable::FdStat = std::mem::zeroed();
+    let status = wasi_fd_fdstat_get(fd, &mut fdstat);
+    assert_eq!(
+        status,
+        wasi_unstable::raw::__WASI_ESUCCESS,
+        "calling fd_fdstat_get"
+    );
+
+    (fdstat.fs_rights_base, fdstat.fs_rights_inheriting)
+}
+
+pub unsafe fn drop_rights(
+    fd: wasi_unstable::Fd,
+    drop_base: wasi_unstable::Rights,
+    drop_inheriting: wasi_unstable::Rights,
+) {
+    let (current_base, current_inheriting) = fd_get_rights(fd);
+
+    let new_base = current_base & !drop_base;
+    let new_inheriting = current_inheriting & !drop_inheriting;
+
+    assert!(
+        wasi_unstable::fd_fdstat_set_rights(fd, new_base, new_inheriting).is_ok(),
+        "dropping fd rights",
+    );
+}

crates/wasi-common/src/hostcalls_impl/fs.rs

@@ -4,7 +4,6 @@ use crate::ctx::WasiCtx;
 use crate::fdentry::{Descriptor, FdEntry};
 use crate::helpers::*;
 use crate::memory::*;
-use crate::sys::fdentry_impl::determine_type_rights;
 use crate::sys::hostcalls_impl::fs_helpers::path_open_rights;
 use crate::sys::{host_impl, hostcalls_impl};
 use crate::{helpers, host, wasi, wasi32, Error, Result};
@@ -542,6 +541,7 @@ pub(crate) unsafe fn path_open(
     let (needed_base, needed_inheriting) =
         path_open_rights(fs_rights_base, fs_rights_inheriting, oflags, fs_flags);
     let fe = wasi_ctx.get_fd_entry(dirfd)?;
+    let parent_inheriting = fe.rights_inheriting;
     let resolved = path_get(
         fe,
         needed_base,
@@ -561,12 +561,16 @@ pub(crate) unsafe fn path_open(
         != 0;
 
     let fd = hostcalls_impl::path_open(resolved, read, write, oflags, fs_flags)?;
-
-    // Determine the type of the new file descriptor and which rights contradict with this type
-    let (_ty, max_base, max_inheriting) = determine_type_rights(&fd)?;
     let mut fe = FdEntry::from(fd)?;
-    fe.rights_base &= max_base;
-    fe.rights_inheriting &= max_inheriting;
+    // We need to manually deny the rights which are not parent's inheriting rights.
+    // This should not be needed, but currently determine_type_and_access_rights,
+    // which is used by FdEntry::from, may grant more rights than requested.
+    //
+    // This is a hotfix. It needs to be discussed whether or not `path_open`
+    // returning a descriptor with more rights than specified
+    // in `rights_base` and `rights_inheriting` violates the WASI spec.
+    fe.rights_base &= parent_inheriting;
+    fe.rights_inheriting &= parent_inheriting;
     let guest_fd = wasi_ctx.insert_fd_entry(fe)?;
 
     trace!("     | *fd={:?}", guest_fd);

crates/wasi-common/src/old/snapshot_0/hostcalls_impl/fs.rs

@@ -4,7 +4,6 @@ use crate::old::snapshot_0::ctx::WasiCtx;
 use crate::old::snapshot_0::fdentry::{Descriptor, FdEntry};
 use crate::old::snapshot_0::helpers::*;
 use crate::old::snapshot_0::memory::*;
-use crate::old::snapshot_0::sys::fdentry_impl::determine_type_rights;
 use crate::old::snapshot_0::sys::hostcalls_impl::fs_helpers::path_open_rights;
 use crate::old::snapshot_0::sys::{host_impl, hostcalls_impl};
 use crate::old::snapshot_0::{helpers, host, wasi, wasi32, Error, Result};
@@ -542,6 +541,7 @@ pub(crate) unsafe fn path_open(
     let (needed_base, needed_inheriting) =
         path_open_rights(fs_rights_base, fs_rights_inheriting, oflags, fs_flags);
     let fe = wasi_ctx.get_fd_entry(dirfd)?;
+    let parent_inheriting = fe.rights_inheriting;
     let resolved = path_get(
         fe,
         needed_base,
@@ -561,12 +561,16 @@ pub(crate) unsafe fn path_open(
         != 0;
 
     let fd = hostcalls_impl::path_open(resolved, read, write, oflags, fs_flags)?;
-
-    // Determine the type of the new file descriptor and which rights contradict with this type
-    let (_ty, max_base, max_inheriting) = determine_type_rights(&fd)?;
     let mut fe = FdEntry::from(fd)?;
-    fe.rights_base &= max_base;
-    fe.rights_inheriting &= max_inheriting;
+    // We need to manually deny the rights which are not parent's inheriting rights.
+    // This should not be needed, but currently determine_type_and_access_rights,
+    // which is used by FdEntry::from, may grant more rights than requested.
+    //
+    // This is a hotfix. It needs to be discussed whether or not `path_open`
+    // returning a descriptor with more rights than specified
+    // in `rights_base` and `rights_inheriting` violates the WASI spec.
+    fe.rights_base &= parent_inheriting;
+    fe.rights_inheriting &= parent_inheriting;
     let guest_fd = wasi_ctx.insert_fd_entry(fe)?;
 
     trace!("     | *fd={:?}", guest_fd);