proposal: crypto/rand: guard against mutation of Reader variable

[akalin]

crypto/rand exposes an io.Reader variable Reader as "a global, shared instance of a cryptographically strong pseudo-random generator." Furthermore, crypto/rand.Read implicitly uses Reader for its crypto source.

This seems problematic to me because then any package can just overwrite crypto/rand.Reader to point to some other object, affecting the security of any packages that rely on crypto/rand.Read or crypto/rand.Reader for security, e.g. x/crypto/nacl.

One can say that a language can never ultimately defend against code running in your same process, but I think it should be possible to write something that depends on crypto/rand for security that wouldn't require auditing other packages for a single malicious variable write.[1]

The main API flaw here, IMO, is that Reader is an io.Reader variable, whereas it should be a function that returns an io.Reader. A new API would look something like:

// Reader returns an io.Reader that reads from a cryptographically strong pseudo-random number generator.
func Reader() io.Reader

// Read is a helper function that calls Reader() and then passes it to io.ReadFull. 
func Read(b []byte) (n int, err error)

Alas, with the Go 1 compatibility guarantee Reader would have to remain, and Read would still have to use Reader. But the above could be added as new functions, say MakeReader() and SafeRead(). And the standard library (and other important external packages like x/crypto/nacl) could be changed to use those safe functions.

[1] Without this flaw, a malicious package would have to use the unsafe package to poke around in the internals of crypto/rand, or call out to the external OS to e.g. try to redirect access to the random device, which seems easier to audit for than a write to crypto/rand.Reader. Of course, I'm already assuming that a project worried about this is vendoring all of its dependencies.

[bradfitz]

If you care about security, you're already auditing all your vendored/dependency code, right?

Overriding rand.Reader in useful in tests, for example.

[akalin]

Yes, if you care about security, you're already auditing all vendored/dependency code. However, there's a difference in required effort between auditing for packages that e.g. make calls to unsafe, try to make system calls, or load DLLs, and auditing for packages that try to sneak in a single write to a global variable.

I don't think the benefits of overriding crypto/rand.Reader globally for tests outweigh the downsides of having one package's crypto be breakable by another package. It is easy enough to plumb through an io.Reader through the code you want to test.

[songgao]

Just wanted to add that, one of the many reasons that Go is fantastic is that it makes good choices and sane defaults for many things, so people don't make mistakes even if they don't have the awareness to, for example, audit vendoring.

[as]

This is similar to saying a class has secure member variables because they're private in other languages. Most operating systems don't even support this kind of security in the same user context, let alone process or package scope. If you're running untrusted code there are bigger things to worry about than package variables.

[akalin]

Well, it depends what you mean by 'secure', and what other languages you're talking about. I agree that, say, private variables in C++ are easy to work around, as well as dynamic languages like Ruby/Python/Javascript. However, for example, in Go, unexported variables are inaccessible outside of package scope by code that doesn't use unsafe, cgo, reflect, or some other similar magic, and assuming there isn't some bug in the go runtime/compiler. In that sense one could say that unexported variables are 'safer' than exported ones.

Note that my threat model isn't arbitrary untrusted code, so the comparison to OS-level security isn't really applicable -- it's third-party code that has to be audited before inclusion in a project. It's reasonably easy to audit the dependencies of a package, i.e. to check that it doesn't use unsafe, cgo, reflect, etc., and to perform greater scrutiny if so. It's a bit harder to audit its access of external package variables, and this bug demonstrates that the potential consequences of something slipping through the cracks isn't just weird stuff happening (like setting io.EOF to nil), but a compromise of a RNG that packages depend on for security.

Someone else mentioned that this has been brought up before for e.g. http.DefaultClient. The difference in this case is that one could create their own instance of http.Client and be diligent about using that. However, that's impossible in the case of crypto/rand.

On the other hand, it's not infeasible to write a tool that automates looking for code that modifies external package variables (except for reflection, unsafe, etc.). Although it's unfortunate that such a tool has to be written.

[rsc]

If you're truly worried about this, check it once in a while.

Maybe for Go 2 we could change the declared type of Reader so that there was nothing you could reassign to it.

[kbolino]

With five and half years of additional experience seeing software supply chain attacks increase, hopefully the stance expressed by the language team here can be softened. Sometimes even an extensive dependency audit is not enough, when things like typosquatting, dead repository resurrection, and renamed repository hijacking can change the underlying landscape. A lot of attacks are low-effort, target low-hanging fruit, and go undetected for a while. While module sums nowadays can protect against some of these attacks, there is a fundamental tension between never updating dependencies (and thus knowing exactly what you're using) and ensuring that critical security issues get addressed (which often requires updating dependencies ASAP).

For a sophisticated attacker with the ability to inject arbitrary code, there are other, better ways to break the security of a Go program. However, thinking in terms of defense-in-depth, just because all of the problems cannot be fixed easily does not mean an obvious and fairly easy-to-fix (or at least, easy-to-address) problem like this should be dismissed.

All that having been said, I think this is mostly a specific case of a more general problem, which is the lack of a way to protect global variables from modification by other packages/modules. Though functions can be used instead of variables, they are often not as ergonomic or discoverable as variables.