From b4d6cad2b845213b887bb5374134a0f606aa29a9 Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Tue, 7 Jan 2020 17:08:51 +0100 Subject: [PATCH 1/2] Add a test for path_open read rights --- .../src/bin/path_open_read_without_rights.rs | 88 +++++++++++++++++++ crates/test-programs/wasi-tests/src/lib.rs | 21 +++++ 2 files changed, 109 insertions(+) create mode 100644 crates/test-programs/wasi-tests/src/bin/path_open_read_without_rights.rs diff --git a/crates/test-programs/wasi-tests/src/bin/path_open_read_without_rights.rs b/crates/test-programs/wasi-tests/src/bin/path_open_read_without_rights.rs new file mode 100644 index 000000000000..c3f499bc9d6b --- /dev/null +++ b/crates/test-programs/wasi-tests/src/bin/path_open_read_without_rights.rs @@ -0,0 +1,88 @@ +use std::{env, process}; +use wasi_tests::open_scratch_directory; +use wasi_tests::{drop_rights, fd_get_rights}; + +const TEST_FILENAME: &'static str = "file"; + +unsafe fn create_testfile(dir_fd: wasi::Fd) { + let fd = wasi::path_open( + dir_fd, + 0, + TEST_FILENAME, + wasi::OFLAGS_CREAT | wasi::OFLAGS_EXCL, + wasi::RIGHTS_FD_READ | wasi::RIGHTS_FD_WRITE, + 0, + 0, + ) + .expect("creating a file"); + wasi::fd_close(fd).expect("closing a file"); +} + +unsafe fn try_read_file(dir_fd: wasi::Fd) { + let fd = wasi::path_open(dir_fd, 0, TEST_FILENAME, 0, 0, 0, 0).expect("opening the file"); + + // Check that we don't have the right to exeucute fd_read + let (rbase, rinher) = fd_get_rights(fd); + assert_eq!( + rbase & wasi::RIGHTS_FD_READ, + 0, + "should not have base RIGHTS_FD_READ" + ); + assert_eq!( + rinher & wasi::RIGHTS_FD_READ, + 0, + "should not have inheriting RIGHTS_FD_READ" + ); + + let contents = &mut [0u8; 1]; + let iovec = wasi::Iovec { + buf: contents.as_mut_ptr() as *mut _, + buf_len: contents.len(), + }; + // Since we no longer have the right to fd_read, trying to read a file + // should be an error. + let err = wasi::fd_read(fd, &[iovec]).expect_err("reading bytes from file should fail"); + assert_eq!(err, wasi::ERRNO_NOTCAPABLE, "the errno should be ENOTCAPABLE"); +} + +unsafe fn test_read_rights(dir_fd: wasi::Fd) { + create_testfile(dir_fd); + drop_rights(dir_fd, wasi::RIGHTS_FD_READ, wasi::RIGHTS_FD_READ); + + let (rbase, rinher) = fd_get_rights(dir_fd); + assert_eq!( + rbase & wasi::RIGHTS_FD_READ, + 0, + "dir should not have base RIGHTS_FD_READ" + ); + assert_eq!( + rinher & wasi::RIGHTS_FD_READ, + 0, + "dir should not have inheriting RIGHTS_FD_READ" + ); + + try_read_file(dir_fd); +} + +fn main() { + let mut args = env::args(); + let prog = args.next().unwrap(); + let arg = if let Some(arg) = args.next() { + arg + } else { + eprintln!("usage: {} ", 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) } +} diff --git a/crates/test-programs/wasi-tests/src/lib.rs b/crates/test-programs/wasi-tests/src/lib.rs index a1080c00bee7..c785b7769a00 100644 --- a/crates/test-programs/wasi-tests/src/lib.rs +++ b/crates/test-programs/wasi-tests/src/lib.rs @@ -43,3 +43,24 @@ pub unsafe fn create_file(dir_fd: wasi::Fd, filename: &str) { ); wasi::fd_close(file_fd).expect("closing a file"); } + +// Returns: (rights_base, rights_inheriting) +pub unsafe fn fd_get_rights(fd: wasi::Fd) -> (wasi::Rights, wasi::Rights) { + let fdstat = wasi::fd_fdstat_get(fd).expect("fd_fdstat_get failed"); + (fdstat.fs_rights_base, fdstat.fs_rights_inheriting) +} + +pub unsafe fn drop_rights( + fd: wasi::Fd, + drop_base: wasi::Rights, + drop_inheriting: wasi::Rights, +) { + let (current_base, current_inheriting) = fd_get_rights(fd); + + let new_base = current_base & !drop_base; + let new_inheriting = current_inheriting & !drop_inheriting; + + wasi::fd_fdstat_set_rights(fd, new_base, new_inheriting).expect( + "dropping fd rights", + ); +} From ed3263ff26a4372bc22b8ceb8cd44bc5b1aeb780 Mon Sep 17 00:00:00 2001 From: Marcin Mielniczuk Date: Tue, 7 Jan 2020 17:19:20 +0100 Subject: [PATCH 2/2] Fix rights checks across the codebase. * Fix path_open granting more rights than requested * Add missing rights checks in: fd_fdstat_set_flags, fd_filestat_get, poll_oneoff * Fix `open_scratch_directory` not requesting any rights. * Properly request needed rights in various tests * Add some extra trace-level logging * Remove a no-op restriction of rights to the ones returned by `determine_type_rights`. It was redundant, because `FdEntry:from` internally also called `determine_type_rights` and only dropped some of them. --- .../wasi-tests/src/bin/fd_advise.rs | 6 +++- .../wasi-tests/src/bin/fd_filestat_set.rs | 6 +++- .../wasi-tests/src/bin/fd_readdir.rs | 5 +++- .../wasi-tests/src/bin/file_allocate.rs | 5 +++- .../wasi-tests/src/bin/file_seek_tell.rs | 2 +- .../wasi-tests/src/bin/path_link.rs | 11 ++++++-- .../src/bin/path_open_read_without_rights.rs | 18 ++---------- .../wasi-tests/src/bin/poll_oneoff.rs | 2 +- crates/test-programs/wasi-tests/src/lib.rs | 3 +- crates/wasi-common/src/fdentry.rs | 17 +++++++++-- crates/wasi-common/src/hostcalls_impl/fs.rs | 28 ++++++++++++++----- crates/wasi-common/src/hostcalls_impl/misc.rs | 4 +-- 12 files changed, 71 insertions(+), 36 deletions(-) diff --git a/crates/test-programs/wasi-tests/src/bin/fd_advise.rs b/crates/test-programs/wasi-tests/src/bin/fd_advise.rs index 359f47fbb6ef..408edf3490dc 100644 --- a/crates/test-programs/wasi-tests/src/bin/fd_advise.rs +++ b/crates/test-programs/wasi-tests/src/bin/fd_advise.rs @@ -9,7 +9,11 @@ unsafe fn test_fd_advise(dir_fd: wasi::Fd) { 0, "file", wasi::OFLAGS_CREAT, - wasi::RIGHTS_FD_READ | wasi::RIGHTS_FD_WRITE, + wasi::RIGHTS_FD_READ + | wasi::RIGHTS_FD_WRITE + | wasi::RIGHTS_FD_ADVISE + | wasi::RIGHTS_FD_FILESTAT_GET + | wasi::RIGHTS_FD_ALLOCATE, 0, 0, ) diff --git a/crates/test-programs/wasi-tests/src/bin/fd_filestat_set.rs b/crates/test-programs/wasi-tests/src/bin/fd_filestat_set.rs index ea1be8fe1aa5..0f7cefcfe562 100644 --- a/crates/test-programs/wasi-tests/src/bin/fd_filestat_set.rs +++ b/crates/test-programs/wasi-tests/src/bin/fd_filestat_set.rs @@ -9,7 +9,11 @@ unsafe fn test_fd_filestat_set(dir_fd: wasi::Fd) { 0, "file", wasi::OFLAGS_CREAT, - wasi::RIGHTS_FD_READ | wasi::RIGHTS_FD_WRITE, + wasi::RIGHTS_FD_READ + | wasi::RIGHTS_FD_WRITE + | wasi::RIGHTS_FD_FILESTAT_GET + | wasi::RIGHTS_FD_FILESTAT_SET_SIZE + | wasi::RIGHTS_FD_FILESTAT_SET_TIMES, 0, 0, ) diff --git a/crates/test-programs/wasi-tests/src/bin/fd_readdir.rs b/crates/test-programs/wasi-tests/src/bin/fd_readdir.rs index 8a3931cde842..8790ea0218a9 100644 --- a/crates/test-programs/wasi-tests/src/bin/fd_readdir.rs +++ b/crates/test-programs/wasi-tests/src/bin/fd_readdir.rs @@ -90,7 +90,10 @@ unsafe fn test_fd_readdir(dir_fd: wasi::Fd) { 0, "file", wasi::OFLAGS_CREAT, - wasi::RIGHTS_FD_READ | wasi::RIGHTS_FD_WRITE, + wasi::RIGHTS_FD_READ + | wasi::RIGHTS_FD_WRITE + | wasi::RIGHTS_FD_READDIR + | wasi::RIGHTS_FD_FILESTAT_GET, 0, 0, ) diff --git a/crates/test-programs/wasi-tests/src/bin/file_allocate.rs b/crates/test-programs/wasi-tests/src/bin/file_allocate.rs index f1df5e8060b6..cadee68ab514 100644 --- a/crates/test-programs/wasi-tests/src/bin/file_allocate.rs +++ b/crates/test-programs/wasi-tests/src/bin/file_allocate.rs @@ -9,7 +9,10 @@ unsafe fn test_file_allocate(dir_fd: wasi::Fd) { 0, "file", wasi::OFLAGS_CREAT, - wasi::RIGHTS_FD_READ | wasi::RIGHTS_FD_WRITE, + wasi::RIGHTS_FD_READ + | wasi::RIGHTS_FD_WRITE + | wasi::RIGHTS_FD_ALLOCATE + | wasi::RIGHTS_FD_FILESTAT_GET, 0, 0, ) diff --git a/crates/test-programs/wasi-tests/src/bin/file_seek_tell.rs b/crates/test-programs/wasi-tests/src/bin/file_seek_tell.rs index 9c179580e6e5..59357d3ad848 100644 --- a/crates/test-programs/wasi-tests/src/bin/file_seek_tell.rs +++ b/crates/test-programs/wasi-tests/src/bin/file_seek_tell.rs @@ -9,7 +9,7 @@ unsafe fn test_file_seek_tell(dir_fd: wasi::Fd) { 0, "file", wasi::OFLAGS_CREAT, - wasi::RIGHTS_FD_READ | wasi::RIGHTS_FD_WRITE, + wasi::RIGHTS_FD_READ | wasi::RIGHTS_FD_WRITE | wasi::RIGHTS_FD_SEEK | wasi::RIGHTS_FD_TELL, 0, 0, ) diff --git a/crates/test-programs/wasi-tests/src/bin/path_link.rs b/crates/test-programs/wasi-tests/src/bin/path_link.rs index 3e39e5f2cbf5..3e005012fd30 100644 --- a/crates/test-programs/wasi-tests/src/bin/path_link.rs +++ b/crates/test-programs/wasi-tests/src/bin/path_link.rs @@ -2,8 +2,15 @@ use more_asserts::assert_gt; use std::{env, process}; use wasi_tests::{create_file, open_scratch_directory}; +const TEST_RIGHTS: wasi::Rights = wasi::RIGHTS_FD_READ + | wasi::RIGHTS_PATH_LINK_SOURCE + | wasi::RIGHTS_PATH_LINK_TARGET + | wasi::RIGHTS_FD_FILESTAT_GET + | wasi::RIGHTS_PATH_OPEN + | wasi::RIGHTS_PATH_UNLINK_FILE; + unsafe fn create_or_open(dir_fd: wasi::Fd, name: &str, flags: wasi::Oflags) -> wasi::Fd { - let file_fd = wasi::path_open(dir_fd, 0, name, flags, 0, 0, 0) + let file_fd = wasi::path_open(dir_fd, 0, name, flags, TEST_RIGHTS, TEST_RIGHTS, 0) .unwrap_or_else(|_| panic!("opening '{}'", name)); assert_gt!( file_fd, @@ -14,7 +21,7 @@ unsafe fn create_or_open(dir_fd: wasi::Fd, name: &str, flags: wasi::Oflags) -> w } unsafe fn open_link(dir_fd: wasi::Fd, name: &str) -> wasi::Fd { - let file_fd = wasi::path_open(dir_fd, 0, name, 0, 0, 0, 0) + let file_fd = wasi::path_open(dir_fd, 0, name, 0, TEST_RIGHTS, TEST_RIGHTS, 0) .unwrap_or_else(|_| panic!("opening a link '{}'", name)); assert_gt!( file_fd, diff --git a/crates/test-programs/wasi-tests/src/bin/path_open_read_without_rights.rs b/crates/test-programs/wasi-tests/src/bin/path_open_read_without_rights.rs index c3f499bc9d6b..47d4c7aaa395 100644 --- a/crates/test-programs/wasi-tests/src/bin/path_open_read_without_rights.rs +++ b/crates/test-programs/wasi-tests/src/bin/path_open_read_without_rights.rs @@ -1,23 +1,9 @@ use std::{env, process}; use wasi_tests::open_scratch_directory; -use wasi_tests::{drop_rights, fd_get_rights}; +use wasi_tests::{drop_rights, fd_get_rights, create_file}; const TEST_FILENAME: &'static str = "file"; -unsafe fn create_testfile(dir_fd: wasi::Fd) { - let fd = wasi::path_open( - dir_fd, - 0, - TEST_FILENAME, - wasi::OFLAGS_CREAT | wasi::OFLAGS_EXCL, - wasi::RIGHTS_FD_READ | wasi::RIGHTS_FD_WRITE, - 0, - 0, - ) - .expect("creating a file"); - wasi::fd_close(fd).expect("closing a file"); -} - unsafe fn try_read_file(dir_fd: wasi::Fd) { let fd = wasi::path_open(dir_fd, 0, TEST_FILENAME, 0, 0, 0, 0).expect("opening the file"); @@ -46,7 +32,7 @@ unsafe fn try_read_file(dir_fd: wasi::Fd) { } unsafe fn test_read_rights(dir_fd: wasi::Fd) { - create_testfile(dir_fd); + create_file(dir_fd, TEST_FILENAME); drop_rights(dir_fd, wasi::RIGHTS_FD_READ, wasi::RIGHTS_FD_READ); let (rbase, rinher) = fd_get_rights(dir_fd); diff --git a/crates/test-programs/wasi-tests/src/bin/poll_oneoff.rs b/crates/test-programs/wasi-tests/src/bin/poll_oneoff.rs index 1af4ed057592..235e2d874132 100644 --- a/crates/test-programs/wasi-tests/src/bin/poll_oneoff.rs +++ b/crates/test-programs/wasi-tests/src/bin/poll_oneoff.rs @@ -197,7 +197,7 @@ unsafe fn test_fd_readwrite_valid_fd(dir_fd: wasi::Fd) { 0, "file", wasi::OFLAGS_CREAT, - wasi::RIGHTS_FD_READ | wasi::RIGHTS_FD_WRITE, + wasi::RIGHTS_FD_READ | wasi::RIGHTS_FD_WRITE | wasi::RIGHTS_POLL_FD_READWRITE, 0, 0, ) diff --git a/crates/test-programs/wasi-tests/src/lib.rs b/crates/test-programs/wasi-tests/src/lib.rs index c785b7769a00..008e570bb438 100644 --- a/crates/test-programs/wasi-tests/src/lib.rs +++ b/crates/test-programs/wasi-tests/src/lib.rs @@ -24,7 +24,8 @@ pub fn open_scratch_directory(path: &str) -> Result { } dst.set_len(stat.u.dir.pr_name_len); if dst == path.as_bytes() { - return Ok(wasi::path_open(i, 0, ".", wasi::OFLAGS_DIRECTORY, 0, 0, 0) + let (base, inherit) = fd_get_rights(i); + return Ok(wasi::path_open(i, 0, ".", wasi::OFLAGS_DIRECTORY, base, inherit, 0) .expect("failed to open dir")); } } diff --git a/crates/wasi-common/src/fdentry.rs b/crates/wasi-common/src/fdentry.rs index 73027cda1d4b..a86e6ce8b448 100644 --- a/crates/wasi-common/src/fdentry.rs +++ b/crates/wasi-common/src/fdentry.rs @@ -155,8 +155,21 @@ impl FdEntry { rights_base: wasi::__wasi_rights_t, rights_inheriting: wasi::__wasi_rights_t, ) -> Result<()> { - if !self.rights_base & rights_base != 0 || !self.rights_inheriting & rights_inheriting != 0 - { + let missing_base = !self.rights_base & rights_base; + let missing_inheriting = !self.rights_inheriting & rights_inheriting; + if missing_base != 0 || missing_inheriting != 0 { + log::trace!( + " | validate_rights failed: required: \ + rights_base = {:#x}, rights_inheriting = {:#x}; \ + actual: rights_base = {:#x}, rights_inheriting = {:#x}; \ + missing_base = {:#x}, missing_inheriting = {:#x}", + rights_base, + rights_inheriting, + self.rights_base, + self.rights_inheriting, + missing_base, + missing_inheriting + ); Err(Error::ENOTCAPABLE) } else { Ok(()) diff --git a/crates/wasi-common/src/hostcalls_impl/fs.rs b/crates/wasi-common/src/hostcalls_impl/fs.rs index 7f93fbc27de5..cd321e47b0c8 100644 --- a/crates/wasi-common/src/hostcalls_impl/fs.rs +++ b/crates/wasi-common/src/hostcalls_impl/fs.rs @@ -5,7 +5,6 @@ use crate::fdentry::{Descriptor, FdEntry}; use crate::helpers::*; use crate::memory::*; use crate::sandboxed_tty_writer::SandboxedTTYWriter; -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}; @@ -308,7 +307,7 @@ pub(crate) unsafe fn fd_fdstat_set_flags( let fd = wasi_ctx .get_fd_entry(fd)? - .as_descriptor(0, 0)? + .as_descriptor(wasi::__WASI_RIGHTS_FD_FDSTAT_SET_FLAGS, 0)? .as_os_handle(); hostcalls_impl::fd_fdstat_set_flags(&fd, fdflags) @@ -574,6 +573,11 @@ pub(crate) unsafe fn path_open( let (needed_base, needed_inheriting) = path_open_rights(fs_rights_base, fs_rights_inheriting, oflags, fs_flags); + trace!( + " | needed_base = {}, needed_inheriting = {}", + needed_base, + needed_inheriting + ); let fe = wasi_ctx.get_fd_entry(dirfd)?; let resolved = path_get( fe, @@ -593,13 +597,20 @@ pub(crate) unsafe fn path_open( | wasi::__WASI_RIGHTS_FD_FILESTAT_SET_SIZE) != 0; + trace!( + " | calling path_open impl: read={}, write={}", + read, + write + ); 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 explicitly requested. + // This should not be needed, but currently determine_type_and_access_rights, + // which is used by FdEntry::from, may grant extra rights while inferring it + // from the open mode. + fe.rights_base &= fs_rights_base; + fe.rights_inheriting &= fs_rights_inheriting; let guest_fd = wasi_ctx.insert_fd_entry(fe)?; trace!(" | *fd={:?}", guest_fd); @@ -709,7 +720,10 @@ pub(crate) unsafe fn fd_filestat_get( filestat_ptr ); - let fd = wasi_ctx.get_fd_entry(fd)?.as_descriptor(0, 0)?.as_file()?; + let fd = wasi_ctx + .get_fd_entry(fd)? + .as_descriptor(wasi::__WASI_RIGHTS_FD_FILESTAT_GET, 0)? + .as_file()?; let host_filestat = hostcalls_impl::fd_filestat_get(fd)?; trace!(" | *filestat_ptr={:?}", host_filestat); diff --git a/crates/wasi-common/src/hostcalls_impl/misc.rs b/crates/wasi-common/src/hostcalls_impl/misc.rs index 696eb1b7bab6..7e266033f8bb 100644 --- a/crates/wasi-common/src/hostcalls_impl/misc.rs +++ b/crates/wasi-common/src/hostcalls_impl/misc.rs @@ -242,9 +242,9 @@ pub(crate) fn poll_oneoff( { let wasi_fd = unsafe { subscription.u.fd_readwrite.file_descriptor }; let rights = if r#type == wasi::__WASI_EVENTTYPE_FD_READ { - wasi::__WASI_RIGHTS_FD_READ + wasi::__WASI_RIGHTS_FD_READ | wasi::__WASI_RIGHTS_POLL_FD_READWRITE } else { - wasi::__WASI_RIGHTS_FD_WRITE + wasi::__WASI_RIGHTS_FD_WRITE | wasi::__WASI_RIGHTS_POLL_FD_READWRITE }; match unsafe {