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

Jump to bottom

math/rand: deprecate Read #20661

Closed
jondb opened this issue Jun 13, 2017 · 20 comments
Closed

math/rand: deprecate Read #20661

jondb opened this issue Jun 13, 2017 · 20 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Proposal Proposal-Accepted Proposal-FinalCommentPeriod v2 A language change or incompatible library change
Milestone
Go1.20

Comments

@jondb
Copy link

jondb commented Jun 13, 2017 •
edited

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
@bradfitz bradfitz added the v2 A language change or incompatible library change label Jun 13, 2017
@bradfitz
Copy link
Contributor

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

@randall77
Copy link
Contributor

You can always do

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

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

@jondb
Copy link
Author

jondb 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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@AlexRouSg
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@ianlancetaylor
Copy link
Contributor

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.

@ianlancetaylor ianlancetaylor added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 20, 2018
@adamdecaf adamdecaf mentioned this issue Apr 10, 2018
Closed
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 16, 2019
@gopherbot gopherbot added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 3, 2019
@rsc
Copy link
Contributor

rsc commented Sep 7, 2022

One possibility is to remove rand.Read in a v2 of math/rand, but another possibility (or a step toward that) would be to mark Read as Deprecated, like

// Deprecated: For almost all use cases, crypto/rand.Read is more appropriate.

@rsc
Copy link
Contributor

rsc commented Sep 7, 2022

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

@rsc rsc changed the title proposal: math/rand: disambiguate Read from crypto/rand.Read proposal: math/rand: deprecate Read Sep 21, 2022
@rsc
Copy link
Contributor

rsc commented Sep 21, 2022

Does anyone object to marking math/rand.Read deprecated?

@rsc
Copy link
Contributor

rsc commented Sep 28, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@gopherbot
Copy link

Change https://go.dev/cl/436955 mentions this issue: math/rand: deprecate Read

@gopherbot
Copy link

Change https://go.dev/cl/436956 mentions this issue: all: replace math/rand.Read with crypto/rand.Read

@rsc rsc mentioned this issue Oct 4, 2022
Closed
@rsc
Copy link
Contributor

rsc commented Oct 6, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: math/rand: deprecate Read math/rand: deprecate Read Oct 6, 2022
@rsc rsc modified the milestones: Proposal, Backlog Oct 6, 2022
gopherbot pushed a commit that referenced this issue Oct 27, 2022
@hopehook @gopherbot
math/rand: deprecate Read
599a1e4 
For #20661.

Change-Id: I1e638cb619e643eadc210d71f92bd1af7bafc912
Reviewed-on: https://go-review.googlesource.com/c/go/+/436955
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: hopehook <hopehook@golangcn.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
romaindoumenc pushed a commit to TroutSoftware/go that referenced this issue Nov 3, 2022
@hopehook @romaindoumenc
math/rand: deprecate Read
c30bce7 
For golang#20661.

Change-Id: I1e638cb619e643eadc210d71f92bd1af7bafc912
Reviewed-on: https://go-review.googlesource.com/c/go/+/436955
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: hopehook <hopehook@golangcn.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
@MaerF0x0
Copy link

MaerF0x0 commented Nov 17, 2022 •
edited

one can use gosec linter with golanglint-ci like so
and watch for G404 code.

golangci-lint run --disable-all --enable gosec

@rsc rsc closed this as completed Nov 23, 2022
@ncw
Copy link
Contributor

ncw commented Nov 24, 2022

Just FYI this exact issue caused a CVE in rclone https://nvd.nist.gov/vuln/detail/CVE-2020-28924

The code when written used the correct crypto/rand but after some refactoring the import on the file got changed to math/rand and no one noticed 😟

I suspect goimports might have been involved. It was certainly in use then!

@dmitshur dmitshur modified the milestones: Backlog, Go1.20 Dec 6, 2022
@prattmic prattmic mentioned this issue Dec 6, 2022
Closed
@BobDu BobDu mentioned this issue Apr 14, 2023
Closed
@BobDu
Copy link

BobDu commented Apr 14, 2023

#59629 suggest add math/rand.InsecureRand

@torcolvin torcolvin mentioned this issue Oct 26, 2023
Merged
1 task
@golang golang locked and limited conversation to collaborators Apr 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Proposal Proposal-Accepted Proposal-FinalCommentPeriod v2 A language change or incompatible library change
Projects
None yet
Milestone
Go1.20
Development

No branches or pull requests