identify: reject signed peer records on peer ID mismatch

and make ConsumeEnvelope harder to misuse

---------

Co-authored-by: Marco Munizaga <[email protected]>

core/record/envelope.go

@@ -106,11 +106,6 @@ func Seal(rec Record, privateKey crypto.PrivKey) (*Envelope, error) {
 //	  doSomethingWithPeerRecord(peerRec)
 //	}
 //
-// Important: you MUST check the error value before using the returned Envelope. In some error
-// cases, including when the envelope signature is invalid, both the Envelope and an error will
-// be returned. This allows you to inspect the unmarshalled but invalid Envelope. As a result,
-// you must not assume that any non-nil Envelope returned from this function is valid.
-//
 // If the Envelope signature is valid, but no Record type is registered for the Envelope's
 // PayloadType, ErrPayloadTypeNotRegistered will be returned, along with the Envelope and
 // a nil Record.
@@ -122,12 +117,12 @@ func ConsumeEnvelope(data []byte, domain string) (envelope *Envelope, rec Record
 
 	err = e.validate(domain)
 	if err != nil {
-		return e, nil, fmt.Errorf("failed to validate envelope: %w", err)
+		return nil, nil, fmt.Errorf("failed to validate envelope: %w", err)
 	}
 
 	rec, err = e.Record()
 	if err != nil {
-		return e, nil, fmt.Errorf("failed to unmarshal envelope payload: %w", err)
+		return nil, nil, fmt.Errorf("failed to unmarshal envelope payload: %w", err)
 	}
 	return e, rec, nil
 }

p2p/protocol/identify/id.go

@@ -766,10 +766,14 @@ func (ids *idService) consumeMessage(mes *pb.Identify, c network.Conn, isPush bo
 
 	// add signed addrs if we have them and the peerstore supports them
 	cab, ok := peerstore.GetCertifiedAddrBook(ids.Host.Peerstore())
-	if ok && signedPeerRecord != nil {
-		_, addErr := cab.ConsumePeerRecord(signedPeerRecord, ttl)
-		if addErr != nil {
-			log.Debugf("error adding signed addrs to peerstore: %v", addErr)
+	if ok && signedPeerRecord != nil && signedPeerRecord.PublicKey != nil {
+		id, err := peer.IDFromPublicKey(signedPeerRecord.PublicKey)
+		if err != nil {
+			log.Debugf("failed to derive peer ID from peer record: %s", err)
+		} else if id != c.RemotePeer() {
+			log.Debugf("received signed peer record for unexpected peer ID. expected %s, got %s", c.RemotePeer(), id)
+		} else if _, err := cab.ConsumePeerRecord(signedPeerRecord, ttl); err != nil {
+			log.Debugf("error adding signed addrs to peerstore: %v", err)
 		}
 	} else {
 		ids.Host.Peerstore().AddAddrs(p, lmaddrs, ttl)

p2p/protocol/identify/id_glass_test.go

@@ -2,13 +2,17 @@ package identify
 
 import (
 	"context"
+	"fmt"
 	"testing"
 	"time"
 
 	"github.com/libp2p/go-libp2p/core/network"
 	"github.com/libp2p/go-libp2p/core/peer"
+	"github.com/libp2p/go-libp2p/core/peerstore"
+	recordPb "github.com/libp2p/go-libp2p/core/record/pb"
 	blhost "github.com/libp2p/go-libp2p/p2p/host/blank"
 	swarmt "github.com/libp2p/go-libp2p/p2p/net/swarm/testing"
+	"google.golang.org/protobuf/proto"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -82,3 +86,90 @@ func TestFastDisconnect(t *testing.T) {
 	// double-check to make sure we didn't actually timeout somewhere.
 	require.NoError(t, ctx.Err())
 }
+
+func TestWrongSignedPeerRecord(t *testing.T) {
+	h1 := blhost.NewBlankHost(swarmt.GenSwarm(t))
+	defer h1.Close()
+	ids, err := NewIDService(h1)
+	require.NoError(t, err)
+	ids.Start()
+	defer ids.Close()
+
+	h2 := blhost.NewBlankHost(swarmt.GenSwarm(t))
+	defer h2.Close()
+	ids2, err := NewIDService(h2)
+	require.NoError(t, err)
+	ids2.Start()
+	defer ids2.Close()
+
+	h3 := blhost.NewBlankHost(swarmt.GenSwarm(t))
+	defer h2.Close()
+	ids3, err := NewIDService(h3)
+	require.NoError(t, err)
+	ids3.Start()
+	defer ids3.Close()
+
+	h2.Connect(context.Background(), peer.AddrInfo{ID: h1.ID(), Addrs: h1.Addrs()})
+	s, err := h2.NewStream(context.Background(), h1.ID(), IDPush)
+	require.NoError(t, err)
+
+	err = ids3.sendIdentifyResp(s, true)
+	// This should fail because the peer record is signed by h3, not h2
+	require.NoError(t, err)
+	time.Sleep(time.Second)
+
+	require.Empty(t, h1.Peerstore().Addrs(h3.ID()), "h1 should not know about h3 since it was relayed over h2")
+}
+
+func TestInvalidSignedPeerRecord(t *testing.T) {
+	h1 := blhost.NewBlankHost(swarmt.GenSwarm(t))
+	defer h1.Close()
+	ids, err := NewIDService(h1)
+	require.NoError(t, err)
+	ids.Start()
+	defer ids.Close()
+
+	h2 := blhost.NewBlankHost(swarmt.GenSwarm(t))
+	defer h2.Close()
+	ids2, err := NewIDService(h2)
+	require.NoError(t, err)
+	// We don't want to start the identify service, we'll manage the messages h2
+	// sends manually so we can tweak it
+	// ids2.Start()
+
+	h2.Connect(context.Background(), peer.AddrInfo{ID: h1.ID(), Addrs: h1.Addrs()})
+	require.Empty(t, h1.Peerstore().Addrs(h2.ID()))
+
+	s, err := h2.NewStream(context.Background(), h1.ID(), IDPush)
+	require.NoError(t, err)
+
+	ids2.updateSnapshot()
+	ids2.currentSnapshot.Lock()
+	snapshot := ids2.currentSnapshot.snapshot
+	ids2.currentSnapshot.Unlock()
+	mes := ids2.createBaseIdentifyResponse(s.Conn(), &snapshot)
+	fmt.Println("Signed record is", snapshot.record)
+	marshalled, err := snapshot.record.Marshal()
+	require.NoError(t, err)
+
+	var envPb recordPb.Envelope
+	err = proto.Unmarshal(marshalled, &envPb)
+	require.NoError(t, err)
+
+	envPb.Signature = []byte("invalid")
+
+	mes.SignedPeerRecord, err = proto.Marshal(&envPb)
+	require.NoError(t, err)
+
+	err = ids2.writeChunkedIdentifyMsg(s, mes)
+	require.NoError(t, err)
+	fmt.Println("Done sending msg")
+	s.Close()
+
+	// Wait a bit for h1 to process the message
+	time.Sleep(1 * time.Second)
+
+	cab, ok := h1.Peerstore().(peerstore.CertifiedAddrBook)
+	require.True(t, ok)
+	require.Nil(t, cab.GetPeerRecord(h2.ID()))
+}