From 5c9b5cfa1d3913dc4b17641b54e8ec835a089b4a Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 8 Oct 2019 12:52:28 -0500 Subject: [PATCH] p2p: only allow ed25519 pubkeys when connecting also, recover from any possible failures in acceptPeers Refs #4030 --- crypto/multisig/threshold_pubkey.go | 5 +++ p2p/conn/secret_connection.go | 13 ++++---- p2p/conn/secret_connection_test.go | 47 +++++++++++++++++++++++++++++ p2p/transport.go | 19 ++++++++++++ 4 files changed, 78 insertions(+), 6 deletions(-) diff --git a/crypto/multisig/threshold_pubkey.go b/crypto/multisig/threshold_pubkey.go index 234d420f1..36e2dc2dd 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} } diff --git a/p2p/conn/secret_connection.go b/p2p/conn/secret_connection.go index 0e10ba241..9d762cc53 100644 --- a/p2p/conn/secret_connection.go +++ b/p2p/conn/secret_connection.go @@ -6,20 +6,21 @@ import ( "crypto/sha256" "crypto/subtle" "encoding/binary" - "errors" "io" "math" "net" "sync" "time" + "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" "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 @@ -123,12 +124,12 @@ func MakeSecretConnection(conn io.ReadWriteCloser, locPrivKey crypto.PrivKey) (* remPubKey, remSignature := authSigMsg.Key, authSigMsg.Sig - if remPubKey == nil { - return nil, errors.New("Peer sent a nil public key") + 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. @@ -210,7 +211,7 @@ func (sc *SecretConnection) Read(data []byte) (n int, err error) { var frame = make([]byte, totalFrameSize) _, err = aead.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 7e264e913..3fd6b84fd 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 8d6ea236e..95c646ac0 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" ) @@ -270,6 +272,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