From 6485e68bebcbf4f91b1bb8fcc52e01e2c97ae6a4 Mon Sep 17 00:00:00 2001 From: Ismail Khoffi Date: Mon, 4 Feb 2019 09:24:54 +0100 Subject: [PATCH] Use ethereum's secp256k1 lib (#3234) * switch from fork (tendermint/btcd) to orig package (btcsuite/btcd); also - remove obsolete check in test `size != -1` is always true - WIP as the serialization still needs to be wrapped * WIP: wrap signature & privkey, pubkey needs to be wrapped as well * wrap pubkey too * use "github.com/ethereum/go-ethereum/crypto/secp256k1" if cgo is available, else use "github.com/btcsuite/btcd/btcec" and take care of lower-S when verifying Annoyingly, had to disable pruning when importing github.com/ethereum/go-ethereum/ :-/ * update comment * update comment * emulate signature_nocgo.go for additional benchmarks: https://github.com/ethereum/go-ethereum/blob/592bf6a59cac9697f0491b24e5093cb759d7e44c/crypto/signature_nocgo.go#L60-L76 * use our format (r || s) in lower-s form when in the non-cgo case * remove comment about using the C library directly * vendor github.com/btcsuite/btcd too * Add test for the !cgo case * update changelog pending Closes #3162 #3163 Refs #1958, #2091, tendermint/btcd#1 --- CHANGELOG_PENDING.md | 2 + Gopkg.lock | 24 ++++---- Gopkg.toml | 16 +++++- crypto/encoding/amino/encode_test.go | 5 +- crypto/secp256k1/secp256k1.go | 30 ++-------- crypto/secp256k1/secp256k1_cgo.go | 24 ++++++++ crypto/secp256k1/secp256k1_nocgo.go | 71 ++++++++++++++++++++++++ crypto/secp256k1/secp256k1_nocgo_test.go | 39 +++++++++++++ crypto/secp256k1/secpk256k1_test.go | 2 +- 9 files changed, 169 insertions(+), 44 deletions(-) create mode 100644 crypto/secp256k1/secp256k1_cgo.go create mode 100644 crypto/secp256k1/secp256k1_nocgo.go create mode 100644 crypto/secp256k1/secp256k1_nocgo_test.go diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index ac4b16284..b66cd4e8e 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -20,6 +20,8 @@ Special thanks to external contributors on this release: ### IMPROVEMENTS: - [tools] add go-deadlock tool to help detect deadlocks +- [crypto] \#3163 use ethereum's libsecp256k1 go-wrapper for signatures when cgo is available +- [crypto] \#3162 wrap btcd instead of forking it to keep up with fixes (used if cgo is not available) ### BUG FIXES: - [node] \#3186 EventBus and indexerService should be started before first block (for replay last block on handshake) execution diff --git a/Gopkg.lock b/Gopkg.lock index 9880f3f39..9e3d5d8a4 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -10,12 +10,11 @@ revision = "3a771d992973f24aa725d07868b467d1ddfceafb" [[projects]] - branch = "master" - digest = "1:c0decf632843204d2b8781de7b26e7038584e2dcccc7e2f401e88ae85b1df2b7" + digest = "1:093bf93a65962e8191e3e8cd8fc6c363f83d43caca9739c906531ba7210a9904" name = "github.com/btcsuite/btcd" packages = ["btcec"] pruneopts = "UT" - revision = "67e573d211ace594f1366b4ce9d39726c4b19bd0" + revision = "ed77733ec07dfc8a513741138419b8d9d3de9d2d" [[projects]] digest = "1:1d8e1cb71c33a9470bbbae09bfec09db43c6bf358dfcae13cd8807c4e2a9a2bf" @@ -35,6 +34,14 @@ revision = "8991bc29aa16c548c550c7ff78260e27b9ab7c73" version = "v1.1.1" +[[projects]] + digest = "1:b42be5a3601f833e0b9f2d6625d887ec1309764bfcac3d518f3db425dcd4ec5c" + name = "github.com/ethereum/go-ethereum" + packages = ["crypto/secp256k1"] + pruneopts = "T" + revision = "9dc5d1a915ac0e0bd8429d6ac41df50eec91de5f" + version = "v1.8.21" + [[projects]] digest = "1:544229a3ca0fb2dd5ebc2896d3d2ff7ce096d9751635301e44e37e761349ee70" name = "github.com/fortytw2/leaktest" @@ -360,14 +367,6 @@ pruneopts = "UT" revision = "6b91fda63f2e36186f1c9d0e48578defb69c5d43" -[[projects]] - digest = "1:83f5e189eea2baad419a6a410984514266ff690075759c87e9ede596809bd0b8" - name = "github.com/tendermint/btcd" - packages = ["btcec"] - pruneopts = "UT" - revision = "80daadac05d1cd29571fccf27002d79667a88b58" - version = "v0.1.1" - [[projects]] digest = "1:ad9c4c1a4e7875330b1f62906f2830f043a23edb5db997e3a5ac5d3e6eadf80a" name = "github.com/tendermint/go-amino" @@ -504,8 +503,10 @@ analyzer-name = "dep" analyzer-version = 1 input-imports = [ + "github.com/btcsuite/btcd/btcec", "github.com/btcsuite/btcutil/base58", "github.com/btcsuite/btcutil/bech32", + "github.com/ethereum/go-ethereum/crypto/secp256k1", "github.com/fortytw2/leaktest", "github.com/go-kit/kit/log", "github.com/go-kit/kit/log/level", @@ -535,7 +536,6 @@ "github.com/syndtr/goleveldb/leveldb/errors", "github.com/syndtr/goleveldb/leveldb/iterator", "github.com/syndtr/goleveldb/leveldb/opt", - "github.com/tendermint/btcd/btcec", "github.com/tendermint/go-amino", "golang.org/x/crypto/bcrypt", "golang.org/x/crypto/chacha20poly1305", diff --git a/Gopkg.toml b/Gopkg.toml index 72ec66594..e5d6b8da2 100644 --- a/Gopkg.toml +++ b/Gopkg.toml @@ -75,14 +75,26 @@ name = "github.com/prometheus/client_golang" version = "^0.9.1" +# we use the secp256k1 implementation: [[constraint]] - name = "github.com/tendermint/btcd" - version = "v0.1.1" + name = "github.com/ethereum/go-ethereum" + version = "^v1.8.21" + + # Prevent dep from pruning build scripts and codegen templates + # note: this leaves the whole go-ethereum package in vendor + # can be removed when https://github.com/golang/dep/issues/1847 is resolved + [[prune.project]] + name = "github.com/ethereum/go-ethereum" + unused-packages = false ################################### ## Some repos dont have releases. ## Pin to revision +[[constraint]] + name = "github.com/btcsuite/btcd" + revision = "ed77733ec07dfc8a513741138419b8d9d3de9d2d" + [[constraint]] name = "golang.org/x/crypto" revision = "505ab145d0a99da450461ae2c1a9f6cd10d1f447" diff --git a/crypto/encoding/amino/encode_test.go b/crypto/encoding/amino/encode_test.go index c8cb24136..955103069 100644 --- a/crypto/encoding/amino/encode_test.go +++ b/crypto/encoding/amino/encode_test.go @@ -25,9 +25,8 @@ func checkAminoBinary(t *testing.T, src, dst interface{}, size int) { assert.Equal(t, byterSrc.Bytes(), bz, "Amino binary vs Bytes() mismatch") } // Make sure we have the expected length. - if size != -1 { - assert.Equal(t, size, len(bz), "Amino binary size mismatch") - } + assert.Equal(t, size, len(bz), "Amino binary size mismatch") + // Unmarshal. err = cdc.UnmarshalBinaryBare(bz, dst) require.Nil(t, err, "%+v", err) diff --git a/crypto/secp256k1/secp256k1.go b/crypto/secp256k1/secp256k1.go index d3528fdd6..78857c45c 100644 --- a/crypto/secp256k1/secp256k1.go +++ b/crypto/secp256k1/secp256k1.go @@ -7,10 +7,12 @@ import ( "fmt" "io" - secp256k1 "github.com/tendermint/btcd/btcec" - amino "github.com/tendermint/go-amino" "golang.org/x/crypto/ripemd160" + secp256k1 "github.com/btcsuite/btcd/btcec" + + amino "github.com/tendermint/go-amino" + "github.com/tendermint/tendermint/crypto" ) @@ -44,16 +46,6 @@ func (privKey PrivKeySecp256k1) Bytes() []byte { return cdc.MustMarshalBinaryBare(privKey) } -// Sign creates an ECDSA signature on curve Secp256k1, using SHA256 on the msg. -func (privKey PrivKeySecp256k1) Sign(msg []byte) ([]byte, error) { - priv, _ := secp256k1.PrivKeyFromBytes(secp256k1.S256(), privKey[:]) - sig, err := priv.Sign(crypto.Sha256(msg)) - if err != nil { - return nil, err - } - return sig.Serialize(), nil -} - // PubKey performs the point-scalar multiplication from the privKey on the // generator point to get the pubkey. func (privKey PrivKeySecp256k1) PubKey() crypto.PubKey { @@ -137,20 +129,6 @@ func (pubKey PubKeySecp256k1) Bytes() []byte { return bz } -func (pubKey PubKeySecp256k1) VerifyBytes(msg []byte, sig []byte) bool { - pub, err := secp256k1.ParsePubKey(pubKey[:], secp256k1.S256()) - if err != nil { - return false - } - parsedSig, err := secp256k1.ParseSignature(sig[:], secp256k1.S256()) - if err != nil { - return false - } - // Underlying library ensures that this signature is in canonical form, to - // prevent Secp256k1 malleability from altering the sign of the s term. - return parsedSig.Verify(crypto.Sha256(msg), pub) -} - func (pubKey PubKeySecp256k1) String() string { return fmt.Sprintf("PubKeySecp256k1{%X}", pubKey[:]) } diff --git a/crypto/secp256k1/secp256k1_cgo.go b/crypto/secp256k1/secp256k1_cgo.go new file mode 100644 index 000000000..30414d2b7 --- /dev/null +++ b/crypto/secp256k1/secp256k1_cgo.go @@ -0,0 +1,24 @@ +// +build cgo + +package secp256k1 + +import ( + "github.com/ethereum/go-ethereum/crypto/secp256k1" + + "github.com/tendermint/tendermint/crypto" +) + +// Sign creates an ECDSA signature on curve Secp256k1, using SHA256 on the msg. +func (privKey PrivKeySecp256k1) Sign(msg []byte) ([]byte, error) { + rsv, err := secp256k1.Sign(crypto.Sha256(msg), privKey[:]) + if err != nil { + return nil, err + } + // we do not need v in r||s||v: + rs := rsv[:len(rsv)-1] + return rs, nil +} + +func (pubKey PubKeySecp256k1) VerifyBytes(msg []byte, sig []byte) bool { + return secp256k1.VerifySignature(pubKey[:], crypto.Sha256(msg), sig) +} diff --git a/crypto/secp256k1/secp256k1_nocgo.go b/crypto/secp256k1/secp256k1_nocgo.go new file mode 100644 index 000000000..34b006faa --- /dev/null +++ b/crypto/secp256k1/secp256k1_nocgo.go @@ -0,0 +1,71 @@ +// +build !cgo + +package secp256k1 + +import ( + "math/big" + + secp256k1 "github.com/btcsuite/btcd/btcec" + + "github.com/tendermint/tendermint/crypto" +) + +// used to reject malleable signatures +// see: +// - https://github.com/ethereum/go-ethereum/blob/f9401ae011ddf7f8d2d95020b7446c17f8d98dc1/crypto/signature_nocgo.go#L90-L93 +// - https://github.com/ethereum/go-ethereum/blob/f9401ae011ddf7f8d2d95020b7446c17f8d98dc1/crypto/crypto.go#L39 +var secp256k1N, _ = new(big.Int).SetString("fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141", 16) +var secp256k1halfN = new(big.Int).Div(secp256k1N, big.NewInt(2)) + +// Sign creates an ECDSA signature on curve Secp256k1, using SHA256 on the msg. +// The returned signature will be of the form R || S (in lower-S form). +func (privKey PrivKeySecp256k1) Sign(msg []byte) ([]byte, error) { + priv, _ := secp256k1.PrivKeyFromBytes(secp256k1.S256(), privKey[:]) + sig, err := priv.Sign(crypto.Sha256(msg)) + if err != nil { + return nil, err + } + sigBytes := serializeSig(sig) + return sigBytes, nil +} + +// VerifyBytes verifies a signature of the form R || S. +// It rejects signatures which are not in lower-S form. +func (pubKey PubKeySecp256k1) VerifyBytes(msg []byte, sigStr []byte) bool { + if len(sigStr) != 64 { + return false + } + pub, err := secp256k1.ParsePubKey(pubKey[:], secp256k1.S256()) + if err != nil { + return false + } + // parse the signature: + signature := signatureFromBytes(sigStr) + // Reject malleable signatures. libsecp256k1 does this check but btcec doesn't. + // see: https://github.com/ethereum/go-ethereum/blob/f9401ae011ddf7f8d2d95020b7446c17f8d98dc1/crypto/signature_nocgo.go#L90-L93 + if signature.S.Cmp(secp256k1halfN) > 0 { + return false + } + return signature.Verify(crypto.Sha256(msg), pub) +} + +// Read Signature struct from R || S. Caller needs to ensure +// that len(sigStr) == 64. +func signatureFromBytes(sigStr []byte) *secp256k1.Signature { + return &secp256k1.Signature{ + new(big.Int).SetBytes(sigStr[:32]), + new(big.Int).SetBytes(sigStr[32:64]), + } +} + +// Serialize signature to R || S. +// R, S are padded to 32 bytes respectively. +func serializeSig(sig *secp256k1.Signature) []byte { + rBytes := sig.R.Bytes() + sBytes := sig.S.Bytes() + sigBytes := make([]byte, 64) + // 0 pad the byte arrays from the left if they aren't big enough. + copy(sigBytes[32-len(rBytes):32], rBytes) + copy(sigBytes[64-len(sBytes):64], sBytes) + return sigBytes +} diff --git a/crypto/secp256k1/secp256k1_nocgo_test.go b/crypto/secp256k1/secp256k1_nocgo_test.go new file mode 100644 index 000000000..95966478b --- /dev/null +++ b/crypto/secp256k1/secp256k1_nocgo_test.go @@ -0,0 +1,39 @@ +// +build !cgo + +package secp256k1 + +import ( + "testing" + + secp256k1 "github.com/btcsuite/btcd/btcec" + + "github.com/stretchr/testify/require" +) + +// Ensure that signature verification works, and that +// non-canonical signatures fail. +// Note: run with CGO_ENABLED=0 or go test -tags !cgo. +func TestSignatureVerificationAndRejectUpperS(t *testing.T) { + msg := []byte("We have lingered long enough on the shores of the cosmic ocean.") + for i := 0; i < 500; i++ { + priv := GenPrivKey() + sigStr, err := priv.Sign(msg) + require.NoError(t, err) + sig := signatureFromBytes(sigStr) + require.False(t, sig.S.Cmp(secp256k1halfN) > 0) + + pub := priv.PubKey() + require.True(t, pub.VerifyBytes(msg, sigStr)) + + // malleate: + sig.S.Sub(secp256k1.S256().CurveParams.N, sig.S) + require.True(t, sig.S.Cmp(secp256k1halfN) > 0) + malSigStr := serializeSig(sig) + + require.False(t, pub.VerifyBytes(msg, malSigStr), + "VerifyBytes incorrect with malleated & invalid S. sig=%v, key=%v", + sig, + priv, + ) + } +} diff --git a/crypto/secp256k1/secpk256k1_test.go b/crypto/secp256k1/secpk256k1_test.go index 2fa483014..0f0b5adce 100644 --- a/crypto/secp256k1/secpk256k1_test.go +++ b/crypto/secp256k1/secpk256k1_test.go @@ -11,7 +11,7 @@ import ( "github.com/tendermint/tendermint/crypto" "github.com/tendermint/tendermint/crypto/secp256k1" - underlyingSecp256k1 "github.com/tendermint/btcd/btcec" + underlyingSecp256k1 "github.com/btcsuite/btcd/btcec" ) type keyData struct {