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

QSP-6: Enforces crypto-secure PRNGs #6401

Merged
merged 22 commits into from Jun 26, 2020
Merged

Conversation

farazdagi
Copy link
Contributor

@farazdagi farazdagi commented Jun 25, 2020

What type of PR is this?

Feature/Enforces Strong Crypto

What does this PR do? Why is it needed?

  • Introduces a easy way to use crypto secure randomness, while not giving up math/rand API (which is convenient)
  • Followup to [QSP-6] Change usages of math/rand to crypto/rand #6362, it replaces and extends it
  • Enforces (via static code analysis) that everywhere cryptographically secure random number generators are used
  • Adds easy way to create such a generator by introducing the new package github.com/pryam/shared/rand (drop-in replacement for math/rand)

Which issues(s) does this PR fix?

Part of #6327

Other notes for review

  • Tool will not allow any packages ending in "/rand", except for "prysm/shared/rand":
    image

  • Tool is capable of handling aliased imports, if you import as mrand "math/rand" and use as, for instance, mrand.Intn(32) (instead of rand.Intn(32)) - analyser will be able to catch that code too!

  • Normally, no rand related code is expected to be imported directly: no imports of math/rand, golang.org/x/exp/rand, or even crypto/rand. Generators must be instantiated via /shared/rand/rand.NewGenerator() (or, occasionally, when you know what you are doing via /shared/rand/rand.NewDetermenisticGenerator().

  • In tests, no enforcement is done -- one can use any source of randomness.

Extra (for brave and curious)
Rationale of defining a new package, directly from github.com/prysm/shared/randpackage docs:

Package rand defines methods of obtaining cryptographically secure random number generators.

One is expected to use randomness from this package only, without introducing any other packages.
This limits the scope of code that needs to be hardened.

There are two modes, one for deterministic and another non-deterministic randomness:

  1. If deterministic pseudo-random generator is enough, use:
import "github.com/prysmaticlabs/prysm/shared/rand"
randGen := rand.NewDeterministicGenerator()
randGen.Intn(32) // or any other func defined in math.rand API

In this mode, only seed is generated using cryptographically secure source (crypto/rand). So,
once seed is obtained, and generator is seeded, the next generations are deterministic, thus fast.
This method is still better than using unix time for source of randomness - since time is not a
good source of seed randomness, when you have many concurrent servers using it (and they have
coinciding random generators' start times).

  1. For cryptographically secure non-deterministic mode (CSPRNG), use:
import "github.com/prysmaticlabs/prysm/shared/rand"
randGen := rand.NewGenerator()
randGen.Intn(32) // or any other func defined in math.rand API

Again, any of the functions from math/rand can be used, however, they all use custom source
of randomness (crypto/rand), on every step. This makes randomness non-deterministic. However,
you take a performance hit -- as it is an order of magnitude slower.

@farazdagi farazdagi self-assigned this Jun 25, 2020
@farazdagi farazdagi changed the title QSP-6: Adds cryptorand analyzer QSP-6: Enforces crypto-secure PRNGs Jun 26, 2020
@farazdagi farazdagi marked this pull request as ready for review June 26, 2020 09:26
@farazdagi farazdagi requested a review from a team as a code owner June 26, 2020 09:26
@farazdagi farazdagi added the Ready For Review A pull request ready for code review label Jun 26, 2020
nisdas
nisdas previously approved these changes Jun 26, 2020
@codecov
Copy link

codecov bot commented Jun 26, 2020

Codecov Report

Merging #6401 into master will increase coverage by 1.30%.
The diff coverage is 69.55%.

@@            Coverage Diff             @@
##           master    #6401      +/-   ##
==========================================
+ Coverage   60.07%   61.37%   +1.30%     
==========================================
  Files         323      349      +26     
  Lines       27422    28741    +1319     
==========================================
+ Hits        16473    17639    +1166     
- Misses       8733     8745      +12     
- Partials     2216     2357     +141     

@farazdagi farazdagi requested a review from nisdas June 26, 2020 10:50
Copy link
Contributor

@rauljordan rauljordan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent PR and thank you for the comprehensive explanation. Having a shared rand package enforced is great as it helps us keep control of how things work.

@rauljordan rauljordan merged commit 78465e2 into master Jun 26, 2020
@delete-merged-branch delete-merged-branch bot deleted the tools-analyzers-cryptorand branch June 26, 2020 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants