From d4c9fdf6ac8180fdc507500836780a740eb8b718 Mon Sep 17 00:00:00 2001 From: Ori Newman Date: Mon, 15 Jun 2020 12:12:38 +0300 Subject: [PATCH] [NOD-614] Add ban score (#760) * [NOD-614] Copy bitcoin-core ban score policy * [NOD-614] Add ban score to disconnects * [NOD-614] Fix wrong branch of AddBanScore * [NOD-614] Add ban score on sending too many addresses * [NOD-614] Add comments * [NOD-614] Remove redundant reject messages * [NOD-614] Fix log message * [NOD-614] Ban every node that sends invalid invs * [NOD-614] Make constants for ban scores --- addrmgr/addrmanager.go | 8 ++--- blockdag/dag.go | 2 +- netsync/manager.go | 57 ++++++++++++++++-------------- peer/banscores.go | 34 ++++++++++++++++++ peer/peer.go | 49 ++++++++++++++++++------- server/p2p/on_addr.go | 14 ++++++-- server/p2p/on_fee_filter.go | 6 ++-- server/p2p/on_filter_add.go | 5 ++- server/p2p/on_filter_clear.go | 5 ++- server/p2p/on_get_block_invs.go | 6 ++-- server/p2p/on_get_block_locator.go | 10 +++--- server/p2p/on_inv.go | 5 ++- server/p2p/p2p.go | 21 ++++++----- wire/msgreject.go | 2 ++ 14 files changed, 148 insertions(+), 76 deletions(-) create mode 100644 peer/banscores.go diff --git a/addrmgr/addrmanager.go b/addrmgr/addrmanager.go index 816fe36fd..1f15cf79e 100644 --- a/addrmgr/addrmanager.go +++ b/addrmgr/addrmanager.go @@ -162,10 +162,10 @@ const ( // to a getAddr. If we have less than this amount, we send everything. getAddrMin = 50 - // getAddrMax is the most addresses that we will send in response + // GetAddrMax is the most addresses that we will send in response // to a getAddr (in practise the most addresses we will return from a // call to AddressCache()). - getAddrMax = 2500 + GetAddrMax = 2500 // getAddrPercent is the percentage of total addresses known that we // will share with a call to AddressCache. @@ -839,8 +839,8 @@ func (a *AddrManager) AddressCache(includeAllSubnetworks bool, subnetworkID *sub } numAddresses := len(allAddr) * getAddrPercent / 100 - if numAddresses > getAddrMax { - numAddresses = getAddrMax + if numAddresses > GetAddrMax { + numAddresses = GetAddrMax } if len(allAddr) < getAddrMin { numAddresses = len(allAddr) diff --git a/blockdag/dag.go b/blockdag/dag.go index c5b58cd0c..4493c5ae4 100644 --- a/blockdag/dag.go +++ b/blockdag/dag.go @@ -165,7 +165,7 @@ type BlockDAG struct { // // This function is safe for concurrent access. func (dag *BlockDAG) IsKnownBlock(hash *daghash.Hash) bool { - return dag.IsInDAG(hash) || dag.IsKnownOrphan(hash) || dag.isKnownDelayedBlock(hash) + return dag.IsInDAG(hash) || dag.IsKnownOrphan(hash) || dag.isKnownDelayedBlock(hash) || dag.IsKnownInvalid(hash) } // AreKnownBlocks returns whether or not the DAG instances has all blocks represented diff --git a/netsync/manager.go b/netsync/manager.go index e0c9be792..977f8cfb4 100644 --- a/netsync/manager.go +++ b/netsync/manager.go @@ -376,9 +376,8 @@ func (sm *SyncManager) handleTxMsg(tmsg *txMsg) { // If we didn't ask for this transaction then the peer is misbehaving. txID := tmsg.tx.ID() if _, exists = state.requestedTxns[*txID]; !exists { - log.Warnf("Got unrequested transaction %s from %s -- "+ - "disconnecting", txID, peer.Addr()) - peer.Disconnect() + peer.AddBanScoreAndPushRejectMsg(wire.CmdTx, wire.RejectNotRequested, (*daghash.Hash)(txID), + peerpkg.BanScoreUnrequestedTx, 0, fmt.Sprintf("got unrequested transaction %s", txID)) return } @@ -412,19 +411,25 @@ func (sm *SyncManager) handleTxMsg(tmsg *txMsg) { // When the error is a rule error, it means the transaction was // simply rejected as opposed to something actually going wrong, // so log it as such. Otherwise, something really did go wrong, - // so log it as an actual error. - if errors.As(err, &mempool.RuleError{}) { - log.Debugf("Rejected transaction %s from %s: %s", - txID, peer, err) - } else { - log.Errorf("Failed to process transaction %s: %s", - txID, err) + // so panic. + ruleErr := &mempool.RuleError{} + if !errors.As(err, ruleErr) { + panic(errors.Wrapf(err, "failed to process transaction %s", txID)) } - // Convert the error into an appropriate reject message and - // send it. - code, reason := mempool.ErrToRejectErr(err) - peer.PushRejectMsg(wire.CmdTx, code, reason, (*daghash.Hash)(txID), false) + shouldIncreaseBanScore := false + if txRuleErr := (&mempool.TxRuleError{}); errors.As(ruleErr.Err, txRuleErr) { + if txRuleErr.RejectCode == wire.RejectInvalid { + shouldIncreaseBanScore = true + } + } else if dagRuleErr := (&blockdag.RuleError{}); errors.As(ruleErr.Err, dagRuleErr) { + shouldIncreaseBanScore = true + } + + if shouldIncreaseBanScore { + peer.AddBanScoreAndPushRejectMsg(wire.CmdTx, wire.RejectInvalid, (*daghash.Hash)(txID), + peerpkg.BanScoreInvalidTx, 0, fmt.Sprintf("rejected transaction %s: %s", txID, err)) + } return } @@ -480,9 +485,8 @@ func (sm *SyncManager) handleBlockMsg(bmsg *blockMsg) { // mode in this case so the DAG code is actually fed the // duplicate blocks. if sm.dagParams != &dagconfig.RegressionNetParams { - log.Warnf("Got unrequested block %s from %s -- "+ - "disconnecting", blockHash, peer.Addr()) - peer.Disconnect() + peer.AddBanScoreAndPushRejectMsg(wire.CmdBlock, wire.RejectNotRequested, blockHash, + peerpkg.BanScoreUnrequestedBlock, 0, fmt.Sprintf("got unrequested block %s", blockHash)) return } } @@ -518,13 +522,8 @@ func (sm *SyncManager) handleBlockMsg(bmsg *blockMsg) { log.Infof("Rejected block %s from %s: %s", blockHash, peer, err) - // Convert the error into an appropriate reject message and - // send it. - code, reason := mempool.ErrToRejectErr(err) - peer.PushRejectMsg(wire.CmdBlock, code, reason, blockHash, false) - - // Disconnect from the misbehaving peer. - peer.Disconnect() + peer.AddBanScoreAndPushRejectMsg(wire.CmdBlock, wire.RejectInvalid, blockHash, + peerpkg.BanScoreInvalidBlock, 0, fmt.Sprintf("got invalid block: %s", err)) return } @@ -718,6 +717,10 @@ func (sm *SyncManager) handleInvMsg(imsg *invMsg) { } if iv.IsBlockOrSyncBlock() { + if sm.dag.IsKnownInvalid(iv.Hash) { + peer.AddBanScoreAndPushRejectMsg(imsg.inv.Command(), wire.RejectInvalid, iv.Hash, peerpkg.BanScoreInvalidInvBlock, 0, fmt.Sprintf("sent inv of invalid block %s", iv.Hash)) + return + } // The block is an orphan block that we already have. // When the existing orphan was processed, it requested // the missing parent blocks. When this scenario @@ -913,9 +916,9 @@ func (sm *SyncManager) handleSelectedTipMsg(msg *selectedTipMsg) { selectedTipHash := msg.selectedTipHash state := sm.peerStates[peer] if !state.peerShouldSendSelectedTip { - log.Warnf("Got unrequested selected tip message from %s -- "+ - "disconnecting", peer.Addr()) - peer.Disconnect() + peer.AddBanScoreAndPushRejectMsg(wire.CmdSelectedTip, wire.RejectNotRequested, nil, + peerpkg.BanScoreUnrequestedSelectedTip, 0, "got unrequested selected tip message") + return } state.peerShouldSendSelectedTip = false if selectedTipHash.IsEqual(peer.SelectedTipHash()) { diff --git a/peer/banscores.go b/peer/banscores.go new file mode 100644 index 000000000..d5a7dc1d8 --- /dev/null +++ b/peer/banscores.go @@ -0,0 +1,34 @@ +package peer + +// Ban scores for misbehaving nodes +const ( + BanScoreUnrequestedBlock = 100 + BanScoreInvalidBlock = 100 + BanScoreInvalidInvBlock = 100 + + BanScoreUnrequestedSelectedTip = 20 + BanScoreUnrequestedTx = 20 + BanScoreInvalidTx = 100 + + BanScoreMalformedMessage = 10 + + BanScoreNonVersionFirstMessage = 1 + BanScoreDuplicateVersion = 1 + BanScoreDuplicateVerack = 1 + + BanScoreSentTooManyAddresses = 20 + BanScoreMsgAddrWithInvalidSubnetwork = 10 + + BanScoreInvalidFeeFilter = 100 + BanScoreNoFilterLoaded = 5 + + BanScoreInvalidMsgGetBlockInvs = 10 + + BanScoreInvalidMsgBlockLocator = 100 + + BanScoreSentTxToBlocksOnly = 20 + + BanScoreNodeBloomFlagViolation = 100 + + BanScoreStallTimeout = 1 +) diff --git a/peer/peer.go b/peer/peer.go index d5288f6ae..90c8a34ec 100644 --- a/peer/peer.go +++ b/peer/peer.go @@ -199,6 +199,13 @@ type Config struct { // the DAG. IsInDAG func(*daghash.Hash) bool + // AddBanScore increases the persistent and decaying ban score fields by the + // values passed as parameters. If the resulting score exceeds half of the ban + // threshold, a warning is logged including the reason provided. Further, if + // the score is above the ban threshold, the peer will be banned and + // disconnected. + AddBanScore func(persistent, transient uint32, reason string) + // HostToNetAddress returns the netaddress for the given host. This can be // nil in which case the host will be parsed as an IP address. HostToNetAddress HostToNetAddrFunc @@ -646,6 +653,22 @@ func (p *Peer) IsSelectedTipKnown() bool { return !p.cfg.IsInDAG(p.selectedTipHash) } +// AddBanScore increases the persistent and decaying ban score fields by the +// values passed as parameters. If the resulting score exceeds half of the ban +// threshold, a warning is logged including the reason provided. Further, if +// the score is above the ban threshold, the peer will be banned and +// disconnected. +func (p *Peer) AddBanScore(persistent, transient uint32, reason string) { + p.cfg.AddBanScore(persistent, transient, reason) +} + +// AddBanScoreAndPushRejectMsg increases ban score and sends a +// reject message to the misbehaving peer. +func (p *Peer) AddBanScoreAndPushRejectMsg(command string, code wire.RejectCode, hash *daghash.Hash, persistent, transient uint32, reason string) { + p.PushRejectMsg(command, code, reason, hash, true) + p.cfg.AddBanScore(persistent, transient, reason) +} + // LastSend returns the last send time of the peer. // // This function is safe for concurrent access. @@ -1239,9 +1262,7 @@ out: continue } - log.Debugf("Peer %s appears to be stalled or "+ - "misbehaving, %s timeout -- "+ - "disconnecting", p, command) + p.AddBanScore(BanScoreStallTimeout, 0, fmt.Sprintf("got timeout for command %s", command)) p.Disconnect() break } @@ -1316,15 +1337,15 @@ out: log.Errorf(errMsg) } - // Push a reject message for the malformed message and wait for - // the message to be sent before disconnecting. + // Add ban score, push a reject message for the malformed message + // and wait for the message to be sent before disconnecting. // // NOTE: Ideally this would include the command in the header if // at least that much of the message was valid, but that is not // currently exposed by wire, so just used malformed for the // command. - p.PushRejectMsg("malformed", wire.RejectMalformed, errMsg, nil, - true) + p.AddBanScoreAndPushRejectMsg("malformed", wire.RejectMalformed, nil, + BanScoreMalformedMessage, 0, errMsg) } break out } @@ -1336,18 +1357,18 @@ out: switch msg := rmsg.(type) { case *wire.MsgVersion: - p.PushRejectMsg(msg.Command(), wire.RejectDuplicate, - "duplicate version message", nil, true) - break out + reason := "duplicate version message" + p.AddBanScoreAndPushRejectMsg(msg.Command(), wire.RejectDuplicate, nil, + BanScoreDuplicateVersion, 0, reason) case *wire.MsgVerAck: // No read lock is necessary because verAckReceived is not written // to in any other goroutine. if p.verAckReceived { - log.Infof("Already received 'verack' from peer %s -- "+ - "disconnecting", p) - break out + p.AddBanScoreAndPushRejectMsg(msg.Command(), wire.RejectDuplicate, nil, + BanScoreDuplicateVerack, 0, "verack sent twice") + log.Warnf("Already received 'verack' from peer %s", p) } p.markVerAckReceived() if p.cfg.Listeners.OnVerAck != nil { @@ -1867,6 +1888,8 @@ func (p *Peer) readRemoteVersionMsg() error { errStr := "A version message must precede all others" log.Errorf(errStr) + p.AddBanScore(BanScoreNonVersionFirstMessage, 0, errStr) + rejectMsg := wire.NewMsgReject(msg.Command(), wire.RejectMalformed, errStr) return p.writeMessage(rejectMsg) diff --git a/server/p2p/on_addr.go b/server/p2p/on_addr.go index e80e5cf12..a75e38de9 100644 --- a/server/p2p/on_addr.go +++ b/server/p2p/on_addr.go @@ -1,6 +1,8 @@ package p2p import ( + "fmt" + "github.com/kaspanet/kaspad/addrmgr" "github.com/kaspanet/kaspad/config" "github.com/kaspanet/kaspad/peer" "github.com/kaspanet/kaspad/wire" @@ -18,10 +20,16 @@ func (sp *Peer) OnAddr(_ *peer.Peer, msg *wire.MsgAddr) { return } + if len(msg.AddrList) > addrmgr.GetAddrMax { + sp.AddBanScoreAndPushRejectMsg(msg.Command(), wire.RejectInvalid, nil, + peer.BanScoreSentTooManyAddresses, 0, fmt.Sprintf("address count excceeded %d", addrmgr.GetAddrMax)) + return + } + if msg.IncludeAllSubnetworks { - peerLog.Errorf("Got unexpected IncludeAllSubnetworks=true in [%s] command from %s", - msg.Command(), sp.Peer) - sp.Disconnect() + sp.AddBanScoreAndPushRejectMsg(msg.Command(), wire.RejectInvalid, nil, + peer.BanScoreMsgAddrWithInvalidSubnetwork, 0, + fmt.Sprintf("got unexpected IncludeAllSubnetworks=true in [%s] command", msg.Command())) return } else if !msg.SubnetworkID.IsEqual(config.ActiveConfig().SubnetworkID) && msg.SubnetworkID != nil { peerLog.Errorf("Only full nodes and %s subnetwork IDs are allowed in [%s] command, but got subnetwork ID %s from %s", diff --git a/server/p2p/on_fee_filter.go b/server/p2p/on_fee_filter.go index 5b015bd28..0749d6034 100644 --- a/server/p2p/on_fee_filter.go +++ b/server/p2p/on_fee_filter.go @@ -1,6 +1,7 @@ package p2p import ( + "fmt" "github.com/kaspanet/kaspad/peer" "github.com/kaspanet/kaspad/util" "github.com/kaspanet/kaspad/wire" @@ -14,9 +15,8 @@ import ( func (sp *Peer) OnFeeFilter(_ *peer.Peer, msg *wire.MsgFeeFilter) { // Check that the passed minimum fee is a valid amount. if msg.MinFee < 0 || msg.MinFee > util.MaxSompi { - peerLog.Debugf("Peer %s sent an invalid feefilter '%s' -- "+ - "disconnecting", sp, util.Amount(msg.MinFee)) - sp.Disconnect() + sp.AddBanScoreAndPushRejectMsg(msg.Command(), wire.RejectInvalid, nil, + peer.BanScoreInvalidFeeFilter, 0, fmt.Sprintf("sent an invalid feefilter '%s'", util.Amount(msg.MinFee))) return } diff --git a/server/p2p/on_filter_add.go b/server/p2p/on_filter_add.go index 132962990..dd8930246 100644 --- a/server/p2p/on_filter_add.go +++ b/server/p2p/on_filter_add.go @@ -17,9 +17,8 @@ func (sp *Peer) OnFilterAdd(_ *peer.Peer, msg *wire.MsgFilterAdd) { } if sp.filter.IsLoaded() { - peerLog.Debugf("%s sent a filteradd request with no filter "+ - "loaded -- disconnecting", sp) - sp.Disconnect() + sp.AddBanScoreAndPushRejectMsg(wire.CmdFilterAdd, wire.RejectInvalid, nil, + peer.BanScoreNoFilterLoaded, 0, "sent a filteradd request with no filter loaded") return } diff --git a/server/p2p/on_filter_clear.go b/server/p2p/on_filter_clear.go index ba3f2e836..bb80ce557 100644 --- a/server/p2p/on_filter_clear.go +++ b/server/p2p/on_filter_clear.go @@ -17,9 +17,8 @@ func (sp *Peer) OnFilterClear(_ *peer.Peer, msg *wire.MsgFilterClear) { } if !sp.filter.IsLoaded() { - peerLog.Debugf("%s sent a filterclear request with no "+ - "filter loaded -- disconnecting", sp) - sp.Disconnect() + sp.AddBanScoreAndPushRejectMsg(wire.CmdFilterClear, wire.RejectInvalid, nil, + peer.BanScoreNoFilterLoaded, 0, "sent a filterclear request with no filter loaded") return } diff --git a/server/p2p/on_get_block_invs.go b/server/p2p/on_get_block_invs.go index c120c50d6..1fdd0b7e7 100644 --- a/server/p2p/on_get_block_invs.go +++ b/server/p2p/on_get_block_invs.go @@ -1,6 +1,7 @@ package p2p import ( + "fmt" "github.com/kaspanet/kaspad/peer" "github.com/kaspanet/kaspad/wire" ) @@ -23,8 +24,9 @@ func (sp *Peer) OnGetBlockInvs(_ *peer.Peer, msg *wire.MsgGetBlockInvs) { hashList, err := dag.AntiPastHashesBetween(msg.LowHash, msg.HighHash, wire.MaxInvPerMsg) if err != nil { - peerLog.Warnf("Error getting antiPast hashes between %s and %s: %s", msg.LowHash, msg.HighHash, err) - sp.Disconnect() + sp.AddBanScoreAndPushRejectMsg(wire.CmdGetBlockInvs, wire.RejectInvalid, nil, + peer.BanScoreInvalidMsgGetBlockInvs, 0, + fmt.Sprintf("error getting antiPast hashes between %s and %s: %s", msg.LowHash, msg.HighHash, err)) return } diff --git a/server/p2p/on_get_block_locator.go b/server/p2p/on_get_block_locator.go index 52d1f2d66..e88cd73f4 100644 --- a/server/p2p/on_get_block_locator.go +++ b/server/p2p/on_get_block_locator.go @@ -11,13 +11,13 @@ import ( func (sp *Peer) OnGetBlockLocator(_ *peer.Peer, msg *wire.MsgGetBlockLocator) { locator, err := sp.server.DAG.BlockLocatorFromHashes(msg.HighHash, msg.LowHash) if err != nil || len(locator) == 0 { - warning := fmt.Sprintf("Couldn't build a block locator between blocks "+ - "%s and %s that was requested from peer %s", msg.HighHash, msg.LowHash, sp) if err != nil { - warning = fmt.Sprintf("%s: %s", warning, err) + peerLog.Warnf("Couldn't build a block locator between blocks "+ + "%s and %s that was requested from peer %s: %s", msg.HighHash, msg.LowHash, sp, err) } - peerLog.Warnf(warning) - sp.Disconnect() + sp.AddBanScoreAndPushRejectMsg(msg.Command(), wire.RejectInvalid, nil, + peer.BanScoreInvalidMsgBlockLocator, 0, + fmt.Sprintf("couldn't build a block locator between blocks %s and %s", msg.HighHash, msg.LowHash)) return } diff --git a/server/p2p/on_inv.go b/server/p2p/on_inv.go index 3de26fbe7..3d47e45ca 100644 --- a/server/p2p/on_inv.go +++ b/server/p2p/on_inv.go @@ -23,9 +23,8 @@ func (sp *Peer) OnInv(_ *peer.Peer, msg *wire.MsgInv) { if invVect.Type == wire.InvTypeTx { peerLog.Tracef("Ignoring tx %s in inv from %s -- "+ "blocksonly enabled", invVect.Hash, sp) - peerLog.Infof("Peer %s is announcing "+ - "transactions -- disconnecting", sp) - sp.Disconnect() + sp.AddBanScoreAndPushRejectMsg(msg.Command(), wire.RejectNotRequested, invVect.Hash, + peer.BanScoreSentTxToBlocksOnly, 0, "announced transactions when blocksonly is enabled") return } err := newInv.AddInvVect(invVect) diff --git a/server/p2p/p2p.go b/server/p2p/p2p.go index 81431a65b..426f3b10c 100644 --- a/server/p2p/p2p.go +++ b/server/p2p/p2p.go @@ -8,6 +8,7 @@ package p2p import ( "crypto/rand" "encoding/binary" + "fmt" "math" "net" "runtime" @@ -328,13 +329,8 @@ func (sp *Peer) pushAddrMsg(addresses []*wire.NetAddress, subnetworkID *subnetwo // the score is above the ban threshold, the peer will be banned and // disconnected. func (sp *Peer) addBanScore(persistent, transient uint32, reason string) { - // No warning is logged and no score is calculated if banning is disabled. - if config.ActiveConfig().DisableBanning { - return - } if sp.isWhitelisted { peerLog.Debugf("Misbehaving whitelisted peer %s: %s", sp, reason) - return } warnThreshold := config.ActiveConfig().BanThreshold >> 1 @@ -348,16 +344,22 @@ func (sp *Peer) addBanScore(persistent, transient uint32, reason string) { } return } + score := sp.DynamicBanScore.Increase(persistent, transient) + logMsg := fmt.Sprintf("Misbehaving peer %s: %s -- ban score increased to %d", + sp, reason, score) if score > warnThreshold { - peerLog.Warnf("Misbehaving peer %s: %s -- ban score increased to %d", - sp, reason, score) - if score > config.ActiveConfig().BanThreshold { + peerLog.Warn(logMsg) + if !config.ActiveConfig().DisableBanning && !sp.isWhitelisted && score > config.ActiveConfig().BanThreshold { peerLog.Warnf("Misbehaving peer %s -- banning and disconnecting", sp) sp.server.BanPeer(sp) sp.Disconnect() } + } else if persistent != 0 { + peerLog.Warn(logMsg) + } else { + peerLog.Trace(logMsg) } } @@ -375,7 +377,7 @@ func (sp *Peer) enforceNodeBloomFlag(cmd string) bool { // Disconnect the peer regardless of whether it was // banned. - sp.addBanScore(100, 0, cmd) + sp.addBanScore(peer.BanScoreNodeBloomFlagViolation, 0, cmd) sp.Disconnect() return false } @@ -937,6 +939,7 @@ func newPeerConfig(sp *Peer) *peer.Config { }, SelectedTipHash: sp.selectedTipHash, IsInDAG: sp.blockExists, + AddBanScore: sp.addBanScore, HostToNetAddress: sp.server.addrManager.HostToNetAddress, Proxy: config.ActiveConfig().Proxy, UserAgentName: userAgentName, diff --git a/wire/msgreject.go b/wire/msgreject.go index e0e090aed..267b4d746 100644 --- a/wire/msgreject.go +++ b/wire/msgreject.go @@ -21,6 +21,7 @@ const ( RejectInvalid RejectCode = 0x10 RejectObsolete RejectCode = 0x11 RejectDuplicate RejectCode = 0x12 + RejectNotRequested RejectCode = 0x13 RejectNonstandard RejectCode = 0x40 RejectDust RejectCode = 0x41 RejectInsufficientFee RejectCode = 0x42 @@ -39,6 +40,7 @@ var rejectCodeStrings = map[RejectCode]string{ RejectInsufficientFee: "REJECT_INSUFFICIENTFEE", RejectFinality: "REJECT_FINALITY", RejectDifficulty: "REJECT_DIFFICULTY", + RejectNotRequested: "REJECT_NOTREQUESTED", } // String returns the RejectCode in human-readable form.