22 ms·

From the article > Go aims to help developers write code that is secure by default. When we observe a common mistake with security consequences, we look for wa

by nickcw 2y ago

From the article > Go aims to help developers write code that is secure by default. When we observe a common mistake with security consequences, we look for ways to reduce the risk of that mistake or eliminate it entirely. In this case, math/rand’s global generator was far too predictable, leading to serious problems in a variety of contexts. > For example, when Go 1.20 deprecated math/rand’s Read, we heard from developers who discovered (thanks to tooling pointing out use of deprecated functionality) they had been using it in places where crypto/rand’s Read was definitely needed, like generating key material. I made exactly this mistake in rclone. I refactored some code which was using the Read function from crypt/rand and during the process the import got automatically changed (probably by goimports when mixing code which did use math/rand) to math/rand. So it changed from using a secure random number generator to a deterministic one rclone seeded with the time of day. I didn't notice in the diffs :-( Hence https://www.cvedetails.com/cve/CVE-2020-28924/ https://www.cvedetails.com/cve/CVE-2020-28924/ So this change gets a big :+1: from me.

neonsunset 2y ago
Would not have happened in C# which uses distinct Random (and Random.Shared) as PRNG and RandomNumberGenerator[0] as CSPRNG even when mixing namespaces. Also has corresponding analyzer rule if you want to enforce this project-wide[1]. [0] https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.randomnumbergenerator https://learn.microsoft.com/en-us/dotnet/api/system.security... [1] https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca5394 https://learn.microsoft.com/en-us/dotnet/fundamentals/code-a...
- rsc 2y ago
  If I had to guess which of Random and RandomNumberGenerator was the cryptographically secure one, I would have guessed wrong. It's not clear either way.
  - onionisafruit 2y ago
    Maybe so, but once you know you won’t need to scroll to the top of the file to know which one you are using.
euroderf 2y ago
It would not be so tough to provide an API call with a name like "PredictableRand".
- Cthulhu_ 2y ago
  Or SemiRand, UnsafeRand, that kinda thing. Bit more explicit naming. I feel like a lot of Go's naming is down to legacy / what people are used to, which on the one side is good because familiarity, but on the other it opens it up to the same mistakes and confusion that has already been made in the past.
  - masklinn 2y ago
    The main issue was math/rand having a `Read` utility function with the same signature as crypto/rand, compounded by packages being namespaced but the namespacing not being preserved by importing. So a call to `rand.Read()` could be cryptographic or not you'd have to check the import to be sure.
- tedunangst 2y ago
  It's not so hard to do `import notrand "math/rand"` which helps avoid mistakes in the mean time.
arp242 2y ago
I've also had some report a vulnerability because they thought math/rand was being used when it wasn't. They just mixed something up with a few different files – not a big deal – but it just goes to show how confusing the entire thing is. text/template and html/template are similar. In hindsight this package name shadowing was a bad idea.
- fl0ki 2y ago
  This is one of my bigger remaining issues with day-to-day use of Go. I love how neat Go `package.Symbol` usually looks -- that objective was met -- but it has at least these problems: * It is syntactically identical to `variable.Member` so even at a glance it's ambiguous. After shadowing occurs, diagnostics get really confused. Go could at least include more error information when this is a possible cause. * The best package names have a lot of overlap with the best local variable names and in some cases good unexported type names [1]. Single words, almost exclusively nouns, usually singular form (prior to `slices`/`maps`), etc. so if you follow these idioms for both packages and variables you risk a lot of shadows. Who among us hasn't wanted to shadow the name `url` or `path`? To this day I feel I waste a lot of time trying to choose good package names without burning the namespace of good variable names. Outside the standard library, the official gRPC library is an arguably even better example of terrible package naming: `codes`, `peer`, `status`, and `resolver` are all likely variable names too. The only properly namespaced package is `grpclog`. * Even if you accept that lowercase is unexported and uppercase is exported, the lowercase side of this makes package-level unexported type names more likely to shadow package names. You can nest those type names inside functions, unless they're generic functions, and these nested types can't have methods so they're very rarely useful. If intra-package namespacing and privacy were more refined, at least we'd have more workarounds to avoid type names shadowing package names. * Semi-related, with packages being the only way to enforce privacy, it seems like we're encouraged to create a lot of packages if we want to enforce privacy a lot. But the more you create, the more globally unique names you have to choose, creating more pressure on that namespace shared with variables and unexported types. * You can't nest `pkg1.pkg2.Symbol` even though you can nest `var1.var2.Member`[2]. This could get ugly fast and I don't actually want this in the language, but if it had existed then it would have been a useful tool to resolve ambiguity and separate namespaces without aliases. In any case it's another inconsistency between syntax and semantics. * Import aliases can solve a lot of problems on the spot, but trades them for other problems. When used inconsistently, humans can get confused jumping around between files, but there's no tooling to enforce consistent use, not even a linter. Even when used consistently, tooling can still get confused, e.g. when you move code to another file that doesn't have the import yet then your tooling has to make its best guess and even the state of the art makes a lot of mistakes. In general, having even a single import alias makes code snippets less "portable" even within a project and even more so across projects, so in practice they should be avoided. * Package names are tied to file organization while still also being tied to privacy. When you're perfectly happy with a project structure, this is very elegant and you forget all about it. When you want to refactor a bit, especially to avoid a circular dependency or adjust privacy control, you have to update all callers at best or are permanently limited by your exported API at worst [3]. I observe that most libraries out there prefer to have just a couple of huge exported packages, giving up privacy on their end in exchange for simplicity for users. * Dot imports were designed in a way that ensures they never get used. You can only dot-import an entire package, not individual symbols from it, so any dot-import is a semver hazard and discouraged. It didn't have to be this way, importing individual symbols (like C++, Python, Java, Rust, and probably many more [4]) would have still reduced clutter without trading it for a semver hazard. It's a strange oversight in a language otherwise so well suited to writing future-proof code. Of my gripes in this comment, I think this is the main one I feel could be resolved by backwards-compatible language extensions, but it's also the least relevant to the original issue of name shadowing. These are all manageable issues when you carefully structure your own projects and pick all of your own package names. Though, I'm sure anyone who has worked in Go long enough has had to contribute to (or outright inherit) a project written with a very different approach that has a lot more shadowing hazards, showing how these individually simple rules can combine to serious quality of life issues on a project. Exported package names often become unfixable, so when you inherit a situation like that, you're going to get routine papercuts with no real way to avoid them. [1] If the separator wasn't the same `.` then this wouldn't be a problem, though I admit any other separator in this position is automatically uglier. [2] Assume the type is in the same package so the middle token can be lowercase. [3] Rust goes a bit far in the other direction by always requiring an explicit module structure, but once you have it, it's actually decoupled from file organization and privacy. That said, my overall experience has been that managing `mod` and `use` hierarchies in Rust is still far more clunky and manual than packages in Go. The good parts are fine privacy control, separate namespaces, and consistent nesting, the bad part is needing boilerplate even in what should be simple cases. [4] I respect that Go doesn't copy other languages and does its own thing, but when a feature like importing individual symbols is common to languages as different as Python and Rust, there might be real value in solving the same problem in Go too.
rsc 2y ago
Ouch, apologies for that. We changed goimports to prefer crypto/rand back in 2016, so I'm not entirely sure what happened during your refactoring. Perhaps code that used other math/rand-only APIs ended up in the same file. https://go-review.googlesource.com/24847 https://go-review.googlesource.com/24847 Anyway, I'm glad we're cleaning all this up!
- nickcw 2y ago
  Yes I'm pretty sure that is what happened. I was consolidating random functions into one file which was already using math/rand so when I moved the function which was using crypto/rand there it picked up the math/rand import. So not goimports problem, my problem for expecting goimports to magically do the right thing like it usually does!
- mholt 2y ago
  Ever since 2016 I actually do a named import for "math/rand" now by calling it "mathrand" or "weakrand" just to be sure I don't accidentally use it for anything secure. Go keeps getting better. Thanks for your hard work!
bradfitz 2y ago
goimports has special-cased math/rand.Read vs crypto/rand.Read from basically the beginning. But https://github.com/golang/tools/commit/0835c735343e0d8e375f0b980b358619ff0853a2 https://github.com/golang/tools/commit/0835c735343e0d8e375f0... in 2016 references a time window where it could resolve "rand.Read" as "math/rand". Maybe you were in that time window?
- nickcw 2y ago
  I think this was my fault. I refactored some code using crypto/rand into a file which was already using math/rand and failed to notice that the imports were wrong and not magically fixed by goimports like they usually are! So goimports rocks and my code review skills suck!
