Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: math/rand: disambiguate Read from crypto/rand.Read #20661

Open
jondb opened this issue Jun 13, 2017 · 10 comments

Comments

Projects
None yet
7 participants
@jondb
Copy link

commented Jun 13, 2017

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

1.7.3

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

all

What did you do?

I wrote https://play.golang.org/p/WpSHEv_Mc7

import "math/rand"
_, err := rand.Read(randBuff)

I should have written https://play.golang.org/p/Ho8Ior-om7

import "crypto/rand"
_, err := rand.Read(randBuff)

In practice, the import statement and the function invocation are not one line apart, but separated by a lot of code. Also, IDEs may automatically import the wrong package.

What did you expect to see?

I expected crypto rand to be obviously different than math rand. Something like:

import "crypto/rand"
_, err := rand.CryptoRand(randBuff)

while,

import "math/rand"
_, err := rand.InsecureRand(randBuff)

This makes it super clear that one is not like the other, and helps the developer decide which to use. Also helps code reviewers determine if there is an obvious error.

Security should be explicit.

What did you see instead?

I saw experienced and inexperienced developers both make the same mistake of using math/rand instead of crypto/rand, and code reviews miss the problem.

@gopherbot gopherbot added this to the Proposal milestone Jun 13, 2017

@gopherbot gopherbot added the Proposal label Jun 13, 2017

@bradfitz bradfitz added the Go2 label Jun 13, 2017

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 13, 2017

We can't change this for Go 1, so I've tagged this for Go2 for consideration in the future.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2017

You can always do

import crytporand "crypto/rand"
cryptorand.Read(randBuff)

That at least makes it clearer at the callsite what you intended.

@jondb

This comment has been minimized.

Copy link
Author

commented Jun 13, 2017

@bradfitz - makes sense; is there a warning capability in the Go 1 compiler?
@randall77 - designing things to be secure by default is the principle here. Your suggestion might work with a lint system to produce a warning when using math.rand.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 13, 2017

makes sense; is there a warning capability in the Go 1 compiler?

Intentionally not. See the https://golang.org/doc/faq#unused_variables_and_imports answer.

@AlexRouSg

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2017

Why can't we just add new alias functions with a suffix like Crypto e.g. ReadCrypto/NewCrypto/NewSourceCrypto/etc...

That way we still have the 1.0 compatibility and makes it easier for people to make sure they are using the correct rand.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 15, 2017

I suppose we could, but we generally try to avoid having two ways to do the same thing.

@AlexRouSg

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2017

Well I'm not in the affected group so I'll let you guys debate if the pros outweigh the cons.

But I can imagine some company having a security vulnerability cause of some late night coding where someone auto imported math/rand instead of crypto/rand.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 15, 2017

Your security vulnerability scenario involves all of the following failures:

  • a tool automatically chose math/rand instead of crypto/rand. goimports doesn't do this as of a year ago. That was fixed in golang/tools@0835c73
  • somebody wrote security code late at night
  • code review didn't happen or missed the problem

It's possible, but it's a bit of a stretch. I don't think it warrants fixing in Go 1.x where it can't be changed cleanly.

In the meantime, file bugs against any auto-import tools that get it wrong.

@rsc rsc changed the title Proposal: Clarify difference between crypto/rand and math/rand. proposal: disambiguate crypto/rand.Read and math/rand.Read Jun 16, 2017

@rsc rsc changed the title proposal: disambiguate crypto/rand.Read and math/rand.Read proposal: math/rand: disambiguate Read from crypto/rand.Read Jun 16, 2017

@ericlagergren

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2017

Also: tools like gas (https://github.com/GoASTScanner/gas) already catch this.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2018

I don't think we should change the name of the Read function, but for Go 2 we could perhaps change the name of the type crypto/rand package to, perhaps, crypto/crand, to decrease the chance of accidental confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.