Integrates the missing message type filteradd to the test framework and checks that the BIP37 implementation is not vulnerable to the “remote crash bug” CVE-2013-5700 anymore. Prior to v.0.8.4, it was possible to trigger a division-by-zero error on the following line in the function CBloomFilter::Hash():
https://github.com/bitcoin/bitcoin/blob/f0d6487e290761a4fb03798240a351b5fddfdb38/src/bloom.cpp#L45
By setting a zero-length filter via filterload, vData.size() is 0, so the modulo operation above, called on any .insert() or .contains() operation then crashed the node. The test uses the approach of just sending an arbitrary filteradd message after, which calls CBloomFilter::insert() (and in turn CBloomFilter::Hash()) on the node. The vulnerability was fixed by commit https://github.com/bitcoin/bitcoin/commit/37c6389c5a0ca63ae3573440ecdfe95d28ad8f07 (an intentional covert fix, according to gmaxwell), which introduced flags isEmpty/isFull that wouldn’t call the Hash() member function if isFull is true (set to true by default constructor).
To validate that the test fails if the implementation is vulnerable, one can simply set the flags to false in the member function UpdateEmptyFull() (that is called after a filter received via filterload is constructed), which activates the vulnerable code path calling Hash in any case on adding or testing for data in the filter:
0diff --git a/src/bloom.cpp b/src/bloom.cpp
1index bd6069b..ef294a3 100644
2--- a/src/bloom.cpp
3+++ b/src/bloom.cpp
4@@ -199,8 +199,8 @@ void CBloomFilter::UpdateEmptyFull()
5 full &= vData[i] == 0xff;
6 empty &= vData[i] == 0;
7 }
8- isFull = full;
9- isEmpty = empty;
10+ isFull = false;
11+ isEmpty = false;
12 }
Resulting in:
0$ ./p2p_filter.py
1[...]
22020-04-03T14:38:59.593000Z TestFramework (INFO): Check that division-by-zero remote crash bug [CVE-2013-5700] is fixed
32020-04-03T14:38:59.695000Z TestFramework (ERROR): Assertion failed
4[...]
5[... some exceptions following ...]
