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?

[FiloSottile]

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.

This is actually a pretty common requirement in tests, and it feels wrong to get tests forever stuck to a combination of GODEBUG=cryptocustomrand=1 and defusing MaybeReadByte just to end up with breakage any time we change the algorithm.

We discussed a solution with @rsc: let's just acknowledge that tests need this, and that in tests it might make sense to take the tradeoff of losing backwards compatibility, and provide an explicit way to get to the same result, which only works in tests and which is also useful for other things.

package testing

// RandReader returns a Reader that produces a deterministic pseudo-random
// stream based on the seed.
//
// The returned reader can also be used as the rand parameter to
// rsa.GenerateKey, rsa.EncryptPKCS1v15, ecdsa.GenerateKey,
// ecdsa.SignASN1, ecdsa.Sign, and ecdsa.PrivateKey.Sign,
// and ecdh.Curve.GenerateKey to cause them to behave deterministically.
// Note that the output of these functions may and will change across Go versions,
// so any tests using this affordance must be prepared to
// update vectors based on the Go version (e.g. with a build tag).
//
// It can only be called in a test (as reported by [Testing]),
// otherwise it panics.
func RandReader(seed int64) io.Reader

[aclements]

Overall this seems reasonable, though it will be a difficult transition.

We've been trying to keep niche things out of the testing package proper. How about testing/cryptotest? Then we can also call it InsecureRandReader to really hammer home that you shouldn't use this.

[FiloSottile]

I proposed it as a generically named thing because "a reproducible sequence of pseudo-tandom bytes, for testing" is something I've needed quite a few times, not only when testing crypto, and I jury-rigged it with AES-CTR or ChaCha8.

I wonder if that's my selection bias, but I've heard from others that they would have needed this for generic purposes.

With the testing.Testing check and such a small seed, I am not really worried about folks using it thinking it's secure.

Ultimately, no strong opinion, although a new package feels like a big lift.

[aclements]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— aclements for the proposal review group

[aclements]

There are a lot of other APIs that take random io.Readers, too, like EncryptOAEP, EncryptPKCS1v15, etc. Though I also see there are a lot of functions that take a random io.Reader but are documented to ignore it. What's your ideal final state here?

It seems really odd that these APIs are going to take an io.Reader that they nearly always ignore. Another option is that we introduce new APIs that don't take the io.Reader and we deprecate the APIs that do. This would let us keep the old APIs working the way they do today, at least for another release or two, but give a strong signal that people need to move off the old APIs. I'd be much more comfortable with further changing the behavior of these APIs if people had a strong signal that this was coming. Though this wouldn't help with the testing issue.

This may be retreading old ground, but another option would be a vet check that requires the reader to be crypto/rand.Reader except in a test.

Can you give more context, or point to a past discussion, on why we decided it was okay to break backwards compatibility for these random sources?

[neild]

I proposed it as a generically named thing because "a reproducible sequence of pseudo-tandom bytes, for testing" is something I've needed quite a few times, not only when testing crypto, and I jury-rigged it with AES-CTR or ChaCha8.

If testing.RandReader is worth adding on its own merits, I think that should be a separate proposal. (My main question there would be: Have we made it too difficult to acquire non-secure bytes out of math/rand/v2? Could this be rand/v2.Rand.InsecureReader?)

[aclements]

Can you give more context, or point to a past discussion, on why we decided it was okay to break backwards compatibility for these random sources?

To clarify my question, I mean that, if I understand correctly, originally we were using the bytes from these readers in a semi-deterministic way: if you always provided the same bytes, you'd get the same results, unless you upgraded Go and there was an algorithm implementation change. At some point we moved to using those bytes but randomly shifting the stream in MaybeReadByte. I'm curious where we had the discussion in which we decided that was okay and not a violation of backwards compatibility.

[seankhliao]

I believe that was #21915 / CL 64451