net: reject oversized outbound messages #35888

l0rinc commented at 10:36 PM on August 4, 2026: contributor

Problem: Outbound transports encode message types in a fixed 12-byte field without checking that internal callers respect the limit. An oversized type can abort V1 or overwrite V2's encoding buffer. The testing-only sendmsgtopeer RPC also accepts payloads above the 4 MB protocol limit, which makes the receiving peer disconnect.

Fix: Treat oversized types and payloads reaching the internal send path as failed assumptions before messages are queued or encoded. Release builds still drop these messages, while sendmsgtopeer rejects oversized payloads as invalid parameters.

DrahtBot added the label P2P on Aug 4, 2026

DrahtBot commented at 10:37 PM on August 4, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35888.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	ajtowns

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35852 (scripted-diff: Use inline const(expr) over static constexpr in headers by maflcko)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

l0rinc marked this as a draft on Aug 5, 2026

l0rinc force-pushed on Aug 5, 2026

l0rinc marked this as ready for review on Aug 5, 2026

in src/test/net_tests.cpp:1381 in 18ce84e1ca

1377 | @@ -1374,10 +1378,38 @@ class V2TransportTester
1378 |      }
1379 |  };
1380 |  
1381 | +const std::string MAX_MESSAGE_TYPE(CMessageHeader::MESSAGE_TYPE_SIZE, 'x');

ajtowns commented at 11:35 PM on August 5, 2026:

constexpr std::string MAX_MESSAGE_TYPE{"xxxxxxxxxxxx"};
static_assert(MAX_MESSAGE_TYPE.size() == MESSAGE_TYPE_SIZE);

would be easier for the reader I think.

in src/net.cpp:848 in 18ce84e1ca

 844 | @@ -845,9 +845,16 @@ CNetMessage V1Transport::GetReceivedMessage(NodeClock::time_point time, bool& re
 845 |      return msg;
 846 |  }
 847 |  
 848 | +static bool IsMessageWithinLimits(const CSerializedNetMsg& msg) noexcept

ajtowns commented at 11:39 PM on August 5, 2026:

Assume(MessageIsWithinLimits(msg)) might read better? Should this test be part of the CSerializedNetMsg class in protocol.h? Would probably be a useful place to add documentation that code constructing these msgs must ensure they're not oversized.

ajtowns commented at 11:40 PM on August 5, 2026: contributor

Concept ACK

Can you explain your thinking / where you were coming from in relation to #35880#pullrequestreview-4858213755 some more? Even if whatever the concerns were are resolved to your satisfaction already, it makes me think there's a hole in my understanding here that I'd like to fill before reviewing properly.

l0rinc force-pushed on Aug 6, 2026

l0rinc commented at 8:24 PM on August 6, 2026: contributor

it makes me think there's a hole in my understanding here

I don't think you're missing anything on the receive side. I had already found that the related send path was missing checks, so when this fuzz failure showed another oversized-message case, I wanted to verify both paths before excluding it. At that point I hadn't established that the fuzzer was bypassing a receive-side guarantee, so I treated the outbound failure as something the send path should reject.

There was also a more directly related reason I did not want to assume every receive-side limit was enforced early: this locator fix found that getblocks and getheaders enforced MAX_LOCATOR_SZ only after deserialization, so a truncated oversized locator could allocate and throw before reaching the disconnect check (will push in a separate upstream PR soon).

I was also cautious because similar fuzz constraints and sanitizer workarounds had hidden actual problems before: the headers-sync target kept the clock above the underflow case, the coins-view target flushed corrupted accounting while UBSan suppressed the affected methods, the crypto target replaced empty vectors to avoid UBSan instead of exposing the empty-key HMAC issue, and the bloom target constrained its parameters away from arithmetic UB.

After tracing this case, I agree that an oversized transaction cannot reach this path from a real peer, so restricting the fuzz target is correct. Thanks for the pushback and feedback.

Concept ACK

Addressed your suggestions, thanks, pushed. Edit: it seems libc++ has some problems with constexpr strings, so I changed it to std::string_view and materialized on call site.

test: characterize outbound message limits

Cover the accepted 12-byte type and 4 MB payload boundaries for both outbound transports.

V2 accepts a 13-byte type when a non-empty payload keeps the type copy in bounds.
Both transports accept a 4,000,001-byte payload.
These cases record the old behavior before the send path treats invalid inputs as failed assumptions.

The existing two-node `sendmsgtopeer` test also confirms that the oversized payload is sent and makes the peer disconnect.

6257b4c94c

net: assume outbound message types fit

`Transport::SetMessageToSend()` encodes message types in a fixed 12-byte wire field.
V1 aborts on a longer type, while V2 can overwrite the first payload byte or write past its encoding buffer.
Normal outbound types are internal constants, and `sendmsgtopeer` already rejects types longer than 12 bytes.

Use `Assume` before `CConnman` queues a message or either transport encodes it.
Debug and fuzz builds expose invalid internal callers, while release builds still return before encoding.

Co-authored-by: Anthony Towns <aj@erisian.com.au>

e1be17cfd9

net: reject oversized outbound payloads

The testing-only `sendmsgtopeer` RPC can send a payload larger than the 4 MB protocol limit and make the receiving peer disconnect.
Normal outbound messages stay within this limit.
The `p2p_private_broadcast` fuzz target filters synthetic oversized transactions before its loopback send path (#35880).

Treat oversized payloads from internal callers as failed assumptions before `CConnman` queues a message or a transport encodes it.
Debug and fuzz builds expose invalid internal callers, while release builds still return before encoding.
`CConnman` also logs and drops the message in release builds.

Reject oversized `sendmsgtopeer` payloads as invalid RPC parameters.

Co-authored-by: Anthony Towns <aj@erisian.com.au>

321b5907e8

l0rinc force-pushed on Aug 6, 2026

DrahtBot added the label CI failed on Aug 6, 2026

DrahtBot commented at 9:31 PM on August 6, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task No wallet: https://github.com/bitcoin/bitcoin/actions/runs/31127401132/job/92703875334</sub> <sub>LLM reason (✨ experimental): CI failed due to a Clang build error in src/test/net_tests.cpp: a constexpr std::string is not a constant expression, causing static_assert(MAX_MESSAGE_TYPE.size()...) to fail.</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

l0rinc closed this on Aug 6, 2026

l0rinc reopened this on Aug 6, 2026

DrahtBot closed this on Aug 7, 2026

DrahtBot reopened this on Aug 7, 2026