Add a test and a hotfix for path_open read rights #618

marmistrz · 2019-11-21T21:07:05Z

With current implementation new file descriptors may have RIGHT_FD_READ even after it was dropped on the parent descriptor beforehand. This makes it possible to read the file content when it should be denied. (on Unix)

Connected with #570 & #617.

marmistrz · 2019-11-25T19:36:01Z

I created a hotfix, but this is blocked by #632.

Add a routine for obtaining an `OsFile` containing a file descriptor for stdin/stdout/stderr so that we can do fd_fdstat_get on them.

This commits renames `OsFile` to `OsHandle` which seems to make more sense semantically as it is permitted to hold a valid OS handle to OS entities other than simply file/dir (e.g., socket, stream, etc.). As such, this commit also renames methods on `Descriptor` struct from `as_actual_file` to `as_file` as this in reality does pertain ops on FS entities such as files/dirs, and `as_file` to `as_os_handle` as in this case it can be anything, from file, through a socket, to a stream.

To prevent a `ManuallyDrop<OsHandleRef>` from outliving the resource it holds on to, create an `OsHandleRef` class parameterized on the lifetime of the `Descriptor`.

While we are waiting for the Rust toolchain to use the new ABI, I thought it might be useful to sync `snapshot_0` with the latest code in `wasi-common` "upstream". This mainly includes the latest refactoring effort to unify the machinery for `fd_readdir` between Linux, Windows and BSD.

Also, 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.

marmistrz · 2019-11-28T15:56:18Z

The hotfix works for me locally and is ready for review. See the commit message for the reason why determine_type_rights was removed.
cc @kubkon @sunfishcode @peterhuene

This PR builds upon #647, hence the merge conflicts.

marmistrz · 2019-12-15T12:10:45Z

@kubkon @sunfishcode @peterhuene could you please review this change?

peterhuene

This makes sense to me! Some minor feedback provided. Let's get @kubkon and @sunfishcode's feedback before moving forward.

peterhuene · 2019-12-16T15:18:36Z

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;


Should we update this to use the wasi crate given it's a new test program?

I'm quite confused how to cope with the snapshot stuff or wasi vs wasi_old, so any input is appreciated.

As an example, the test program added for #711 uses the wasi crate, which will currently target the preview1 impl.

I assume we'll be moving all the test programs over to the wasi crate sooner or later.

Additionally, I think we'll want dedicated tests targeted at previous previews for the sole purpose of testing backwards compatibility for WASI interface changes so we don't have to snapshot all of the test programs themselves for every snapshot of WASI, but perhaps @sunfishcode @alexcrichton @kubkon know more regarding that.

Yep, the current trend is to use wasi in tests as @peterhuene suggested. As far as I remember, @alexcrichton has already started porting some of the tests to using wasi instead of wasi_old.

In the future, I definitely agree we should have a way to test different versions of the snapshot, preferably in a way that's automatic.

peterhuene · 2019-12-16T15:20:31Z

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


This function may not be necessary if we do update the test propram to use wasi given how it greatly simplifies things.

peterhuene · 2019-12-16T15:20:59Z

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

+    (fdstat.fs_rights_base, fdstat.fs_rights_inheriting)
+}
+
+pub unsafe fn drop_rights(


Likewise for this function, although a utility function that does the same thing using wasi would make sense.

peterhuene · 2019-12-16T15:24:03Z

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

+    // 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`


Let's have that discussion here so we don't add a comment with this is a hotfix.

I think @sunfishcode is more qualified to answer this than I. On the surface it sounds wrong to me, though.

Perhaps we could change "This is a hotfix." for "TODO"?

I intend to remove the it needs to be discussed part when we'll have discussed it ;)

sunfishcode · 2019-12-16T21:44:49Z

The documentation for path_open says that we're allowed to return fewer rights if and only if those rights don't apply to the type of file being opened. So if the user has requested rights that are not in the parent directory's inheriting set, we should return an ERRNO_NOTCAPABLE rather than silently return a file descriptor with those rights masked out.

Code which doesn't know exactly which rights it will need can do an fd_fdstat on the parent file descriptor; this is the case in WASI libc, and it already does do this.

marmistrz · 2019-12-17T15:28:27Z

@sunfishcode but the documentation says nothing about granting more rights than requested. Those rights were not requested by the user, but were implicitly granted by the runtime, as per #617. The file was opened as

     let status = wasi_path_open(dir_fd, 0, TEST_FILENAME, 0, 0, 0, 0, &mut fd);

and we still have the right to fd_read.

The resaon is that, in the current codebase, FdEntry::from will call determine_type_and_access_rights internally to infer the access rights from the fd open mode.

sunfishcode · 2019-12-20T00:29:14Z

We shouldn't grant more rights than the user requests. I think the algorithm for path_open should be:

start with the rights_base and rights_inheriting parameters passed in
if either of those has any flag set that isn't set in dirfd's rights_inheriting field, return ERRNO_NOTCAPABLE.
then, silently clear any flags not set in the sets computed by determine_type_rights

marmistrz · 2019-12-22T10:00:39Z

I agree that such semantics are desirable.

This in particular means that some of the tests are incorrect. For instance in file_allocate.rs the fd is opened without explicit RIGHT_FD_ALLOCATE:

wasmtime/crates/test-programs/wasi-tests/src/bin/file_allocate.rs

Lines 11 to 20 in a582389

    
           let status = wasi_path_open( 
        
               dir_fd, 
        
               0, 
        
               "file", 
        
               wasi_unstable::O_CREAT, 
        
               wasi_unstable::RIGHT_FD_READ | wasi_unstable::RIGHT_FD_WRITE, 
        
               0, 
        
               0, 
        
               &mut file_fd, 
        
           );

but later on fd_allocate is expected to succeed:

wasmtime/crates/test-programs/wasi-tests/src/bin/file_allocate.rs

Lines 52 to 55 in a582389

    
           assert!( 
        
               wasi_unstable::fd_allocate(file_fd, 0, 100).is_ok(), 
        
               "allocating size" 
        
           );

I'll change tests like this one to explicitly pass in the required rights.

One tricky question: if path_open is called with O_TRUNC, do we require the caller to pass in RIGHT_PATH_FILESTAT_SET_SIZE in fs_rights_base or is it enough to only have the right on the parent file descriptor. In other words, should we expect this call to succeed:

wasmtime/crates/test-programs/wasi-tests/src/bin/truncation_rights.rs

Lines 40 to 49 in a582389

    
           status = wasi_path_open( 
        
               dir_fd, 
        
               0, 
        
               "file", 
        
               wasi_unstable::O_TRUNC, 
        
               0, 
        
               0, 
        
               0, 
        
               &mut file_fd, 
        
           );

or should we rather change it to:

        status = wasi_path_open(
            dir_fd,
            0,
            "file",
            wasi_unstable::O_TRUNC,
            wasi_unstable::RIGHT_PATH_FILESTAT_SET_SIZE,
            0,
            0,
            &mut file_fd,
        );

This was referenced Nov 21, 2019

Implicit "read" access when no rights specified in path_open on Unix #617

Open

Do not allow truncating files which are not opened for writing. #570

Closed

marmistrz force-pushed the read-vuln branch from 73e50e9 to f44760c Compare November 26, 2019 06:58

marmistrz mentioned this pull request Nov 26, 2019

Support fd_fdstat_get and fd_renumber on stdin/stdout/stderr #631

Merged

sunfishcode and others added 9 commits November 26, 2019 22:26

Don't dup file descriptors in fd_renumber.

06302fb

Support fd_fdstat_get on stdin/stdout/stderr.

5c6f9c0

Add a routine for obtaining an `OsFile` containing a file descriptor for stdin/stdout/stderr so that we can do fd_fdstat_get on them.

Add a testcase for fd_fdstat_get etc. on stdin etc.

3120af9

Fix compilation on macOS

e7288e9

Fix compilation on Linux

c1c5aca

Introduce OsHandleRef for borrowing OS resources.

f52e1ef

To prevent a `ManuallyDrop<OsHandleRef>` from outliving the resource it holds on to, create an `OsHandleRef` class parameterized on the lifetime of the `Descriptor`.

Fix scoping to pub-priv and backport to snapshot_0

5d49e9d

marmistrz force-pushed the read-vuln branch from f44760c to 4fc3153 Compare November 28, 2019 15:48

marmistrz changed the title ~~Add a test for path_open read rights~~ Add a test and a hotfix for path_open read rights Nov 28, 2019

marmistrz added 3 commits November 28, 2019 16:53

Add a test for path_open read rights

0b7b985

Add a hotfix for wrong path_open rights.

ebdff46

Also, 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.

Fast-forward the snapshot to make the change testable.

175dc81

marmistrz force-pushed the read-vuln branch from 4fc3153 to 175dc81 Compare November 28, 2019 15:53

marmistrz added 4 commits November 28, 2019 17:55

Merge remote-tracking branch 'upstream/master' into read-vuln

ab17eb4

Merge remote-tracking branch 'upstream/master' into read-vuln

80bec5c

Rename wasi -> wasi_old

8a03a16

Merge remote-tracking branch 'upstream/master' into read-vuln

7b4efed

marmistrz mentioned this pull request Dec 15, 2019

Implement fd_fdstat_set_flags for Windows. #711

Merged

peterhuene requested review from sunfishcode, kubkon and peterhuene December 16, 2019 15:17

peterhuene added the wasi:impl Issues pertaining to WASI implementation in Wasmtime label Dec 16, 2019

peterhuene reviewed Dec 16, 2019

View reviewed changes

marmistrz mentioned this pull request Jan 7, 2020

Fix rights checks across the codebase. #770

Merged

sunfishcode closed this in #770 Jan 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a test and a hotfix for path_open read rights #618

Add a test and a hotfix for path_open read rights #618

marmistrz commented Nov 21, 2019 •

edited

Loading

marmistrz commented Nov 25, 2019

marmistrz commented Nov 28, 2019

marmistrz commented Dec 15, 2019

peterhuene left a comment

peterhuene Dec 16, 2019

marmistrz Dec 16, 2019

peterhuene Dec 16, 2019

kubkon Dec 16, 2019

peterhuene Dec 16, 2019

peterhuene Dec 16, 2019

peterhuene Dec 16, 2019

kubkon Dec 16, 2019

marmistrz Dec 16, 2019

sunfishcode commented Dec 16, 2019

marmistrz commented Dec 17, 2019

sunfishcode commented Dec 20, 2019

marmistrz commented Dec 22, 2019

		@@ -0,0 +1,96 @@
		use std::{env, process};
		use wasi_old::wasi_unstable;

Add a test and a hotfix for path_open read rights #618

Add a test and a hotfix for path_open read rights #618

Conversation

marmistrz commented Nov 21, 2019 • edited Loading

marmistrz commented Nov 25, 2019

marmistrz commented Nov 28, 2019

marmistrz commented Dec 15, 2019

peterhuene left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunfishcode commented Dec 16, 2019

marmistrz commented Dec 17, 2019

sunfishcode commented Dec 20, 2019

marmistrz commented Dec 22, 2019

marmistrz commented Nov 21, 2019 •

edited

Loading