SFT-7490: fail closed on malformed peer frames - #57
Open
georgesFoundation wants to merge 1 commit into
Open
georgesFoundation wants to merge 1 commit into
georgesFoundation wants to merge 1 commit into
Conversation
Malformed pool input could panic the client, wedge it permanently, or
poison every frame behind it. Validate lengths and domains before
slicing, shifting or resizing, and consume a frame before reporting its
error.
Panics, all reachable from peer input:
- mining.notify sliced prev_hash at fixed offsets without checking its
length.
- roll() shifted by 0i32.trailing_zeros(), i.e. 32, when the pool
granted a zero version mask, and added ntime_bits to a pool-supplied
ntime without wrapping.
- set_extranonces assigned extranonce2_size before the resize that could
fail, leaving it out of sync with the buffer roll() indexes.
- send() wrote the terminator at tx_buf[len] without checking there was
room for it.
- serde-json-core's custom-error-messages formats into a
heapless::String<64> with write!(..).unwrap(), and serde's Display
impls ignore the {:.64} precision, so any message over 64 bytes
panicked. A response carrying the standard "jsonrpc" field is enough
to reach it, so this fired against well-behaved pools too. The feature
is now off and those errors carry no message.
poll_message returned parser and unknown-id errors before advancing past
the offending line, so it stayed at the head of the buffer and every
later poll failed identically. A buffer filled without a terminator read
into an empty slice forever; it is now dropped with LineTooLong and the
client resynchronizes on the next terminator.
Server-selected sizes are bounded before they reach an allocation or a
resize. The new limits module documents the maxima; they are the
capacities the heapless build already gets from its field types, so both
builds now reject the same frames with the same errors.
Adds a mock-transport integration suite covering short hashes, unknown
ids, invalid-then-valid JSON, fragmented reads, full buffers and
oversized server sizes; a stateful fuzz target over structured frames,
since a byte-level fuzzer never discovers "mining.notify" and would
never get past parse_method; and a flake check for the alloc build,
which the all-features checks have to exclude and which was therefore
untested.
|
+1100 lines :/ |
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.
Malformed pool input could panic the client, wedge it permanently, or poison every frame behind it. Validate lengths and domains before slicing, shifting or resizing, and consume a frame before reporting its error.
Panics, all reachable from peer input:
poll_message returned parser and unknown-id errors before advancing past the offending line, so it stayed at the head of the buffer and every later poll failed identically. A buffer filled without a terminator read into an empty slice forever; it is now dropped with LineTooLong and the client resynchronizes on the next terminator.
Server-selected sizes are bounded before they reach an allocation or a resize. The new limits module documents the maxima; they are the capacities the heapless build already gets from its field types, so both builds now reject the same frames with the same errors.
Adds a mock-transport integration suite covering short hashes, unknown ids, invalid-then-valid JSON, fragmented reads, full buffers and oversized server sizes; a stateful fuzz target over structured frames, since a byte-level fuzzer never discovers "mining.notify" and would never get past parse_method; and a flake check for the alloc build, which the all-features checks have to exclude and which was therefore untested.