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

Revise "not a crypto library" policy and SECURITY.md #1565

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Jan 28, 2025

Attempt to slightly improve the wording left by #1514.

See also Reddit discussion.

@benjamin-lieser
Copy link
Member

Users are expected to determine for themselves
whether rand's functionality meets their own security requirements.

I really like this, because it describes very well what rand is. It certainly can be used for crypto, but it requires the user to read the doc carefully and decide which part of rand to use.

rand_core/src/lib.rs Outdated Show resolved Hide resolved
@dhardy dhardy changed the title Reword "not a crypto library" policy, note on SeedableRng Reword "not a crypto library" policy Jan 29, 2025
@dhardy
Copy link
Member Author

dhardy commented Jan 30, 2025

@newpavlov I updated the section on WASM. BTW the WASI & Emscripten links in the getrandom docs both appear to be outdated.

@dhardy dhardy changed the title Reword "not a crypto library" policy Revise "not a crypto library" policy and SECURITY.md Jan 30, 2025
@dhardy
Copy link
Member Author

dhardy commented Jan 30, 2025

More significant revisions of SECURITY.md included (some content copied from getrandom).

Also CC @josephlr

Also CC @dcmiddle, @vks and @tarcieri who commented on the previous PR.

@newpavlov
Copy link
Member

BTW the WASI & Emscripten links in the getrandom docs both appear to be outdated.

Update PR: rust-random/getrandom#597

Comment on lines +16 to +20
- If the RNG implements `Default`, it may be default-constructed
- If the RNG implements `SeedableRng`, it may be constructed and seeded using
`SeedableRng::from_seed` with a cryptographically secure seed value
- If the RNG implements `SeedableRng`, it may be constructed and seeded from
another RNG which is itself cryptographically secure
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I reading this correctly ...
Default means it will be seeded securely and automatically without any work by the caller.
SeedableRng relies on the caller providing a seed that is cryptographically secure from some other source, possibly from another RNG.

If so maybe this is more clear to have one SeedableRng bullet...

Suggested change
- If the RNG implements `Default`, it may be default-constructed
- If the RNG implements `SeedableRng`, it may be constructed and seeded using
`SeedableRng::from_seed` with a cryptographically secure seed value
- If the RNG implements `SeedableRng`, it may be constructed and seeded from
another RNG which is itself cryptographically secure
- If the RNG implements `Default`, it may be default-constructed
- If the RNG implements `SeedableRng`, it may be constructed and seeded using
`SeedableRng::from_seed` with a cryptographically secure seed value, possibly
from another RNG which is itself cryptographically secure

Copy link
Member Author

Choose a reason for hiding this comment

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

OsRng and ThreadRng can be default constructed and be secure.

Another way to read this is that a CryptoRng should not support Default if the result would be insecure.

I admit that there is potential for confusion here. Perhaps I should add to the first point:

Note that generators should only implement Default where a default-constructed instance is no more predictable than a securely seeded instance; for example OsRng (which is a stateless) supports Default construction.

@dhardy
Copy link
Member Author

dhardy commented Jan 31, 2025

Perhaps I should also add doc to the *CryptoRng traits on Default.

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

Successfully merging this pull request may close these issues.

5 participants