BIP37: 'getdata' request for filtered blocks is answered with 'merkleblock's even if no filter is set

theStack commented at 3:03 pm on March 31, 2020: member

According to BIP37, getdata commands with a request for filtered blocks via type MSG_FILTERED_BLOCK in the inv submessage are only responded to if a filter is set:

The getdata command is extended to allow a new type in the inv submessage. The type field can now be MSG_FILTERED_BLOCK (== 3) rather than MSG_BLOCK. If no filter has been set on the connection, a request for filtered blocks is ignored. If a filter has been set, a merkleblock message is returned for the requested block hash.

Expected behavior

When no BIP37 filter is set and a we request a filtered block, there should be no response from the node.

Actual behavior

When no BIP37 filter is set and a we request a filtered block, we get a merkleblock message in reply.

To reproduce

The behaviour can reproduced by adding the following lines to the functional test p2p_filter.py (introduced recently with PR #18334, commit https://github.com/bitcoin/bitcoin/pull/18334/commits/fa156999695ddaeb016d8320bee62f8d96679d55), that has already a callback implemented which requests filtered blocks as reply to inv messages:

0    # add this lines after filtered P2P connection is added to the node, but before filter is set
1    filter_node.merkleblock_received = False
2    self.nodes[0].generatetoaddress(1, self.node[0].getnewaddress())[0]
3    filter_node.sync_with_ping()
4    assert not filter_node.merkleblock_received

The assertion fails, meaning that a merkleblock was indeed received.

Possible solution Looking at the code in net_processing.cpp/net.h, it is quite obvious why this happens: the condition to answer to a filtered block request is just whether the (unique) pointer pfrom->m_tx_relay->pfilter is set, and this condition is always true. Initially the pointer is set in the constructor of the struct TxRelay, pointing to a default-constructed instance of CBloomFilter: https://github.com/bitcoin/bitcoin/blob/d52ba21dfff99173abb927bc964ce7ceb711d789/src/net.h#L812 And after receiving a filterclear message it is also reset to an empty instance again: https://github.com/bitcoin/bitcoin/blob/d52ba21dfff99173abb927bc964ce7ceb711d789/src/net_processing.cpp#L3201 In the following code, the boolean variable sendMerkleBlock will thus always be set to true: https://github.com/bitcoin/bitcoin/blob/d52ba21dfff99173abb927bc964ce7ceb711d789/src/net_processing.cpp#L1500-L1508

A simple solution would be to just set pfilter to nullptr initially and after receiving a filterclear message. The question is if its worth the change, as the described behaviour is IMHO not harmful at all (it just doesn’t fit the description in the BIP37 document, which could be adapted?) and connection bloom filters are deprecated anway and not recommended to be used due to privacy concerns. It is noteworthy though that possibly other parts in net_processing.cpp assume the test on pfilter means “a bloom filter has been set”, and the fact that this always evaluates to true could possibly lead to some unintended behaviour. For example, in the following code part, where a NodeEvictionCandidate is created, its member fBloomFilter will always be set to true: https://github.com/bitcoin/bitcoin/blob/d52ba21dfff99173abb927bc964ce7ceb711d789/src/net.cpp#L864-L875

theStack added the label Bug on Mar 31, 2020

theStack commented at 3:14 pm on March 31, 2020: member

To show another example of the consequences of the “pfilter is never nullptr”-inaccuracy, in the following filteradd-processing code, the last else-branch is dead code: https://github.com/bitcoin/bitcoin/blob/d52ba21dfff99173abb927bc964ce7ceb711d789/src/net_processing.cpp#L3177-L3187

MarcoFalke added the label P2P on Mar 31, 2020

MarcoFalke commented at 4:41 pm on March 31, 2020: member

A simple solution would be to just set pfilter to nullptr initially and after receiving a filterclear message.

I remember chatting to @naumenkogs about this when reviewing #17164 due to memory usage concerns. The empty constructor leaves the vData member untouched https://doxygen.bitcoincore.org/bloom_8h_source.html#l00067. So at least it doesn’t waste too much memory. Though, the logic bugs should be fixed in the long term and we should add test coverage for all net processing code.

MarcoFalke added this to the milestone 0.21.0 on Mar 31, 2020

sipa commented at 5:55 pm on March 31, 2020: member

Is the errorneously sent merkleblock empty, or does it have transactions too?

MarcoFalke commented at 6:17 pm on March 31, 2020: member

CBloomFilter() : isFull(true), so the block should be full as well?

sipa commented at 6:21 pm on March 31, 2020: member

Could this be related to #18324 ?

naumenkogs commented at 10:51 pm on March 31, 2020: member

Thanks for the very well-described issue, it’s indeed an odd behavior. #18324 is a good example of why this issue matters (even if the cause of #18324 is something else). We don’t want to waste bandwidth like that. I think we should make the code follow the BIP, and add the test.

Sending a post to the mailing list would be a good idea too I guess.

theStack commented at 12:32 pm on April 1, 2020: member

Could this be related to #18324 ?

The issue described could be the cause of #18324 in the unlikely case that – for whatever reason – the filterload message sent from the client doesn’t arrive at the node. Then obviously no filter is set but due to the bug described in this issue, requested filtered blocks leads to getting all blocks. @naumenkogs: Okay, will look deeper into the issue then what possible side-effects a possible bugfix would have and how to test this thoroughly. Posting to the mailing list is also a good plan.

schildbach commented at 12:54 pm on April 1, 2020: contributor

I think it should be prevented in any case that an empty filtered block is sent, or any filtered block resulting from some arbitrary filter operation. Because this would lead to missed transactions for wallets, fixable only by replaying the entire blockchain (from wallet birthdate).

Looking at the issue description, I think the expected behaviour would be the connection should be dropped. It simply makes no sense to request a filtered block with an undefined filter.

schildbach commented at 1:29 pm on April 1, 2020: contributor

@theStack Just curious, what kind of client did you see this first? Is it based on bitcoinj? If yes, can you see the version from the user agent?

theStack commented at 1:58 pm on April 1, 2020: member

Looking at the issue description, I think the expected behaviour would be the connection should be dropped. It simply makes no sense to request a filtered block with an undefined filter.

Expected was meant in the sense of “matches the description in the BIP”. While I agree that dropping the connection would be a good idea (requesting filtered blocks without filter clearly doesn’t make sense), I don’t know though if this would need a change of the BIP. Maybe some deeper in the BIP process can comment on this. Will anyways send out a mailing list post soon to reach more people in this discussion.

@theStack Just curious, what kind of client did you see this first? Is it based on bitcoinj? If yes, can you see the version from the user agent?

Unfortunately not. I didn’t use a real client, but noticed the behaviour while experimenting with the new BIP37 test in the functional test framework, then digged into the network processing code to verify this.

theStack commented at 2:33 pm on April 2, 2020: member

After digging deeper into the issue, I found out that commit https://github.com/bitcoin/bitcoin/commit/37c6389c5a0ca63ae3573440ecdfe95d28ad8f07 (particularly https://github.com/bitcoin/bitcoin/commit/37c6389c5a0ca63ae3573440ecdfe95d28ad8f07#diff-7ec3c68a81efff79b6ca22ac1f1eabba and https://github.com/bitcoin/bitcoin/commit/37c6389c5a0ca63ae3573440ecdfe95d28ad8f07#diff-1a8b9d1ad0a6fda5e751286c73102fc2) introduced this behaviour, first showing up in v0.8.4. As the commit message says “It sets a default match everything filter for peers […]”, i.e. the state of “no filter set” is not available anymore since then.

MarcoFalke commented at 2:40 pm on April 2, 2020: member

I can’t follow the part where it replaces NULL with CBloomFilter{}. That part should probably just be reverted?

theStack commented at 5:49 pm on April 2, 2020: member

I can’t follow the part where it replaces NULL with CBloomFilter{}. That part should probably just be reverted?

I also don’t get what the advantage of having a default match-everything-filter is over not having a filter. At a first glance at the code, reverting that part would be unproblematic in my opinion. Maybe @gmaxwell as the author of the commit could comment on what the reason was to introduce this change?

P.S.: Also posted about the issue on the mailing list now: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2020-April/017743.html (Just as quick info for anyone not subscribed there)

sipa commented at 6:05 pm on April 2, 2020: member

@theStack That commit was a fix for CVE-2013-5700 (a division by zero in the Bloom filter code), FWIW.

gmaxwell commented at 4:35 am on April 3, 2020: contributor

The change in behaviour was an intentional covert fix for the remote crasher bug introduced by BIP37. It obviously could be changed now the vulnerably was made public, but I seem to recall that prior discussion around cleaning up that code was just answered by comments about the desire to deactivate and eventually remove BIP37 entirely.

theStack commented at 12:08 pm on April 3, 2020: member

@gmaxwell: Thanks for clarifying. I think it makes sense to add a functional test checking that this vulnerability remains fixed in the course of changing the behaviour back. I couldn’t find any concrete description how to trigger this remote crash, but looking into bloom.cpp it seems as simple as:

sending a filterload message with zero-length data (//EDIT: and nHashFuncs >= 1)
sending a random filteradd message

That should be sufficient to trigger a modulo-by-0 operation in the following line? https://github.com/bitcoin/bitcoin/blob/f0d6487e290761a4fb03798240a351b5fddfdb38/src/bloom.cpp#L45

MarcoFalke referenced this in commit cf21293ef7 on Apr 5, 2020

MarcoFalke referenced this in commit da4cbb7927 on Apr 20, 2020

MarcoFalke closed this on Apr 20, 2020

sidhujag referenced this in commit eeacf95574 on Apr 20, 2020

ComputerCraftr referenced this in commit 0a8fbef869 on Jun 10, 2020

MarcoFalke locked this on Feb 15, 2022

vijaydasmp referenced this in commit 892a85af86 on May 6, 2023

BIP37: ‘getdata’ request for filtered blocks is answered with ‘merkleblock’s even if no filter is set #18483