Skip to content

SFT-7490: fail closed on malformed peer frames - #57

Open
georgesFoundation wants to merge 1 commit into
mainfrom
georges/sft-7490-stratum-v1-client-fail-closed-on-malformed-peer-frames
Open

georgesFoundation wants to merge 1 commit into
mainfrom
georges/sft-7490-stratum-v1-client-fail-closed-on-malformed-peer-frames

Conversation

@georgesFoundation

Copy link
Copy Markdown
Collaborator

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.

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.
@badicsalex

Copy link
Copy Markdown

+1100 lines :/

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