cloudflare / circl

CIRCL: Cloudflare Interoperable Reusable Cryptographic Library
http://blog.cloudflare.com/introducing-circl
Other
1.22k stars 136 forks source link

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

Closed bwesterb closed 1 year ago

bwesterb commented 1 year ago

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.

Lekensteyn commented 7 months ago

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:

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:

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.