protocol: Bound the job-validation subcommands to the received length - #11
jasonsopko wants to merge 1 commit into
Conversation
904caa3 to
c1de85a
Compare
| pk_u16le(msg, i, req_count); i += 2; | ||
|
|
||
| for(j=0;j<req_count;j++) { | ||
| if (k + 2 > len) { |
There was a problem hiding this comment.
Can't we predict where k + 2 ends up outside the loop?
There was a problem hiding this comment.
Yes, it's now one check before the lock: len < 3 + 2 * req_count. Added a test for it that fails without the check.
| unsigned char cmd = data[0]; | ||
| unsigned char *p = data; | ||
|
|
There was a problem hiding this comment.
Right, it reads the static receive buffer and returns on len < 2 before using it. Moved back.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 5
Open (5)
datais read-only. Consider changing these parameters (andp) to… · Newdatais read-only. Consider changing these parameters (andp) to… · Newdatais read-only. Consider changing these parameters (andp) to… · Newdatais read-only. Consider changing these parameters (andp) to… · Newbreak;statements afterreturnare unreachable. Removing them reduces dead code and avoids… · New
What changed in this PR
Bounds DATUM job-validation subcommand parsing to the actual received frame length to prevent out-of-bounds reads and potential data leakage in replies.
Changes:
- Updated job-validation subcommand handlers to accept
(len, data)and added minimum-length guards. - Fixed
datum_protocol_job_validation_cmd()to validatelenbefore reading the subcommand byte and to pass bounded lengths to handlers. - Added per-request bounds checking for txn-by-id request parsing (
req_countids).
| File | Description |
|---|---|
| src/datum_protocol.c | Adds length-aware parsing and guards for job-validation subcommands to prevent OOB reads. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
|
|
||
| int datum_protocol_job_validation_stxlist(unsigned char *data) { | ||
| int datum_protocol_job_validation_stxlist(int len, unsigned char *data) { |
| } | ||
|
|
||
| int datum_protocol_job_validation_stxlist_byid(unsigned char *data) { | ||
| int datum_protocol_job_validation_stxlist_byid(int len, unsigned char *data) { |
| } | ||
|
|
||
| int datum_protocol_job_validation_sblock(unsigned char *data) { | ||
| int datum_protocol_job_validation_sblock(int len, unsigned char *data) { |
There was a problem hiding this comment.
int datum_protocol_job_validation_sblock(const int len, const unsigned char * const data) { would be even better (assuming neither are manipulated)
There was a problem hiding this comment.
Done for all three handlers.
datum_protocol_job_validation_cmd() passed no length to the stxlist, stxlist-by-id and sblock handlers, so they read the DATUM server's frame without knowing how many bytes arrived. stxlist-by-id read a three-byte header and then two bytes per requested id, the id count bounded only against the template's transaction count, never against the received bytes: a short or crafted request reads past the frame and, with a valid job, streams the surplus back to the pool. The reply side of the same exchange was bounded already; this is the request side. Pass the remaining length to each handler, reject a short header, and check that the whole id list arrived before reading it. The test sends a request one byte short of its id list, and one whose count is far past what arrived, each in a buffer of exactly its size; both fail without the check. Found by the new fuzz_protocol_cmd5 harness.
c1de85a to
b701aa6
Compare
What
Bounds the DATUM job-validation subcommands to the number of bytes the server actually sent.
datum_protocol_job_validation_cmd()read the subcommand byte before itslen < 2check and then passed only a pointer, no length, to the stxlist, stxlist-by-id and sblock handlers.Why
datum_protocol_mining_cmd5()dispatches withcmd_lentaken from the server's header, and the receive loop delivers exactly that many bytes, which can be as few as one. The subcommand handlers then read without knowing how much arrived._stxlist_byid()is the sharp case: it reads a three-byte header (job_index, thenreq_count) and then two bytes per requested id,req_countbounded only against the template's transaction count, never against the received length. A short or crafted request reads past the frame, and with a valid job present the surplus is copied into the reply and sent back to the server.server_recv_bufferis a large static array, so on x86 this reads stale bytes rather than faulting, but it is an out-of-bounds read of attacker-influenced length reachable from the pool. This is the request-side companion to the reply-side bound already in the tree.How I tested
Found by a libFuzzer harness over
datum_protocol_mining_cmd5(offered separately). Minimal reproducer, a command-5 plaintext of50 11 00(job validation, txn-by-id, one payload byte), read three bytes under AddressSanitizer before the fix and is clean after. Builds withgcc -Wall -Werror;datum_gateway --testpasses. Each guard is areturn 0on a frame too short for what the code then reads, so it is a no-op on any well-formed request and changes behavior only on the frames that previously read out of bounds.Risk and rollback
Valid pool traffic takes the same path. Revert the commit.