Merge #18515: test: add BIP37 remote crash bug [CVE-2013-5700] test to p2p_filter.py

0ed2d8e test: add BIP37 remote crash bug [CVE-2013-5700] test to p2p_filter.py (Sebastian Falbesoner)

Pull request description:

  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](https://cve.mitre.org/cgi-bin/cvename.cgi?name=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 bitcoin/bitcoin@37c6389 (an intentional covert fix, [according to gmaxwell](bitcoin/bitcoin#18483 (comment))), 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:
  ```diff
  diff --git a/src/bloom.cpp b/src/bloom.cpp
  index bd6069b..ef294a3 100644
  --- a/src/bloom.cpp
  +++ b/src/bloom.cpp
  @@ -199,8 +199,8 @@ void CBloomFilter::UpdateEmptyFull()
           full &= vData[i] == 0xff;
           empty &= vData[i] == 0;
       }
  -    isFull = full;
  -    isEmpty = empty;
  +    isFull = false;
  +    isEmpty = false;
   }
  ```
  Resulting in:
  ```
  $ ./p2p_filter.py
  [...]
  2020-04-03T14:38:59.593000Z TestFramework (INFO): Check that division-by-zero remote crash bug [CVE-2013-5700] is fixed
  2020-04-03T14:38:59.695000Z TestFramework (ERROR): Assertion failed
  [...]
  [... some exceptions following ...]
  ```

ACKs for top commit:
  naumenkogs:
    utACK 0ed2d8e

Tree-SHA512: 02d0253d13eab70c4bd007b0750c56a5a92d05d419d53033523eeb3ed80318bc95196ab90f7745ea3ac9ebae7caee3adbf2a055a40a4124e0915226e49018fe8

Author: MarcoFalke

test/functional/p2p_filter.py

@@ -11,6 +11,7 @@
     MSG_FILTERED_BLOCK,
     msg_getdata,
     msg_filterload,
+    msg_filteradd,
     msg_filterclear,
 )
 from test_framework.mininode import (
@@ -103,6 +104,10 @@ def run_test(self):
             txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 7)
             filter_node.wait_for_tx(txid)
 
+        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'))
+
 
 if __name__ == '__main__':
     FilterTest().main()

test/functional/test_framework/messages.py

@@ -1356,6 +1356,25 @@ def __repr__(self):
             self.data, self.nHashFuncs, self.nTweak, self.nFlags)
 
 
+class msg_filteradd:
+    __slots__ = ("data")
+    command = b"filteradd"
+
+    def __init__(self, data):
+        self.data = data
+
+    def deserialize(self, f):
+        self.data = deser_string(f)
+
+    def serialize(self):
+        r = b""
+        r += ser_string(self.data)
+        return r
+
+    def __repr__(self):
+        return "msg_filteradd(data={})".format(self.data)
+
+
 class msg_filterclear:
     __slots__ = ()
     command = b"filterclear"

test/functional/test_framework/mininode.py

@@ -30,6 +30,7 @@
     msg_blocktxn,
     msg_cmpctblock,
     msg_feefilter,
+    msg_filteradd,
     msg_filterclear,
     msg_filterload,
     msg_getaddr,
@@ -65,6 +66,7 @@
     b"blocktxn": msg_blocktxn,
     b"cmpctblock": msg_cmpctblock,
     b"feefilter": msg_feefilter,
+    b"filteradd": msg_filteradd,
     b"filterclear": msg_filterclear,
     b"filterload": msg_filterload,
     b"getaddr": msg_getaddr,
@@ -324,6 +326,7 @@ def on_block(self, message): pass
     def on_blocktxn(self, message): pass
     def on_cmpctblock(self, message): pass
     def on_feefilter(self, message): pass
+    def on_filteradd(self, message): pass
     def on_filterclear(self, message): pass
     def on_filterload(self, message): pass
     def on_getaddr(self, message): pass