From 45d3c6fff662ddd6938982e7e9309ad5fa2ad8dd Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Tue, 30 May 2023 21:28:57 +0300 Subject: [PATCH] identify: reject signed peer records on peer ID mismatch and make ConsumeEnvelope harder to misuse --------- Co-authored-by: Marco Munizaga --- core/record/envelope.go | 9 +-- p2p/protocol/identify/id.go | 12 ++-- p2p/protocol/identify/id_glass_test.go | 91 ++++++++++++++++++++++++++ 3 files changed, 101 insertions(+), 11 deletions(-) diff --git a/core/record/envelope.go b/core/record/envelope.go index 6643c9a655..86ad142538 100644 --- a/core/record/envelope.go +++ b/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 } diff --git a/p2p/protocol/identify/id.go b/p2p/protocol/identify/id.go index 4173f2bb79..b122697ec9 100644 --- a/p2p/protocol/identify/id.go +++ b/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) diff --git a/p2p/protocol/identify/id_glass_test.go b/p2p/protocol/identify/id_glass_test.go index 3477d52da4..777cef01ed 100644 --- a/p2p/protocol/identify/id_glass_test.go +++ b/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())) +}