crypto: Private keys are not guaranteed to be valid #3439
Description
We currently generate private keys via reading 32 bytes from a random source:
tendermint/crypto/secp256k1/secp256k1.go
Lines 75 to 84 in f571ee8
As far as I can see, we do not make sure that those 32 bytes are guaranteed to be a valid secp256k1 private key (see for instance https://github.com/golang/go/blob/7ccd3583eddcd79679fb29cfc83a6e6fb6973f1e/src/crypto/ecdsa/ecdsa.go#L89-L100 how this can be ensured).
Depending on the underlying library signing, or, signature verification might fail if the key is 32 bytes which does not represent an integer which is > 0 and smaller than the curve's order of the basepoint.
Here is a test that (passes but) explains the problem in golang:
// crypto/secp256k1/secp256k1_nocgo_test.go
func TestSignWithInvalidKeys(t *testing.T) {
msg := []byte("Suite B Implementer’s Guide to FIPS 186-3")
// this is probably prevented by crypto/rand? (didn't verify):
zeros := PrivKeySecp256k1([32]byte{})
sigStr, err := zeros.Sign(msg)
assert.NoError(t, err)
pub := zeros.PubKey()
// signature verification will fail but nothing obvious prevents
// GenPrivKey to create an all-zero key ...
require.False(t, pub.VerifyBytes(msg, sigStr))
var n [32]byte
nb := secp256k1.S256().N.Bytes()
copy(n[:], nb)
N := PrivKeySecp256k1(n)
sigStr, err = N.Sign(msg)
assert.NoError(t, err)
pub = N.PubKey()
// again: signature verification will fail but nothing prevents
// GenPrivKey to randomnly create a key that represents N
// (valid keys are < N):
require.False(t, pub.VerifyBytes(msg, sigStr))
one := new(big.Int).SetInt64(1)
n1b := secp256k1.S256().N.Add(secp256k1.S256().N, one).Bytes()
var nPlusOne [32]byte
copy(nPlusOne[:], n1b)
NplusOne := PrivKeySecp256k1(nPlusOne)
// ouch, this one even panics
// (nothing prevents GenPrivateKey to generate N+1)
assert.Panics(t, func() { NplusOne.Sign(msg) })
}Note: these cases are very unlikely to happen but we should still make sure we only generate valid keys (as far as I can see we never did).
cc @ValarDragon