#35888 net: reject oversized outbound messages
https://github.com/bitcoin/bitcoin/pull/35888 · · +87/-8 in 5 files, 4 commits · labels: P2P
Goal
- Prevent transport aborts and memory overwrites caused by oversized outbound messages
- Validate message header lengths and payload sizes before queueing them to peers
Adds an assumption and validation check in CConnman::PushMessage() to ensure outbound messages adhere to protocol limits (maximum 12 bytes for message type, maximum 4 MB for payload). In debug and fuzz builds, invalid messages trigger an assertion to expose buggy internal callers, while release builds log and drop the message before it enters the send queue. Also updates the testing-only sendmsgtopeer RPC to explicitly reject oversized payloads.
Problem: Outbound transports expect valid message headers and lengths, but CConnman previously did not validate them before queueing. An oversized message type would cause V1 transport encoding to abort and could cause V2 transport encoding to overwrite memory or buffer data.
Category: P2P (#27 of 65)
P3 · bug fix
- P3 because it hardens transport safety against memory corruption and aborts from internal bugs
- Outbound message types and payloads are already bounded in normal operation so impact is defensive
Prevents potential transport aborts or memory overwrites in V2 encoding if an internal caller constructs an oversized message type. Because outbound types are internal constants and payloads are bounded under normal operation, this is primarily defensive hardening and fuzzing reliability rather than an urgent consensus or P2P reliability issue.
Membership: Modifies outbound message queuing and transport invariant enforcement in src/net.cpp and src/net.h.
Factors: security/stability 1, bug 1, performance 0, user value 0, leverage 1
Reviewability: Ready
- Ready for review
- Clean CI with no merge conflicts and prior reviewer feedback addressed
The PR has clean CI, no merge conflicts, and addresses earlier reviewer feedback. Only a non-blocking nit remains open.
Author status: active
Open concerns:
- naiyoma suggested using std::copy_n to safely clamp message type copies in the transport.
Resolved concerns:
- ajtowns inquired about fuzzing context and suggested moving limits checks into CSerializedNetMsg and using string_view for tests.
- mzumsande suggested moving checks up from transport level to CConnman::PushMessage() to prevent connection stalls from SetMessageToSend(false).
Agreement: Strong
- Strong consensus on enforcing protocol limits before queueing outbound messages
- Concept approval and co-authored commits shaping the design (ajtowns, mzumsande)
- Concept approval with an open non-blocking nit on clamping message types (naiyoma)
Strong consensus on approach with Concept ACKs and co-authorship from ajtowns and mzumsande.
Reviewers agreed on enforcing invariants at the PushMessage layer rather than inside individual transports, and co-authored commits to finalize the implementation.
- Concept ACK from ajtowns, who co-authored the commits.
- mzumsande provided architectural feedback on PushMessage enforcement and co-authored the commits.
- Concept ACK from naiyoma with a minor inline nit.
Review verdicts (DrahtBot): 0
Files
75 lines under test/bench/ci.
- src/test/net_tests.cpp +60/-2
- test/functional/rpc_net.py +9/-4
- src/net.h +10/-1
- src/rpc/net.cpp +4/-1
- src/net.cpp +4/-0
Card
Enforces wire protocol limits on message types (12 bytes) and payloads (4 MB) in CConnman::PushMessage() before they are queued for transmission. This provides defense-in-depth against internal caller bugs that could cause V1 encoding to abort or V2 encoding to overwrite buffers. The test-only sendmsgtopeer RPC is also updated to reject oversized payloads. Review is positive with Concept ACKs and co-authorship from ajtowns and mzumsande, and no significant blockers remain.