takeda 2y ago
I also noticed when I tried to search for "secure password generation golang" nearly all examples use math/rand. And to make things worse all of them initialize seed with current time just before generating the password. This was discovered after I found in our code someone used math rand and was curious where they copied it from :)
gchaincl 2y ago
I had a similar goimport issue importing the wrong pkg. I've added a forbidigo linter rule to fail when certain packages were imported.

by nickcw 2y ago

neonsunset 2y ago

Would not have happened in C# which uses distinct Random (and Random.Shared) as PRNG and RandomNumberGenerator[0] as CSPRNG even when mixing namespaces. Also has corresponding analyzer rule if you want to enforce this project-wide[1]. [0] https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.randomnumbergenerator https://learn.microsoft.com/en-us/dotnet/api/system.security... [1] https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca5394 https://learn.microsoft.com/en-us/dotnet/fundamentals/code-a...

rsc 2y ago
If I had to guess which of Random and RandomNumberGenerator was the cryptographically secure one, I would have guessed wrong. It's not clear either way.
- onionisafruit 2y ago
  Maybe so, but once you know you won’t need to scroll to the top of the file to know which one you are using.

euroderf 2y ago

It would not be so tough to provide an API call with a name like "PredictableRand".

Cthulhu_ 2y ago
Or SemiRand, UnsafeRand, that kinda thing. Bit more explicit naming. I feel like a lot of Go's naming is down to legacy / what people are used to, which on the one side is good because familiarity, but on the other it opens it up to the same mistakes and confusion that has already been made in the past.
- masklinn 2y ago
  The main issue was math/rand having a `Read` utility function with the same signature as crypto/rand, compounded by packages being namespaced but the namespacing not being preserved by importing. So a call to `rand.Read()` could be cryptographic or not you'd have to check the import to be sure.
tedunangst 2y ago
It's not so hard to do `import notrand "math/rand"` which helps avoid mistakes in the mean time.

arp242 2y ago

I've also had some report a vulnerability because they thought math/rand was being used when it wasn't. They just mixed something up with a few different files – not a big deal – but it just goes to show how confusing the entire thing is. text/template and html/template are similar. In hindsight this package name shadowing was a bad idea.

