protocol: Bound the job-validation parser and guard clz(0) - #236
Open
jasonsopko wants to merge 2 commits into
Open
jasonsopko wants to merge 2 commits into
jasonsopko wants to merge 2 commits into
Conversation
datum_protocol_job_validation_cmd() read the subcommand byte before its length check, and 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. Check the length before reading the subcommand byte, pass the remaining length to each handler, reject a short header, and stop the id loop before it reads past what arrived. Found by the new fuzz_protocol_cmd5 harness.
1ULL << (63 - __builtin_clzll(x)) is undefined for x == 0, since __builtin_clzll(0) is undefined. floorPoT next to it already guards its zero case. A DATUM server minimum-difficulty of 0 reaches it through client configuration, and the vardiff path reaches it from miner input. Return 0 for 0. Found by the new fuzz_protocol_cmd5 harness.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two out-of-bounds reads in the DATUM protocol parser, both reachable from the pool the gateway connects to. Same code as CONVOY, where these went up as separate PRs.
datum_protocol_job_validation_cmd()read the subcommand byte before itslen < 2check and passed no length to the stxlist, stxlist-by-id and sblock handlers._stxlist_byid()then read a three-byte header and two bytes per requested id, the id count bounded 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 the surplus is copied into the reply and sent back to the pool.roundDownToPowerOfTwo_64(0)evaluated1ULL << (63 - __builtin_clzll(0));__builtin_clzll(0)is undefined.floorPoT()next to it already guarded its zero case. Reached through a server minimum-difficulty of 0 and through the vardiff path.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, so the handlers read frames shorter than they assume.server_recv_bufferis a large static array, so on x86 these are reads of stale bytes rather than faults, but they are out-of-bounds reads of attacker-influenced length in a network parser.How I tested
Found by a libFuzzer harness over
datum_protocol_mining_cmd5on the CONVOY tree; the same functions are byte-identical here. Built this fork withgcc -Wall -Werror,datum_gateway --testpasses, and the three reproducing inputs replay clean under AddressSanitizer and UndefinedBehaviorSanitizer with the fixes. Each guard is areturn 0on a frame too short for what the code then reads, a no-op on well-formed traffic.Risk and rollback
Valid pool traffic takes the same path. Revert either commit independently.