From dbc94af5cac218de28c15d910f679cbb3f69e460 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 8 Oct 2019 12:52:28 -0500 Subject: [PATCH 1/2] p2p: only allow ed25519 pubkeys when connecting, also recover from any possible failures in acceptPeers --- crypto/multisig/threshold_pubkey.go | 8 +++++ p2p/conn/secret_connection.go | 18 ++++++----- p2p/conn/secret_connection_test.go | 47 +++++++++++++++++++++++++++++ p2p/transport.go | 19 ++++++++++++ 4 files changed, 85 insertions(+), 7 deletions(-) diff --git a/crypto/multisig/threshold_pubkey.go b/crypto/multisig/threshold_pubkey.go index 234d420f1..5ed7651cb 100644 --- a/crypto/multisig/threshold_pubkey.go +++ b/crypto/multisig/threshold_pubkey.go @@ -21,6 +21,11 @@ func NewPubKeyMultisigThreshold(k int, pubkeys []crypto.PubKey) crypto.PubKey { if len(pubkeys) < k { panic("threshold k of n multisignature: len(pubkeys) < k") } + for _, pubkey := range pubkeys { + if pubkey == nil { + panic("nil pubkey") + } + } return PubKeyMultisigThreshold{uint(k), pubkeys} } @@ -53,6 +58,9 @@ func (pk PubKeyMultisigThreshold) VerifyBytes(msg []byte, marshalledSig []byte) sigIndex := 0 for i := 0; i < size; i++ { if sig.BitArray.GetIndex(i) { + if pk.PubKeys[i] == nil { + return false + } if !pk.PubKeys[i].VerifyBytes(msg, sig.Sigs[sigIndex]) { return false } diff --git a/p2p/conn/secret_connection.go b/p2p/conn/secret_connection.go index 77aea8f02..f8e784bbc 100644 --- a/p2p/conn/secret_connection.go +++ b/p2p/conn/secret_connection.go @@ -7,20 +7,21 @@ import ( "crypto/sha256" "crypto/subtle" "encoding/binary" - "errors" "io" "net" "sync" "time" + pool "github.com/libp2p/go-buffer-pool" + "github.com/pkg/errors" "golang.org/x/crypto/chacha20poly1305" "golang.org/x/crypto/curve25519" + "golang.org/x/crypto/hkdf" "golang.org/x/crypto/nacl/box" - pool "github.com/libp2p/go-buffer-pool" "github.com/tendermint/tendermint/crypto" + "github.com/tendermint/tendermint/crypto/ed25519" cmn "github.com/tendermint/tendermint/libs/common" - "golang.org/x/crypto/hkdf" ) // 4 + 1024 == 1028 total frame size @@ -106,11 +107,11 @@ func MakeSecretConnection(conn io.ReadWriteCloser, locPrivKey crypto.PrivKey) (* sendAead, err := chacha20poly1305.New(sendSecret[:]) if err != nil { - return nil, errors.New("Invalid send SecretConnection Key") + return nil, errors.New("invalid send SecretConnection Key") } recvAead, err := chacha20poly1305.New(recvSecret[:]) if err != nil { - return nil, errors.New("Invalid receive SecretConnection Key") + return nil, errors.New("invalid receive SecretConnection Key") } // Construct SecretConnection. sc := &SecretConnection{ @@ -132,8 +133,11 @@ func MakeSecretConnection(conn io.ReadWriteCloser, locPrivKey crypto.PrivKey) (* } remPubKey, remSignature := authSigMsg.Key, authSigMsg.Sig + if _, ok := remPubKey.(ed25519.PubKeyEd25519); !ok { + return nil, errors.Errorf("expected ed25519 pubkey, got %T", remPubKey) + } if !remPubKey.VerifyBytes(challenge[:], remSignature) { - return nil, errors.New("Challenge verification failed") + return nil, errors.New("challenge verification failed") } // We've authorized. @@ -214,7 +218,7 @@ func (sc *SecretConnection) Read(data []byte) (n int, err error) { defer pool.Put(frame) _, err = sc.recvAead.Open(frame[:0], sc.recvNonce[:], sealedFrame, nil) if err != nil { - return n, errors.New("Failed to decrypt SecretConnection") + return n, errors.New("failed to decrypt SecretConnection") } incrNonce(sc.recvNonce) // end decryption diff --git a/p2p/conn/secret_connection_test.go b/p2p/conn/secret_connection_test.go index 76982ed97..648e6529f 100644 --- a/p2p/conn/secret_connection_test.go +++ b/p2p/conn/secret_connection_test.go @@ -17,7 +17,9 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/tendermint/tendermint/crypto" "github.com/tendermint/tendermint/crypto/ed25519" + "github.com/tendermint/tendermint/crypto/secp256k1" cmn "github.com/tendermint/tendermint/libs/common" ) @@ -363,6 +365,51 @@ func TestDeriveSecretsAndChallengeGolden(t *testing.T) { } } +type privKeyWithNilPubKey struct { + orig crypto.PrivKey +} + +func (pk privKeyWithNilPubKey) Bytes() []byte { return pk.orig.Bytes() } +func (pk privKeyWithNilPubKey) Sign(msg []byte) ([]byte, error) { return pk.orig.Sign(msg) } +func (pk privKeyWithNilPubKey) PubKey() crypto.PubKey { return nil } +func (pk privKeyWithNilPubKey) Equals(pk2 crypto.PrivKey) bool { return pk.orig.Equals(pk2) } + +func TestNilPubkey(t *testing.T) { + var fooConn, barConn = makeKVStoreConnPair() + var fooPrvKey = ed25519.GenPrivKey() + var barPrvKey = privKeyWithNilPubKey{ed25519.GenPrivKey()} + + go func() { + _, err := MakeSecretConnection(barConn, barPrvKey) + assert.NoError(t, err) + }() + + assert.NotPanics(t, func() { + _, err := MakeSecretConnection(fooConn, fooPrvKey) + if assert.Error(t, err) { + assert.Equal(t, "expected ed25519 pubkey, got ", err.Error()) + } + }) +} + +func TestNonEd25519Pubkey(t *testing.T) { + var fooConn, barConn = makeKVStoreConnPair() + var fooPrvKey = ed25519.GenPrivKey() + var barPrvKey = secp256k1.GenPrivKey() + + go func() { + _, err := MakeSecretConnection(barConn, barPrvKey) + assert.NoError(t, err) + }() + + assert.NotPanics(t, func() { + _, err := MakeSecretConnection(fooConn, fooPrvKey) + if assert.Error(t, err) { + assert.Equal(t, "expected ed25519 pubkey, got secp256k1.PubKeySecp256k1", err.Error()) + } + }) +} + // Creates the data for a test vector file. // The file format is: // Hex(diffie_hellman_secret), loc_is_least, Hex(recvSecret), Hex(sendSecret), Hex(challenge) diff --git a/p2p/transport.go b/p2p/transport.go index 6c4503ebd..fd5674b85 100644 --- a/p2p/transport.go +++ b/p2p/transport.go @@ -6,6 +6,8 @@ import ( "net" "time" + "github.com/pkg/errors" + "github.com/tendermint/tendermint/crypto" "github.com/tendermint/tendermint/p2p/conn" ) @@ -266,6 +268,23 @@ func (mt *MultiplexTransport) acceptPeers() { // // [0] https://en.wikipedia.org/wiki/Head-of-line_blocking go func(c net.Conn) { + defer func() { + if r := recover(); r != nil { + err := ErrRejected{ + conn: c, + err: errors.Errorf("recovered from panic: %v", r), + isAuthFailure: true, + } + select { + case mt.acceptc <- accept{err: err}: + case <-mt.closec: + // Give up if the transport was closed. + _ = c.Close() + return + } + } + }() + var ( nodeInfo NodeInfo secretConn *conn.SecretConnection From ab4f4164ccede8bb6bc11a66b30d7918359d77bb Mon Sep 17 00:00:00 2001 From: HaoyangLiu Date: Sat, 12 Oct 2019 17:25:41 +0800 Subject: [PATCH 2/2] Add change log --- CHANGELOG.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8901ea083..195b44da7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,20 @@ # Changelog +## v0.31.5-binance.3 +*Oct 12th, 2019* +### Bugfix: + +The previous patch was insufficient because the attacker could still find a way +to submit a `nil` pubkey by constructing a `PubKeyMultisigThreshold` pubkey +with `nil` subpubkeys for example. + +This release provides multiple fixes, which include recovering from panics when +accepting new peers and only allowing `ed25519` pubkeys. + +### SECURITY: + +- [p2p] [\#4030](https://github.com/tendermint/tendermint/issues/4030) Only allow ed25519 pubkeys when connecting + ## v0.31.5-binance.2 *Sep 6th, 2019* ### FEATURES: