crypto/rand: Legacy RtlGenRandom use on Windows

[tolginator]

What version of Go are you using (go version)? 1.18.3 (Windows)

$ go version
go version go1.18.3 windows/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

Windows 10, x64

go env Output

$ go env
set GOARCH=amd64

set GOEXE=.exe

set GOHOSTARCH=amd64

set GOHOSTOS=windows

set GOOS=windows

set GOPROXY=https://proxy.golang.org,direct

set GOROOT=C:\Program Files\Go

set GOSUMDB=sum.golang.org

set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_amd64

set GOVERSION=go1.18.3

set GCCGO=gccgo

set GOAMD64=v1

What 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.

[ericlagergren]

Dupe of #33542

[dmitshur]

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.

[rolandshoemaker]

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.

[zx2c4]

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.

(b) we only know how it works because of reversing

RtlGenRandom is documented actually.

[rolandshoemaker]

RtlGenRandom is documented actually.

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.)

[zx2c4]

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

[qmuntal]

CL 536235 just replaced RtlGenRandom with ProcessPrng, which is not deprecated and is what RtlGenRandom uses under the hood.

We can close this issue, as it is about not using a legacy random number generator, which is not the case anymore.

[rolandshoemaker]

@gopherbot please open backport issues, this reduces the impact of a security issue.

[gopherbot]

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.

[gopherbot]

Change https://go.dev/cl/545356 mentions this issue: [release-branch.go1.20] crypto/rand,runtime: switch RtlGenRandom for ProcessPrng

[gopherbot]

Change https://go.dev/cl/545355 mentions this issue: [release-branch.go1.21 crypto/rand,runtime: switch RtlGenRandom for ProcessPrng