p2p: Disconnect manual peers and not discourage all peers with the same address on getblocktxn msg corruption #25257

MarcoFalke commented at 6:13 PM on May 31, 2022: member

Misbehaving has a lot of issues:

It discourages and disconnects all peers on the same address as the misbehaving peer. This may disconnect honest peers that share the same address. Also, it gives a false sense of security, considering that malicious actors can trivially work around that by using ipv6 addresses or hopping ipv4 addresses in "the cloud", both of which are cheap to do.
It does not disconnect manual connections. This may keep untrusted manual connections connected even though they violate the protocol. It would be better to only keep the connection if the user explicitly asked for the manual connection to be tagged with the noban permission flag.

This is the first change in a series fixing the issues.

For reference, BIP 152 does not specify what should happen when the getblocktxn message is corrupt [1]. However, disconnecting the remote peer seems preferable, to allow them to recover instead of stalling.

[1]: https://en.bitcoin.it/wiki/BIP_0152#getblocktxn:

Upon receipt of a properly-formatted getblocktxn message, nodes which recently provided the sender of such a message a cmpctblock for the block hash identified in this message MUST respond with either an appropriate blocktxn message, or a full block message. A blocktxn response MUST contain exactly and only each transaction which is present in the appropriate block at the index specified in the getblocktxn indexes list, in the order requested.

Disconnect manual peers and not discourage all peers with the same address on getblocktxn msg corruption faead9a2c3

fanquake added the label P2P on May 31, 2022

in src/net_processing.cpp:1449 in faead9a2c3

1442 | @@ -1441,6 +1443,25 @@ void PeerManagerImpl::AddToCompactExtraTransactions(const CTransactionRef& tx)
1443 |      vExtraTxnForCompactIt = (vExtraTxnForCompactIt + 1) % max_extra_txn;
1444 |  }
1445 |  
1446 | +void PeerManagerImpl::MaybeDisconnect(CNode& peer, std::string_view message)
1447 | +{
1448 | +    if (peer.HasPermission(NetPermissionFlags::NoBan)) {
1449 | +        LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "Not disconnecting noban peer %d!\n", peer.GetId());

jonatack commented at 6:42 PM on May 31, 2022:

Working on the logging in this file right now and saw similar messages; this might be clearer

        LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "Not disconnecting misbehaving peer %d because it has noban permission\n", peer.GetId());

sipa commented at 9:26 PM on May 31, 2022: member

My primary issue with Misbehaving is actually neither of the things you bring up, but the fact that the scoring mechanism is arbitrary, and just introduces confusion. We should aim to have a protocol where all behavior is either OK or not OK, without hard-to-reason about levels in between. So I'm much more interested in seeing the scoring mechanism disappear than not using misbehaving/discouraging at all.

Regarding the arguments you bring up:

It's a fair point that IP addresses are cheap, but that doesn't mean per-IP discouragement is useless. At the very least, it helps in avoiding connections to non-malicious but broken peers.
If we're concerned about malicious behavior, the possibility exists for a netgroup-level discouragement too. We already rely on certain (weakish) assumptions around attackers being unable to control many netgroups (as it's the basis for some of the eclipse attack protection in addrman) - so I think we could extend discouragement to this level too. It'd need to be weaker in effect than the per-IP discouragement in order to prevent a malicious peer from partitioning off the entire network it's in, but I do feel that it's not unreasonable to start deprioritizing connections to/from a netgroup if it turns out to host many misbehaving peers.
I don't see what the problem is with punishing all peers with a certain IP when one of them misbehaves. We should be able to assume they're controlled by the same entity?
No objection to dropping the exception to not disconnect manual connections and instead just rely on the NOBAN permission - but I'm unsure how much effect it'll have. At least if we're talking about non-oneshot addnode peers, the connection logic will try to reestablish the connection anyway.

MarcoFalke commented at 6:55 AM on June 1, 2022: member

Thanks for the extended reply.

I don't see what the problem is with punishing all peers with a certain IP when one of them misbehaves. We should be able to assume they're controlled by the same entity?

It is probably more of an edge case, but I imagined a group sharing the same IP (for example a family with several connected devices). Though, I can also see how they can be seen as one entity.

No objection to dropping the exception to not disconnect manual connections and instead just rely on the NOBAN permission - but I'm unsure how much effect it'll have. At least if we're talking about non-oneshot addnode peers, the connection logic will try to reestablish the connection anyway.

I had the same thought, so I was about to add it back to make the changes smaller. Though, given that discouragement isn't useless, it may be best to close this patch and instead focus on removing the misbehaviour score.

MarcoFalke closed this on Jun 1, 2022

MarcoFalke deleted the branch on Jun 1, 2022

luke-jr commented at 1:22 AM on June 18, 2022: member

I don't see what the problem is with punishing all peers with a certain IP when one of them misbehaves. We should be able to assume they're controlled by the same entity?

CGNAT is also a thing, and becoming more and more common :/

DrahtBot locked this on Jun 18, 2023