crypto/rand: Legacy RtlGenRandom use on Windows #53192
Comments
|
Dupe of #33542 |
|
Thanks—it does look like the decision to go with RtlGenRandom over BCryptGenRandom was made fairly recently in #33542. Unless something significant has changed since #33542 (comment), I think this issue can be closed. CC @golang/security. |
dmitshur
added
the
WaitingForInfo
|
It may be reasonable to revisit this decision. It sounds like Rust (and rust-random/getrandom) switched to BCryptGenRandom after we made our decision. It sounds like one of the main drivers for BoringSSL sticking with RtlGenRandom was that BCryptGenRandom may require reading information from the registry, which is not viable inside of sandboxes (which makes it non-viable for Chromium.) It also sounds like performance concerns are perhaps unfounded, as both incur similar start-up costs, and then perform basically the same. We don't really have these concerns (we use other Windows APIs that act similarly.) I'd generally err on the side of using something that is (a) recommended in Windows API documentation, and (b) has it's internals documented, over something which is (a) deprecated and (b) we only know how it works because of reversing. That said, it seems like there is no real pressing concerns for why we should switch away. RtlGenRandom is not broken, nor does it look like Microsoft is going to excise it any time soon. If BCryptGenRandom was noticeably faster that would be a good reason to switch, but it sounds like performance is ~basically the same. cc @zx2c4, who previously had opinions about this. |
|
I haven't seen any thing change on the Windows side of things. I can go on another reverse engineering diving tour and see if there's anything interesting down there. But barring that, I don't see any reason to revisit this decision at the moment.
RtlGenRandom is documented actually. |
dmitshur
removed
the
WaitingForInfo
seankhliao
added
the
NeedsDecision
Oh really, do you have a link? I was under the impression there was only an officially published vague outline of how the algorithm worked (and/or had no guarantee that it would stick to the outline as it was specified at the time.) |
|
CL 536235 just replaced We can close this issue, as it is about not using a legacy random number generator, which is not the case anymore. |
dmitshur
added
NeedsFix
|
@gopherbot please open backport issues, this reduces the impact of a security issue. |
|
Backport issue(s) opened: #64412 (for 1.20), #64413 (for 1.21). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases. |
|
Change https://go.dev/cl/545356 mentions this issue: |
|
Change https://go.dev/cl/545355 mentions this issue: |
…ProcessPrng RtlGenRandom is a semi-undocumented API, also known as SystemFunction036, which we use to generate random data on Windows. It's definition, in cryptbase.dll, is an opaque wrapper for the documented API ProcessPrng. Instead of using RtlGenRandom, switch to using ProcessPrng, since the former is simply a wrapper for the latter, there should be no practical change on the user side, other than a minor change in the DLLs we load. Updates #53192 Fixes #64413 Change-Id: Ie6891bf97b1d47f5368cccbe92f374dba2c2672a Reviewed-on: https://go-review.googlesource.com/c/go/+/536235 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Quim Muntal <quimmuntal@gmail.com> Auto-Submit: Roland Shoemaker <roland@golang.org> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> (cherry picked from commit 693def1) Reviewed-on: https://go-review.googlesource.com/c/go/+/545355 Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
…ProcessPrng RtlGenRandom is a semi-undocumented API, also known as SystemFunction036, which we use to generate random data on Windows. It's definition, in cryptbase.dll, is an opaque wrapper for the documented API ProcessPrng. Instead of using RtlGenRandom, switch to using ProcessPrng, since the former is simply a wrapper for the latter, there should be no practical change on the user side, other than a minor change in the DLLs we load. Updates #53192 Fixes #64412 Change-Id: Ie6891bf97b1d47f5368cccbe92f374dba2c2672a Reviewed-on: https://go-review.googlesource.com/c/go/+/536235 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Quim Muntal <quimmuntal@gmail.com> Auto-Submit: Roland Shoemaker <roland@golang.org> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> (cherry picked from commit 693def1) Reviewed-on: https://go-review.googlesource.com/c/go/+/545356 Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
What version of Go are you using (
go version)? 1.18.3 (Windows)Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env)?Windows 10, x64
go envOutputWhat did you do?
I identified this issue in a security code review.
What did you expect to see?
Use a recommended Windows random number as specified in https://docs.microsoft.com/en-us/security/sdl/cryptographic-recommendations#random-number-generators.
Id recommend BCryptGenRandom with the BCRYPT_USE_SYSTEM_PREFERRED_RNG flag, and remove any calls to the RtlGenRandom API.
What did you see instead?
In file rand_windows.go, random numbers are generated by calling a legacy PRNG API, RtlGenRandom.
The text was updated successfully, but these errors were encountered: