Conversation
Codecov Report
@@ Coverage Diff @@
## master #6364 +/- ##
==========================================
+ Coverage 60.80% 60.87% +0.06%
==========================================
Files 283 283
Lines 26945 26866 -79
==========================================
- Hits 16384 16354 -30
+ Misses 8859 8814 -45
+ Partials 1702 1698 -4
|
There was a problem hiding this comment.
Very excited this is getting changed! Thanks for doing it!
Re: The gosec concern, I think outside of tests, its useful to mark the lines explicitly with nolint gosec, so its explicit to the reader that these are intentionally not cryptographically secure.
For tests, maybe the entire file should get the nolint: gosec applied?
| func NewRand() *mrand.Rand { | ||
| var seed int64 | ||
| binary.Read(crand.Reader, binary.BigEndian, &seed) | ||
| return mrand.New(mrand.NewSource(seed)) |
There was a problem hiding this comment.
I think this actually isn't thread-safe, we instead need to use mrand.Seed for it to be thread-safe. (Per comment on https://golang.org/pkg/math/rand/#Seed, and NewSource not being thread safe comment on https://golang.org/pkg/math/rand/#NewSource)
There was a problem hiding this comment.
Good catch! It's probably best to make this function not return anything but only call math/rand.Seed instead. So wherever a freshly seeded math/rand.Rand was used, I'll just reseed the global one lives internally in math/Rand by calling this function instead. Tomorrow...
| } else { | ||
| chars = append(chars, strChars[v]) | ||
| if len(chars) == length { | ||
| break MAIN_LOOP |
There was a problem hiding this comment.
woo, happy this is getting deleted!
Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
…nally not cryptographically secure: // nolint:gosec // G404: Use of weak random number generator
… to read from OS randomness at this point
| // nolint:gosec // G404: Use of weak random number generator | ||
| j := mrand.Intn(len(allAddr)-i) + i |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Co-authored-by: Callum Waters <cmwaters19@gmail.com>
use mrand.Uint32() + 1 for height
Bytes/Str are not reseeded each time they are called
tac0turtle
left a comment
There was a problem hiding this comment.
Thanks for the PR. Could you add a changelog entry before we merge?
|
Of course, will do it later today 👍🏼 |
|
Did they screw up on that codecov action update or did someone tamper with their bash script again? https://github.com/tendermint/tendermint/pull/6364/checks?check_run_id=2392223532#step:9:1 It was the update: https://twitter.com/thomasrockhu/status/1384619989634797576 and merging in #6379 will fix that. |
|
I am going to merge this now. Thanks for the contribution @liamsi |
The only functions that are left (for convenience) are:
Everything else is either replaced with vanilla math/rand (in tests) or with
math/rand.Randinstances fromNewRandthat are seeded with crypto/rand (which is as "secure" as before, maybe even a bit more "secure" as there is no global object that is only seeded once).TODO: there are a bunch ofG404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec). We can either use crypto/rand (e.g. in real code) or stick with math/rand and silence the linter (in tests). Note, that this wasn't (cryptographically) secure before either. It was just obfuscated because of the use of libs/rand (no one has written a linter here although @ValarDragon kinda warned us in #2956 and probably on various other channels too :-D ).Edit: made the above more explicit as suggested in #6364 (review)
closes #2956