net: limit BIP37 filter lifespan (active between 'filterload'..'filterclear') #18544

theStack commented at 5:58 PM on April 6, 2020: member

This PR fixes #18483. On the master branch, there is currently always a BIP37 filter set for every peer: if not a specific filter is set through a filterload message, a default match-everything filter is instanciated and pointed to via the CBloomFilter default constructor; that happens both initially, when the containing structure TxRelay is constructed:

https://github.com/bitcoin/bitcoin/blob/c0b389b33516fb3eaaad7c30bd11dba768882a7e/src/net.h#L812

and after a loaded filter is removed again through a filterclear message:

https://github.com/bitcoin/bitcoin/blob/c0b389b33516fb3eaaad7c30bd11dba768882a7e/src/net_processing.cpp#L3201

The behaviour was introduced by commit https://github.com/bitcoin/bitcoin/commit/37c6389c5a0ca63ae3573440ecdfe95d28ad8f07 (an intentional covert fix for CVE-2013-5700, according to gmaxwell).

This default match-everything filter leads to some unintended side-effects:

getdata request for filtered blocks (i.e. type MSG_FILTERED_BLOCK) are always responded to with merkleblocks, even if no filter was set by the peer, see issue #18483 (strictly speaking, this is a violation of BIP37) https://github.com/bitcoin/bitcoin/blob/c0b389b33516fb3eaaad7c30bd11dba768882a7e/src/net_processing.cpp#L1504-L1507
if a peer sends a filteradd message without having loaded a filter via filterload before, the intended increasing of the banscore never happens (triggered if bad is set to true, a few lines below) https://github.com/bitcoin/bitcoin/blob/c0b389b33516fb3eaaad7c30bd11dba768882a7e/src/net_processing.cpp#L3182-L3186

This PR basically activates the else-branch code paths for all checks of pfilter again (on the master branch, they are dead code) by limiting the pointer's lifespan: instead of always having a filter set, the pfilter is only pointing to a CBloomFilter-instance after receiving a filterload message and the instance is destroyed again (and the pointer nullified) after receiving a filterclear message.

Here is a before/after comparison in behaviour:

code part / scenario	master branch	PR branch
`getdata` processing for `MSG_FILTERED_BLOCK`	always responds with `merkleblock`	only responds if filter was set via `filterload`
`filteradd` processing, no filter was loaded	nothing	peer's banscore increases by 100 (i.e. disconnect)

On the other code parts where pfilter is checked there is no change in the logic behaviour (except that CBloomFilter::IsRelevantAndUpdate() is unnecessarily called and immediately returned in the master branch). Note that the default constructor of CBloomFilter is only used for deserializing the received filterload message and nowhere else. The PR also contains a functional test checking that sending getdata for filtered blocks is ignored by the node if no bloom filter is set.

DrahtBot added the label P2P on Apr 6, 2020

DrahtBot added the label Tests on Apr 6, 2020

MarcoFalke commented at 8:49 PM on April 6, 2020: member

Could add a test for the disconnect?

theStack force-pushed on Apr 6, 2020

theStack commented at 9:55 PM on April 6, 2020: member

Could add a test for the disconnect?

Indeed, I added a test checking that sending filteradd is treated as misbehavior by the node through getting the peer's banscore via RPC getpeerinfo before and after (As the node is started with noban on localhost, no disconnect/ban happens).

net: limit BIP37 filter lifespan (active between 'filterload' and 'filterclear')

Previously, a default match-everything bloom filter was set for every peer,
i.e. even before receiving a 'filterload' message and after receiving a
'filterclear' message code branches checking for the existence of the filter
by testing the pointer "pfilter" were _always_ executed.

5eae034996

theStack force-pushed on Apr 9, 2020

theStack commented at 9:29 AM on April 9, 2020: member

Rebased, after #18533 has been merged.

