Merge #20995: fuzz: Avoid initializing version to less than MIN_PEER_PROTO_VERSION

MarcoFalke · MarcoFalke · commit 4d5eaf7a90bc · 2021-01-28T15:13:25.000+01:00
fad3d76 fuzz: Avoid initializing version to less than MIN_PEER_PROTO_VERSION (MarcoFalke) fa99e33 fuzz: move-only FillNode implementation to cpp file (MarcoFalke) Pull request description: This fixes a fuzz bug introduced in #20881. Previously the nodes in the fuzz tests had their version initialized to a constant (`PROTOCOL_VERSION`). After #20881, the nodes have their version initialized to an arbitrary signed integer. This is problematic for several reasons: * Both `nVersion` and `m_greatest_common_version` may be initialized to `0`. If a `version` message is processed, this leads to a crash, because `m_greatest_common_version` must be `INIT_PROTO_VERSION` while the `version` message is processed. See #20138 * The "valid" range for `nVersion` is `[MIN_PEER_PROTO_VERSION, std::numeric_limits<int32_t>::max()]` (see check in net_processing) * The "valid" range for `m_greatest_common_version` is `std::min(nVersion, PROTOCOL_VERSION)` (see net_processing) Fix all issues by initializing `nVersion` and `m_greatest_common_version` to their valid ranges. ----- The crashers, if someone wants to try this at home: ``` ( echo 'dmVyc2lvbgAWFhYWFhYWFhYWFhYWFhYWFhYWFhZp/29uAPX//xYWFhYWFhYWFhYWFhYWFhYWFhYW FhYWFhYWaW9uAOr1//8WFhYWFha0ZXJzaW9uAPX//wAAAAAAABAAAAAAAAAAAAC0ZXJzaW9uAPX/ /wBPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT08AAAAAABAAAAAAAAAAAAAAAAAA AAAAAAAAAAAAAAAAAAAAAAAAAAAACgAAAAAAAAAAgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA AAAAAAAAAAAAAAB2ZXJzaW9uAACDJIO9vXYKAAAAAAAAAAAAAAAAAAAAAAB2ZfS1qmu1qhUVFWs=' | base64 --decode > /tmp/a ) && FUZZ=process_message_version ./src/test/fuzz/fuzz /tmp/a ``` ``` ( echo 'dmVyc2lvbgD//wAhTmiqN///NDcAAACENDL/iv//8DYAAHL///////79/RtcAJqamhqa/QEAAAD/ ///+/f1oZWFkZXJzAAAAAM8BAAAAIAYibkYRGgtZyq8SaGVhZGVycwAAAAD/NDcAAACENDL/iv// 8DYAAHL///////79/RtcAJqamhqa/QEAAAD////+/f1oZWFkZXJzAAAAAM8BAAAAIAYibkYRGgtZ yq8SaGVhZGVycwAAAADPAQAAACAGIm5GERoLWS1wb3J061u/KMNPOkwFXqZ///b5IgIAAD+5ubkb XD5hZGRyAJqamhqasP0BAAAAAAAAAP0BAAAAIf39/R0dHQAAAAAAMgAA///7//+gXqZ///b5IgIA AD+5ubm5ubm5AAAAAAAAAAAAAAAAAAAAAAAAAACAAAAAAAAAFgAAAAAAAAAAAAlBmv39/f1/f39B f39hZGRyAG5vAACaLgAdGzY2zwEAAAAgBiJuRhEaC1ktcG9ydOtbvyjDTzpMBV6mf//2+SICAAA/ ubm5G1w+YWRkcgCampoamrD9AQAAAAAAAAD9AQAAACH9/f0dHR0AAAAAADIAAP//+///oF6mf//2 +SICAAA/ubm5ubm5uQAAAAAAAAAAAAAAAAAAAAAAAAAAgAAAAAAAABYAAAAAAAAAAAAJQZr9/f39 f39/QX9/YWRkcgBubwAAmi4AHRs2NjY2NjY2NjYCAgI2NgIA/f39/f39Nv39/TUmABxc' | base64 --decode > /tmp/b ) && FUZZ=process_message_version ./src/test/fuzz/fuzz /tmp/b ``` ACKs for top commit: practicalswift: cr ACK fad3d76 Tree-SHA512: ea64ee99b94d8e619e3949d2d21252c1236412c0e40f44f2b73595ca70cd2da0bdab005fb1a54f65fb291e7b07fdd33577ce4a3a078ca933246b511ebcb0e52a
diff --git a/src/Makefile.test_fuzz.include b/src/Makefile.test_fuzz.include
@@ -16,6 +16,7 @@ libtest_fuzz_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(MINIUPNPC_CPPFLAG
 libtest_fuzz_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
 libtest_fuzz_a_SOURCES = \
   test/fuzz/fuzz.cpp \
+  test/fuzz/util.cpp \
   $(TEST_FUZZ_H)
 
 LIBTEST_FUZZ += $(LIBBITCOIN_SERVER)
diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp
@@ -60,10 +60,12 @@ void fuzz_target(const std::vector<uint8_t>& buffer, const std::string& LIMIT_TO
         return;
     }
     CNode& p2p_node = *ConsumeNodeAsUniquePtr(fuzzed_data_provider).release();
-    FillNode(fuzzed_data_provider, p2p_node);
-    p2p_node.fSuccessfullyConnected = true;
+
+    const bool successfully_connected{true};
+    p2p_node.fSuccessfullyConnected = successfully_connected;
     connman.AddTestNode(p2p_node);
     g_setup->m_node.peerman->InitializeNode(&p2p_node);
+    FillNode(fuzzed_data_provider, p2p_node, /* init_version */ successfully_connected);
 
     const auto mock_time = ConsumeTime(fuzzed_data_provider);
     SetMockTime(mock_time);
diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp
@@ -45,11 +45,12 @@ FUZZ_TARGET_INIT(process_messages, initialize_process_messages)
     for (int i = 0; i < num_peers_to_add; ++i) {
         peers.push_back(ConsumeNodeAsUniquePtr(fuzzed_data_provider, i).release());
         CNode& p2p_node = *peers.back();
-        FillNode(fuzzed_data_provider, p2p_node);
 
-        p2p_node.fSuccessfullyConnected = true;
+        const bool successfully_connected{true};
+        p2p_node.fSuccessfullyConnected = successfully_connected;
         p2p_node.fPauseSend = false;
         g_setup->m_node.peerman->InitializeNode(&p2p_node);
+        FillNode(fuzzed_data_provider, p2p_node, /* init_version */ successfully_connected);
 
         connman.AddTestNode(p2p_node);
     }
diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp
@@ -0,0 +1,25 @@
+// Copyright (c) 2021 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <test/fuzz/util.h>
+#include <version.h>
+
+void FillNode(FuzzedDataProvider& fuzzed_data_provider, CNode& node, bool init_version) noexcept
+{
+    const ServiceFlags remote_services = ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS);
+    const NetPermissionFlags permission_flags = ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS);
+    const int32_t version = fuzzed_data_provider.ConsumeIntegralInRange<int32_t>(MIN_PEER_PROTO_VERSION, std::numeric_limits<int32_t>::max());
+    const bool filter_txs = fuzzed_data_provider.ConsumeBool();
+
+    node.nServices = remote_services;
+    node.m_permissionFlags = permission_flags;
+    if (init_version) {
+        node.nVersion = version;
+        node.SetCommonVersion(std::min(version, PROTOCOL_VERSION));
+    }
+    if (node.m_tx_relay != nullptr) {
+        LOCK(node.m_tx_relay->cs_filter);
+        node.m_tx_relay->fRelayTxes = filter_txs;
+    }
+}
diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h
@@ -320,24 +320,9 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<N
         return CNode{node_id, local_services, socket, address, keyed_net_group, local_host_nonce, addr_bind, addr_name, conn_type, inbound_onion};
     }
 }
-inline std::unique_ptr<CNode> ConsumeNodeAsUniquePtr(FuzzedDataProvider& fdp, const std::optional<NodeId>& node_id_in = nullopt) { return ConsumeNode<true>(fdp, node_id_in); }
-
-inline void FillNode(FuzzedDataProvider& fuzzed_data_provider, CNode& node, const std::optional<int32_t>& version_in = std::nullopt) noexcept
-{
-    const ServiceFlags remote_services = ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS);
-    const NetPermissionFlags permission_flags = ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS);
-    const int32_t version = version_in.value_or(fuzzed_data_provider.ConsumeIntegral<int32_t>());
-    const bool filter_txs = fuzzed_data_provider.ConsumeBool();
-
-    node.nServices = remote_services;
-    node.m_permissionFlags = permission_flags;
-    node.nVersion = version;
-    node.SetCommonVersion(version);
-    if (node.m_tx_relay != nullptr) {
-        LOCK(node.m_tx_relay->cs_filter);
-        node.m_tx_relay->fRelayTxes = filter_txs;
-    }
-}
+inline std::unique_ptr<CNode> ConsumeNodeAsUniquePtr(FuzzedDataProvider& fdp, const std::optional<NodeId>& node_id_in = std::nullopt) { return ConsumeNode<true>(fdp, node_id_in); }
+
+void FillNode(FuzzedDataProvider& fuzzed_data_provider, CNode& node, bool init_version) noexcept;
 
 template <class T = const BasicTestingSetup>
 std::unique_ptr<T> MakeFuzzingContext(const std::string& chain_name = CBaseChainParams::REGTEST, const std::vector<const char*>& extra_args = {})
