Skip to content

Commit

Permalink
Rollup merge of rust-lang#96917 - marti4d:master, r=ChrisDenton
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Dylan-DPC authored May 18, 2022
2 parents 49048ea + aba3454 commit 927a40b
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 4 deletions.
4 changes: 4 additions & 0 deletions library/std/src/sys/windows/c.rs
Original file line number Diff line number Diff line change
Expand Up @@ -788,6 +788,10 @@ if #[cfg(not(target_vendor = "uwp"))] {

#[link(name = "advapi32")]
extern "system" {
// Forbidden when targeting UWP
#[link_name = "SystemFunction036"]
pub fn RtlGenRandom(RandomBuffer: *mut u8, RandomBufferLength: ULONG) -> BOOLEAN;

// Allowed but unused by UWP
pub fn OpenProcessToken(
ProcessHandle: HANDLE,
Expand Down
74 changes: 70 additions & 4 deletions library/std/src/sys/windows/rand.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,60 @@
use crate::io;
use crate::lazy;
use crate::mem;
use crate::sys::c;

/// The kinds of HashMap RNG that may be available
#[derive(Clone, Copy, Debug, PartialEq)]
enum HashMapRng {
Preferred,
Fallback,
}

pub fn hashmap_random_keys() -> (u64, u64) {
match get_hashmap_rng() {
HashMapRng::Preferred => {
preferred_rng().expect("couldn't generate random bytes with preferred RNG")
}
HashMapRng::Fallback => {
fallback_rng().expect("couldn't generate random bytes with fallback RNG")
}
}
}

/// Returns the HashMap RNG that should be used
///
/// Panics if they are both broken
fn get_hashmap_rng() -> HashMapRng {
// Assume that if the preferred RNG is broken the first time we use it, it likely means
// that: the DLL has failed to load, there is no point to calling it over-and-over again,
// and we should cache the result
static VALUE: lazy::SyncOnceCell<HashMapRng> = lazy::SyncOnceCell::new();
*VALUE.get_or_init(choose_hashmap_rng)
}

/// Test whether we should use the preferred or fallback RNG
///
/// If the preferred RNG is successful, we choose it. Otherwise, if the fallback RNG is successful,
/// we choose that
///
/// Panics if both the preferred and the fallback RNG are both non-functional
fn choose_hashmap_rng() -> HashMapRng {
let preferred_error = match preferred_rng() {
Ok(_) => return HashMapRng::Preferred,
Err(e) => e,
};

match fallback_rng() {
Ok(_) => return HashMapRng::Fallback,
Err(fallback_error) => panic!(
"preferred RNG broken: `{}`, fallback RNG broken: `{}`",
preferred_error, fallback_error
),
}
}

/// Generate random numbers using the preferred RNG function (BCryptGenRandom)
fn preferred_rng() -> Result<(u64, u64), io::Error> {
use crate::ptr;

let mut v = (0, 0);
Expand All @@ -14,8 +66,22 @@ pub fn hashmap_random_keys() -> (u64, u64) {
c::BCRYPT_USE_SYSTEM_PREFERRED_RNG,
)
};
if ret != 0 {
panic!("couldn't generate random bytes: {}", io::Error::last_os_error());
}
return v;

if ret == 0 { Ok(v) } else { Err(io::Error::last_os_error()) }
}

/// Generate random numbers using the fallback RNG function (RtlGenRandom)
#[cfg(not(target_vendor = "uwp"))]
fn fallback_rng() -> Result<(u64, u64), io::Error> {
let mut v = (0, 0);
let ret =
unsafe { c::RtlGenRandom(&mut v as *mut _ as *mut u8, mem::size_of_val(&v) as c::ULONG) };

if ret != 0 { Ok(v) } else { Err(io::Error::last_os_error()) }
}

/// We can't use RtlGenRandom with UWP, so there is no fallback
#[cfg(target_vendor = "uwp")]
fn fallback_rng() -> Result<(u64, u64), io::Error> {
Err(io::const_io_error!(io::ErrorKind::Unsupported, "RtlGenRandom() not supported on UWP"))
}

0 comments on commit 927a40b

Please sign in to comment.