diff options
author | MarcoFalke <falke.marco@gmail.com> | 2020-04-20 06:56:56 -0400 |
---|---|---|
committer | MarcoFalke <falke.marco@gmail.com> | 2020-04-20 06:59:53 -0400 |
commit | da4cbb7927497ca3261c1504c3b85dd3f5800673 (patch) | |
tree | bc462d8efcd4db8ebc4762bcd6c60701a3272f55 /test | |
parent | dc5da7fe7b0c51dac269d6c630d9b54581b07900 (diff) | |
parent | a9ecbdfcaa15499644d16e9c8ad2c63dfc45b37b (diff) | |
download | bitcoin-da4cbb7927497ca3261c1504c3b85dd3f5800673.tar.xz |
Merge #18544: net: limit BIP37 filter lifespan (active between 'filterload'..'filterclear')
a9ecbdfcaa15499644d16e9c8ad2c63dfc45b37b test: add more inactive filter tests to p2p_filter.py (Sebastian Falbesoner)
5eae034996b340c19cebab9efb6c89d20fe051ef net: limit BIP37 filter lifespan (active between 'filterload' and 'filterclear') (Sebastian Falbesoner)
Pull request description:
This PR fixes https://github.com/bitcoin/bitcoin/issues/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](https://github.com/bitcoin/bitcoin/pull/18515), according to gmaxwell).
This default match-everything filter leads to some unintended side-effects:
1. `getdata` request for filtered blocks (i.e. type `MSG_FILTERED_BLOCK`) are always responded to with `merkleblock`s, 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
2. 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.
ACKs for top commit:
MarcoFalke:
re-ACK a9ecbdfcaa, only change is in test code 🕙
Tree-SHA512: 1a656a6d74ccaf628e7fdca063ba63fbab2089e0b6d0a11be9bbd387c2ee6d3230706ff8ffc1a55711481df3d4547137dd7c9d9184d89eaa43ade4927792d0b6
Diffstat (limited to 'test')
-rwxr-xr-x | test/functional/p2p_filter.py | 26 |
1 files changed, 22 insertions, 4 deletions
diff --git a/test/functional/p2p_filter.py b/test/functional/p2p_filter.py index 2eeb03bfaa..3879f4ffe6 100755 --- a/test/functional/p2p_filter.py +++ b/test/functional/p2p_filter.py @@ -7,15 +7,17 @@ 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 +from test_framework.util import assert_equal class FilterNode(P2PInterface): @@ -35,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) @@ -103,6 +106,21 @@ class FilterTest(BitcoinTestFramework): txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 7) filter_node.wait_for_tx(txid) + self.log.info('Check that request for filtered blocks is ignored if no filter is set') + filter_node.merkleblock_received = False + 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_BLOCK, int(block_hash, 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) + filter_node.send_and_ping(msg_filteradd(data=b'letsmisbehave')) + assert_equal(self.nodes[0].getpeerinfo()[0]['banscore'], 100) + self.log.info("Check that division-by-zero remote crash bug [CVE-2013-5700] is fixed") filter_node.send_and_ping(msg_filterload(data=b'', nHashFuncs=1)) filter_node.send_and_ping(msg_filteradd(data=b'letstrytocrashthisnode')) |