Skip to content

Reject oversized node response size before zeroing the receive buffer - #135

Merged
fnordspace merged 1 commit into
qubic:mainfrom
parthod0x:fix/validate-node-response-size
Oct 20, 2026
Merged

fnordspace merged 1 commit into
qubic:mainfrom
parthod0x:fix/validate-node-response-size

Conversation

@parthod0x

Copy link
Copy Markdown
Contributor

receivePacketWithHeaderAs computes
remainingSize = header.size() - sizeof(RequestResponseHeader) from a
node-controlled value, then runs
std::fill_n(mBuffer.begin(), remainingSize, 0) before any bound check.

RequestResponseHeader::size() returns INT32_MAX when the 24-bit size field
is zero (the code comments "size is never zero, zero means broken packets"). So
a node that sends a response header with a zero size field produces
remainingSize == 2147483639, and fill_n writes ~2.1 GB into the 16 MB
mBuffer — well past its end.

Confirmed with AddressSanitizer: global-buffer-overflow, WRITE of size 2147483639. A malicious or malfunctioning node can corrupt the memory of any
client that queries it (the client trusts the node only for its response, but
this happens before the size is validated).

Fix

Require remainingSize > 0 and reject a remainingSize larger than the receive
buffer before fill_n. receiveData already throws on an oversized read,
but fill_n ran first; this closes that window.

Verified: the oversize case overflows under ASAN on current main; with the
guard it is rejected before fill_n, and g++ -std=c++20 -fsyntax-only is clean.

This is the response-parsing path, distinct from the user-input buffer fixes in
#133 and #134, and is the more security-relevant of the set because it is
triggerable by the remote node rather than by local input.

@fnordspace fnordspace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, need slight adjustments. Also please keep comments short and concise and only where the code is not self-explanatory.

Thanks for the submission!

Comment thread connection.cpp Outdated
…buffer

receivePacketWithHeaderAs zeroed `remainingSize` bytes of mBuffer, but
remainingSize is derived from the node-controlled header size() (which
returns INT32_MAX when the size field is zero), so it can exceed mBuffer
and std::fill_n would write past it.

Fill only sizeof(T) -- the bytes actually read back into result -- and let
receiveData()'s existing `sz > buffer.size()` guard reject an oversized
payload.
@parthod0x
parthod0x force-pushed the fix/validate-node-response-size branch from 53098ee to 4f11964 Compare October 15, 2026 18:18
@parthod0x

Copy link
Copy Markdown
Contributor Author

Adopted your approach: fill only sizeof(T) (the bytes read back into result) and let receiveData()'s existing sz > buffer.size() guard reject an oversized — or underflowed — size. Dropped my explicit check and the verbose comment. Rebased onto current main (the argparser.h hunks are already there via #133). Compiles clean under the CMake /WX build. Thanks!

@fnordspace
fnordspace merged commit 1112f1f into qubic:main Oct 20, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants