proposal: crypto: ignore rand io.Reader where behavior is not specified

[FiloSottile]

There are a few crypto APIs that take an io.Reader as a source of random bytes, but that don't commit to how those bytes are used. This caused issues over and over, for example any time we wanted to change the algorithm. These days we both document that they are not deterministic, and use randutil.MaybeReadByte to somewhat enforce it. See #58637.

Now that we have GODEBUGs, it might be time to rip the band-aid off. I propose we start ignoring the random io.Reader parameter of the following APIs, and always use the system random source (crypto/internal/sysrand.Read, not crypto/rand.Reader which may be overridden by the application).

• rsa.GenerateKey and rsa.GenerateMultiPrimeKey
• rsa.EncryptPKCS1v15
• ecdsa.GenerateKey
• ecdsa.SignASN1, ecdsa.Sign, and ecdsa.PrivateKey.Sign
• ecdh.Curve.GenerateKey

Using GODEBUG=cryptocustomrand=1 restores the old behavior. (Suggestions for a better name welcome.) This is a GODEBUG that I would like to remove in a few releases.

rsa.SignPKCS1v15 is not randomized, while rsa.SignPSS and rsa.EncryptOAEP have a fairly well-specified way to use random bytes. Aside from those and ed25519.GenerateKey (see below), I think I listed all APIs in non-deprecated packages that take a random io.Reader.

This might be an issue for the crypto/tls tests, which defuse MaybeReadByte by producing a stream of identical bytes. That's an abuse of GenerateKey anyway, because there is no guarantee that algorithms that expect random inputs will work with constant repeating streams. See for example #70643.

ed25519.GenerateKey is a weird exception in that it is well defined, but also documented to use crypto/rand.Reader if nil is passed. This is annoying because it forces a dependency on crypto/rand and therefore on math/big. We can't just use crypto/internal/sysrand.Read because the user might have overridden crypto/rand.Reader. I am tempted to also propose replacing "crypto/rand.Reader" with "the system random source" but it's probably not worth the risk.

/cc @golang/security

[gabyhelp]

Related Issues

• proposal: crypto/rand: guard against mutation of Reader variable #24160
• proposal: crypto/rand: use private reader, not Reader, in Read #42713 (closed)
• crypto/rsa: vary amount of randomness used by operations #21915 (closed)

Related Code Changes

• crypto/{ecdsa,rsa}: always use io.ReadFull with crypto/rand.Reader.
• crypto: randomly read an extra byte of randomness in some places.
• crypto: use provided random Reader in FIPS mode
• [dev.boringcrypto] crypto/rsa: drop random source reading emulation
• crypto: document non-determinism of GenerateKey
• [dev.boringcrypto] crypto/rsa: add test for, fix observable reads from custom randomness

Related Documentation

• Evolving the Go Standard Library with math/rand/v2 > Problems with math/rand > The Read Mistake

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[seankhliao]

it seems quite rare to override rand.Reader outside of tests https://github.com/search?q=language%3Ago+%2Frand.Reader+%3D+%2F+-path%3A*_test.go&type=code

[mateusz834]

Are there any real world use-cases where it is currently necessary to provide a different random source, than crypto/rand.Reader? seccomp?