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. V2 sends it under a truncated type with its payload intact, or writes past its encoding buffer when the payload is shorter than the excess type bytes. The send path also did not enforce the 4 MB payload limit.

Fix: Treat oversized types and payloads as failed assumptions in CConnman::PushMessage() before they enter the send queue.

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, naiyoma

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:

#34486 (net: Reduce local network activity when networkactive=0 by willcl-ark)

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: #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.

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).

naiyoma commented at 6:25 PM on September 13, 2026: contributor

Concept ACK

nit: Maybe it also makes sense to restrict the message type copy to MESSAGE_TYPE_SIZE?

-        std::copy(msg.m_type.begin(), msg.m_type.end(), contents.data() + 1);
+        std::copy_n(msg.m_type.begin(),
+                std::min(msg.m_type.size(), CMessageHeader::MESSAGE_TYPE_SIZE),
+                contents.data() + 1);

test: characterize outbound message limits

Round-trip a 12-byte type and a 4 MB payload through the transports.
Record that `PushMessage()` queues oversized types and payloads, and that `sendmsgtopeer` sends an oversized payload which disconnects its peer.
Keep a valid message pending to observe queueing without encoding the oversized type.

0dfdd0c4a0

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 sends a truncated type with the payload intact or writes past its encoding buffer when the payload is shorter than the excess type bytes.
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>

caea75f11b

fuzz: bound generated outbound payloads

Constrain connman payloads and both private-broadcast transaction sources to the send path limit.
The seed limits also bound echoed TX messages, making the receive-side size filter redundant.

1ee760923e

net: reject oversized outbound payloads

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.

Nonstandard test-chain configurations can still admit transactions above this limit.
Relaying them fails the assumption in debug builds.

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

48ca5ed0cd

l0rinc force-pushed on Sep 29, 2026

l0rinc commented at 11:24 PM on September 29, 2026: contributor

Rebased and simplified the tests and fuzz setup, production fix is unchanged, with the message type added to the log.

nit: Maybe it also makes sense to restrict the message type copy

PushMessage already rejects oversized types before they reach v2, and clamping there would silently truncate an invalid type, so it's not required for this fix