-
Notifications
You must be signed in to change notification settings - Fork 18.3k
Description
What version of Go are you using (go version
)?
go version go1.13.4 darwin/amd64
Does this issue reproduce with the latest release?
Yes
What did you do?
Provide non-random input to rsa.GenerateKey (ie: A passkey phrase repeatable generated key algorithm)
salt := "ABCDEF" + userEnteredCredential
b := []byte(salt)
// Dummy key stretch
maxLen := 4096
b = append(b, make([]byte, maxLen-len(b))...)
for i := len(salt); i < maxLen; i++ {
b[i] = []byte(strconv.Itoa(i % maxLen))[0]
}
sr := bytes.NewReader(b)
privateKey, err := rsa.GenerateKey(sr, 128)
What did you expect to see?
Based on the rsa algorithm definition and the input: The same key for the same input.
What did you see instead?
2 different keys.
Root cause analysis:
The package https://golang.org/src/crypto/rsa/rsa.go contains the following lines:
func GenerateMultiPrimeKey(random io.Reader, nprimes int, bits int) (*PrivateKey, error) {
randutil.MaybeReadByte(random)
The line
randutil.MaybeReadByte(random)
actually flips a coin (bit) and can result in a 0 or 1.
When combined with random input, this behaviour would random (but the input is already random),
When combined with deterministic input, the algorithm becomes useless.
Suggestion is to remove the line randutil.MaybeReadByte(random)
and leave the randomness to the input only.
Related issue #29290 mentions the reason in release 1.11.2 where tests were the root issue to change this (the here proposed to be removed line, was added). However a standard behaviour for the algorithm is I think better, and fixing the test to be such that it can rely on deterministic behaviour is probably better.
Also this "fix" is such that a test still has a 50% chance of just getting the same key on the second request, thus making the test reliability not per definition better (just more confusing).