[2rdzy]

Summary

Backport of Bitcoin Core bitcoin#35550 ("net_processing: fix BIP152 first integer interpretation"). BIP152 says the first integer of sendcmpct "is interpreted as a boolean and MUST have a value of either 1 or 0". Knots reads it as a bool, so any byte from 2 to 255 is silently accepted and treated as true. This change reads the byte as a uint8_t and, if it is greater than 1, calls Misbehaving() and returns before any peer state is touched. It is a protocol-conformance fix; a peer sending 2 currently gets the same behaviour as a peer sending 1, so there is no known exploit.

Problem

src/net_processing.cpp:3750-3763 (base 58398baf33):

bool sendcmpct_hb{false};
uint64_t sendcmpct_version{0};
vRecv >> sendcmpct_hb >> sendcmpct_version;
...
nodestate->m_requested_hb_cmpctblocks = sendcmpct_hb;
...
pfrom.m_bip152_highbandwidth_from = sendcmpct_hb;

Unserialize(Stream&, bool&) (src/serialize.h:286) is uint8_t f = ser_readdata8(s); a = f;, so every non-zero byte becomes true with no error. A sendcmpct with announce=2 (or 255) and version 2 sets m_provides_cmpctblocks, m_requested_hb_cmpctblocks and m_bip152_highbandwidth_from exactly as announce=1 does, and the peer is not penalised.

Reproduction

Base 58398baf33 (upstream/29.x-knots), full build (Release, BUILD_TESTS=ON, BUILD_GUI=ON, BUILD_GUI_TESTS=ON, ENABLE_WALLET=ON, Qt5, WITH_BDB=OFF).

1. Add the new test to test/functional/p2p_compactblocks.py (included in this PR's diff) and run it without the net_processing.cpp change:

   python3 build/test/functional/p2p_compactblocks.py
   

2. Optional state check (not part of the diff), which sends announce = 0, 1, 2, 255 and reads bip152_hb_from from getpeerinfo:

repro_sendcmpct_state.py (unmodified-base version)

#!/usr/bin/env python3
from test_framework.messages import msg_sendcmpct
from test_framework.p2p import P2PInterface
from test_framework.test_framework import BitcoinTestFramework


class SendCmpctAnnounceState(BitcoinTestFramework):
    def set_test_params(self):
        self.num_nodes = 1

    def run_test(self):
        node = self.nodes[0]
        results = {}
        for val in (0, 1, 2, 255):
            p = node.add_p2p_connection(P2PInterface())
            with node.assert_debug_log([], unexpected_msgs=["Misbehaving", "invalid sendcmpct"]):
                p.send_and_ping(msg_sendcmpct(announce=val, version=2))
            info = [x for x in node.getpeerinfo() if x["id"] == max(y["id"] for y in node.getpeerinfo())][0]
            self.log.info("announce=%d -> connected=%s bip152_hb_from=%s" % (val, p.is_connected, info["bip152_hb_from"]))
            results[val] = (p.is_connected, info["bip152_hb_from"])
            node.disconnect_p2ps()
        for val in (2, 255):
            assert not results[val][0] or results[val][1] is False, "BUG: announce=%d accepted (connected, hb_from=True)" % val


if __name__ == '__main__':
    SendCmpctAnnounceState(__file__).main()

No injected delay or other test-only hack is needed.

Before

New test in p2p_compactblocks.py on the unmodified base (the peer is never disconnected, and the node logs received: sendcmpct (9 bytes) with no Misbehaving line after it):

TestFramework (INFO): Testing invalid announce field in sendcmpct message...
AssertionError: Predicate '''
        def test_function():
            if check_connected:
                assert self.is_connected
            return test_function_in()
''' not true after 30 seconds

State script on the unmodified base:

announce=0   -> connected=True bip152_hb_from=False
announce=1   -> connected=True bip152_hb_from=True
announce=2   -> connected=True bip152_hb_from=True
announce=255 -> connected=True bip152_hb_from=True
AssertionError: BUG: announce=2 accepted (connected, hb_from=True)

Change

--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -3747,10 +3747,17 @@
     if (msg_type == NetMsgType::SENDCMPCT) {
-        bool sendcmpct_hb{false};
+        uint8_t sendcmpct_hb{0};
         uint64_t sendcmpct_version{0};
         vRecv >> sendcmpct_hb >> sendcmpct_version;
 
+        // BIP152: the first integer is interpreted as a boolean and MUST have a
+        // value of either 1 or 0.
+        if (sendcmpct_hb > 1) {
+            Misbehaving(*peer, "invalid sendcmpct announce field");
+            return;
+        }
+
         // Only support compact block relay with witnesses

Plus the 13-line test test_invalid_sendcmpct_announce in test/functional/p2p_compactblocks.py, taken unchanged from Core. The only adaptation from Core is Misbehaving(*peer, ...) instead of Misbehaving(peer, ...), because peer is a pointer in this version of ProcessMessage. The later bool assignments (m_requested_hb_cmpctblocks, m_bip152_highbandwidth_from) still compile, and after the check the value is only 0 or 1.

After

New test in p2p_compactblocks.py:

TestFramework (INFO): Testing invalid announce field in sendcmpct message...
TestFramework (INFO): Tests successful

State script, rewritten to expect rejection for values above 1:

announce=0   -> connected=True bip152_hb_from=False
announce=1   -> connected=True bip152_hb_from=True
announce=2   -> rejected, disconnected
announce=255 -> rejected, disconnected
TestFramework (INFO): Tests successful

Test results

Tested: base 58398baf33 plus this change; the tested working tree was then committed unchanged as c080c0e25f. Full build as above, Release.

	Base	With change
ctest	147/147 passed	147/147 passed
Functional --extended (359 tests, ALL row excluded)	294 passed, 65 skipped, 0 failed	294 passed, 65 skipped, 0 failed

The two functional runs skipped the same 65 tests. Reasons, taken from the logs:

• 53: BDB has not been compiled (the --legacy-wallet variants, including wallet_pruning)
• 5: bitcoind has not been built with USDT tracepoints enabled
• 4: previous releases not available or disabled
• 1: python3-zmq module not available
• 1: feature_bind_port_externalip needs routable address 1.1.1.1
• 1: feature_bind_port_discover needs routable addresses 1.1.1.1 and 2.2.2.2

The baseline run used the unmodified p2p_compactblocks.py; the run with the change used the version containing the new test.

Caveats

• Peers that send a first byte other than 0 or 1 will now be discouraged and disconnected (NoBan and manual peers are not, per MaybeDiscourageAndDisconnect). Core accepted the same trade-off; I did not test against real-world peers.
• The NoBan, manual and local-peer paths were not exercised with the new check.
• The state script is not part of the diff; only the p2p_compactblocks.py test is.

[kwsantiago]

Tested ACK c080c0e merged into 29.x-knots. New test fails on base. Of 156 mainnet peers that sent sendcmpct to me, including 63 signaling BLAKE2b, none sent above 1.