From 8d95adcb0fb96a92d9f2376323dfb81b911238c0 Mon Sep 17 00:00:00 2001 From: Stephen Buttolph Date: Thu, 15 Feb 2024 12:34:02 -0500 Subject: [PATCH] Verify BLS signature provided in Handshake messages (#2735) --- network/network.go | 1 + network/peer/config.go | 1 + network/peer/peer.go | 124 +++++++++-- network/peer/peer_test.go | 445 ++++++++++++++++++++++++++++++++++++++ network/peer/test_peer.go | 1 + 5 files changed, 551 insertions(+), 21 deletions(-) diff --git a/network/network.go b/network/network.go index 15438b5a2b7e..5e4b3cdc4e82 100644 --- a/network/network.go +++ b/network/network.go @@ -260,6 +260,7 @@ func NewNetwork( VersionCompatibility: version.GetCompatibility(config.NetworkID), MySubnets: config.TrackedSubnets, Beacons: config.Beacons, + Validators: config.Validators, NetworkID: config.NetworkID, PingFrequency: config.PingFrequency, PongTimeout: config.PingPongTimeout, diff --git a/network/peer/config.go b/network/peer/config.go index 0a01cf87fb92..3eb8319216d7 100644 --- a/network/peer/config.go +++ b/network/peer/config.go @@ -35,6 +35,7 @@ type Config struct { VersionCompatibility version.Compatibility MySubnets set.Set[ids.ID] Beacons validators.Manager + Validators validators.Manager NetworkID uint32 PingFrequency time.Duration PongTimeout time.Duration diff --git a/network/peer/peer.go b/network/peer/peer.go index a3cc6c96eeca..58b4e648125e 100644 --- a/network/peer/peer.go +++ b/network/peer/peer.go @@ -23,6 +23,7 @@ import ( "github.com/ava-labs/avalanchego/utils" "github.com/ava-labs/avalanchego/utils/bloom" "github.com/ava-labs/avalanchego/utils/constants" + "github.com/ava-labs/avalanchego/utils/crypto/bls" "github.com/ava-labs/avalanchego/utils/ips" "github.com/ava-labs/avalanchego/utils/json" "github.com/ava-labs/avalanchego/utils/set" @@ -142,6 +143,14 @@ type peer struct { supportedACPs set.Set[uint32] objectedACPs set.Set[uint32] + // txIDOfVerifiedBLSKey is the txID that added the BLS key that was most + // recently verified to have signed the IP. + // + // Invariant: Prior to the handshake being completed, this can only be + // accessed by the reader goroutine. After the handshake has been completed, + // this can only be accessed by the message sender goroutine. + txIDOfVerifiedBLSKey ids.ID + observedUptimesLock sync.RWMutex // [observedUptimesLock] must be held while accessing [observedUptime] // Subnet ID --> Our uptime for the given subnet as perceived by the peer @@ -698,16 +707,11 @@ func (p *peer) sendNetworkMessages() { return } - if p.finishedHandshake.Get() { - if err := p.VersionCompatibility.Compatible(p.version); err != nil { - p.Log.Debug("disconnecting from peer", - zap.String("reason", "version not compatible"), - zap.Stringer("nodeID", p.id), - zap.Stringer("peerVersion", p.version), - zap.Error(err), - ) - return - } + // Only check if we should disconnect after the handshake is + // finished to avoid race conditions and accessing uninitialized + // values. + if p.finishedHandshake.Get() && p.shouldDisconnect() { + return } primaryUptime, subnetUptimes := p.getUptimes() @@ -728,6 +732,68 @@ func (p *peer) sendNetworkMessages() { } } +// shouldDisconnect is called both during receipt of the Handshake message and +// periodically when sending a Ping message (after finishing the handshake!). +// +// It is called during the Handshake to prevent marking a peer as connected and +// then immediately disconnecting from them. +// +// It is called when sending a Ping message to account for validator set +// changes. It's called when sending a Ping rather than in a validator set +// callback to avoid signature verification on the P-chain accept path. +func (p *peer) shouldDisconnect() bool { + if err := p.VersionCompatibility.Compatible(p.version); err != nil { + p.Log.Debug("disconnecting from peer", + zap.String("reason", "version not compatible"), + zap.Stringer("nodeID", p.id), + zap.Stringer("peerVersion", p.version), + zap.Error(err), + ) + return true + } + + // Enforce that all validators that have registered a BLS key are signing + // their IP with it after the activation of Durango. + vdr, ok := p.Validators.GetValidator(constants.PrimaryNetworkID, p.id) + if !ok || vdr.PublicKey == nil || vdr.TxID == p.txIDOfVerifiedBLSKey { + return false + } + + postDurango := p.Clock.Time().After(version.GetDurangoTime(constants.MainnetID)) + if postDurango && p.ip.BLSSignature == nil { + p.Log.Debug("disconnecting from peer", + zap.String("reason", "missing BLS signature"), + zap.Stringer("nodeID", p.id), + ) + return true + } + + // If Durango hasn't activated on mainnet yet, we don't require BLS + // signatures to be provided. However, if they are provided, verify that + // they are correct. + if p.ip.BLSSignature == nil { + return false + } + + validSignature := bls.VerifyProofOfPossession( + vdr.PublicKey, + p.ip.BLSSignature, + p.ip.UnsignedIP.bytes(), + ) + if !validSignature { + p.Log.Debug("disconnecting from peer", + zap.String("reason", "invalid BLS signature"), + zap.Stringer("nodeID", p.id), + ) + return true + } + + // Avoid unnecessary signature verifications by only verifing the signature + // once per validation period. + p.txIDOfVerifiedBLSKey = vdr.TxID + return false +} + func (p *peer) handle(msg message.InboundMessage) { switch m := msg.Message().(type) { // Network-related message types case *p2p.Ping: @@ -966,16 +1032,6 @@ func (p *peer) handleHandshake(msg *p2p.Handshake) { } } - if err := p.VersionCompatibility.Compatible(p.version); err != nil { - p.Log.Verbo("peer version not compatible", - zap.Stringer("nodeID", p.id), - zap.Stringer("peerVersion", p.version), - zap.Error(err), - ) - p.StartClose() - return - } - // handle subnet IDs for _, subnetIDBytes := range msg.TrackedSubnets { subnetID, err := ids.ToID(subnetIDBytes) @@ -1078,13 +1134,13 @@ func (p *peer) handleHandshake(msg *p2p.Handshake) { Timestamp: msg.IpSigningTime, }, TLSSignature: msg.IpNodeIdSig, - // TODO: Populate the BLS Signature here. } maxTimestamp := myTime.Add(p.MaxClockDifference) if err := p.ip.Verify(p.cert, maxTimestamp); err != nil { if _, ok := p.Beacons.GetValidator(constants.PrimaryNetworkID, p.id); ok { p.Log.Warn("beacon has invalid signature or is out of sync", zap.Stringer("nodeID", p.id), + zap.String("signatureType", "tls"), zap.Uint64("peerTime", msg.MyTime), zap.Uint64("myTime", myTimeUnix), zap.Error(err), @@ -1092,6 +1148,7 @@ func (p *peer) handleHandshake(msg *p2p.Handshake) { } else { p.Log.Debug("peer has invalid signature or is out of sync", zap.Stringer("nodeID", p.id), + zap.String("signatureType", "tls"), zap.Uint64("peerTime", msg.MyTime), zap.Uint64("myTime", myTimeUnix), zap.Error(err), @@ -1102,6 +1159,31 @@ func (p *peer) handleHandshake(msg *p2p.Handshake) { return } + // TODO: After v1.11.x is activated, require the key to be provided. + if len(msg.IpBlsSig) > 0 { + signature, err := bls.SignatureFromBytes(msg.IpBlsSig) + if err != nil { + p.Log.Debug("peer has malformed signature", + zap.Stringer("nodeID", p.id), + zap.String("signatureType", "bls"), + zap.Error(err), + ) + p.StartClose() + return + } + + p.ip.BLSSignature = signature + p.ip.BLSSignatureBytes = msg.IpBlsSig + } + + // If the peer is running an incompatible version or has an invalid BLS + // signature, disconnect from them prior to marking the handshake as + // completed. + if p.shouldDisconnect() { + p.StartClose() + return + } + p.gotHandshake.Set(true) peerIPs := p.Network.Peers(p.id, knownPeers, salt) diff --git a/network/peer/peer_test.go b/network/peer/peer_test.go index 5b3a04be5b81..e52273fc6fbe 100644 --- a/network/peer/peer_test.go +++ b/network/peer/peer_test.go @@ -29,6 +29,7 @@ import ( "github.com/ava-labs/avalanchego/utils/math/meter" "github.com/ava-labs/avalanchego/utils/resource" "github.com/ava-labs/avalanchego/utils/set" + "github.com/ava-labs/avalanchego/utils/timer/mockable" "github.com/ava-labs/avalanchego/version" ) @@ -103,6 +104,7 @@ func makeRawTestPeers(t *testing.T, trackedSubnets set.Set[ids.ID]) (*rawTestPee MySubnets: trackedSubnets, UptimeCalculator: uptime.NoOpCalculator, Beacons: validators.NewManager(), + Validators: validators.NewManager(), NetworkID: constants.LocalID, PingFrequency: constants.DefaultPingFrequency, PongTimeout: constants.DefaultPingPongTimeout, @@ -383,6 +385,449 @@ func TestPingUptimes(t *testing.T) { } } +// Test that a peer using the wrong BLS key is disconnected from. +func TestInvalidBLSKeyDisconnects(t *testing.T) { + require := require.New(t) + + rawPeer0, rawPeer1 := makeRawTestPeers(t, nil) + require.NoError(rawPeer0.config.Validators.AddStaker( + constants.PrimaryNetworkID, + rawPeer1.nodeID, + bls.PublicFromSecretKey(rawPeer1.config.IPSigner.blsSigner), + ids.GenerateTestID(), + 1, + )) + + bogusBLSKey, err := bls.NewSecretKey() + require.NoError(err) + require.NoError(rawPeer1.config.Validators.AddStaker( + constants.PrimaryNetworkID, + rawPeer0.nodeID, + bls.PublicFromSecretKey(bogusBLSKey), // This is the wrong BLS key for this peer + ids.GenerateTestID(), + 1, + )) + peer0 := &testPeer{ + Peer: Start( + rawPeer0.config, + rawPeer0.conn, + rawPeer1.cert, + rawPeer1.nodeID, + NewThrottledMessageQueue( + rawPeer0.config.Metrics, + rawPeer1.nodeID, + logging.NoLog{}, + throttling.NewNoOutboundThrottler(), + ), + ), + inboundMsgChan: rawPeer0.inboundMsgChan, + } + peer1 := &testPeer{ + Peer: Start( + rawPeer1.config, + rawPeer1.conn, + rawPeer0.cert, + rawPeer0.nodeID, + NewThrottledMessageQueue( + rawPeer1.config.Metrics, + rawPeer0.nodeID, + logging.NoLog{}, + throttling.NewNoOutboundThrottler(), + ), + ), + inboundMsgChan: rawPeer1.inboundMsgChan, + } + + // Because peer1 thinks that peer0 is using the wrong BLS key, they should + // disconnect from each other. + require.NoError(peer0.AwaitClosed(context.Background())) + require.NoError(peer1.AwaitClosed(context.Background())) +} + +func TestShouldDisconnect(t *testing.T) { + peerID := ids.GenerateTestNodeID() + txID := ids.GenerateTestID() + blsKey, err := bls.NewSecretKey() + require.NoError(t, err) + + tests := []struct { + name string + initialPeer *peer + expectedPeer *peer + expectedShouldDisconnect bool + }{ + { + name: "peer is reporting old version", + initialPeer: &peer{ + Config: &Config{ + Log: logging.NoLog{}, + VersionCompatibility: version.GetCompatibility(constants.UnitTestID), + }, + version: &version.Application{ + Name: version.Client, + Major: 0, + Minor: 0, + Patch: 0, + }, + }, + expectedPeer: &peer{ + Config: &Config{ + Log: logging.NoLog{}, + VersionCompatibility: version.GetCompatibility(constants.UnitTestID), + }, + version: &version.Application{ + Name: version.Client, + Major: 0, + Minor: 0, + Patch: 0, + }, + }, + expectedShouldDisconnect: true, + }, + { + name: "peer is not a validator", + initialPeer: &peer{ + Config: &Config{ + Log: logging.NoLog{}, + VersionCompatibility: version.GetCompatibility(constants.UnitTestID), + Validators: validators.NewManager(), + }, + version: version.CurrentApp, + }, + expectedPeer: &peer{ + Config: &Config{ + Log: logging.NoLog{}, + VersionCompatibility: version.GetCompatibility(constants.UnitTestID), + Validators: validators.NewManager(), + }, + version: version.CurrentApp, + }, + expectedShouldDisconnect: false, + }, + { + name: "peer is a validator without a BLS key", + initialPeer: &peer{ + Config: &Config{ + Log: logging.NoLog{}, + VersionCompatibility: version.GetCompatibility(constants.UnitTestID), + Validators: func() validators.Manager { + vdrs := validators.NewManager() + require.NoError(t, vdrs.AddStaker( + constants.PrimaryNetworkID, + peerID, + nil, + txID, + 1, + )) + return vdrs + }(), + }, + id: peerID, + version: version.CurrentApp, + }, + expectedPeer: &peer{ + Config: &Config{ + Log: logging.NoLog{}, + VersionCompatibility: version.GetCompatibility(constants.UnitTestID), + Validators: func() validators.Manager { + vdrs := validators.NewManager() + require.NoError(t, vdrs.AddStaker( + constants.PrimaryNetworkID, + peerID, + nil, + txID, + 1, + )) + return vdrs + }(), + }, + id: peerID, + version: version.CurrentApp, + }, + expectedShouldDisconnect: false, + }, + { + name: "already verified peer", + initialPeer: &peer{ + Config: &Config{ + Log: logging.NoLog{}, + VersionCompatibility: version.GetCompatibility(constants.UnitTestID), + Validators: func() validators.Manager { + vdrs := validators.NewManager() + require.NoError(t, vdrs.AddStaker( + constants.PrimaryNetworkID, + peerID, + bls.PublicFromSecretKey(blsKey), + txID, + 1, + )) + return vdrs + }(), + }, + id: peerID, + version: version.CurrentApp, + txIDOfVerifiedBLSKey: txID, + }, + expectedPeer: &peer{ + Config: &Config{ + Log: logging.NoLog{}, + VersionCompatibility: version.GetCompatibility(constants.UnitTestID), + Validators: func() validators.Manager { + vdrs := validators.NewManager() + require.NoError(t, vdrs.AddStaker( + constants.PrimaryNetworkID, + peerID, + bls.PublicFromSecretKey(blsKey), + txID, + 1, + )) + return vdrs + }(), + }, + id: peerID, + version: version.CurrentApp, + txIDOfVerifiedBLSKey: txID, + }, + expectedShouldDisconnect: false, + }, + { + name: "past durango without a signature", + initialPeer: &peer{ + Config: &Config{ + Clock: func() mockable.Clock { + clk := mockable.Clock{} + clk.Set(mockable.MaxTime) + return clk + }(), + Log: logging.NoLog{}, + VersionCompatibility: version.GetCompatibility(constants.UnitTestID), + Validators: func() validators.Manager { + vdrs := validators.NewManager() + require.NoError(t, vdrs.AddStaker( + constants.PrimaryNetworkID, + peerID, + bls.PublicFromSecretKey(blsKey), + txID, + 1, + )) + return vdrs + }(), + }, + id: peerID, + version: version.CurrentApp, + ip: &SignedIP{}, + }, + expectedPeer: &peer{ + Config: &Config{ + Clock: func() mockable.Clock { + clk := mockable.Clock{} + clk.Set(mockable.MaxTime) + return clk + }(), + Log: logging.NoLog{}, + VersionCompatibility: version.GetCompatibility(constants.UnitTestID), + Validators: func() validators.Manager { + vdrs := validators.NewManager() + require.NoError(t, vdrs.AddStaker( + constants.PrimaryNetworkID, + peerID, + bls.PublicFromSecretKey(blsKey), + txID, + 1, + )) + return vdrs + }(), + }, + id: peerID, + version: version.CurrentApp, + ip: &SignedIP{}, + }, + expectedShouldDisconnect: true, + }, + { + name: "pre durango without a signature", + initialPeer: &peer{ + Config: &Config{ + Clock: func() mockable.Clock { + clk := mockable.Clock{} + clk.Set(time.Time{}) + return clk + }(), + Log: logging.NoLog{}, + VersionCompatibility: version.GetCompatibility(constants.UnitTestID), + Validators: func() validators.Manager { + vdrs := validators.NewManager() + require.NoError(t, vdrs.AddStaker( + constants.PrimaryNetworkID, + peerID, + bls.PublicFromSecretKey(blsKey), + txID, + 1, + )) + return vdrs + }(), + }, + id: peerID, + version: version.CurrentApp, + ip: &SignedIP{}, + }, + expectedPeer: &peer{ + Config: &Config{ + Clock: func() mockable.Clock { + clk := mockable.Clock{} + clk.Set(time.Time{}) + return clk + }(), + Log: logging.NoLog{}, + VersionCompatibility: version.GetCompatibility(constants.UnitTestID), + Validators: func() validators.Manager { + vdrs := validators.NewManager() + require.NoError(t, vdrs.AddStaker( + constants.PrimaryNetworkID, + peerID, + bls.PublicFromSecretKey(blsKey), + txID, + 1, + )) + return vdrs + }(), + }, + id: peerID, + version: version.CurrentApp, + ip: &SignedIP{}, + }, + expectedShouldDisconnect: false, + }, + { + name: "pre durango with an invalid signature", + initialPeer: &peer{ + Config: &Config{ + Clock: func() mockable.Clock { + clk := mockable.Clock{} + clk.Set(time.Time{}) + return clk + }(), + Log: logging.NoLog{}, + VersionCompatibility: version.GetCompatibility(constants.UnitTestID), + Validators: func() validators.Manager { + vdrs := validators.NewManager() + require.NoError(t, vdrs.AddStaker( + constants.PrimaryNetworkID, + peerID, + bls.PublicFromSecretKey(blsKey), + txID, + 1, + )) + return vdrs + }(), + }, + id: peerID, + version: version.CurrentApp, + ip: &SignedIP{ + BLSSignature: bls.SignProofOfPossession(blsKey, []byte("wrong message")), + }, + }, + expectedPeer: &peer{ + Config: &Config{ + Clock: func() mockable.Clock { + clk := mockable.Clock{} + clk.Set(time.Time{}) + return clk + }(), + Log: logging.NoLog{}, + VersionCompatibility: version.GetCompatibility(constants.UnitTestID), + Validators: func() validators.Manager { + vdrs := validators.NewManager() + require.NoError(t, vdrs.AddStaker( + constants.PrimaryNetworkID, + peerID, + bls.PublicFromSecretKey(blsKey), + txID, + 1, + )) + return vdrs + }(), + }, + id: peerID, + version: version.CurrentApp, + ip: &SignedIP{ + BLSSignature: bls.SignProofOfPossession(blsKey, []byte("wrong message")), + }, + }, + expectedShouldDisconnect: true, + }, + { + name: "pre durango with a valid signature", + initialPeer: &peer{ + Config: &Config{ + Clock: func() mockable.Clock { + clk := mockable.Clock{} + clk.Set(time.Time{}) + return clk + }(), + Log: logging.NoLog{}, + VersionCompatibility: version.GetCompatibility(constants.UnitTestID), + Validators: func() validators.Manager { + vdrs := validators.NewManager() + require.NoError(t, vdrs.AddStaker( + constants.PrimaryNetworkID, + peerID, + bls.PublicFromSecretKey(blsKey), + txID, + 1, + )) + return vdrs + }(), + }, + id: peerID, + version: version.CurrentApp, + ip: &SignedIP{ + BLSSignature: bls.SignProofOfPossession(blsKey, (&UnsignedIP{}).bytes()), + }, + }, + expectedPeer: &peer{ + Config: &Config{ + Clock: func() mockable.Clock { + clk := mockable.Clock{} + clk.Set(time.Time{}) + return clk + }(), + Log: logging.NoLog{}, + VersionCompatibility: version.GetCompatibility(constants.UnitTestID), + Validators: func() validators.Manager { + vdrs := validators.NewManager() + require.NoError(t, vdrs.AddStaker( + constants.PrimaryNetworkID, + peerID, + bls.PublicFromSecretKey(blsKey), + txID, + 1, + )) + return vdrs + }(), + }, + id: peerID, + version: version.CurrentApp, + ip: &SignedIP{ + BLSSignature: bls.SignProofOfPossession(blsKey, (&UnsignedIP{}).bytes()), + }, + txIDOfVerifiedBLSKey: txID, + }, + expectedShouldDisconnect: false, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + require := require.New(t) + + shouldDisconnect := test.initialPeer.shouldDisconnect() + require.Equal(test.expectedPeer, test.initialPeer) + require.Equal(test.expectedShouldDisconnect, shouldDisconnect) + }) + } +} + // Helper to send a message from sender to receiver and assert that the // receiver receives the message. This can be used to test a prior message // was handled by the peer. diff --git a/network/peer/test_peer.go b/network/peer/test_peer.go index 13e2f46c8914..eb1a79476480 100644 --- a/network/peer/test_peer.go +++ b/network/peer/test_peer.go @@ -122,6 +122,7 @@ func StartTestPeer( VersionCompatibility: version.GetCompatibility(networkID), MySubnets: set.Set[ids.ID]{}, Beacons: validators.NewManager(), + Validators: validators.NewManager(), NetworkID: networkID, PingFrequency: constants.DefaultPingFrequency, PongTimeout: constants.DefaultPingPongTimeout,