in test/functional/p2p_filter.py:114 in c4d5ba83b2 outdated

 104 | @@ -104,6 +105,18 @@ def run_test(self):
 105 |              txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 7)
 106 |              filter_node.wait_for_tx(txid)
 107 |  
 108 | +        self.log.info('Check that request for filtered blocks is ignored if no filter is set')
 109 | +        filter_node.merkleblock_received = False
 110 | +        self.nodes[0].generatetoaddress(1, self.nodes[0].getnewaddress())
 111 | +        filter_node.sync_with_ping()
 112 | +        filter_node.sync_with_ping()

chanhosuh commented at 2:19 AM on April 13, 2020:

I see Marco's "wait enough pings" approach is becoming a pattern now.. ;) . Might I suggest wrapping this up into a nicely-named method? Not only will this help future contributors understand intent but if anyone ever comes up with a better approach, they can easily replace all the right instances of multiple sync_with_pings.

MarcoFalke commented at 2:11 PM on April 13, 2020:

Why is this calling sync_with_ping? Shouldn't it send a getdata(blockhash, MSG_FILTERED_BLOCK)?

theStack commented at 3:21 PM on April 13, 2020:

Sending of getdata happens in the on_inv() callback method, i.e. as reply to an received inv message from the node. The sync_with_pings are there to have some implicit delay to ensure there was enough time for the inv/getdata exchange and a (in this case, fictive) merkleblock message to arrive, so that the assert below has a value. (If you remember, we discussed in PR #18334 about this multiple-ping approach; I was sceptical but your explanation led me to the conclusion that there is no better way of doing this.)

MarcoFalke commented at 3:29 PM on April 13, 2020:

Oh, I didn't realize that inv's and getdata's are already sent back and forth. Maybe clarify this with an assert_debug_log?

theStack commented at 3:29 PM on April 13, 2020:

Hi chanhosuh, thanks for reviewing and the suggestion! Seems like you have followed the discussion in PR #18334 where this pattern first appeared :) I like the idea of a method, but don't have an idea of a good descriptive name. I anyways think it would be too much stuffing this also in this PR. May you want to come up with a follow-up PR?

theStack commented at 3:55 PM on April 13, 2020:

That's a good idea. I haven't used assert_debug_log before but from what I can see it waits for the expected messages, i.e. after such an debug-log-assert block with the expected messages of 'sending inv' and 'received getdata' we have the guarantee that the node has already received the getdata request and one following sync_with_ping should be enough. :tada:

MarcoFalke commented at 4:23 PM on April 13, 2020:

Yeah, it happens to wait for it to appear, but we shouldn't rely on that when possible. This has been added only as an "emergency measure" for rpc_setban.py. ;)

chanhosuh commented at 11:25 PM on April 13, 2020:

I don't have an idea for a good name either :). Anyway, the recent changes have seemingly deprecated this topic.

MarcoFalke removed the label Tests on Apr 13, 2020

MarcoFalke added this to the milestone 0.21.0 on Apr 13, 2020

MarcoFalke approved

MarcoFalke commented at 2:13 PM on April 13, 2020: member

ACK c4d5ba83b2ca5781821cc288295da81156a28277 👍

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 94f5012633296cb966cec859d16ca56127a4af35eb21df461ff9a83d033c85d2 -

</details>

theStack force-pushed on Apr 13, 2020

theStack commented at 3:59 PM on April 13, 2020: member

Updated the functional test commit to also check for the inv/getdata exchange via assert_debug_log, as suggested by MarcoFalke above. Also added the check that no tx is received on a request for filtered blocks if no filter is set.

in test/functional/p2p_filter.py:111 in 7d6cd75b4f outdated

 104 | @@ -104,6 +105,20 @@ def run_test(self):
 105 |              txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 7)
 106 |              filter_node.wait_for_tx(txid)
 107 |  
 108 | +        self.log.info('Check that request for filtered blocks is ignored if no filter is set')
 109 | +        filter_node.merkleblock_received = False
 110 | +        filter_node.tx_received = False
 111 | +        with self.nodes[0].assert_debug_log(expected_msgs=['sending inv', 'received getdata'], timeout=10):

