Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Call RtlGenRandom() instead of CryptGetRandom() on Windows #111

Closed
cpeterso opened this issue Jul 22, 2016 · 3 comments · Fixed by #159
Closed

Call RtlGenRandom() instead of CryptGetRandom() on Windows #111

cpeterso opened this issue Jul 22, 2016 · 3 comments · Fixed by #159
Labels
X-bug Type: bug report

Comments

@cpeterso
Copy link
Contributor

On Windows, os.rs can call RtlGenRandom() directly instead of jumping through hoops to obtain an HCRYPTPROV handle from CryptAcquireContextA()/CryptReleaseContext() before calling CryptGenRandom() (which just calls RtlGenRandom() itself).

Chromium, boringssl and ring, and Firefox all call RtlGenRandom() directly (which is actually exported from advapi32.dll as SystemFunction036()).

@alexcrichton alexcrichton added the X-bug Type: bug report label Jun 14, 2017
@alexcrichton
Copy link
Contributor

The documentation for RtlGenRandom has a scary warning at the top:

The RtlGenRandom function is available for use in the operating systems specified in the Requirements section. It may be altered or unavailable in subsequent versions. Instead, use the CryptGenRandom function.

That seems... worrying?

@GabrielMajeri
Copy link
Contributor

GabrielMajeri commented Jul 18, 2017

I don't think there are any reasons to worry about using this function:

  • the function is used by the rand_s function from the CRT. A lot of applications statically link the CRT, Microsoft wouldn't risk removing this function and break them.

  • on this Firefox Bugzilla thread, a commenter said:

I've mailed a dev inside Microsoft to see what's the story behind this, as the warning on RtlGenRandom's documentation is not really founded on a realistic scenario and causes confusion: what's the deal: will they change it or not.

The response:

I got word back from Microsoft. The unofficial word is that it's not very likely that RtlGenRandom will change, also due to the reliance of their own rand_s function in the CRT.

Furthermore, from that same thread:

There's actually an advantage of using RtlGenRandom over CryptGenRandom which is that RtlGenRandom just does the RNG work and nothing else, while CryptGenRandom requires more overhead because it loads the whole CryptAPI code.

@alexcrichton
Copy link
Contributor

sounds convincing to me!

alexcrichton added a commit to alexcrichton/rust that referenced this issue Oct 18, 2017
This commit updates the OS random number generator on Windows to match the
upstream implementation in the `rand` crate. First proposed in
rust-random/rand#111 this implementation uses a "private" API of
`RtlGenRandom`. Despite the [documentation][dox] indicating this is a private
function its widespread use in Chromium and Firefox as well as [comments] from
Microsoft internally indicates that it's highly unlikely to break.

Another motivation for switching this is to also attempt to make progress
on rust-lang#44911. It may be the case that this function succeeds while the previous
implementation may fail in "weird" scenarios.

[dox]: https://msdn.microsoft.com/en-us/library/windows/desktop/aa387694(v=vs.85).aspx
[comments]: rust-random/rand#111 (comment)
bors added a commit to rust-lang/rust that referenced this issue Oct 21, 2017
std: Update randomness implementation on Windows

This commit updates the OS random number generator on Windows to match the
upstream implementation in the `rand` crate. First proposed in
rust-random/rand#111 this implementation uses a "private" API of
`RtlGenRandom`. Despite the [documentation][dox] indicating this is a private
function its widespread use in Chromium and Firefox as well as [comments] from
Microsoft internally indicates that it's highly unlikely to break.

Another motivation for switching this is to also attempt to make progress
on #44911. It may be the case that this function succeeds while the previous
implementation may fail in "weird" scenarios.

[dox]: https://msdn.microsoft.com/en-us/library/windows/desktop/aa387694(v=vs.85).aspx
[comments]: rust-random/rand#111 (comment)
mvinel pushed a commit to Parrot-Developers/libfutils that referenced this issue May 7, 2021
rand_s() is readily available in Windows's CRT,
accessible through <stdlib.h>. Unfortunately its
output is limited to an integer, thus requiring
multiple calls to fill a large buffer.

RtlGenRandom() is the other hand can accomodate
any buffer size, but is a pure Windows (NT) function,
not available in Windows's CRT, as it's declared
in <ntsecapi.h> and available in advapi32.dll.

And according to Microsoft documentation, the
availability of the function is unclear, it's
future too.

https://docs.microsoft.com/en-us/windows/win32/api/ntsecapi/nf-ntsecapi-rtlgenrandom

But according to Rust community  the function is
used in Firefox, Chromium, etc, thus it's unlikely
to be removed.

rust-random/rand#111

Especially since the rand_s() documentation state
its uses RtlGenRandom():

https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/rand-s?view=vs-2019

So rand_s() uses RtlGenRandom(), hence make use of
advapi32.dll, then there's no need to use rand_s()
when RtlGenRandom() is available.

This patch makes use of RtlGenRandom(), and fallback
to rand_s() if the former fail.

Note: for newer Windows versions (>= 10), RtlGenRandom()
could be replaced by newer, recommanded functions:

   BCryptGenRandom(BCRYPT_RNG_ALG_HANDLE,
                   buffer, len,
                   0);

https://docs.microsoft.com/en-us/windows/win32/api/bcrypt/nf-bcrypt-bcryptgenrandom
https://docs.microsoft.com/en-us/windows/win32/seccng/cng-algorithm-pseudo-handles
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
X-bug Type: bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants