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

std: implement the random feature #129120

Closed
wants to merge 1 commit into from
Closed

Conversation

joboet
Copy link
Member

@joboet joboet commented Aug 15, 2024

Implements the ACP rust-lang/libs-team#393.

API

This PR contains two minor deviations to the API proposed in the ACP (I can easily undo them if necessary):

  • gen_bytes is renamed to fill_bytes
  • DefaultRng is renamed to DefaultRandomSource

Most names in std do not contain abbreviations, and even the documentation proposed in the ACP describes gen_bytes with "Fill buf with random bytes". DefaultRandomSource mirrors DefaultHasher, both names contain the relevant trait.

Implementation

The implementation in this PR mostly reuses the old hash-map key generation code, but contains a few deviations:

  • The Linux code was amended to poll /dev/random before reading /dev/urandom which is required to ensure that the entropy pool is initialized.
  • The fallback code to /dev/urandom has been removed for FreeBSD and some other UNIX-like OSs, they now all use getentropy instead of getrandom. This function is always available on all of them (FreeBSD 12 is our minimum version, see the module documentation in sys/random/unix.rs for the relevant documentation) and has been included in the lastest version of the POSIX standard, issue 8, so using it whereever possible seems like a good idea, especially since NetBSD even indicates that getrandom might be removed in the future. Falling back is unnecessary as if getentropy fails, /dev/urandom will, too, as FreeBSD does not support retrieving insecure random data.

I'd appreciate help from all platform maintainers to help verify that all of these implementations are correct and use a suitable API.

Discussion about the API

I have some issues with the API myself, but to keep this PR focussed on discussion the actual implementation, I'd like to ask everyone to keep these discussions to the ACP issue.

@rustbot
Copy link
Collaborator

rustbot commented Aug 15, 2024

r? @cuviper

rustbot has assigned @cuviper.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added O-hermit Operating System: Hermit O-SGX Target: SGX O-solid Operating System: SOLID O-unix Operating system: Unix-like O-wasi Operating system: Wasi, Webassembly System Interface O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 15, 2024
library/core/src/random.rs Outdated Show resolved Hide resolved
@joboet
Copy link
Member Author

joboet commented Aug 15, 2024

While implementing, I couldn't help but notice that quite a lot of platforms warn against using their randomness syscalls for non-seed-generating purposes. For instance, the newly introduced POSIX function getentropy specifies that:

"The intended use of this function is to create a seed for other pseudo-random number generators."

My gut instinct is that people will attempt to use DefaultRandomSource for much more than that, meaning we expose platforms to loads of small random data request, which they might not be equipped to deal with. So perhaps we should use things like arc4random_buf instead and clarify that the random numbers are not the highest quality the system can provide (but good enough in practise, arc4random is still cryptographically secure)?

@joshtriplett
Copy link
Member

No objection to renaming to DefaultRandomSource. I'd personally prefer not to rename gen_bytes to fill_bytes though. It's a random number generator, and if people do end up calling it directly I think that name would be more evocative. This isn't a blocker, though, and if others on libs-api want it renamed I won't object.

@the8472
Copy link
Member

the8472 commented Aug 15, 2024

There are edge-cases where anything short of the OS entropy provider or cpu instructions¹ will not be suitable for cryptographic use. If it involves fork() or cloning VM snapshots you end up with reusing RNG state and thus potential key/nonce reuse which can be catastrophic for some cryptographic schemes. A libc implementation can guard against forking, but not against VM clones.

¹ and those have had bugs, like returning all-0 "entropy".
random_number

use things like arc4random_buf instead and clarify that the random numbers are not the highest quality the system can provide (but good enough in practise, arc4random is still cryptographically secure)?

Old implementations that actually use RC4 are broken. Newer ones that switch to modern stream ciphers should be secure except for the reuse issue.

@clarfonthey
Copy link
Contributor

You mention discussing on the ACP, but the ACP has been approved and is closed. You should file a tracking issue and link that on the features you implement so we can better track design concerns there.

@joboet
Copy link
Member Author

joboet commented Aug 18, 2024

I'd personally prefer not to rename gen_bytes to fill_bytes though. It's a random number generator, and if people do end up calling it directly I think that name would be more evocative.

How about just generate, then? It's short, clear and not an abbreviation. Either way, I don't really mind.

@cuviper
Copy link
Member

cuviper commented Aug 18, 2024

Given the continued discussion on the ACP(s), I think this should have an API reviewer.

r? libs-api

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 18, 2024
@rustbot rustbot assigned BurntSushi and unassigned cuviper Aug 18, 2024
@bors
Copy link
Contributor

bors commented Aug 22, 2024

☔ The latest upstream changes (presumably #129398) made this pull request unmergeable. Please resolve the merge conflicts.

Comment on lines 69 to 65
pub(crate) struct BestEffortRandomSource;

impl RandomSource for BestEffortRandomSource {
fn fill_bytes(&mut self, bytes: &mut [u8]) {
sys::fill_bytes(bytes, true)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this type only ever be used for creating hash-map seeds?

As noted in #129402, a future wasip2 implementation might use wasi:random/insecure for best effort randomness and wasi:random/random for the default random source. However, wasip2 also has the specific wasi:random/insecure-seed interface which seems an even closer fit for generating hash map keys.

If this type would only ever be used for initialising hash map random keys, the wasip2 implementation could just use wasi:random/insecure-seed. However, if this type became more widely used inside std (or even exposed), it might need to fall back to wasi:random/insecure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, I'll just reintroduce hashmap_random_seeds.

@joboet
Copy link
Member Author

joboet commented Aug 22, 2024

Rebased to reintroduce hashmap_random_keys, fix a Linux poll bug (a timeout of zero represents immediate timeout, not infinite as I though) and use allocation addresses for weak random data on the unsupported platforms.

@bors
Copy link
Contributor

bors commented Aug 27, 2024

☔ The latest upstream changes (presumably #128134) made this pull request unmergeable. Please resolve the merge conflicts.

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 23, 2024
…shtriplett

std: implement the `random` feature (alternative version)

Implements the ACP rust-lang/libs-team#393.

This PR is an alternative version of rust-lang#129120 that replaces `getentropy` with `CCRandomGenerateBytes` (on macOS) and `arc4random_buf` (other BSDs), since that function is not suited for generating large amounts of data and should only be used to seed other CPRNGs. `CCRandomGenerateBytes`/`arc4random_buf` on the other hand is (on modern platforms) just as secure and uses its own, very strong CPRNG (ChaCha20 on the BSDs, AES on macOS) periodically seeded with `getentropy`.
@joboet
Copy link
Member Author

joboet commented Sep 23, 2024

Closing in favour of #129201.

@joboet joboet closed this Sep 23, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 23, 2024
…triplett

std: implement the `random` feature (alternative version)

Implements the ACP rust-lang/libs-team#393.

This PR is an alternative version of rust-lang#129120 that replaces `getentropy` with `CCRandomGenerateBytes` (on macOS) and `arc4random_buf` (other BSDs), since that function is not suited for generating large amounts of data and should only be used to seed other CPRNGs. `CCRandomGenerateBytes`/`arc4random_buf` on the other hand is (on modern platforms) just as secure and uses its own, very strong CPRNG (ChaCha20 on the BSDs, AES on macOS) periodically seeded with `getentropy`.
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 23, 2024
…shtriplett

std: implement the `random` feature (alternative version)

Implements the ACP rust-lang/libs-team#393.

This PR is an alternative version of rust-lang#129120 that replaces `getentropy` with `CCRandomGenerateBytes` (on macOS) and `arc4random_buf` (other BSDs), since that function is not suited for generating large amounts of data and should only be used to seed other CPRNGs. `CCRandomGenerateBytes`/`arc4random_buf` on the other hand is (on modern platforms) just as secure and uses its own, very strong CPRNG (ChaCha20 on the BSDs, AES on macOS) periodically seeded with `getentropy`.
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 23, 2024
…shtriplett

std: implement the `random` feature (alternative version)

Implements the ACP rust-lang/libs-team#393.

This PR is an alternative version of rust-lang#129120 that replaces `getentropy` with `CCRandomGenerateBytes` (on macOS) and `arc4random_buf` (other BSDs), since that function is not suited for generating large amounts of data and should only be used to seed other CPRNGs. `CCRandomGenerateBytes`/`arc4random_buf` on the other hand is (on modern platforms) just as secure and uses its own, very strong CPRNG (ChaCha20 on the BSDs, AES on macOS) periodically seeded with `getentropy`.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 23, 2024
Rollup merge of rust-lang#129201 - joboet:random_faster_sources, r=joshtriplett

std: implement the `random` feature (alternative version)

Implements the ACP rust-lang/libs-team#393.

This PR is an alternative version of rust-lang#129120 that replaces `getentropy` with `CCRandomGenerateBytes` (on macOS) and `arc4random_buf` (other BSDs), since that function is not suited for generating large amounts of data and should only be used to seed other CPRNGs. `CCRandomGenerateBytes`/`arc4random_buf` on the other hand is (on modern platforms) just as secure and uses its own, very strong CPRNG (ChaCha20 on the BSDs, AES on macOS) periodically seeded with `getentropy`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-hermit Operating System: Hermit O-SGX Target: SGX O-solid Operating System: SOLID O-unix Operating system: Unix-like O-wasi Operating system: Wasi, Webassembly System Interface O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.