Fix rights checks across the codebase. #770
Conversation
peterhuene
added
wasi:impl
There was a problem hiding this comment.
LGTM! Just a couple of minor nits.
Do you guys think it would useful to add an assertion in each testcase that the (main) tested syscall fails without the required rights as expected? It's a loose thought that I don't intend we address in this PR, but perhaps we could in a subsequent PR.
| }; | ||
| // Since we no longer have the right to fd_read, trying to read a file | ||
| // should be an error. | ||
| wasi::fd_read(fd, &[iovec]).expect_err("reading bytes from file should fail"); |
There was a problem hiding this comment.
Could you also test for the returned error value here as well?
There was a problem hiding this comment.
You mean that I should assert that the error code is precisely ENOTCAPABLE?
| assert!( | ||
| wasi::fd_fdstat_set_rights(fd, new_base, new_inheriting).is_ok(), | ||
| "dropping fd rights", | ||
| ); | ||
| } |
There was a problem hiding this comment.
You could probably make it a little clearer by using an expect rather than assert on Ok :-)
There was a problem hiding this comment.
Yes, but then there will be no line info displayed in case of a panic. (I know that I'm using expect in other places but it's worth discussing)
There was a problem hiding this comment.
Apropos of this: rust-lang/rust#67887 just landed, so this issue should go away once that makes its way to stable rust :-).
crates/wasi-common/src/fdentry.rs
Outdated
| @@ -157,6 +157,10 @@ impl FdEntry { | |||
| ) -> Result<()> { | |||
| if !self.rights_base & rights_base != 0 || !self.rights_inheriting & rights_inheriting != 0 | |||
| { | |||
| log::trace!( | |||
| " | validate_rights failed: required: rights_base = {}, rights_inheriting = {}; actual: rights_base = {}, rights_inheriting = {}", | |||
There was a problem hiding this comment.
Can you log the rights as hex (#:x)? Also, would it make sense to log those rights which were required but not present, so that it's easy to see at a glance why a capability check failed?
marmistrz
force-pushed
the
read-vuln2
branch
5 times, most recently
from
c44a861 to
9a2940b
Compare
* 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.
|
Great, thanks! |
See the commit messages for a detailed description.
Obsoletes and closes #618. It was easier to manually port the commit than to fight merge conflicts and I didn't want to invalidate the discussions in that PR.
cc @kubkon @sunfishcode @peterhuene