Firefox crashes on Windows because BCryptGenRandom fails

[msmania]

Hi, I'm an engineer working for Mozilla on Firefox. We recently noticed we'd been getting crash reports indicating Firefox crashes during env_logger initialization. Upon debugging, we found out it was caused by a failure of BCryptGenRandom, that was introduced by #84096.

Almost all reports are from Windows 7 32-bit. We don't know the reason why BCryptGenRandom failed. Possibly the cryptography settings in the registry was corrupted and the process couldn't find the module.

To mitigate this crash, can you add a fallback logic, using RtlGenRandom if BCryptGenRandom fails for whatever reason. Probably we can limit it to Win7.

Here's the link to the tracking bug in Bugzilla.

https://bugzilla.mozilla.org/show_bug.cgi?id=1754490

[newpavlov]

Are you sure that RtlGenRandom will work in the case of BCryptGenRandom failure? I don't think it's correct to introduce such workaround hacks into std, since it's clearly an OS problem.

IIUC RtlGenRandom is deprecated in favor of BCryptGenRandom and ideally should not be used outside of Windows XP. Relevant discussion can be found in: rust-random/getrandom#65

One potential solution on the Firefox side is to try to call getrandom and check whether it returns an error. If it does, you could display an error window to user. Entropy retrieval in std and getrandom use nearly identical code, so if the latter has failed, then the former will probably fail as well.

Also note that we can not target only Win7 without affecting other Windows versions using only cfg gating, since Windows version is not part of target triplet.

[Milo123459]

Also note that we can not target only Win7 without affecting other Windows versions using only cfg gating, since Windows version is not part of target triplet.

Is there not some manual way we could check inside of the cfg block?

[marti4d]

Hi there! Sorry about the radio silence for a while on this issue -- The OP isn't able to continue working on this, and I've been asked to help resolve it.

I have added PR #96917 that implements the idea of re-introducing RtlGenRandom() as a fallback API in the (very rare) case that BCryptGenRandom() doesn't work on a machine.

Are you sure that RtlGenRandom will work in the case of BCryptGenRandom failure? I don't think it's correct to introduce such workaround hacks into std, since it's clearly an OS problem.

We are very certain of this, as we don't see equivalent crashes in Firefox in our own use of RtlGenRandom, and I don't see any issue reported for Chrome/Edge in their code

We are not convinced that it is an "OS problem", persay... We suspect that a virus scanner or other security software may be interfering with certain machines' ability to load the BCrypt library. We don't even know for certain that this issue only affects Firefox; it could just be that we're the only ones with beefy enough crash reporting to catch it.

I agree with you in principle that it's not great to have to introduce workarounds for things like "a virus scanner is breaking Rust code" into the standard library, but if we assume that Firefox is not the only piece of software that has this issue then where else should the change be made? Even if it is only Firefox, we're left with the options of either "seeing through" the abstraction that std offers and testing with getrandom (only to still crash, ultimately), continuing to crash, or forking std

The PR I have attached should make no difference in the 99.999%+ machines where this is not a problem, but in that small fraction of machines where BCrypt is broken, this only improves the robustness of StdLib.

Also note that we can not target only Win7 without affecting other Windows versions using only cfg gating, since Windows version is not part of target triplet.

As it turns out, the name of this issue is perhaps a bit wrong. This issue is seen on multiple versions of Windows (example crash on Windows 10). My PR is agnostic to this anyway -- if BCryptGenRandom() fails for whatever reason, it will then use RtlGenRandom() for the rest of the runtime.

[ChrisDenton]

Thank you for following up on this issue. As you say, we are reliant on Firefox's data here. I've updated the title to better reflect that not just x86 Windows 7 is affected.