Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support more than 64 rights #63

Closed
AndrewScheidecker opened this issue Jun 30, 2019 · 6 comments
Closed

Support more than 64 rights #63

AndrewScheidecker opened this issue Jun 30, 2019 · 6 comments
Labels
cross-module Issues that span multiple wasi modules

Comments

@AndrewScheidecker
Copy link

Sets of rights (e.g. __WASI_RIGHT_FD_READ) are currently encoded as a 64-bit bitmask:

typedef uint64_t __wasi_rights_t;
#define __WASI_RIGHT_FD_DATASYNC (UINT64_C(0x0000000000000001))
#define __WASI_RIGHT_FD_READ (UINT64_C(0x0000000000000002))
#define __WASI_RIGHT_FD_SEEK (UINT64_C(0x0000000000000004))
// etc..

This limits the API to 64 rights, of which 29 are currently used. It seems likely that WASI will exceed that limit at some point. The simple solution would be to just double or quadruple the width of __wasi_rights_t. Maybe 128 or 256 rights is enough?

A more complex solution would be to make everywhere that deals with rights take a variable length array of 64-bit masks. An API version could define a maximum length, so both sides of the API could use statically sized arrays. As long as the API functions take a dynamic length argument, their signature doesn't need to change if the maximum length increases.

There's also an issue with the current fd_fdstat_set_rights function (or the handle_set_rights of #62): it can only be used to remove rights, but it's easy to accidentally remove rights you don't know about! It should perhaps take a set of rights to remove, instead of a set of rights to keep.

@dumblob
Copy link

dumblob commented Jun 30, 2019

A more complex solution would be to make everywhere that deals with rights take a variable length array of 64-bit masks. An API version could define a maximum length, so both sides of the API could use statically sized arrays. As long as the API functions take a dynamic length argument, their signature doesn't need to change if the maximum length increases.

This sounds better to me for the future.

@programmerjake
Copy link
Contributor

There's also an issue with the current fd_fdstat_set_rights function (or the handle_set_rights of #62): it can only be used to remove rights, but it's easy to accidentally remove rights you don't know about! It should perhaps take a set of rights to remove, instead of a set of rights to keep.

I think it would be better to add an extra argument to fd_fdstat_set_rights that allows selecting keeping or removing the specified rights, because there are cases when you do want only a few rights and want to remove all others (passing fd to other more-sandboxed code, for example).

@vitiral
Copy link

vitiral commented Aug 21, 2019

There is a separate problem I see... why are fd rights and path rights part of the same bitmap? Shouldn't they be separate?

If we can break apart "classes" of rights, would 64 be enough? If we needed more, we could simply add functions (__wasi_fd_fdstat_set_rights for the first bitmap and __wasi_fd2_fdstat_set_rights for the second, with items titled __WASI_RIGHT_FD2_SOMERIGHT for the "second group of fd".

I suppose this could leave a vulnerability where you might be allowing rights through in the second group (__wasi_fd_fdstat_set_rights to clear most items, but did not __wasi_fd2_fdstat_set_rights because you didn't know about the second group in legacy code). So there should probably still be something like a __wasi_fdfull_fdstat_set_rights which takes an array as suggested (and clears unspecified rights).

There's also an issue with the current fd_fdstat_set_rights function (or the handle_set_rights of #62): it can only be used to remove rights, but it's easy to accidentally remove rights you don't know about! It should perhaps take a set of rights to remove, instead of a set of rights to keep.

Retrieving the existing rights and using a bitmask should easily solve this problem though, although it may be more expensive.

@vitiral
Copy link

vitiral commented Aug 21, 2019

There are also similar constants where the argument could be made that they should be arrays for forwards-compatibility guarantees, i.e. __wasi_fdflags_t which is currently only a 16bit integer.

@lukewagner
Copy link
Member

FWIW, if/when WASI export signatures get interface-type'd, one could imagine that bit-vectors could be replaced, without meaningful loss of efficiency, by record types (with named bool fields). The expectation here is the the adapter instructions for creating records would statically declare field names and so the compiler would have all the knowledge and freedom to pack the bools as much as it wanted.

@sunfishcode sunfishcode added the cross-module Issues that span multiple wasi modules label Sep 8, 2019
@sunfishcode
Copy link
Member

In the WASI Preview2 presentation, I proposed removing the rights system altogether, and no one objected. A few notes:

  • new WASI APIs can still define their own "rights"-like systems if they want to; the conversation here is just about the existing filesystem-oriented rights.
  • the filesystem rights currently in wasi_snapshot_preview1 are very limited, and adding more bits wouldn't likely be able to add enough bits to make them truly useful.
  • WASI will be able to support all of the things the rights flags could represent, and much more, using API virtualization.

sunfishcode pushed a commit to sunfishcode/WASI that referenced this issue Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cross-module Issues that span multiple wasi modules
Projects
None yet
Development

No branches or pull requests

6 participants