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

Is unsafe the best way to indicate ambient authority? #141

Closed
sunfishcode opened this issue Jan 22, 2021 · 5 comments · Fixed by #166
Closed

Is unsafe the best way to indicate ambient authority? #141

sunfishcode opened this issue Jan 22, 2021 · 5 comments · Fixed by #166
Labels
help wanted Extra attention is needed question Further information is requested

Comments

@sunfishcode
Copy link
Member

cap-std crates currently use unsafe to indicate function which utilize ambient authorities, for example Dir::open_ambient_dir. As discussed here, this follows the precedent of Rust's standard library in making File::from_raw_fd be unsafe.

However, when porting code to cap-std, such as this Web server example and looking at it from the perspective of a user of the API incrementally adopting it, the use of unsafe has felt somewhat uncomfortable. So, what's the best approach here?

Some possible options include:

  • Write a linting system. See also this issue. This is a fair amount of work, and would be great for users that want to scan large codebases for ambient authorities, however it'd be less convenient for incremental users, such as the Web server example mentioned above.
  • Put unsafe functions in a separate module, or even a separate crate.
  • Continue to use unsafe.
@sunfishcode sunfishcode added help wanted Extra attention is needed question Further information is requested labels Jan 22, 2021
@sunfishcode
Copy link
Member Author

Another option suggested here: introduce an UNSANDBOXED type that would need to be passed into such functions, which could only be constructed by a specific function that could easily be searched for or detected with clippy.

@vi
Copy link

vi commented Apr 14, 2021

For filesystem access specifically it can be argued that cap-std strives to bring OS interaction into scope of Rust's memory safety rules and can really handle the /proc/self/mem problem that can represent OS-assisted unsafety.

It should not be used just to warn about possible security logic violation that cannot be "upgraded" to memory safety errors, but I expect that many subsystems can ultimately lead to OS-assisted memory unsafety. For example, unrestricted starting of child processes can lead to a debugger debugging the parent process.

@sunfishcode
Copy link
Member Author

sunfishcode commented Apr 14, 2021

I've started to experiment with the type idea, and have created a crate named ambient-authority which defines an AmbientAuthority type that could be used by crates which wish to incorporate this concept. The repo includes a clippy.toml which can be used along with #![deny(clippy::disallowed_method)] as a rough way to statically detect any uses of ambient authority, either those that explicitly opt in with AmbientAuthority or a list of functions in std and a few other crates known to do so.

A next step here might be to rewrite the pub unsafe functions in Wasmtime to have an AmbientAuthority parameter, and to remove the unsafe.

sunfishcode added a commit that referenced this issue Apr 14, 2021
Use the newly-created ambient-authority crate to indicate functions
which have ambient authority, rather than using `unsafe`.

Fixes #141.
sunfishcode added a commit that referenced this issue Apr 14, 2021
Use the newly-created ambient-authority crate to indicate functions
which have ambient authority, rather than using `unsafe`.

Fixes #141.
sunfishcode added a commit that referenced this issue Apr 14, 2021
Use the newly-created ambient-authority crate to indicate functions
which have ambient authority, rather than using `unsafe`.

Fixes #141.
sunfishcode added a commit that referenced this issue Apr 14, 2021
Use the newly-created ambient-authority crate to indicate functions
which have ambient authority, rather than using `unsafe`.

Fixes #141.
sunfishcode added a commit that referenced this issue Apr 14, 2021
Use the newly-created ambient-authority crate to indicate functions
which have ambient authority, rather than using `unsafe`.

Fixes #141.
sunfishcode added a commit that referenced this issue Apr 14, 2021
Use the newly-created ambient-authority crate to indicate functions
which have ambient authority, rather than using `unsafe`.

Fixes #141.
sunfishcode added a commit that referenced this issue Apr 14, 2021
Use the newly-created ambient-authority crate to indicate functions
which have ambient authority, rather than using `unsafe`.

Fixes #141.
@sunfishcode
Copy link
Member Author

#166 is now a PR which implements this. Along with a few other cleanups, it removes the last of the unsafe code from cap-primitives, the crate with all the filesystem sandboxing logic, which is nice. I want to do some experimenting with this from a user perspective first though.

sunfishcode added a commit that referenced this issue May 12, 2021
Use the newly-created ambient-authority crate to indicate functions
which have ambient authority, rather than using `unsafe`.

Fixes #141.
sunfishcode added a commit that referenced this issue May 24, 2021
Use the newly-created ambient-authority crate to indicate functions
which have ambient authority, rather than using `unsafe`.

Fixes #141.
@sunfishcode
Copy link
Member Author

Ok, let's do it! Even if there could theoretically be a concept of "filesystem path safety", it wouldn't work the same way as memory safety or I/O safety, because concepts like ownership and lifetime don't work for filesystem namespaces the way they do for memory address spaces or handle index spaces. So I've convinced myself that even at a theoretical level, there's a natural boundary around unsafe, and ambient filesystem/network/clock safety are outside of it.

sunfishcode added a commit that referenced this issue May 26, 2021
Use the newly-created ambient-authority crate to indicate functions
which have ambient authority, rather than using `unsafe`.

Fixes #141.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed question Further information is requested
Projects
None yet
2 participants