secret connection check all zeroes (#3347)

* reject the shared secret if is all zeros in case the blacklist was not
sufficient

* Add test that verifies lower order pub-keys are rejected at the DH step

* Update changelog

* fix typo in test-comment

CHANGELOG_PENDING.md

@@ -22,4 +22,6 @@ Special thanks to external contributors on this release:
 ### IMPROVEMENTS:
 
 ### BUG FIXES:
-- [libs/pubsub] \#951, \#1880 use non-blocking send when dispatching messages [ADR-33](https://github.com/tendermint/tendermint/blob/develop/docs/architecture/adr-033-pubsub.md)
+
+- [p2p/conn] \#3347 Reject all-zero shared secrets in the Diffie-Hellman step of secret-connection
+- [libs/pubsub] \#951, \#1880 use non-blocking send when dispatching messages [ADR-33](https://github.com/tendermint/tendermint/blob/develop/docs/architecture/adr-033-pubsub.md)

p2p/conn/secret_connection.go

@@ -29,7 +29,10 @@ const aeadSizeOverhead = 16 // overhead of poly 1305 authentication tag
 const aeadKeySize = chacha20poly1305.KeySize
 const aeadNonceSize = chacha20poly1305.NonceSize
 
-var ErrSmallOrderRemotePubKey = errors.New("detected low order point from remote peer")
+var (
+	ErrSmallOrderRemotePubKey = errors.New("detected low order point from remote peer")
+	ErrSharedSecretIsZero     = errors.New("shared secret is all zeroes")
+)
 
 // SecretConnection implements net.Conn.
 // It is an implementation of the STS protocol.
@@ -90,7 +93,10 @@ func MakeSecretConnection(conn io.ReadWriteCloser, locPrivKey crypto.PrivKey) (*
 	locIsLeast := bytes.Equal(locEphPub[:], loEphPub[:])
 
 	// Compute common diffie hellman secret using X25519.
-	dhSecret := computeDHSecret(remEphPub, locEphPriv)
+	dhSecret, err := computeDHSecret(remEphPub, locEphPriv)
+	if err != nil {
+		return nil, err
+	}
 
 	// generate the secret used for receiving, sending, challenge via hkdf-sha2 on dhSecret
 	recvSecret, sendSecret, challenge := deriveSecretAndChallenge(dhSecret, locIsLeast)
@@ -230,9 +236,12 @@ func (sc *SecretConnection) SetWriteDeadline(t time.Time) error {
 
 func genEphKeys() (ephPub, ephPriv *[32]byte) {
 	var err error
+	// TODO: Probably not a problem but ask Tony: different from the rust implementation (uses x25519-dalek),
+	// we do not "clamp" the private key scalar:
+	// see: https://github.com/dalek-cryptography/x25519-dalek/blob/34676d336049df2bba763cc076a75e47ae1f170f/src/x25519.rs#L56-L74
 	ephPub, ephPriv, err = box.GenerateKey(crand.Reader)
 	if err != nil {
-		panic("Could not generate ephemeral keypairs")
+		panic("Could not generate ephemeral key-pair")
 	}
 	return
 }
@@ -349,9 +358,20 @@ func deriveSecretAndChallenge(dhSecret *[32]byte, locIsLeast bool) (recvSecret,
 	return
 }
 
-func computeDHSecret(remPubKey, locPrivKey *[32]byte) (shrKey *[32]byte) {
+// computeDHSecret computes a shared secret Diffie-Hellman secret
+// from a the own local private key and the others public key.
+//
+// It returns an error if the computed shared secret is all zeroes.
+func computeDHSecret(remPubKey, locPrivKey *[32]byte) (shrKey *[32]byte, err error) {
 	shrKey = new([32]byte)
 	curve25519.ScalarMult(shrKey, locPrivKey, remPubKey)
+
+	// reject if the returned shared secret is all zeroes
+	// related to: https://github.com/tendermint/tendermint/issues/3010
+	zero := new([32]byte)
+	if subtle.ConstantTimeCompare(shrKey[:], zero[:]) == 1 {
+		return nil, ErrSharedSecretIsZero
+	}
 	return
 }
 

p2p/conn/secret_connection_test.go

@@ -100,8 +100,12 @@ 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:
@@ -126,6 +130,19 @@ func TestShareLowOrderPubkey(t *testing.T) {
 	}
 }
 
+// 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 {
+		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)