aboutsummaryrefslogtreecommitdiff
path: root/test
diff options
context:
space:
mode:
authorMarcoFalke <falke.marco@gmail.com>2020-04-05 21:17:50 +0800
committerMarcoFalke <falke.marco@gmail.com>2020-04-05 21:18:11 +0800
commitcf21293ef7fd304efb0843abeb17adc6159ca927 (patch)
tree8416e2c02e04e9e78b7b09f7348997150f6569eb /test
parent96a30b98c925b4ca63993759ddf7b4dd1fa58ec1 (diff)
parent0ed2d8e07d3806d78d03a77d2153f22f9d733a07 (diff)
Merge #18515: test: add BIP37 remote crash bug [CVE-2013-5700] test to p2p_filter.py
0ed2d8e07d3806d78d03a77d2153f22f9d733a07 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 https://github.com/bitcoin/bitcoin/commit/37c6389c5a0ca63ae3573440ecdfe95d28ad8f07 (an intentional covert fix, [according to gmaxwell](https://github.com/bitcoin/bitcoin/issues/18483#issuecomment-608224095)), 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 0ed2d8e07d3806d78d03a77d2153f22f9d733a07 Tree-SHA512: 02d0253d13eab70c4bd007b0750c56a5a92d05d419d53033523eeb3ed80318bc95196ab90f7745ea3ac9ebae7caee3adbf2a055a40a4124e0915226e49018fe8
Diffstat (limited to 'test')
-rwxr-xr-xtest/functional/p2p_filter.py5
-rwxr-xr-xtest/functional/test_framework/messages.py19
-rwxr-xr-xtest/functional/test_framework/mininode.py3
3 files changed, 27 insertions, 0 deletions
diff --git a/test/functional/p2p_filter.py b/test/functional/p2p_filter.py
index 2940542e5e..188b130a57 100755
--- a/test/functional/p2p_filter.py
+++ b/test/functional/p2p_filter.py
@@ -11,6 +11,7 @@ from test_framework.messages import (
MSG_FILTERED_BLOCK,
msg_getdata,
msg_filterload,
+ msg_filteradd,
msg_filterclear,
)
from test_framework.mininode import (
@@ -103,6 +104,10 @@ class FilterTest(BitcoinTestFramework):
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()
diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py
index ff0c763b72..5f8fcc6fd8 100755
--- a/test/functional/test_framework/messages.py
+++ b/test/functional/test_framework/messages.py
@@ -1356,6 +1356,25 @@ class msg_filterload:
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"
diff --git a/test/functional/test_framework/mininode.py b/test/functional/test_framework/mininode.py
index ce51513ce9..ad330f2a93 100755
--- a/test/functional/test_framework/mininode.py
+++ b/test/functional/test_framework/mininode.py
@@ -30,6 +30,7 @@ from test_framework.messages import (
msg_blocktxn,
msg_cmpctblock,
msg_feefilter,
+ msg_filteradd,
msg_filterclear,
msg_filterload,
msg_getaddr,
@@ -65,6 +66,7 @@ MESSAGEMAP = {
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 @@ class P2PInterface(P2PConnection):
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