Merge pull request #125 from binance-chain/develop

[R4R] prepare release v0.31.5-binance.3

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:

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
 			}

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

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 <nil>", 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)

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