Fixes tendermint#3439

* make sure we create valid private keys:

 - genPrivKey samples and rejects invalid fieldelems (like libsecp256k1)
 - GenPrivKeySecp256k1 uses `(sha(secret) mod (n − 1)) + 1`
 - fix typo, rename test file: s/secpk256k1/secp256k1/

* Update crypto/secp256k1/secp256k1.go

crypto/secp256k1/secp256k1.go

@@ -6,6 +6,7 @@ import (
 	"crypto/subtle"
 	"fmt"
 	"io"
+	"math/big"
 
 	"golang.org/x/crypto/ripemd160"
 
@@ -65,32 +66,61 @@ func (privKey PrivKeySecp256k1) Equals(other crypto.PrivKey) bool {
 }
 
 // GenPrivKey generates a new ECDSA private key on curve secp256k1 private key.
-// It uses OS randomness in conjunction with the current global random seed
-// in tendermint/libs/common to generate the private key.
+// It uses OS randomness to generate the private key.
 func GenPrivKey() PrivKeySecp256k1 {
 	return genPrivKey(crypto.CReader())
 }
 
 // genPrivKey generates a new secp256k1 private key using the provided reader.
 func genPrivKey(rand io.Reader) PrivKeySecp256k1 {
-	privKeyBytes := [32]byte{}
-	_, err := io.ReadFull(rand, privKeyBytes[:])
-	if err != nil {
-		panic(err)
+	var privKeyBytes [32]byte
+	d := new(big.Int)
+	for {
+		privKeyBytes = [32]byte{}
+		_, err := io.ReadFull(rand, privKeyBytes[:])
+		if err != nil {
+			panic(err)
+		}
+
+		d.SetBytes(privKeyBytes[:])
+		// break if we found a valid point (i.e. > 0 and < N == curverOrder)
+		isValidFieldElement := 0 < d.Sign() && d.Cmp(secp256k1.S256().N) < 0
+		if isValidFieldElement {
+			break
+		}
 	}
-	// crypto.CRandBytes is guaranteed to be 32 bytes long, so it can be
-	// casted to PrivKeySecp256k1.
+
 	return PrivKeySecp256k1(privKeyBytes)
 }
 
+var one = new(big.Int).SetInt64(1)
+
 // GenPrivKeySecp256k1 hashes the secret with SHA2, and uses
 // that 32 byte output to create the private key.
+//
+// It makes sure the private key is a valid field element by setting:
+//
+// c = sha256(secret)
+// k = (c mod (n − 1)) + 1, where n = curve order.
+//
 // NOTE: secret should be the output of a KDF like bcrypt,
 // if it's derived from user input.
 func GenPrivKeySecp256k1(secret []byte) PrivKeySecp256k1 {
-	privKey32 := sha256.Sum256(secret)
-	// sha256.Sum256() is guaranteed to be 32 bytes long, so it can be
-	// casted to PrivKeySecp256k1.
+	secHash := sha256.Sum256(secret)
+	// to guarantee that we have a valid field element, we use the approach of:
+	// "Suite B Implementer’s Guide to FIPS 186-3", A.2.1
+	// https://apps.nsa.gov/iaarchive/library/ia-guidance/ia-solutions-for-classified/algorithm-guidance/suite-b-implementers-guide-to-fips-186-3-ecdsa.cfm
+	// see also https://github.com/golang/go/blob/0380c9ad38843d523d9c9804fe300cb7edd7cd3c/src/crypto/ecdsa/ecdsa.go#L89-L101
+	fe := new(big.Int).SetBytes(secHash[:])
+	n := new(big.Int).Sub(secp256k1.S256().N, one)
+	fe.Mod(fe, n)
+	fe.Add(fe, one)
+
+	feB := fe.Bytes()
+	var privKey32 [32]byte
+	// copy feB over to fixed 32 byte privKey32 and pad (if necessary)
+	copy(privKey32[32-len(feB):32], feB)
+
 	return PrivKeySecp256k1(privKey32)
 }
 

crypto/secp256k1/secp256k1_cgo_test.go

@@ -0,0 +1,39 @@
+// +build libsecp256k1
+
+package secp256k1
+
+import (
+	"github.com/magiconair/properties/assert"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestPrivKeySecp256k1SignVerify(t *testing.T) {
+	msg := []byte("A.1.2 ECC Key Pair Generation by Testing Candidates")
+	priv := GenPrivKey()
+	tests := []struct {
+		name             string
+		privKey          PrivKeySecp256k1
+		wantSignErr      bool
+		wantVerifyPasses bool
+	}{
+		{name: "valid sign-verify round", privKey: priv, wantSignErr: false, wantVerifyPasses: true},
+		{name: "invalid private key", privKey: [32]byte{}, wantSignErr: true, wantVerifyPasses: false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := tt.privKey.Sign(msg)
+			if tt.wantSignErr {
+				require.Error(t, err)
+				t.Logf("Got error: %s", err)
+				return
+			}
+			require.NoError(t, err)
+			require.NotNil(t, got)
+
+			pub := tt.privKey.PubKey()
+			assert.Equal(t, tt.wantVerifyPasses, pub.VerifyBytes(msg, got))
+		})
+	}
+}

crypto/secp256k1/secp256k1_internal_test.go

@@ -0,0 +1,45 @@
+package secp256k1
+
+import (
+	"bytes"
+	"math/big"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	underlyingSecp256k1 "github.com/btcsuite/btcd/btcec"
+)
+
+func Test_genPrivKey(t *testing.T) {
+
+	empty := make([]byte, 32)
+	oneB := big.NewInt(1).Bytes()
+	onePadded := make([]byte, 32)
+	copy(onePadded[32-len(oneB):32], oneB)
+	t.Logf("one padded: %v, len=%v", onePadded, len(onePadded))
+
+	validOne := append(empty, onePadded...)
+	tests := []struct {
+		name        string
+		notSoRand   []byte
+		shouldPanic bool
+	}{
+		{"empty bytes (panics because 1st 32 bytes are zero and 0 is not a valid field element)", empty, true},
+		{"curve order: N", underlyingSecp256k1.S256().N.Bytes(), true},
+		{"valid because 0 < 1 < N", validOne, false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.shouldPanic {
+				require.Panics(t, func() {
+					genPrivKey(bytes.NewReader(tt.notSoRand))
+				})
+				return
+			}
+			got := genPrivKey(bytes.NewReader(tt.notSoRand))
+			fe := new(big.Int).SetBytes(got[:])
+			require.True(t, fe.Cmp(underlyingSecp256k1.S256().N) < 0)
+			require.True(t, fe.Sign() > 0)
+		})
+	}
+}

crypto/secp256k1/secp256k1_nocgo_test.go

@@ -6,7 +6,6 @@ import (
 	"testing"
 
 	secp256k1 "github.com/btcsuite/btcd/btcec"
-
 	"github.com/stretchr/testify/require"
 )
 

crypto/secp256k1/secpk256k1_test.go → crypto/secp256k1/secp256k1_test.go

@@ -2,6 +2,7 @@ package secp256k1_test
 
 import (
 	"encoding/hex"
+	"math/big"
 	"testing"
 
 	"github.com/btcsuite/btcutil/base58"
@@ -84,3 +85,28 @@ func TestSecp256k1LoadPrivkeyAndSerializeIsIdentity(t *testing.T) {
 		require.Equal(t, privKeyBytes[:], serializedBytes)
 	}
 }
+
+func TestGenPrivKeySecp256k1(t *testing.T) {
+	// curve oder N
+	N := underlyingSecp256k1.S256().N
+	tests := []struct {
+		name   string
+		secret []byte
+	}{
+		{"empty secret", []byte{}},
+		{"some long secret", []byte("We live in a society exquisitely dependent on science and technology, in which hardly anyone knows anything about science and technology.")},
+		{"another seed used in cosmos tests #1", []byte{0}},
+		{"another seed used in cosmos tests #2", []byte("mySecret")},
+		{"another seed used in cosmos tests #3", []byte("")},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotPrivKey := secp256k1.GenPrivKeySecp256k1(tt.secret)
+			require.NotNil(t, gotPrivKey)
+			// interpret as a big.Int and make sure it is a valid field element:
+			fe := new(big.Int).SetBytes(gotPrivKey[:])
+			require.True(t, fe.Cmp(N) < 0)
+			require.True(t, fe.Sign() > 0)
+		})
+	}
+}