From 2d50226308437bc6d5ef818d35f1ba09e48a5024 Mon Sep 17 00:00:00 2001 From: Gregory Maxwell Date: Tue, 7 Aug 2018 20:37:45 +0000 Subject: [PATCH 1/2] Introduce a maximum size for locators. The largest sensible size for a locator is log in the number of blocks. But, as noted by Coinr8d on BCT a maximum size message could encode a hundred thousand locators. If height were used to limit the messages that could open new attacks where peers on long low diff forks would get disconnected and end up stuck. Ideally, nodes first first learn to limit the size of locators they send before limiting what would be processed, but common implementations back off with an exponent of 2 and have an implicit limit of 2^32 blocks, so they already cannot produce locators over some size. This sets the limit to an absurdly high amount of 101 in order to maximize compatibility with existing software. Cherry-picked from: e254ff5d --- src/net.h | 2 ++ src/net_processing.cpp | 12 ++++++++++++ 2 files changed, 14 insertions(+) diff --git a/src/net.h b/src/net.h index bbde0e336..03aea02d9 100644 --- a/src/net.h +++ b/src/net.h @@ -53,6 +53,8 @@ static const int TIMEOUT_INTERVAL = 20 * 60; static const int FEELER_INTERVAL = 120; /** The maximum number of entries in an 'inv' protocol message */ static const unsigned int MAX_INV_SZ = 50000; +/** The maximum number of entries in a locator */ +static const unsigned int MAX_LOCATOR_SZ = 101; /** The maximum number of new addresses to accumulate before announcing. */ static const unsigned int MAX_ADDR_TO_SEND = 1000; /** Maximum length of incoming protocol messages (no message over 4 MB is currently acceptable). */ diff --git a/src/net_processing.cpp b/src/net_processing.cpp index cc3b865e5..96360484e 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1630,6 +1630,12 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr uint256 hashStop; vRecv >> locator >> hashStop; + if (locator.vHave.size() > MAX_LOCATOR_SZ) { + LogPrint("net", "getblocks locator size %lld > %d, disconnect peer=%d\n", locator.vHave.size(), MAX_LOCATOR_SZ, pfrom->GetId()); + pfrom->fDisconnect = true; + return true; + } + // We might have announced the currently-being-connected tip using a // compact block, which resulted in the peer sending a getblocks // request, which we would otherwise respond to without the new block. @@ -1741,6 +1747,12 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr uint256 hashStop; vRecv >> locator >> hashStop; + if (locator.vHave.size() > MAX_LOCATOR_SZ) { + LogPrint("net", "getheaders locator size %lld > %d, disconnect peer=%d\n", locator.vHave.size(), MAX_LOCATOR_SZ, pfrom->GetId()); + pfrom->fDisconnect = true; + return true; + } + LOCK(cs_main); if (IsInitialBlockDownload() && !pfrom->fWhitelisted) { LogPrint("net", "Ignoring getheaders from peer=%d because node is in initial block download\n", pfrom->id); From c93ea51273e7fada009c3463fc592105a776a402 Mon Sep 17 00:00:00 2001 From: Patrick Lodder Date: Wed, 8 Aug 2018 11:24:59 -0400 Subject: [PATCH 2/2] qa: Add p2p_invalid_locator test Adds a custom mininode implementation to test connects and disconnects based on the number of allowed locators Inspired by: fa85c985 Original Author: MarcoFalke --- qa/pull-tester/rpc-tests.py | 1 + qa/rpc-tests/p2p_invalid_locator.py | 108 ++++++++++++++++++++++++ qa/rpc-tests/test_framework/mininode.py | 1 + 3 files changed, 110 insertions(+) create mode 100755 qa/rpc-tests/p2p_invalid_locator.py diff --git a/qa/pull-tester/rpc-tests.py b/qa/pull-tester/rpc-tests.py index 373477a10..986a765ca 100755 --- a/qa/pull-tester/rpc-tests.py +++ b/qa/pull-tester/rpc-tests.py @@ -128,6 +128,7 @@ testScripts = [ 'rawtransactions.py', 'reindex.py', # vv Tests less than 30s vv + 'p2p_invalid_locator.py', 'mempool_resurrect_test.py', 'txn_doublespend.py --mineblock', 'txn_clone.py', diff --git a/qa/rpc-tests/p2p_invalid_locator.py b/qa/rpc-tests/p2p_invalid_locator.py new file mode 100755 index 000000000..6de0dd606 --- /dev/null +++ b/qa/rpc-tests/p2p_invalid_locator.py @@ -0,0 +1,108 @@ +#!/usr/bin/env python3 +# Copyright (c) 2015-2017 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +""" +Test node responses to invalid locators. +""" + +from test_framework.mininode import * +from test_framework.util import * +from test_framework.test_framework import BitcoinTestFramework + +class TestNode(SingleNodeConnCB): + def __init__(self): + SingleNodeConnCB.__init__(self) + self.last_headers = None + self.last_block = None + self.last_header_received = None + + def on_block(self, conn, message): + self.last_block = message.block + self.last_block.calc_sha256() + + def on_headers(self, conn, message): + self.last_headers = message + if len(message.headers): + message.headers[-1].calc_sha256() + self.last_header_received = message.headers[-1] + + def on_close(self, conn): + self.disconnected = True + + def wait_for_verack(self): + while True: + with mininode_lock: + if self.verack_received: + return + time.sleep(0.05) + + def wait_for_block(self, blockhash, timeout=60): + test_function = lambda: self.last_block != None and self.last_block.sha256 == blockhash + assert(wait_until(test_function, timeout=timeout)) + return + + def wait_for_header(self, blockhash, timeout=60): + test_function = lambda: self.last_header_received != None and self.last_header_received.sha256 == blockhash + assert(wait_until(test_function, timeout=timeout)) + return + + # wait for the socket to be in a closed state + def wait_for_disconnect(self, timeout=60): + if self.connection == None: + return True + sleep_time = 0.05 + is_closed = self.connection.state == "closed" + while not is_closed and timeout > 0: + time.sleep(sleep_time) + timeout -= sleep_time + is_closed = self.connection.state == "closed" + return is_closed + + +class InvalidLocatorTest(BitcoinTestFramework): + def __init__(self): + self.num_nodes = 1 + self.setup_clean_chain = False + self.connections = [] + self.test_nodes = [] + + def setup_network(self): + self.nodes = [] + self.nodes.append(start_node(0, self.options.tmpdir, ["-debug=net"])) + + for i in range(4): + self.test_nodes.append(TestNode()) + self.connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], self.test_nodes[i])) + self.test_nodes[i].add_connection(self.connections[i]) + + NetworkThread().start() + + def run_test(self): + node = self.nodes[0] # convenience reference to the node + node.generate(1) # Get node out of IBD + + # Test max locator size + block_count = node.getblockcount() + for msg in [msg_getheaders(), msg_getblocks()]: + # Wait for disconnect when sending MAX_LOCATOR_SZ + 1 (102) hashes in locator + test_node = self.test_nodes.pop() + test_node.wait_for_verack() + msg.locator.vHave = [int(node.getblockhash(i - 1), 16) for i in range(block_count, block_count - (MAX_LOCATOR_SZ + 1), -1)] + test_node.connection.send_message(msg) + assert_equal(test_node.wait_for_disconnect(), True) + + # Wait for response when sending MAX_LOCATOR_SZ (101) hashes in locator + test_node = self.test_nodes.pop() + test_node.wait_for_verack() + msg.locator.vHave = [int(node.getblockhash(i - 1), 16) for i in range(block_count, block_count - (MAX_LOCATOR_SZ), -1)] + test_node.connection.send_message(msg) + if type(msg) == msg_getheaders: + test_node.wait_for_header(int(node.getbestblockhash(), 16)) + else: + test_node.wait_for_block(int(node.getbestblockhash(), 16)) + + [ c.disconnect_node() for c in self.connections ] + +if __name__ == '__main__': + InvalidLocatorTest().main() diff --git a/qa/rpc-tests/test_framework/mininode.py b/qa/rpc-tests/test_framework/mininode.py index 964e98a1c..10240e86e 100755 --- a/qa/rpc-tests/test_framework/mininode.py +++ b/qa/rpc-tests/test_framework/mininode.py @@ -45,6 +45,7 @@ MY_SUBVERSION = b"/python-mininode-tester:0.0.3/" MY_RELAY = 1 # from version 70001 onwards, fRelay should be appended to version messages (BIP37) MAX_INV_SZ = 50000 +MAX_LOCATOR_SZ = 101 MAX_BLOCK_BASE_SIZE = 1000000 COIN = 100000000 # mlumin 5/2021: In terms of Dogecoin, 1 dogecoin or 100,000,000 koinu.