error handling for invalid curve25519 public keys (#2709)

The function DhSecret which computes a Diffie-Hellman shared secret key
now panics on an error. This is an issue as a malicious node could send
a low order point public key to intentionally cause the key generation
to fail. As this panic doesn't seem to be recovered anywhere it will
crash the node.

The fix adds back error handling for DH secret generation and a test.

Author: pompon0

sei-tendermint/internal/p2p/conn/evil_secret_connection_test.go

@@ -17,6 +17,11 @@ import (
 	"github.com/tendermint/tendermint/libs/utils/scope"
 )
 
+func TestLowOrderPoint(t *testing.T) {
+	_, err := newSecretConnection(nil, genEphKey(), ephPublic{})
+	require.True(t, errors.Is(err, errDH))
+}
+
 type evilConn struct {
 	net.Conn
 	remEphKey utils.AtomicSend[utils.Option[ephPublic]]
@@ -57,11 +62,11 @@ func (c *evilConn) Run(ctx context.Context) error {
 		return nil
 	}
 	loc := genEphKey()
-	ephKey := loc.public[:]
+	ephKey := loc.public
 	if c.badEphKey {
-		ephKey = []byte("drop users;")
+		ephKey = ephPublic{}
 	}
-	ephKeyMsg := &gogotypes.BytesValue{Value: ephKey}
+	ephKeyMsg := &gogotypes.BytesValue{Value: ephKey[:]}
 	if _, err := c.writer.Write(utils.OrPanic1(protoio.MarshalDelimited(ephKeyMsg))); err != nil {
 		return err
 	}
@@ -74,7 +79,7 @@ func (c *evilConn) Run(ctx context.Context) error {
 	if err != nil {
 		return err
 	}
-	sc := newSecretConnection(WriterConn{writer: c.writer}, loc, rem.OrPanic())
+	sc := utils.OrPanic1(newSecretConnection(WriterConn{writer: c.writer}, loc, rem.OrPanic()))
 	challenge := sc.challenge[:]
 	if c.badAuthSignature {
 		challenge = []byte("invalid challenge")

sei-tendermint/internal/p2p/conn/secret_connection.go

@@ -94,15 +94,18 @@ type SecretConnection struct {
 	remPubKey crypto.PubKey
 }
 
-func newSecretConnection(conn net.Conn, loc ephSecret, rem ephPublic) *SecretConnection {
+func newSecretConnection(conn net.Conn, loc ephSecret, rem ephPublic) (*SecretConnection, error) {
 	pubs := utils.Slice(loc.public, rem)
 	if bytes.Compare(pubs[0][:], pubs[1][:]) > 0 {
 		pubs[0], pubs[1] = pubs[1], pubs[0]
 	}
 	transcript := merlin.NewTranscript("TENDERMINT_SECRET_CONNECTION_TRANSCRIPT_HASH")
 	transcript.AppendMessage(labelEphemeralLowerPublicKey, pubs[0][:])
 	transcript.AppendMessage(labelEphemeralUpperPublicKey, pubs[1][:])
-	dh := loc.DhSecret(rem)
+	dh, err := loc.DhSecret(rem)
+	if err != nil {
+		return nil, err
+	}
 	transcript.AppendMessage(labelDHSecret, dh[:])
 	var challenge [32]byte
 	transcript.ExtractBytes(challenge[:], labelSecretConnectionMac)
@@ -116,7 +119,7 @@ func newSecretConnection(conn net.Conn, loc ephSecret, rem ephPublic) *SecretCon
 		challenge: challenge,
 		recvState: utils.NewMutex(newRecvState(aead.recv.Cipher())),
 		sendState: utils.NewMutex(newSendState(aead.send.Cipher())),
-	}
+	}, nil
 }
 
 // MakeSecretConnection performs handshake and returns a new authenticated
@@ -135,8 +138,10 @@ func MakeSecretConnection(ctx context.Context, conn net.Conn, locPrivKey crypto.
 	if err != nil {
 		return nil, err
 	}
-	sc := newSecretConnection(conn, loc, rem)
-
+	sc, err := newSecretConnection(conn, loc, rem)
+	if err != nil {
+		return nil, err
+	}
 	return scope.Run1(ctx, func(ctx context.Context, s scope.Scope) (*SecretConnection, error) {
 		// Share (in secret) each other's pubkey & challenge signature
 		s.Spawn(func() error {
@@ -315,8 +320,12 @@ func (s dhSecret) AeadSecrets(locIsLeast bool) aeadSecrets {
 
 // computeDHSecret computes a Diffie-Hellman shared secret key
 // from our own local private key and the other's public key.
-func (s ephSecret) DhSecret(remPubKey ephPublic) dhSecret {
-	return dhSecret(utils.OrPanic1(curve25519.X25519(s.secret[:], remPubKey[:])))
+func (s ephSecret) DhSecret(remPubKey ephPublic) (dhSecret, error) {
+	dhSecretRaw, err := curve25519.X25519(s.secret[:], remPubKey[:])
+	if err != nil {
+		return dhSecret{}, fmt.Errorf("%w: %v", errDH, err)
+	}
+	return dhSecret(dhSecretRaw), nil
 }
 
 type authSigMessage struct {