MarcoFalke commented at 4:31 PM on April 13, 2020:

        with self.nodes[0].assert_debug_log(expected_msgs=['sending inv', 'received getdata']):

I don't really like having the debug log as a synchronisation mechanism when we have proper mininode methods like wait_for_inv to achieve the same.

theStack force-pushed on Apr 13, 2020

in test/functional/p2p_filter.py:111 in fb0434a9b1 outdated

 104 | @@ -104,6 +105,20 @@ def run_test(self):
 105 |              txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 7)
 106 |              filter_node.wait_for_tx(txid)
 107 |  
 108 | +        self.log.info('Check that request for filtered blocks is ignored if no filter is set')
 109 | +        filter_node.merkleblock_received = False
 110 | +        filter_node.tx_received = False
 111 | +        with self.nodes[0].assert_debug_log(expected_msgs=['sending inv', 'received getdata']):

MarcoFalke commented at 6:07 PM on April 13, 2020:

I know I suggested to use assert_debug_log for the inv and getdata, but a better solution would be to use wait_for_inv, which also checks the hash:

diff --git a/test/functional/p2p_filter.py b/test/functional/p2p_filter.py
index 0b338caa9a..6a8f54e376 100755
--- a/test/functional/p2p_filter.py
+++ b/test/functional/p2p_filter.py
@@ -7,12 +7,13 @@ Test BIP 37
 """
 
 from test_framework.messages import (
+    CInv,
     MSG_BLOCK,
     MSG_FILTERED_BLOCK,
-    msg_getdata,
-    msg_filterload,
     msg_filteradd,
     msg_filterclear,
+    msg_filterload,
+    msg_getdata,
 )
 from test_framework.mininode import P2PInterface
 from test_framework.test_framework import BitcoinTestFramework
@@ -103,10 +104,11 @@ class FilterTest(BitcoinTestFramework):
         filter_node.merkleblock_received = False
         filter_node.tx_received = False
         with self.nodes[0].assert_debug_log(expected_msgs=['sending inv', 'received getdata'], timeout=10):
-            self.nodes[0].generatetoaddress(1, self.nodes[0].getnewaddress())
-        filter_node.sync_with_ping()
-        assert not filter_node.merkleblock_received
-        assert not filter_node.tx_received
+            h = self.nodes[0].generatetoaddress(1, self.nodes[0].getnewaddress())[0]
+            filter_node.wait_for_inv([CInv(MSG_FILTERED_BLOCK, int(h, 16))])
+            filter_node.sync_with_ping()
+            assert not filter_node.merkleblock_received
+            assert not filter_node.tx_received
 
         self.log.info('Check that sending "filteradd" if no filter is set is treated as misbehavior (+100)')
         assert_equal(self.nodes[0].getpeerinfo()[0]['banscore'], 0)

theStack commented at 9:02 PM on April 13, 2020:

Alright :+1: The assert_debug_log can then be removed completely? I force-pushed an update.

MarcoFalke commented at 9:37 PM on April 13, 2020:

I'd say to keep it for the 'received getdata', but no strong opinion

theStack commented at 12:40 PM on April 14, 2020:

Done.

theStack force-pushed on Apr 13, 2020

theStack force-pushed on Apr 14, 2020

theStack commented at 12:39 PM on April 14, 2020: member

Updated with @MarcoFalke's suggestion of explicitely waiting for an inv with specified blockhash via wait_for_inv(...), and checking that getdata was received by the node via assert_debug_log(...).

test: add more inactive filter tests to p2p_filter.py

check the following expected behaviors if no filter is set:
-> filtered block requests are ignored by the node
-> sending a 'filteradd' message is treated as misbehavior
   (i.e. the peer's banscore increases by 100)

also fixes a bug in the on_inv() callback method, which
directly modified the type from BLOCK to FILTERED_BLOCK
in the received 'inv' message rather than just for the reply

Co-authored-by: MarcoFalke <falke.marco@gmail.com>

a9ecbdfcaa

in test/functional/p2p_filter.py:114 in 3c76e61a71 outdated

 105 | @@ -104,6 +106,21 @@ def run_test(self):
 106 |              txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 7)
 107 |              filter_node.wait_for_tx(txid)
 108 |  
 109 | +        self.log.info('Check that request for filtered blocks is ignored if no filter is set')
 110 | +        filter_node.merkleblock_received = False
 111 | +        filter_node.tx_received = False
 112 | +        with self.nodes[0].assert_debug_log(expected_msgs=['received getdata']):
 113 | +            block_hash = self.nodes[0].generatetoaddress(1, self.nodes[0].getnewaddress())[0]
 114 | +            filter_node.wait_for_inv([CInv(MSG_FILTERED_BLOCK, int(block_hash, 16))])

MarcoFalke commented at 1:52 PM on April 14, 2020:

Sorry, I found a bug in my test code.

diff --git a/test/functional/p2p_filter.py b/test/functional/p2p_filter.py
index 39050d98dd..7f8da2ba90 100755
--- a/test/functional/p2p_filter.py
+++ b/test/functional/p2p_filter.py
@@ -37,8 +37,9 @@ class FilterNode(P2PInterface):
         for i in message.inv:
             # inv messages can only contain TX or BLOCK, so translate BLOCK to FILTERED_BLOCK
             if i.type == MSG_BLOCK:
-                i.type = MSG_FILTERED_BLOCK
-            want.inv.append(i)
+                want.inv.append(CInv(MSG_FILTERED_BLOCK, i.hash))
+            else:
+                want.inv.append(i)
         if len(want.inv):
             self.send_message(want)
 
@@ -105,7 +106,7 @@ class FilterTest(BitcoinTestFramework):
         filter_node.tx_received = False
         with self.nodes[0].assert_debug_log(expected_msgs=['received getdata']):
             block_hash = self.nodes[0].generatetoaddress(1, self.nodes[0].getnewaddress())[0]
-            filter_node.wait_for_inv([CInv(MSG_FILTERED_BLOCK, int(block_hash, 16))])
+            filter_node.wait_for_inv([CInv(MSG_BLOCK, int(block_hash, 16))])
             filter_node.sync_with_ping()
             assert not filter_node.merkleblock_received
             assert not filter_node.tx_received

MarcoFalke commented at 1:53 PM on April 14, 2020:

Reason is that invs are only allowed to be BLOCK or TX, nothing else

theStack commented at 2:34 PM on April 14, 2020:

Oh, that's true. But I'm a bit confused on why it was still possible that the test passed? Was the code in on_inv() directly modifying the object that was stored in the last_message dictionary?

MarcoFalke commented at 2:39 PM on April 14, 2020:

Yes, in python everything is a reference (like a shared pointer, but as a reference). So when modifying a member variable or an element in a list or so, it is going to modify all other instances that point to that list or object.

MarcoFalke commented at 2:40 PM on April 14, 2020:

The test passed because it checked for MSG_FILTERED_BLOCK, whereas it should have tested for MSG_BLOCK

theStack commented at 2:58 PM on April 14, 2020:

Thanks for the explanation, that makes sense. I didn't consider that in Python every name is a reference :see_no_evil:

theStack force-pushed on Apr 14, 2020

theStack commented at 2:45 PM on April 14, 2020: member

Force-pushed again, now also fixes the bug described in #18544 (review). Updated the commit message to also describe the bugfix and include MarcoFalke as co-author.

MarcoFalke commented at 2:47 PM on April 14, 2020: member

re-ACK a9ecbdfcaa, only change is in test code 🕙

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 7884d646af42356bdba68ff936ba7ce28889d7b45c67195205417d5cf91f47d6 -

</details>

DrahtBot commented at 11:24 PM on April 16, 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:

#18672 (test: add further BIP37 size limit checks to p2p_filter.py by theStack)

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.

MarcoFalke merged this on Apr 20, 2020

MarcoFalke closed this on Apr 20, 2020

sidhujag referenced this in commit eeacf95574 on Apr 20, 2020