Windows: RtlGenRandom vs BCryptGenRandom

@chouquette

IIUC both functions use the same algorithm under the hood (also see this). The main difference is that BCryptGenRandom will not work on Windows XP, but otherwise it looks like BCryptGenRandom should be preferred. IIRC Rust explicitly does not support Windows XP anymore, so it should not matter.

@chouquette
Can you help here? In rust-lang/rust#60260 you have used BCryptGenRandom for UWP targets and left RtlGenRandom for non-UWP targets. Is there a reason for that?

cc @GabrielMajeri

Hi,

Mostly because I was asked to :) rust-lang/rust#60260 (comment)

My understanding is that Microsoft advises to use CryptGenRandom over RtlGenRandom, but CryptGenRandom is deprecated and the "next generation cryptography API" should be used instead, and BCryptGenRandom is part of that API, so if you have the choice, BcryptGenRandom is probably to be favored, however it seems in the context of Rust, there are some historical reasons for keeping RtlGenRandom

In the case of UWP the choice is easy, you can't use RtlGenRandom

I believe XP is still technically supported by std (https://forge.rust-lang.org/platform-support.html#tier-3) and that's why it needs both implementations. When XP support is officially dropped then the RtlGenRandom version can be removed and BCryptGenRandom can be used for all Windows targets. I suggest that getrandom just copies std for now.

~~Ah, indeed. In that case I think we can use RtlGenRandom fallback only for XP targets, and BCryptGenRandom for all other Windows targets.~~

UPD: Never mind, we can't distinguish between XP and later versions by using only target triplet.

Relevant discussion: https://internals.rust-lang.org/t/8745

Relevant issue: openssl/openssl#8644

So we will have to wait for dropping of Windows XP (and hopefully Vista) from the list of supported Rust targets, until then we will have to keep using RtlGenRandom.

I'd recommend using git blame and doing some digging to learn about the rationale for the usage in Rust's standard library. Doing that, for example, leads to rust-lang/rust#45370 which has a lot more information about why things are the way they are.

The compiler team considers dropping Windows XP support, assuming it will be done, I think we can drop its support in getrandom v0.2 as well.

SGTM, how long after they drop support should we update the library? Given that rand 0.8 isn't out yet, I'd be fine doing it sooner.

Right after the final decision will be made? IIRC Rust Windows XP support was arguably broken for a long time, so it should not be a problem. I also think that ideally we should do it before releasing rand v0.8.

In case you're interested, Go had some discussion on this and decided to stick with RtlGenRandom for non-UWP: golang/go#33542

So it looks like Go's final decision was based on three main sources:

What we (Rust) did
What BoringSSL does (this then determines what Chrome does)
What Firefox does

They didn't really provide their own independent reasoning for preferring one over another. We're changing what Rust does, so the main arguments in favor of RtlGenRandom come from this BoringSSL thread and this BoringSSL CL where the BoringSSL folks decided to not adopt BCryptGenRandom as the universal way to get randomness on Windows.

The main downside is that BCryptGenRandom (on its very first call with BCRYPT_USE_SYSTEM_PREFERRED_RNG) might read some preferences from the Windows registry, which is not allowed inside the Chromium sandbox.

Both RtlGenRandom and BCryptGenRandom will be slow on their first call:

RtlGenRandom (part of advapi32.dll) has to load bcrypt.dll which contains the Windows cryptographic code
BCryptGenRandom is part of bcrypt.dll, so no additional DLL loading is necessary, but it has to find the "system preferred" RNG, which might require reading values from the windows registry

Windows 10 introduced CNG Algorithm Pseudo-handles to avoid these issues, which would allow us to invoke BCryptGenRandom like:

BCryptGenRandom(
    BCRYPT_RNG_ALG_HANDLE,
    chunk.as_mut_ptr(),
    chunk.len() as u32,
    0,
)

but that (maybe) only works on Windows 10. In all cases, performance was basically the same (as they all call the same bcrypt-internal crypto code).

My decision would be to switch to BCryptGenRandom. The advantages of avoiding advapi32.dll and having only one implementation outweigh the very Chromium-specific sandboxing concerns.

If either sandboxing or performance ever become an issue (unlikely), I think we should just try to invoke BCryptGenRandom with BCRYPT_RNG_ALG_HANDLE first, before falling back to invoking it with BCRYPT_USE_SYSTEM_PREFERRED_RNG.

saw this thread linked from the golang issue.

My decision would be to switch to BCryptGenRandom

that is a very wise decision, because if one day a user decides to use the rust "crypto" wrappers and asks you "what is the underlying algorithm implementation behind RtlGenRandom and is it compliant with standard X?", only a Microsoft employee would be able to answer for sure and there is no documented guarantee whether such an implementation detail can change for RtlGenRandom in Windows-next without any notice.

Rollup merge of rust-lang#96917 - marti4d:master, r=ChrisDenton

Rollup merge of #96917 - marti4d:master, r=ChrisDenton

add fixed-seed support

fix no-std support

fix no-std support (replace getrandom)

Navigation Menu

Repository navigation

Windows: RtlGenRandom vs BCryptGenRandom #65

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Windows: RtlGenRandom vs BCryptGenRandom #65

Description

Activity

chouquette commented on Jul 26, 2019

ollie27 commented on Jul 26, 2019

newpavlov commented on Jul 26, 2019

newpavlov commented on Jul 28, 2019

alexcrichton commented on Jul 29, 2019

newpavlov commented on Oct 31, 2020

josephlr commented on Oct 31, 2020

newpavlov commented on Oct 31, 2020

bluetech commented on Dec 18, 2020

josephlr commented on Jan 3, 2021

neolit123 commented on Feb 18, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions