fix(stratum_v1): bound pool reads by length and idle time - #89
Open
Schnitzel wants to merge 2 commits into
Open
Conversation
The stratum reader used read_line into a growable String with no length limit. A malicious pool — or any MITM, easy on the unencrypted transport — could stream an unterminated line and grow the daemon's memory until the OOM killer ended it. Read through a length-limited adapter and reject over-long lines with a new MessageTooLarge error, which terminates the connection; the caller reconnects with the usual backoff. 64 KiB is roughly an order of magnitude above the largest legitimate message (mining.notify with a big coinbase and many merkle branches).
The client's main loop awaited pool messages with no timeout, so a dead or maliciously quiet connection left the miner neither working nor reconnecting; TCP keepalive takes hours to notice. Wrap pool reads in a 20-minute idle timeout: comfortably past the longest expected gap between mining.notify messages (new blocks average 10 minutes) and far below keepalive detection. A timeout errors the connection and the caller reconnects with the usual jittered backoff.
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.
Problem
Two related robustness gaps in the stratum client, both reachable by a malicious pool or a MITM (the transport is plaintext):
Connection::read_messageloopsBufReader::read_lineinto a growableStringwith no length cap. A peer that never sends\ngrows the daemon's memory until the OOM killer ends it (≈1 GB per 25 s at 100 Mbps, or arbitrarily slowly via a drip).Found during a source review of the pool-facing input handling.
Fix
MAX_MESSAGE_LEN): the read goes through a length-limited adapter, and a line that fills the cap without a terminating newline is rejected with a newStratumError::MessageTooLarge. 64 KiB is ~10× the largest legitimate message (mining.notifywith a big coinbase and many merkle branches). The error terminates the connection; the source reconnects with the usual jittered backoff.POOL_IDLE_TIMEOUT) around pool reads in the client main loop. Pools sendmining.notifyat least as often as new blocks arrive (10 min average), so 20 minutes of silence means the connection is dead; a timeout errors the connection and triggers the normal reconnect path.Neither change affects the handshake/submit paths, which already have 30 s request timeouts.
Tests
test_oversized_message_rejected: an unterminated 64 KiB+1 line is rejected withMessageTooLarge.test_max_length_message_accepted: a well-formed message just under the cap still parses.test_silent_pool_recycles_connection: paused-time test; after a mock handshake the pool says nothing and the client exits withTimeout, i.e. the caller reconnects.cargo fmt,cargo clippy(no new warnings), andcargo test(355 passed) are green; each commit passes on its own.