Use BCryptGenRandom instead of RtlGenRandom on Windows. #84096
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
m-ou-se
added
T-libs-api
Xanewok
approved these changes
Apr 17, 2021
dtolnay
approved these changes
Apr 21, 2021
There was a problem hiding this comment.
Looks good to me. rust-random/getrandom#65 (comment) was a helpful summary of the situation and justification for BCryptGenRandom.
Equivalent change removing RtlGenRandom in the getrandom crate after dropping XP support: rust-random/getrandom#177.
|
@bors r+ |
|
📌 Commit b8a9112 has been approved by |
|
🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened. |
bors
added
S-waiting-on-bors
m-ou-se
added a commit
to m-ou-se/rust
that referenced
this pull request
Apr 21, 2021
…olnay Use BCryptGenRandom instead of RtlGenRandom on Windows. This removes usage of RtlGenRandom on Windows, in favour of BCryptGenRandom. BCryptGenRandom isn't available on XP, but we dropped XP support a while ago.
|
@bors r- Failed in #84405: https://github.com/rust-lang-ci/rust/runs/2403918836#step:24:30801 |
bors
added
S-waiting-on-author
I think we just need to add the appropriate bcrypt flags here: rust/src/test/run-make-fulldeps/tools.mk Line 104 in 673d0db |
josephlr
approved these changes
Apr 29, 2021
JohnCSimon
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #85594) made this pull request unmergeable. Please resolve the merge conflicts. |
crlf0710
added
S-waiting-on-author
JohnCSimon
added
S-waiting-on-author
|
Finished benchmarking commit (c102653): comparison url. Summary: This benchmark run did not return any relevant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
This was referenced Dec 13, 2021
gj
added a commit
to osohq/oso
that referenced
this pull request
Dec 13, 2021
Used to be needed when Rust sometimes relied on RtlGenRandom instead of BCryptGenRandom, but that changed as of rust-lang/rust#84096 advapi32 is still used in Rust core, but only in the deprecated `env::home_dir` API, which we don't use: - https://github.com/rust-lang/rust/blob/22f8bde876f2fa9c5c4e95be1bce29cc271f2b51/library/std/src/sys/windows/c.rs#L681-L689 - https://github.com/rust-lang/rust/blob/22f8bde876f2fa9c5c4e95be1bce29cc271f2b51/library/std/src/sys/windows/os.rs#L292 - https://github.com/rust-lang/rust/blob/22f8bde876f2fa9c5c4e95be1bce29cc271f2b51/library/std/src/sys/windows/os.rs#L319 - https://github.com/rust-lang/rust/blob/22f8bde876f2fa9c5c4e95be1bce29cc271f2b51/library/std/src/env.rs#L566-L575
jschwe
added a commit
to jschwe/corrosion
that referenced
this pull request
Dec 18, 2021
std depends on Bcrypt on Windows starting with rust 1.57. See rust-lang/rust#84096. Signed-off-by: Jonathan Schwender <schwenderjonathan@gmail.com>
ogoffart
pushed a commit
to corrosion-rs/corrosion
that referenced
this pull request
Dec 30, 2021
std depends on Bcrypt on Windows starting with rust 1.57. See rust-lang/rust#84096. Signed-off-by: Jonathan Schwender <schwenderjonathan@gmail.com>
This was referenced Jan 6, 2022
Marwes
pushed a commit
to influxdata/pkg-config
that referenced
this pull request
Jan 17, 2022
Due to rust-lang/rust#84096 we need to link to bcrypt on windows else we get linker errors. cc influxdata/flux#4406 https://app.circleci.com/pipelines/github/influxdata/flux/11766/workflows/d1ef0c61-babb-4682-8c74-7087013c873e/jobs/38341 ``` C:\\Users\\circleci\\AppData\\Local\\go-build\\pkgconfig\\windows_amd64\\lib/libflux-3dd4b4346280d5a830d03b58d109be24cf44fd0ab5964e0113b470e7ba1223ed.a(std-c90c2a9f63fc5684.std.04e7289e-cgu.0.rcgu.o): In function `std::sys::windows::rand::hashmap_random_keys': /rustc/02072b482a8b5357f7fb5e5637444ae30e423c40\/library\std\src\sys\windows/rand.rs:10: undefined reference to `BCryptGenRandom' /rustc/02072b482a8b5357f7fb5e5637444ae30e423c40\/library\std\src\sys\windows/rand.rs:10: undefined reference to `BCryptGenRandom' collect2.exe: error: ld returned 1 exit status Exited with code exit status 2 ```
saschanaz
added a commit
to saschanaz/ime-rs
that referenced
this pull request
Jan 23, 2022
Rust 1.57 added the dependency: rust-lang/rust#84096
antaljanosbenjamin
added a commit
to antaljanosbenjamin/rust_projects
that referenced
this pull request
Mar 14, 2022
marti4d
added a commit
to marti4d/rust
that referenced
this pull request
May 10, 2022
Issue rust-lang#84096 changed the hashmap RNG to use BCryptGenRandom instead of RtlGenRandom on Windows. Mozilla Firefox started experiencing random failures in env_logger::Builder::new() (Issue rust-lang#94098) during initialization of their unsandboxed main process with an "Access Denied" error message from BCryptGenRandom(), which is used by the HashMap contained in env_logger::Builder The root cause appears to be a virus scanner or other software interfering with BCrypt DLLs loading. This change adds a fallback option if BCryptGenRandom is unusable for whatever reason. It will fallback to RtlGenRandom in this case. Fixes rust-lang#94098
Dylan-DPC
added a commit
to Dylan-DPC/rust
that referenced
this pull request
May 17, 2022
Make HashMap fall back to RtlGenRandom if BCryptGenRandom fails With PR rust-lang#84096, Rust `std::collections::hash_map::RandomState` changed from using `RtlGenRandom()` ([msdn](https://docs.microsoft.com/en-us/windows/win32/api/ntsecapi/nf-ntsecapi-rtlgenrandom)) to `BCryptGenRandom()` ([msdn](https://docs.microsoft.com/en-us/windows/win32/api/bcrypt/nf-bcrypt-bcryptgenrandom)) as its source of secure randomness after much discussion ([here](rust-random/getrandom#65 (comment)), among other places). Unfortunately, after that PR landed, Mozilla Firefox started experiencing fairly-rare crashes during startup while attempting to initialize the `env_logger` crate. ([docs for env_logger](https://docs.rs/env_logger/latest/env_logger/)) The root issue is that on some machines, `BCryptGenRandom()` will fail with an `Access is denied. (os error 5)` error message. ([Bugzilla issue 1754490](https://bugzilla.mozilla.org/show_bug.cgi?id=1754490)) (Discussion in issue rust-lang#94098) Note that this is happening upon startup of Firefox's unsandboxed Main Process, so this behavior is different and separate from previous issues ([like this](https://bugzilla.mozilla.org/show_bug.cgi?id=1746254)) where BCrypt DLLs were blocked by process sandboxing. In the case of sandboxing, we knew we were doing something abnormal and expected that we'd have to resort to abnormal measures to make it work. However, in this case we are in a regular unsandboxed process just trying to initialize `env_logger` and getting a panic. We suspect that this may be caused by a virus scanner or some other security software blocking the loading of the BCrypt DLLs, but we're not completely sure as we haven't been able to replicate locally. It is also possible that Firefox is not the only software affected by this; we just may be one of the pieces of Rust software that has the telemetry and crash reporting necessary to catch it. I have read some of the historical discussion around using `BCryptGenRandom()` in Rust code, and I respect the decision that was made and agree that it was a good course of action, so I'm not trying to open a discussion about a return to `RtlGenRandom()`. Instead, I'd like to suggest that perhaps we use `RtlGenRandom()` as a "fallback RNG" in the case that BCrypt doesn't work. This pull request implements this fallback behavior. I believe this would improve the robustness of this essential data structure within the standard library, and I see only 2 potential drawbacks: 1. Slight added overhead: It should be quite minimal though. The first call to `sys::rand::hashmap_random_keys()` will incur a bit of initialization overhead, and every call after will incur roughly 2 non-atomic global reads and 2 easily predictable branches. Both should be negligible compared to the actual cost of generating secure random numbers 2. `RtlGenRandom()` is deprecated by Microsoft: Technically true, but as mentioned in [this comment on GoLang](golang/go#33542 (comment)), this API is ubiquitous in Windows software and actually removing it would break lots of things. Also, Firefox uses it already in [our C++ code](https://searchfox.org/mozilla-central/rev/5f88c1d6977e03e22d3420d0cdf8ad0113c2eb31/mfbt/RandomNum.cpp#25), and [Chromium uses it in their code as well](https://source.chromium.org/chromium/chromium/src/+/main:base/rand_util_win.cc) (which transitively means that Microsoft uses it in their own web browser, Edge). If there did come a time when Microsoft truly removes this API, it should be easy enough for Rust to simply remove the fallback in the code I've added here
Dylan-DPC
added a commit
to Dylan-DPC/rust
that referenced
this pull request
May 18, 2022
Make HashMap fall back to RtlGenRandom if BCryptGenRandom fails With PR rust-lang#84096, Rust `std::collections::hash_map::RandomState` changed from using `RtlGenRandom()` ([msdn](https://docs.microsoft.com/en-us/windows/win32/api/ntsecapi/nf-ntsecapi-rtlgenrandom)) to `BCryptGenRandom()` ([msdn](https://docs.microsoft.com/en-us/windows/win32/api/bcrypt/nf-bcrypt-bcryptgenrandom)) as its source of secure randomness after much discussion ([here](rust-random/getrandom#65 (comment)), among other places). Unfortunately, after that PR landed, Mozilla Firefox started experiencing fairly-rare crashes during startup while attempting to initialize the `env_logger` crate. ([docs for env_logger](https://docs.rs/env_logger/latest/env_logger/)) The root issue is that on some machines, `BCryptGenRandom()` will fail with an `Access is denied. (os error 5)` error message. ([Bugzilla issue 1754490](https://bugzilla.mozilla.org/show_bug.cgi?id=1754490)) (Discussion in issue rust-lang#94098) Note that this is happening upon startup of Firefox's unsandboxed Main Process, so this behavior is different and separate from previous issues ([like this](https://bugzilla.mozilla.org/show_bug.cgi?id=1746254)) where BCrypt DLLs were blocked by process sandboxing. In the case of sandboxing, we knew we were doing something abnormal and expected that we'd have to resort to abnormal measures to make it work. However, in this case we are in a regular unsandboxed process just trying to initialize `env_logger` and getting a panic. We suspect that this may be caused by a virus scanner or some other security software blocking the loading of the BCrypt DLLs, but we're not completely sure as we haven't been able to replicate locally. It is also possible that Firefox is not the only software affected by this; we just may be one of the pieces of Rust software that has the telemetry and crash reporting necessary to catch it. I have read some of the historical discussion around using `BCryptGenRandom()` in Rust code, and I respect the decision that was made and agree that it was a good course of action, so I'm not trying to open a discussion about a return to `RtlGenRandom()`. Instead, I'd like to suggest that perhaps we use `RtlGenRandom()` as a "fallback RNG" in the case that BCrypt doesn't work. This pull request implements this fallback behavior. I believe this would improve the robustness of this essential data structure within the standard library, and I see only 2 potential drawbacks: 1. Slight added overhead: It should be quite minimal though. The first call to `sys::rand::hashmap_random_keys()` will incur a bit of initialization overhead, and every call after will incur roughly 2 non-atomic global reads and 2 easily predictable branches. Both should be negligible compared to the actual cost of generating secure random numbers 2. `RtlGenRandom()` is deprecated by Microsoft: Technically true, but as mentioned in [this comment on GoLang](golang/go#33542 (comment)), this API is ubiquitous in Windows software and actually removing it would break lots of things. Also, Firefox uses it already in [our C++ code](https://searchfox.org/mozilla-central/rev/5f88c1d6977e03e22d3420d0cdf8ad0113c2eb31/mfbt/RandomNum.cpp#25), and [Chromium uses it in their code as well](https://source.chromium.org/chromium/chromium/src/+/main:base/rand_util_win.cc) (which transitively means that Microsoft uses it in their own web browser, Edge). If there did come a time when Microsoft truly removes this API, it should be easy enough for Rust to simply remove the fallback in the code I've added here
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes usage of RtlGenRandom on Windows, in favour of BCryptGenRandom.
BCryptGenRandom isn't available on XP, but we dropped XP support a while ago.