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

Use GRND_INSECURE instead of /dev/urandom when possible #95824

Merged
merged 2 commits into from May 21, 2022

Conversation

zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Apr 8, 2022

From reading the source code, it appears like the desired semantic of
std::unix::rand is to always provide some bytes and never block. For
that reason GRND_NONBLOCK is checked before calling getrandom(0), so
that getrandom(0) won't block. If it would block, then the function
falls back to using /dev/urandom, which for the time being doesn't
block. There are some drawbacks to using /dev/urandom, however, and so
getrandom(GRND_INSECURE) was created as a replacement for this exact
circumstance.

getrandom(GRND_INSECURE) is the same as /dev/urandom, except:

  • It won't leave a warning in dmesg if used at early boot time, which is
    a common occurance (and the reason why I found this issue);

  • It won't introduce a tiny delay at early boot on newer kernels when
    /dev/urandom tries to opportunistically create jitter entropy;

  • It only requires 1 syscall, rather than 3.

Other than that, it returns the same "quality" of randomness as
/dev/urandom, and never blocks.

It's only available on kernels ≥5.6, so we try to use it, cache the
result of that attempt, and fall back to to the previous code if it
didn't work.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 8, 2022
@rust-log-analyzer

This comment has been minimized.

@zx2c4 zx2c4 force-pushed the grnd_insecure branch 4 times, most recently from f38e3e9 to 2495bb7 Compare April 8, 2022 22:11
@frewsxcv frewsxcv added T-libs Relevant to the library team, which will review and decide on the PR/issue. A-security Area: Security related issues (example: address space layout randomization) O-unix Operating system: Unix-like labels Apr 9, 2022
library/std/src/sys/unix/rand.rs Outdated Show resolved Hide resolved
library/std/src/sys/unix/rand.rs Outdated Show resolved Hide resolved
library/std/src/sys/unix/rand.rs Outdated Show resolved Hide resolved
@zx2c4
Copy link
Contributor Author

zx2c4 commented Apr 9, 2022

@faptc thanks for the review. Just committed your suggested changes.

Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

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

This looks fine to me, pending update of the constant.

library/std/src/sys/unix/rand.rs Outdated Show resolved Hide resolved
@thomcc
Copy link
Member

thomcc commented May 20, 2022

r? @thomcc

@rust-highfive rust-highfive assigned thomcc and unassigned kennytm May 20, 2022
@thomcc
Copy link
Member

thomcc commented May 20, 2022

A comment explaining (or linking to docs for) GRND_INSECURE wouldn't be out of place either, since it looks surprising.

@zx2c4
Copy link
Contributor Author

zx2c4 commented May 20, 2022

Alright, constant updated, comment added. Should be good to go.

@thomcc
Copy link
Member

thomcc commented May 20, 2022

@bors r+

@bors
Copy link
Contributor

bors commented May 20, 2022

📌 Commit 309025c57e0c70a51cf98ad381c614dc5dc975cb has been approved by thomcc

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 20, 2022
@thomcc
Copy link
Member

thomcc commented May 20, 2022

Ah, hm, is the constant not available yet?

@zx2c4
Copy link
Contributor Author

zx2c4 commented May 20, 2022

What to make of the failure?

error[E0425]: cannot find value `GRND_INSECURE` in crate `libc`
  --> library/std/src/sys/unix/rand.rs:51:84
   |
51 |             let ret = unsafe { getrandom(buf.as_mut_ptr().cast(), buf.len(), libc::GRND_INSECURE) };
   |                                                                                    ^^^^^^^^^^^^^ not found in `libc`

Looks like it's actually not available?

@thomcc
Copy link
Member

thomcc commented May 20, 2022

https://github.com/rust-lang/rust/blob/master/library/std/Cargo.toml#L18 If you bump this to 0.2.126, it should be fine. Please do it in a separate commit.

@zx2c4
Copy link
Contributor Author

zx2c4 commented May 20, 2022

Let's see if that does the trick.

@zx2c4
Copy link
Contributor Author

zx2c4 commented May 20, 2022

Er, need to update the lock file. Futzing with it now...

@thomcc
Copy link
Member

thomcc commented May 20, 2022

Running something like ./x.py check --stage 0 library/std locally should do the trick.

zx2c4 added 2 commits May 21, 2022 00:02
This is required for the next commit, which uses libc::GRND_INSECURE.
From reading the source code, it appears like the desired semantic of
std::unix::rand is to always provide some bytes and never block. For
that reason GRND_NONBLOCK is checked before calling getrandom(0), so
that getrandom(0) won't block. If it would block, then the function
falls back to using /dev/urandom, which for the time being doesn't
block. There are some drawbacks to using /dev/urandom, however, and so
getrandom(GRND_INSECURE) was created as a replacement for this exact
circumstance.

getrandom(GRND_INSECURE) is the same as /dev/urandom, except:

- It won't leave a warning in dmesg if used at early boot time, which is
  a common occurance (and the reason why I found this issue);

- It won't introduce a tiny delay at early boot on newer kernels when
  /dev/urandom tries to opportunistically create jitter entropy;

- It only requires 1 syscall, rather than 3.

Other than that, it returns the same "quality" of randomness as
/dev/urandom, and never blocks.

It's only available on kernels ≥5.6, so we try to use it, cache the
result of that attempt, and fall back to to the previous code if it
didn't work.
@zx2c4
Copy link
Contributor Author

zx2c4 commented May 20, 2022

Thanks for the tip. All set now.

@thomcc
Copy link
Member

thomcc commented May 20, 2022

@bors r+

@bors
Copy link
Contributor

bors commented May 20, 2022

📌 Commit 18a9d58 has been approved by thomcc

@bors
Copy link
Contributor

bors commented May 20, 2022

⌛ Testing commit 18a9d58 with merge e57884b...

@bors
Copy link
Contributor

bors commented May 21, 2022

☀️ Test successful - checks-actions
Approved by: thomcc
Pushing e57884b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 21, 2022
@bors bors merged commit e57884b into rust-lang:master May 21, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 21, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e57884b): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 0 2 0 3 0
mean2 N/A 1.7% N/A -1.4% N/A
max N/A 2.2% N/A -1.6% N/A

Cycles

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 1 2 0 1 1
mean2 3.2% 2.7% N/A -1.5% 3.2%
max 3.2% 3.2% N/A -1.5% 3.2%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes 2

  2. the arithmetic mean of the percent change 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-security Area: Security related issues (example: address space layout randomization) merged-by-bors This PR was explicitly merged by bors. O-unix Operating system: Unix-like S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet