diff --git a/CHANGELOG.md b/CHANGELOG.md index 971d5fa2c..6d34ae06e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,28 @@ # Changelog +## v0.32.6 + +*October 8, 2019* + +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. + +**All clients are recommended to upgrade** + +Special thanks to [fudongbai](https://hackerone.com/fudongbai) for pointing +this out. + +Friendly reminder, we have a [bug bounty +program](https://hackerone.com/tendermint). + +### SECURITY: + +- [p2p] [\#4030](https://github.com/tendermint/tendermint/issues/4030) Only allow ed25519 pubkeys when connecting + ## v0.32.5 *October 1, 2019* @@ -223,6 +246,29 @@ program](https://hackerone.com/tendermint). - [node] [\#3716](https://github.com/tendermint/tendermint/issues/3716) Fix a bug where `nil` is recorded as node's address - [node] [\#3741](https://github.com/tendermint/tendermint/issues/3741) Fix profiler blocking the entire node +## v0.31.10 + +*October 8, 2019* + +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. + +**All clients are recommended to upgrade** + +Special thanks to [fudongbai](https://hackerone.com/fudongbai) for pointing +this out. + +Friendly reminder, we have a [bug bounty +program](https://hackerone.com/tendermint). + +### SECURITY: + +- [p2p] [\#4030](https://github.com/tendermint/tendermint/issues/4030) Only allow ed25519 pubkeys when connecting + ## v0.31.9 *October 1, 2019* diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index bf2cc5b3e..f4a10102f 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -1,4 +1,4 @@ -## v0.32.6 +## v0.32.7 \*\* 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 f57e589e6..088c81252 100644 --- a/p2p/conn/secret_connection.go +++ b/p2p/conn/secret_connection.go @@ -7,21 +7,22 @@ import ( "crypto/sha256" "crypto/subtle" "encoding/binary" - "errors" "io" "math" "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 @@ -107,11 +108,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{ @@ -134,12 +135,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. @@ -222,7 +223,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 743fcf4c4..0438ec259 100644 --- a/p2p/conn/secret_connection_test.go +++ b/p2p/conn/secret_connection_test.go @@ -16,7 +16,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" ) @@ -365,6 +367,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 diff --git a/version/version.go b/version/version.go index b31eb8895..42ec287ab 100644 --- a/version/version.go +++ b/version/version.go @@ -20,7 +20,7 @@ const ( // Must be a string because scripts like dist.sh read this file. // XXX: Don't change the name of this variable or you will break // automation :) - TMCoreSemVer = "0.32.5" + TMCoreSemVer = "0.32.6" // ABCISemVer is the semantic version of the ABCI library ABCISemVer = "0.16.1"