p2p: Disconnect, not discourage, misbehaving NODE

MarcoFalke commented at 3:15 pm on October 5, 2020: member

This continues the story started in commit 3a10d935ac8ebabdfd336569d943f042ff84b13e

p2p: Disconnect, not discourage, misbehaving NODE_BLOOM peers

Receiving an oversized FILERLOAD or FILTERADD message is about as costly
as receiving any large-sized unknown message. Light clients are already
preferred for eviction, so no need to additionally discourage them.

faf3044029

in src/net_processing.cpp:3719 in fa79ba7663 outdated

3717             LOCK(pfrom.m_tx_relay->cs_filter);
3718             if (pfrom.m_tx_relay->pfilter) {
3719                 pfrom.m_tx_relay->pfilter->insert(vData);
3720             } else {
3721-                bad = true;
3722+                pfrom.fDisconnect = true;

Xekyo commented at 3:32 pm on October 5, 2020:

This behavior seems to have previously triggered log messages too-large bloom filter or bad filteradd message. I see that in other instances of pfrom.fDisconect it seems to be often preceded by log messages, e.g. LogPrintf("Disconnecting outbound peer %d -- headers chain has insufficient work\n", pfrom.GetId());.

Should there be a log message here for the disconnect as well?

jnewbery commented at 8:42 am on December 17, 2020:

I think it’s a good idea to log to the NET category whenever net_processing disconnects a peer.

Perhaps we should make this into a function CConnman::DisconnectPeer(CNode& peer, std::string msg) and require a log message.

jnewbery commented at 3:49 pm on October 5, 2020: member

Can you add a motivation to the PR description please? :)

MarcoFalke force-pushed on Oct 5, 2020

DrahtBot added the label P2P on Oct 5, 2020

MarcoFalke added the label Waiting for author on Oct 5, 2020

DrahtBot commented at 4:13 am on December 17, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20676 (Replace m_tx_relay/nullptr checks in net_processing.cpp by sdaftuar)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/net_processing.cpp:3714 in faf3044029

3708@@ -3709,20 +3709,16 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
3709 
3710         // Nodes must NEVER send a data item > 520 bytes (the max size for a script data object,
3711         // and thus, the maximum size any matched object can have) in a filteradd message
3712-        bool bad = false;
3713         if (vData.size() > MAX_SCRIPT_ELEMENT_SIZE) {
3714-            bad = true;
3715+            pfrom.fDisconnect = true;
3716         } else if (pfrom.m_tx_relay != nullptr) {

jnewbery commented at 8:45 am on December 17, 2020:

Should we also disconnect if pfrom.m_tx_relay == nullptr? That means this is a block-relay-only connection that we opened and the peer has asked us to load a bloom filter. Disconnecting seems like the polite thing to do.

Maybe out of scope for this PR.

jnewbery commented at 11:34 am on December 17, 2020:

Actually this is impossible, since we’ll never advertise NODE_BLOOM for outbound peers.

in test/functional/p2p_filter.py:122 in faf3044029

129-        with self.nodes[0].assert_debug_log(['Misbehaving']):
130-            filter_peer.send_and_ping(msg_filteradd(data=b'\xcc'*(MAX_SCRIPT_ELEMENT_SIZE+1)))
131+        with self.nodes[0].assert_debug_log(['disconnecting peer']):
132+            filter_peer = self.send_and_wait_for_disconnect(filter_peer, msg_filteradd(data=b'\xcc' * (MAX_SCRIPT_ELEMENT_SIZE + 1)))
133 
134         filter_peer.send_and_ping(msg_filterclear())

jnewbery commented at 8:47 am on December 17, 2020:

Maybe not required now that the connection gets reset in the previous test?

in test/functional/p2p_filter.py:200 in faf3044029

197@@ -197,21 +198,25 @@ def test_filter(self, filter_peer):
198             assert not filter_peer.tx_received
199 
200         self.log.info('Check that sending "filteradd" if no filter is set is treated as misbehavior')

jnewbery commented at 8:49 am on December 17, 2020:

Maybe change the word misbehavior here to avoid ambiguity with the function in net_processing.

in test/functional/p2p_filter.py:209 in faf3044029

206         self.log.info("Check that division-by-zero remote crash bug [CVE-2013-5700] is fixed")
207         filter_peer.send_and_ping(msg_filterload(data=b'', nHashFuncs=1))
208         filter_peer.send_and_ping(msg_filteradd(data=b'letstrytocrashthisnode'))
209         self.nodes[0].disconnect_p2ps()
210 
211+    def send_and_wait_for_disconnect(self, filter_peer, msg):

jnewbery commented at 8:50 am on December 17, 2020:

I like this function, and think it could be generally useful in other tests, but the name is slightly confusing for me, since it doesn’t indicate that we’ll also reconnect.

jnewbery commented at 10:42 am on December 17, 2020: member

I’m not sure if this is an improvement. Setting fDisconnect directly bypasses the logic in MaybeDiscourageAndDisconnect() that prevents us from disconnecting peers with PF_NOBAN or manual connections. Is that intentional?

MarcoFalke commented at 10:43 am on December 17, 2020: member

Sorry, this was opened accidentally (not ready yet)

MarcoFalke closed this on Dec 17, 2020

DrahtBot locked this on Feb 15, 2022

MarcoFalke deleted the branch on Aug 15, 2022

p2p: Disconnect, not discourage, misbehaving NODE_BLOOM peers #20083

Conflicts