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, but CConnman did not ensure internal callers respected this limit before queuing messages. An oversized type aborts V1 encoding, while V2 can overwrite the first payload byte or write past its encoding buffer. Normal outbound payloads are also expected to stay within the 4 MB protocol limit, but the send path did not enforce this invariant.

Fix: Treat oversized types and payloads as failed assumptions in CConnman::PushMessage() before they enter the send queue. Debug and fuzz builds expose invalid internal callers, while release builds log and drop the messages. The testing-only sendmsgtopeer RPC rejects oversized payloads before calling PushMessage(), while p2p_invalid_messages.py continues to cover inbound handling.

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

No conflicts as of last run.

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: #35936 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. (edited)

I was also cautious because similar fuzz constraints and sanitizer workarounds had hidden actual problems before:

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.

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

l0rinc force-pushed on Aug 7, 2026

DrahtBot removed the label CI failed on Aug 7, 2026

in src/rpc/net.cpp:1098 in 84eddd6802 outdated

1093 | @@ -1094,6 +1094,9 @@ static RPCMethod sendmsgtopeer()
1094 |              if (!msg.has_value()) {
1095 |                  throw JSONRPCError(RPC_INVALID_PARAMETER, "Error parsing input for msg");
1096 |              }
1097 | +            if (msg->size() > MAX_PROTOCOL_MESSAGE_LENGTH) {
1098 | +                throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Error: msg too large, max size is %u bytes", MAX_PROTOCOL_MESSAGE_LENGTH));

mzumsande commented at 4:14 PM on August 10, 2026:

I don't think it's a "problem" per se that sendmsgtopeer accepts payloads above the 4 MB protocol limit - it's a test RPC after all. The story should be that you want to add the assumption to prevent future regressions etc., and need to change the rpc for that - which is no loss because it's just a test rpc and oversized messages are being tested elsewhere.

l0rinc commented at 7:51 PM on August 14, 2026:

Pushed, let me know if I understood the request.

in src/net.cpp:851 in 84eddd6802

 847 | @@ -848,7 +848,7 @@ CNetMessage V1Transport::GetReceivedMessage(NodeClock::time_point time, bool& re
 848 |  bool V1Transport::SetMessageToSend(CSerializedNetMsg& msg) noexcept
 849 |  {
 850 |      AssertLockNotHeld(m_send_mutex);
 851 | -    if (!Assume(msg.m_type.size() <= CMessageHeader::MESSAGE_TYPE_SIZE)) return false;
 852 | +    if (!Assume(msg.IsWithinLimits())) return false;

mzumsande commented at 5:06 PM on August 10, 2026:

What is the reason for the non-debug build return instead of only adding the Assume? I think if this was somehow reached in production, returning false would result in a stall of the connection for minutes (until it get disconnected for inactivity), since SocketSendData will never make progress.

Also, wouldn't it be nicer from an architectural point of view if arbitrary application-level rules such as the 4MB maximum message length wouldn't be enforced at the transport level, but only at the higher level (PushMessage) anyway? Then we wouldn't need the same assumption 3 different places.

l0rinc commented at 7:26 PM on August 10, 2026:

What is the reason for the non-debug build return instead of only adding the Assume?

It was suggested in #35880 (comment). I initially applied it because SetMessageToSend() already returns a boolean, and I treated false as a safe fallback in release builds while Assume exposed invalid callers in debug and fuzz builds. Good catch that false will just try again later, so a permanently invalid message would remain queued and stall the connection. I moved both size assumptions to PushMessage(), before the message enters the send queue. This also keeps the 4 MB application-level rule out of the transports - added you as coauthor, thanks for the review.

DrahtBot added the label Needs rebase on Aug 14, 2026

l0rinc force-pushed on Aug 14, 2026

DrahtBot removed the label Needs rebase on Aug 14, 2026

l0rinc commented at 8:02 PM on August 14, 2026: contributor

Rebased and addressed @mzumsande's review: outbound limits are now enforced only in CConnman::PushMessage(), before messages enter the send queue, so a permanent rejection cannot stall SocketSendData() through SetMessageToSend(false). The sendmsgtopeer behavior and the PR rationale now reflect the RPC's testing-only role.

test: characterize outbound message type limits

Cover the accepted 12-byte type limit for both outbound transports.

`CConnman::PushMessage()` queues a 13-byte type.
Keep a valid message pending in the transport to observe queueing without encoding this invalid input.
This records the old behavior before the send path treats an oversized type as a failed assumption.

b4cdcb2ad8

net: assume outbound message types fit

Outbound transports encode 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.

Treat oversized types from internal callers as failed assumptions in `CConnman::PushMessage()` before they enter the send queue.
Debug and fuzz builds expose invalid internal callers, while release builds drop the messages.

Co-authored-by: Anthony Towns <aj@erisian.com.au>
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

4b10305dee

test: characterize outbound payload limits

Cover the accepted 4 MB payload limit for both outbound transports.

`CConnman::PushMessage()` queues a 4,000,001-byte payload.
This records the old behavior before the send path treats an oversized payload as a failed assumption.

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

c10ff78c80

net: reject oversized outbound payloads

Normal outbound messages stay within the 4 MB protocol limit.
Treat an oversized payload from an internal caller as a failed assumption in `CConnman::PushMessage()` before it enters the send queue.
Debug and fuzz builds expose invalid internal callers, while release builds log and drop the message.

This requires the testing-only `sendmsgtopeer` RPC to reject oversized payloads before calling `PushMessage()`.
Oversized inbound messages remain testable through raw P2P inputs, so this does not remove coverage of their receive-side handling.
The `p2p_private_broadcast` fuzz target separately filters synthetic oversized transactions before its loopback send path (#35880).

Co-authored-by: Anthony Towns <aj@erisian.com.au>
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

f9188114c7

l0rinc force-pushed on Sep 6, 2026

l0rinc commented at 9:05 PM on September 6, 2026: contributor

AI review flagged that I was missing coverage so added PushMessage() tests because the transport tests bypassed the guard and didn’t verify rejection before queueing. Also rebased and split the type and payload characterization/fix pairs so each fix updates its own expectations (otherwise it's not obvious why multiple fixes are needed).