Repository navigation
Windows: should we fall back to RtlGenRandom if BCryptGenRandom fails #314
Description
Activity
Unfortunately, it does not look like anyone has uncovered root of the issue. So the most practical choice (though I personally dislike it) will be to follow
stdand introduce theBCryptGenRandomfallback, but with the clear indication that it's a hack for the OS issue.- The Rust stdlib uses the RNG just for the HashMap anti-DoS mechanism, right? That requires a lower level of security than what other uses of
getrandomrequire. - I think
RtlGenRandomisn't supported on ARM and the problems Firefox reports withBCryptGenRandomprobably don't apply to ARM Windows machines. So perhaps only do the fallback on x86/x86_64. - https://bugs.chromium.org/p/boringssl/issues/detail?id=307 has more discussion, including some good notes.
- Chromium also uses
RtlGenRandom, according to https://source.chromium.org/search?q=RtlGenRandom&sq=.
- The Rust stdlib uses the RNG just for the HashMap anti-DoS mechanism, right? That requires a lower level of security than what other uses of
The Rust stdlib uses the RNG just for the HashMap anti-DoS mechanism, right? That requires a lower level of security than what other uses of getrandom require.
AFAIK there are no security issues with
RtlGenRandom. The reason why we avoid it is because it was deprecated by Microsoft and not present on newer targets (such as UWP).The Rust stdlib uses the RNG just for the HashMap anti-DoS mechanism, right? That requires a lower level of security than what other uses of getrandom require.
AFAIK there are no security issues with
RtlGenRandom. The reason why we avoid it is because it was deprecated by Microsoft and not present on newer targets (such as UWP and ARM).Given
RtlGenRandomis still used in security critical software (Chrome, Edge, etc...) I'm not concerned with the security implications of falling back to this method. I think the main point here is about usability and stability. Given this, I think our default should be to:- Try
BCryptGenRandom - Fallback to
RtlGenRandom(only on non-UMP) - If both fail, return the error from
BCryptGenRandomas that actually has error information - Our
LazyBoolsynchronization should avoid any of the synchronization problems found in libstd (std::collections::hash::map::newstuck on Windows 7 rust-lang/rust#99341).
- Try
- I think
RtlGenRandomisn't supported on ARM and the problems Firefox reports withBCryptGenRandomprobably don't apply to ARM Windows machines. So perhaps only do the fallback on x86/x86_64.
Looking at various docs/implementations, it seems like
CryptGenRandomis only available on x86 (also see this). However, there doesn't seem to be any such restriction forRtlGenRandomthat I could find, and Chrome/BoringSSL seem to use it unconditional of architecture.Reacted by Artyom Pavlov- I think
Just some notes from std:
- The vast majority of issues were with 32bit Windows 7 but there were a few crashes with x64 and newer versions too. Though maybe too few to have been on our radar alone.
- The root cause has not be confirmed. One theory is it's caused by a somehow broken system configuration (e.g. corrupt registry or some other problem reading from it).
- I believe (but can't confirm) that on Windows 10+ using
BCryptGenRandomwithBCRYPT_RNG_ALG_HANDLEwould be enough to fix the issue because it avoids the need to load the system configuration. However this isn't supported on earlier versions.
An alternative to using a
RtlGenRandomfallback would be to manually open an algorithm handle. The downside is the added complexity.Thanks for the explainer @ChrisDenton. For those not aware, Rust libstd changed the implementation recently to a BCrypt-Only solution: rust-lang/rust#101325 rust-lang/rust#101476 rust-lang/rust#102044
It also seems like the libstd folks also ran into the crash I noted in #318 when using
BCRYPT_RNG_ALG_HANDLE. This is sad, because usingBCRYPT_RNG_ALG_HANDLEas a first attempt would be very clean, but there doesn't appear to be a good way to test for it's support (without crashing).Reacted by Chris DentonHi all,
As an FYI, Rust Std had to undo its "BCrypt fallback" change, and is going back to
RtlGenRandom. See the Rust-lang issue and the incoming PR from @ChrisDenton.As I said in the discussion on there -- There's a good chance this isn't a Firefox-specific issue; it's likely that any Rust program that uses
HashMapor other RNG-reliant things will crash on these machines too. I think it makes a lot of sense forgetrandomto do the same thing.Thanks!
Reacted by Chris Denton and Joe RicheyNote that
RtlGenRandom(wellSystemFunction036inadvapi32.dll) is a wrapper aroundProcessPrnginBCryptPrimitives.dll(see also RNG Whitepaper).So I don't think the problem is (directly) related to loading
BCryptPrimitives.dlland it should in theory be possible to useProcessPrngin place ofRtlGenRandom. Though one difference is thatadvapi32.dlluses the delay load mechanism to load crypt dlls which may make a difference.- added 2 commits that reference this issue
on Feb 21, 2023 Hello,
Note that
RtlGenRandom(wellSystemFunction036inadvapi32.dll) is a wrapper aroundProcessPrnginBCryptPrimitives.dll(see also RNG Whitepaper).RtlGenRandomisSystemFunction036fromadvapi32.dll, which is a forwarder forSystemFunction036fromcryptbase.dll. The architecture you mention holds for recent versions of Windows, e.g. on my Windows 11 machine it is true thatSystemFunction036will useProcessPrngfrombcryptprimitives.dll. But this is the result of a posteriori code refactoring, it doesn't hold for Windows 7, which is the version we see in the Firefox crashes, where the two APIs are independent. You can download the most up-to-date Windows 7 version ofcryptbase.dll(6.1.7601.24499) here and check that it doesn't import anything frombcryptPrimitives.dll.# Windows 11 > dumpbin /imports C:\Windows\System32\cryptbase.dll ... Dump of file C:\Windows\System32\cryptbase.dll ... bcryptPrimitives.dll ... 0000000180002946 8 ProcessPrng # Windows 11 > dumpbin /imports C:\Windows\System32\cryptbase.dll | Select-String "\.dll" Dump of file C:\Windows\System32\cryptbase.dll ntdll.dll api-ms-win-core-errorhandling-l1-1-0.dll api-ms-win-core-libraryloader-l1-2-0.dll api-ms-win-core-processthreads-l1-1-0.dll api-ms-win-core-profile-l1-1-0.dll api-ms-win-core-sysinfo-l1-1-0.dll api-ms-win-core-delayload-l1-1-1.dll api-ms-win-core-delayload-l1-1-0.dll RPCRT4.dll bcryptPrimitives.dll # Windows 7 (6.1.7601.24499) > dumpbin /imports C:\Users\user\Downloads\cryptbase.dll | Select-String "\.dll" Dump of file C:\Users\user\Downloads\cryptbase.dll ntdll.dll API-MS-Win-Core-ErrorHandling-L1-1-0.dll API-MS-Win-Core-Handle-L1-1-0.dll API-MS-Win-Core-Heap-L1-1-0.dll API-MS-Win-Core-Interlocked-L1-1-0.dll API-MS-Win-Core-IO-L1-1-0.dll API-MS-Win-Core-LibraryLoader-L1-1-0.dll API-MS-Win-Core-ProcessThreads-L1-1-0.dll API-MS-Win-Core-Profile-L1-1-0.dll API-MS-Win-Core-Synch-L1-1-0.dll API-MS-Win-Core-SysInfo-L1-1-0.dll API-MS-Win-Core-DelayLoad-L1-1-0.dll RPCRT4.dllSo I don't think the problem is (directly) related to loading
BCryptPrimitives.dlland it should in theory be possible to useProcessPrngin place ofRtlGenRandom. Though one difference is thatadvapi32.dlluses the delay load mechanism to load crypt dlls which may make a difference.I think that if we move to
ProcessPrng, we will still have crashes on the Windows 7 machines that are unable to loadbcryptprimitives.dll.RtlGenRandomworks for them because on Windows 7, there is no link betweenRtlGenRandomandbcryptprimitives.dll.- added a commit that references this issue
on Feb 22, 2023 As an update on this, #337 went with the approach of:
- Always try
BCryptGenRandom - If it fails and we aren't on UWP, try
RtlGenRandom
Given all the headache we've had w/ Windows, we aren't going to use a sparsely documented primitive function like
ProcessPrng.- Always try
Update: BoringSSL has switched to ProcessPrng. google/boringssl@e79649b
In #65 we decided to use
BCryptGenRandominstead ofRtlGenRandom. I still think this is the correct decision.However, Rust has seen issues (rust-lang/rust#94098 and rust-lang/rust#99341) around such use. To fix these issues they decided to use
RtlGenRandomagain, but only as a fallback mechanism (rust-lang/rust#96917).Should we do the same?