fl0ki 2y ago
This is one of my bigger remaining issues with day-to-day use of Go. I love how neat Go `package.Symbol` usually looks -- that objective was met -- but it has at least these problems: * It is syntactically identical to `variable.Member` so even at a glance it's ambiguous. After shadowing occurs, diagnostics get really confused. Go could at least include more error information when this is a possible cause. * The best package names have a lot of overlap with the best local variable names and in some cases good unexported type names [1]. Single words, almost exclusively nouns, usually singular form (prior to `slices`/`maps`), etc. so if you follow these idioms for both packages and variables you risk a lot of shadows. Who among us hasn't wanted to shadow the name `url` or `path`? To this day I feel I waste a lot of time trying to choose good package names without burning the namespace of good variable names. Outside the standard library, the official gRPC library is an arguably even better example of terrible package naming: `codes`, `peer`, `status`, and `resolver` are all likely variable names too. The only properly namespaced package is `grpclog`. * Even if you accept that lowercase is unexported and uppercase is exported, the lowercase side of this makes package-level unexported type names more likely to shadow package names. You can nest those type names inside functions, unless they're generic functions, and these nested types can't have methods so they're very rarely useful. If intra-package namespacing and privacy were more refined, at least we'd have more workarounds to avoid type names shadowing package names. * Semi-related, with packages being the only way to enforce privacy, it seems like we're encouraged to create a lot of packages if we want to enforce privacy a lot. But the more you create, the more globally unique names you have to choose, creating more pressure on that namespace shared with variables and unexported types. * You can't nest `pkg1.pkg2.Symbol` even though you can nest `var1.var2.Member`[2]. This could get ugly fast and I don't actually want this in the language, but if it had existed then it would have been a useful tool to resolve ambiguity and separate namespaces without aliases. In any case it's another inconsistency between syntax and semantics. * Import aliases can solve a lot of problems on the spot, but trades them for other problems. When used inconsistently, humans can get confused jumping around between files, but there's no tooling to enforce consistent use, not even a linter. Even when used consistently, tooling can still get confused, e.g. when you move code to another file that doesn't have the import yet then your tooling has to make its best guess and even the state of the art makes a lot of mistakes. In general, having even a single import alias makes code snippets less "portable" even within a project and even more so across projects, so in practice they should be avoided. * Package names are tied to file organization while still also being tied to privacy. When you're perfectly happy with a project structure, this is very elegant and you forget all about it. When you want to refactor a bit, especially to avoid a circular dependency or adjust privacy control, you have to update all callers at best or are permanently limited by your exported API at worst [3]. I observe that most libraries out there prefer to have just a couple of huge exported packages, giving up privacy on their end in exchange for simplicity for users. * Dot imports were designed in a way that ensures they never get used. You can only dot-import an entire package, not individual symbols from it, so any dot-import is a semver hazard and discouraged. It didn't have to be this way, importing individual symbols (like C++, Python, Java, Rust, and probably many more [4]) would have still reduced clutter without trading it for a semver hazard. It's a strange oversight in a language otherwise so well suited to writing future-proof code. Of my gripes in this comment, I think this is the main one I feel could be resolved by backwards-compatible language extensions, but it's also the least relevant to the original issue of name shadowing. These are all manageable issues when you carefully structure your own projects and pick all of your own package names. Though, I'm sure anyone who has worked in Go long enough has had to contribute to (or outright inherit) a project written with a very different approach that has a lot more shadowing hazards, showing how these individually simple rules can combine to serious quality of life issues on a project. Exported package names often become unfixable, so when you inherit a situation like that, you're going to get routine papercuts with no real way to avoid them. [1] If the separator wasn't the same `.` then this wouldn't be a problem, though I admit any other separator in this position is automatically uglier. [2] Assume the type is in the same package so the middle token can be lowercase. [3] Rust goes a bit far in the other direction by always requiring an explicit module structure, but once you have it, it's actually decoupled from file organization and privacy. That said, my overall experience has been that managing `mod` and `use` hierarchies in Rust is still far more clunky and manual than packages in Go. The good parts are fine privacy control, separate namespaces, and consistent nesting, the bad part is needing boilerplate even in what should be simple cases. [4] I respect that Go doesn't copy other languages and does its own thing, but when a feature like importing individual symbols is common to languages as different as Python and Rust, there might be real value in solving the same problem in Go too.

rsc 2y ago

Ouch, apologies for that. We changed goimports to prefer crypto/rand back in 2016, so I'm not entirely sure what happened during your refactoring. Perhaps code that used other math/rand-only APIs ended up in the same file. https://go-review.googlesource.com/24847 https://go-review.googlesource.com/24847 Anyway, I'm glad we're cleaning all this up!

nickcw 2y ago
Yes I'm pretty sure that is what happened. I was consolidating random functions into one file which was already using math/rand so when I moved the function which was using crypto/rand there it picked up the math/rand import. So not goimports problem, my problem for expecting goimports to magically do the right thing like it usually does!
mholt 2y ago
Ever since 2016 I actually do a named import for "math/rand" now by calling it "mathrand" or "weakrand" just to be sure I don't accidentally use it for anything secure. Go keeps getting better. Thanks for your hard work!

bradfitz 2y ago

goimports has special-cased math/rand.Read vs crypto/rand.Read from basically the beginning. But https://github.com/golang/tools/commit/0835c735343e0d8e375f0b980b358619ff0853a2 https://github.com/golang/tools/commit/0835c735343e0d8e375f0... in 2016 references a time window where it could resolve "rand.Read" as "math/rand". Maybe you were in that time window?

nickcw 2y ago
I think this was my fault. I refactored some code using crypto/rand into a file which was already using math/rand and failed to notice that the imports were wrong and not magically fixed by goimports like they usually are! So goimports rocks and my code review skills suck!

takeda 2y ago

I also noticed when I tried to search for "secure password generation golang" nearly all examples use math/rand. And to make things worse all of them initialize seed with current time just before generating the password. This was discovered after I found in our code someone used math rand and was curious where they copied it from :)

gchaincl 2y ago

I had a similar goimport issue importing the wrong pkg. I've added a forbidigo linter rule to fail when certain packages were imported.