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

Check for crypto/rand errors and ReadFull io.Readers #417

Merged
merged 2 commits into from Mar 16, 2023
Merged

Conversation

bwesterb
Copy link
Member

In practice crypto/rand.Read never returns an error, but that is not guaranteed. Check for those errors.

In contrast to crypto/rand.Reader, a user-provided io.Reader, might not fill the buffer without returning an error. Though marginal, we should deal with that corner-case as well.

@bwesterb bwesterb requested a review from armfazh March 16, 2023 16:35
In practice crypto/rand.Read never returns an error, but that is not
guaranteed. Check for those errors.

In contrast to crypto/rand.Reader, a user-provided io.Reader,
might not fill the buffer without returning an error. Though marginal,
we should deal with that corner-case as well.
New version panic()s if reads from crypto/rand fail.
@armfazh armfazh merged commit f4c0e87 into main Mar 16, 2023
10 checks passed
@armfazh armfazh deleted the bas/rand-read-err branch March 16, 2023 17:41
@Lekensteyn
Copy link
Contributor

#466 asked about why this fix for CVE-2023-1732 was released so late. The reason is that these are not high-severity issues that affect real systems, despite the misleading CVE score. More details below:


We indeed missed checking the error for crypto/rand.Read, this affected all kem users:

  • kem/frodo/frodo640shake/frodo.go
  • kem/kyber/kyber1024/kyber.go
  • kem/kyber/kyber512/kyber.go
  • kem/kyber/kyber768/kyber.go
  • kem/kyber/templates/pkg.templ.go
  • kem/sike/sikep434/sike.go
  • kem/sike/sikep503/sike.go
  • kem/sike/sikep751/sike.go
  • kem/sike/templates/pkg.templ.go

Failing to read from this random number is quite unlikely. It does not happen in normal environments. On Linux, it could happen if someone blocks the getrandom syscall, and does not have /dev mounted, or blocks reads to /dev/urandom. These conditions result in a broken system.


All users of crypto/rand.Read (cryptoRand.Read) are not affected by partial reads. They either succeed, or fail (partial reads, etc.). This is the documentation and code from crypto/rand that never really changed since introduction:

// Read is a helper function that calls Reader.Read using io.ReadFull.
// On return, n == len(b) if and only if err == nil.
func Read(b []byte) (n int, err error) {
	return io.ReadFull(Reader, b)
}

The following might be affected depending on configuration:

  • abe/cpabe/tkn20/internal/tkn/bk.go - if you use (*tkn20.PublicKey).Encrypt from abe/cpabe/tkn20 with a nil rand io.Reader, then you are safe since it uses crypto/rand.Reader with the above semantics. The tests in abe/cpabe/tkn20/tkn20_test.go also show how to do this.
  • blindsign/blindrsa/blindrsa.go - if you use (blindrsa.RSAVerifier).Blind from blindrsa/blindrsa with a crypto/rand.Reader, you should be safe. The TestRoundTrip sample in blindsign/blindrsa/blindrsa_test.go shows how to do this.

The standard practice is to use crypto/rand.Read which does not suffer from the issue reported in CVE-2023-1732. This is also what is recommended through the examples in our test suite.

As you can see, only two users (abe/cpabe/tkn20 and blindrsa/blindrsa) are potentially affected by the theoretical issue.
If someone deliberately creates a io.Reader which returns a partial buffer, then it is a programming error. Someone could also deliberately create an io.Reader which returns all zeroes which would equally be insecure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants