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

Document how the fallback to File I/O works regarding file handle usage and sandboxing #227

Closed
briansmith opened this issue Sep 30, 2021 · 6 comments

Comments

@briansmith
Copy link
Contributor

The ring documentation says:

When /dev/urandom is used, a file handle for /dev/urandom won't be opened until fill is called; [...] The file handle will never be closed, until the operating system closes it at process shutdown. All [uses] will share a single file handle. To properly implement seccomp filtering [...] allow getrandom through. [For the fallback to reading from /dev/[u]random to work), allow file opening, getrandom, and read up until the first call to fill() succeeds; after that, allow getrandom and read.

The intent of that documentation is to:

  • Tell the user which (approximately) syscalls need to be allowed through the syscall filter for things to work.
  • Tell the user that before they can block file opening syscalls, they need to force the file to be opened first, then block opening syscalls, if they want the fallback to File I/O to work.
  • Reassure the user that once initialized, the RNG will not fail b/c of too many open file handles.
  • Reassure the user that the RNG will not contributing to excessive file handle use; at most one file handle will ever be used.
  • Warn users that if they "close all file handles," as is sometimes suggested as a hardening measure, then they will get undefined and very bad behavior from the RNG.

All of this could be clearer in the ring documentation, so I wouldn't suggest copying it. However, I do think it would be useful for guarantees to be made in the getrandom documentation or for the documentation to call out that no such guarantees are made. In particular, how should one use getrandom in conjunction with sandboxing and what can we expect w.r.t. file descriptor usage?

@briansmith
Copy link
Contributor Author

Also, I believe ring and getrandom make the same trade-off: To get the benefits described, we run the risk of very bad results if the file descriptor is closed. Arguably it may be better to have slower performance for ancient Linux systems and run the risk of more often failing due to open file descriptor limits to get rid of the problem described by the last bullet point. I believe the python implementation of this functionality does that, or at least fstats the file descriptor to make sure it is still open and still a special device file handle, which isn't perfect, but which is safer than what we do.

@briansmith
Copy link
Contributor Author

Maybe this should be a separate issue, but I find it strange that getrandom syscall returning EPERM results in falling back to File I/O. It seems like a sandbox is badly misconfigured if it blocks getrandom but allows file I/O to /dev/[u]random. (The EPERM condition was added in response to rust-lang/rust#52609.)

@briansmith
Copy link
Contributor Author

Regarding sandboxing, BoringSSL has similar documentation to what I'm requesting: https://github.com/google/boringssl/blob/master/SANDBOXING.md

@briansmith
Copy link
Contributor Author

Besides File I/O, see #65 (comment) regarding Windows:

The main downside is that BCryptGenRandom (on its very first call with BCRYPT_USE_SYSTEM_PREFERRED_RNG) might read some preferences from the Windows registry, which is not allowed inside the Chromium sandbox.

Thus we need to provide documentation on how to deal with this.

@newpavlov
Copy link
Member

Regarding sandboxing, BoringSSL has similar documentation to what I'm requesting

Even this document does not go to the level of detail you requested in the OP. It does not contains much information relevant in our case. Almost everything is in the "entropy" section. We more or less cover this information in the "supported targets" table.

Could you open a PR with docs to resolve this issue? Adding a "sandbox support" section could be worthwhile, but I don't think it should be bigger than 2-3 paragraphs.

@newpavlov
Copy link
Member

Closing this issue since it's not clear how to proceed with it. We are open to documentation improvements, so it may be better to just open a PR for it. We can discuss finer details in its discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants