p2p: make SecretConnection non-malleable (#3668)

## Issue:

This is an approach to fixing secret connection that is more noise-ish than actually noise.

but it essentially fixes the problem that #3315 is trying to solve by making the secret connection handshake non-malleable. It's easy to understand and I think will be acceptable to @jaekwon

.. the formal reasoning is basically, if the "view" of the transcript between diverges between the sender and the receiver at any point in the protocol, the handshake would terminate.

The base protocol of Station to Station mistakenly assumes that if the sender and receiver arrive at shared secret they have the same view. This is only true for a DH on prime order groups.

This robustly solves the problem by having each cryptographic operation commit to operators view of the protocol.

Another nice thing about a transcript is it provides the basis for "secure" (barring cryptographic breakages, horrible design flaws, or implementation bugs) downgrades, where a backwards compatible handshake can be used to offer newer protocol features/extensions, peers agree to the common subset of what they support, and both sides have to agree on what the other offered for the transcript MAC to verify.

With something like Protos/Amino you already get "extensions" for free (TLS uses a simple TLV format https://tools.ietf.org/html/rfc8446#section-4.2 for extensions not too far off from Protos/Amino), so as long as you cryptographically commit to what they contain in the transcript, it should be possible to extend the protocol in a backwards-compatible manner.

## Commits:

* Minimal changes to remove malleability of secret connection removes the need to check for lower order points.

Breaks compatibility. Secret connections that have no been updated will fail

* Remove the redundant blacklist

* remove remainders of blacklist in tests to make the code compile again

Signed-off-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>

* Apply suggestions from code review

Apply Ismail's error handling

Co-Authored-By: Ismail Khoffi <Ismail.Khoffi@gmail.com>

* fix error check for io.ReadFull

Signed-off-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>

* Update p2p/conn/secret_connection.go

Co-Authored-By: Ismail Khoffi <Ismail.Khoffi@gmail.com>

* Update p2p/conn/secret_connection.go

Co-Authored-By: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com>

* update changelog and format the code

* move hkdfInit closer to where it's used

CHANGELOG_PENDING.md

@@ -32,6 +32,9 @@ program](https://hackerone.com/tendermint).
 - Go API
   - [libs/pubsub] [\#4070](https://github.com/tendermint/tendermint/pull/4070) `Query#(Matches|Conditions)` returns an error.
 
+- P2P Protocol
+  - [p2p] [\3668](https://github.com/tendermint/tendermint/pull/3668) Make `SecretConnection` non-malleable
+
 ### FEATURES:
 
 ### IMPROVEMENTS:

p2p/conn/secret_connection.go

@@ -77,24 +77,40 @@ type SecretConnection struct {
 // Caller should call conn.Close()
 // See docs/sts-final.pdf for more information.
 func MakeSecretConnection(conn io.ReadWriteCloser, locPrivKey crypto.PrivKey) (*SecretConnection, error) {
-	locPubKey := locPrivKey.PubKey()
+	var (
+		hash      = sha256.New
+		hdkfState = new([32]byte)
+		locPubKey = locPrivKey.PubKey()
+	)
 
 	// Generate ephemeral keys for perfect forward secrecy.
 	locEphPub, locEphPriv := genEphKeys()
 
 	// Write local ephemeral pubkey and receive one too.
-	// NOTE: every 32-byte string is accepted as a Curve25519 public key
-	// (see DJB's Curve25519 paper: http://cr.yp.to/ecdh/curve25519-20060209.pdf)
+	// NOTE: every 32-byte string is accepted as a Curve25519 public key (see
+	// DJB's Curve25519 paper: http://cr.yp.to/ecdh/curve25519-20060209.pdf)
 	remEphPub, err := shareEphPubKey(conn, locEphPub)
 	if err != nil {
 		return nil, err
 	}
 
 	// Sort by lexical order.
-	loEphPub, _ := sort32(locEphPub, remEphPub)
+	loEphPub, hiEphPub := sort32(locEphPub, remEphPub)
+
+	hkdfInit := hkdf.New(hash, []byte("INIT_HANDSHAKE"), nil, []byte("TENDERMINT_SECRET_CONNECTION_TRANSCRIPT_HASH"))
+	if _, err := io.ReadFull(hkdfInit, hdkfState[:]); err != nil {
+		return nil, err
+	}
+
+	hkdfLoEphPub := hkdf.New(hash, loEphPub[:], hdkfState[:], []byte("TENDERMINT_SECRET_CONNECTION_TRANSCRIPT_HASH"))
+	if _, err := io.ReadFull(hkdfLoEphPub, hdkfState[:]); err != nil {
+		return nil, err
+	}
+
+	hkdfHiEphPub := hkdf.New(hash, hiEphPub[:], hdkfState[:], []byte("TENDERMINT_SECRET_CONNECTION_TRANSCRIPT_HASH"))
 
-	// Check if the local ephemeral public key
-	// was the least, lexicographically sorted.
+	// Check if the local ephemeral public key was the least, lexicographically
+	// sorted.
 	locIsLeast := bytes.Equal(locEphPub[:], loEphPub[:])
 
 	// Compute common diffie hellman secret using X25519.
@@ -102,9 +118,19 @@ func MakeSecretConnection(conn io.ReadWriteCloser, locPrivKey crypto.PrivKey) (*
 	if err != nil {
 		return nil, err
 	}
+	if _, err := io.ReadFull(hkdfHiEphPub, hdkfState[:]); err != nil {
+		return nil, err
+	}
+
+	hkdfSecret := hkdf.New(hash, dhSecret[:], hdkfState[:], []byte("TENDERMINT_SECRET_CONNECTION_TRANSCRIPT_HASH"))
+	if _, err := io.ReadFull(hkdfSecret, hdkfState[:]); err != nil {
+		return nil, err
+	}
 
-	// generate the secret used for receiving, sending, challenge via hkdf-sha2 on dhSecret
-	recvSecret, sendSecret, challenge := deriveSecretAndChallenge(dhSecret, locIsLeast)
+	// Generate the secret used for receiving, sending, challenge via HKDF-SHA2
+	// on the transcript state (which itself also uses HKDF-SHA2 to derive a key
+	// from the dhSecret).
+	recvSecret, sendSecret, challenge := deriveSecretAndChallenge(hdkfState, locIsLeast)
 
 	sendAead, err := chacha20poly1305.New(sendSecret[:])
 	if err != nil {
@@ -114,7 +140,7 @@ func MakeSecretConnection(conn io.ReadWriteCloser, locPrivKey crypto.PrivKey) (*
 	if err != nil {
 		return nil, errors.New("invalid receive SecretConnection Key")
 	}
-	// Construct SecretConnection.
+
 	sc := &SecretConnection{
 		conn:       conn,
 		recvBuffer: nil,
@@ -134,11 +160,9 @@ 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")
 	}
@@ -285,9 +309,7 @@ func shareEphPubKey(conn io.ReadWriter, locEphPub *[32]byte) (remEphPub *[32]byt
 			if err2 != nil {
 				return nil, err2, true // abort
 			}
-			if hasSmallOrder(_remEphPub) {
-				return nil, ErrSmallOrderRemotePubKey, true
-			}
+
 			return _remEphPub, nil, false
 		},
 	)
@@ -303,52 +325,6 @@ func shareEphPubKey(conn io.ReadWriter, locEphPub *[32]byte) (remEphPub *[32]byt
 	return &_remEphPub, nil
 }
 
-// use the samne blacklist as lib sodium (see https://eprint.iacr.org/2017/806.pdf for reference):
-// https://github.com/jedisct1/libsodium/blob/536ed00d2c5e0c65ac01e29141d69a30455f2038/src/libsodium/crypto_scalarmult/curve25519/ref10/x25519_ref10.c#L11-L17
-var blacklist = [][32]byte{
-	// 0 (order 4)
-	{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
-	// 1 (order 1)
-	{0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
-	// 325606250916557431795983626356110631294008115727848805560023387167927233504
-	//   (order 8)
-	{0xe0, 0xeb, 0x7a, 0x7c, 0x3b, 0x41, 0xb8, 0xae, 0x16, 0x56, 0xe3,
-		0xfa, 0xf1, 0x9f, 0xc4, 0x6a, 0xda, 0x09, 0x8d, 0xeb, 0x9c, 0x32,
-		0xb1, 0xfd, 0x86, 0x62, 0x05, 0x16, 0x5f, 0x49, 0xb8, 0x00},
-	// 39382357235489614581723060781553021112529911719440698176882885853963445705823
-	//    (order 8)
-	{0x5f, 0x9c, 0x95, 0xbc, 0xa3, 0x50, 0x8c, 0x24, 0xb1, 0xd0, 0xb1,
-		0x55, 0x9c, 0x83, 0xef, 0x5b, 0x04, 0x44, 0x5c, 0xc4, 0x58, 0x1c,
-		0x8e, 0x86, 0xd8, 0x22, 0x4e, 0xdd, 0xd0, 0x9f, 0x11, 0x57},
-	// p-1 (order 2)
-	{0xec, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
-		0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
-		0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f},
-	// p (=0, order 4)
-	{0xed, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
-		0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
-		0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f},
-	// p+1 (=1, order 1)
-	{0xee, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
-		0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
-		0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f},
-}
-
-func hasSmallOrder(pubKey [32]byte) bool {
-	isSmallOrderPoint := false
-	for _, bl := range blacklist {
-		if subtle.ConstantTimeCompare(pubKey[:], bl[:]) == 1 {
-			isSmallOrderPoint = true
-			break
-		}
-	}
-	return isSmallOrderPoint
-}
-
 func deriveSecretAndChallenge(
 	dhSecret *[32]byte,
 	locIsLeast bool,

p2p/conn/secret_connection_test.go

@@ -101,51 +101,6 @@ func TestSecretConnectionHandshake(t *testing.T) {
 	}
 }
 
-// Test that shareEphPubKey rejects lower order public keys based on an
-// (incomplete) blacklist.
-func TestShareLowOrderPubkey(t *testing.T) {
-	var fooConn, barConn = makeKVStoreConnPair()
-	defer fooConn.Close()
-	defer barConn.Close()
-	locEphPub, _ := genEphKeys()
-
-	// all blacklisted low order points:
-	for _, remLowOrderPubKey := range blacklist {
-		remLowOrderPubKey := remLowOrderPubKey
-		_, _ = cmn.Parallel(
-			func(_ int) (val interface{}, err error, abort bool) {
-				_, err = shareEphPubKey(fooConn, locEphPub)
-
-				require.Error(t, err)
-				require.Equal(t, err, ErrSmallOrderRemotePubKey)
-
-				return nil, nil, false
-			},
-			func(_ int) (val interface{}, err error, abort bool) {
-				readRemKey, err := shareEphPubKey(barConn, &remLowOrderPubKey)
-
-				require.NoError(t, err)
-				require.Equal(t, locEphPub, readRemKey)
-
-				return nil, nil, false
-			})
-	}
-}
-
-// Test that additionally that the Diffie-Hellman shared secret is non-zero.
-// The shared secret would be zero for lower order pub-keys (but tested against the blacklist only).
-func TestComputeDHFailsOnLowOrder(t *testing.T) {
-	_, locPrivKey := genEphKeys()
-	for _, remLowOrderPubKey := range blacklist {
-		remLowOrderPubKey := remLowOrderPubKey
-		shared, err := computeDHSecret(&remLowOrderPubKey, locPrivKey)
-		assert.Error(t, err)
-
-		assert.Equal(t, err, ErrSharedSecretIsZero)
-		assert.Empty(t, shared)
-	}
-}
-
 func TestConcurrentWrite(t *testing.T) {
 	fooSecConn, barSecConn := makeSecretConnPair(t)
 	fooWriteText := cmn.RandStr(dataMaxSize)