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

theStack · 2020-04-06T17:58:57Z

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:

bitcoin/src/net.h

Line 812 in c0b389b

TxRelay() { pfilter = MakeUnique<CBloomFilter>(); }

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

bitcoin/src/net_processing.cpp

Line 3201 in c0b389b

pfrom->m_tx_relay->pfilter.reset(new CBloomFilter());

The behaviour was introduced by commit 37c6389 (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 BIP37: 'getdata' request for filtered blocks is answered with 'merkleblock's even if no filter is set #18483 (strictly speaking, this is a violation of BIP37)

bitcoin/src/net_processing.cpp

Lines 1504 to 1507 in c0b389b

    
           if (pfrom->m_tx_relay->pfilter) { 
        
               sendMerkleBlock = true; 
        
               merkleBlock = CMerkleBlock(*pblock, *pfrom->m_tx_relay->pfilter); 
        
           }

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)

bitcoin/src/net_processing.cpp

Lines 3182 to 3186 in c0b389b

    
           if (pfrom->m_tx_relay->pfilter) { 
        
               pfrom->m_tx_relay->pfilter->insert(vData); 
        
           } else { 
        
               bad = true; 
        
           }

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.

maflcko · 2020-04-06T20:49:30Z

Could add a test for the disconnect?

theStack · 2020-04-06T21:55:27Z

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).

…lterclear') 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.

theStack · 2020-04-09T09:29:17Z

Rebased, after #18533 has been merged.

test/functional/p2p_filter.py

maflcko

ACK c4d5ba8 👍

Show signature and timestamp

Signature:

Timestamp of file with hash 94f5012633296cb966cec859d16ca56127a4af35eb21df461ff9a83d033c85d2 -

theStack · 2020-04-13T15:59:28Z

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.

test/functional/p2p_filter.py

theStack · 2020-04-14T12:39:07Z

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/functional/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>

theStack · 2020-04-14T14:45:45Z

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

maflcko · 2020-04-14T14:47:49Z

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

Show signature and timestamp

Signature:

Timestamp of file with hash 7884d646af42356bdba68ff936ba7ce28889d7b45c67195205417d5cf91f47d6 -

DrahtBot · 2020-04-16T23:24:17Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

test: add further BIP37 size limit checks to p2p_filter.py #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.

… 'filterload'..'filterclear') a9ecbdf test: add more inactive filter tests to p2p_filter.py (Sebastian Falbesoner) 5eae034 net: limit BIP37 filter lifespan (active between 'filterload' and 'filterclear') (Sebastian Falbesoner) Pull request description: This PR fixes bitcoin#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 bitcoin@37c6389 (an intentional covert fix for [CVE-2013-5700](bitcoin#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 bitcoin#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 a9ecbdf, only change is in test code 🕙 Tree-SHA512: 1a656a6d74ccaf628e7fdca063ba63fbab2089e0b6d0a11be9bbd387c2ee6d3230706ff8ffc1a55711481df3d4547137dd7c9d9184d89eaa43ade4927792d0b6

jonasschnelli · 2020-04-21T08:03:56Z

This merge broke the master build on bitcoinbuilds.org.
p2p_filter.py seems to fail always since this merge.

See: https://bitcoinbuilds.org/index.php?ansilog=b3d4f4bf-5d4b-4957-9bf3-91ffe4a799fb.log#l2935
Or build 2509: https://bitcoinbuilds.org/index.php?build=2509

c743718 test: add further BIP37 size limit checks to p2p_filter.py (Sebastian Falbesoner) Pull request description: This is a follow-up PR to #18628. In addition to the hash-functions limit test introduced with commit fa4c29b, it adds checks for the following size limits as defined in [BIP37](https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki): ad message type `filterload`: > The filter itself is simply a bit field of arbitrary byte-aligned size. The maximum size is **36,000 bytes**. ad message type `filteradd`: > The data field must be smaller than or equal to **520 bytes** in size (the maximum size of any potentially matched object). Also introduces new constants for the limits (or reuses the max script size constant in case for the `filteradd` limit). Also fixes #18711 by changing the misbehaviour check on "filteradd without filterset" (introduced with #18544) below to also use the more commonly used `assert_debug_log` method. ACKs for top commit: MarcoFalke: ACK c743718 robot-visions: ACK c743718 jonasschnelli: utACK c743718. Seems to fix it: https://bitcoinbuilds.org/index.php?build=2524 Tree-SHA512: a03e7639263eb36a381922afb4e1d0ed2ae286f2ad2e7bbd922509a043ddf6cfd08747e01d54d29bfb8f54b66908f653974b9c347e4ca4f43332b586778893be

maflcko · 2020-04-21T12:00:14Z

Yes, it was a silent merge conflict on the test script. Should be fixed now.

hebasto · 2020-04-21T12:03:40Z

Yes, it was a silent merge conflict on the test script. Should be fixed now.

See #18721

…_filter.py c743718 test: add further BIP37 size limit checks to p2p_filter.py (Sebastian Falbesoner) Pull request description: This is a follow-up PR to bitcoin#18628. In addition to the hash-functions limit test introduced with commit bitcoin@fa4c29b, it adds checks for the following size limits as defined in [BIP37](https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki): ad message type `filterload`: > The filter itself is simply a bit field of arbitrary byte-aligned size. The maximum size is **36,000 bytes**. ad message type `filteradd`: > The data field must be smaller than or equal to **520 bytes** in size (the maximum size of any potentially matched object). Also introduces new constants for the limits (or reuses the max script size constant in case for the `filteradd` limit). Also fixes bitcoin#18711 by changing the misbehaviour check on "filteradd without filterset" (introduced with bitcoin#18544) below to also use the more commonly used `assert_debug_log` method. ACKs for top commit: MarcoFalke: ACK c743718 robot-visions: ACK c743718 jonasschnelli: utACK c743718. Seems to fix it: https://bitcoinbuilds.org/index.php?build=2524 Tree-SHA512: a03e7639263eb36a381922afb4e1d0ed2ae286f2ad2e7bbd922509a043ddf6cfd08747e01d54d29bfb8f54b66908f653974b9c347e4ca4f43332b586778893be

… 'filterload'..'filterclear') a9ecbdf test: add more inactive filter tests to p2p_filter.py (Sebastian Falbesoner) 5eae034 net: limit BIP37 filter lifespan (active between 'filterload' and 'filterclear') (Sebastian Falbesoner) Pull request description: This PR fixes bitcoin#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 bitcoin@37c6389 (an intentional covert fix for [CVE-2013-5700](bitcoin#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 bitcoin#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 a9ecbdf, only change is in test code 🕙 Tree-SHA512: 1a656a6d74ccaf628e7fdca063ba63fbab2089e0b6d0a11be9bbd387c2ee6d3230706ff8ffc1a55711481df3d4547137dd7c9d9184d89eaa43ade4927792d0b6

…rclear') Summary: > 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. > 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 > > 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> This is a backport of Core [[bitcoin/bitcoin#18544 | PR18544]] I had to change the last test to use a different way to detect misbehavior, because banscore was deprecated in D8798 (this would have been applied in [[bitcoin/bitcoin#18672 | PR18672]], if the backports had been done in sequential order) Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8949

…n (active between 'filterload'..'filterclear') (#4043) * partial backport 18544: net: limit BIP37 filter lifespan (active between 'filterload'..'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. * net: Match the backport PR a bit more Co-authored-by: xdustinface <xdustinfacex@gmail.com>

DrahtBot added P2P Tests labels Apr 6, 2020

theStack force-pushed the 20200406-net-limit_bip37_filter_lifetime branch from 82b8976 to 1f3566e Compare April 6, 2020 21:48

theStack force-pushed the 20200406-net-limit_bip37_filter_lifetime branch from 1f3566e to c4d5ba8 Compare April 9, 2020 09:28

chanhosuh reviewed Apr 13, 2020

View reviewed changes