Fix Modbus handler DoS from unbounded, byte-at-a-time oversized reads (#619) - #623
Open
Krisztian766 wants to merge 1 commit into
Open
Fix Modbus handler DoS from unbounded, byte-at-a-time oversized reads (#619)#623Krisztian766 wants to merge 1 commit into
Krisztian766 wants to merge 1 commit into
Conversation
…mushorg#619) A client could declare an MBAP length up to 0xFFFF and the handler would read the body one byte at a time via sock.recv(1) in a loop with no upper bound. Two compounding problems made this a real DoS, not just a slow path: - With the full declared body delivered in one segment, the byte-at-a-time reads plus repeated `request += new_byte` (an O(n) bytes copy per iteration, so O(n^2) overall) measurably pins the CPU for over a second on a 64KB frame. - If the peer instead half-closes after sending only the header, recv(1) returns b'' immediately on every call instead of raising - `request` never grows and the loop never terminates. Confirmed manually: this hangs the process indefinitely (15s+ wall clock, unrecoverable even via gevent.Timeout, since the tight loop never yields to gevent's hub). Since Conpot runs every protocol as greenlets on one shared event loop, either case stalls or fully freezes every other emulated service for as long as the malicious connection is being processed. Fix: reject any declared length over 254 bytes immediately (a conforming Modbus/TCP frame is at most 1-byte unit id + 253-byte PDU, the limit inherited from serial Modbus - RFC/spec max, not an arbitrary number), and replace the byte-at-a-time read with a single bounded sock.recv(remaining) for whatever's left within that cap. Added a regression test that reproduces the half-close hang deterministically (not timing-based - the underlying bug is a genuine infinite loop) and verified: all 7 tests in test_modbus_server.py pass with the fix, and the new test reliably hangs the process before it (manually confirmed, not included as a second automated test to avoid a permanently-hanging CI job if this ever regresses again).
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.
Fixes #619.
Root cause
lengthcomes straight from the client with no upper bound (up to 0xFFFF),and the loop reads it back one byte at a time, appending onto
requesteach time. This is two compounding problems, not one:
request += new_bytecopies theentire accumulated buffer every iteration (bytes are immutable), so a
64KB frame is O(n²) byte copies, not O(n). Measured in isolation with
plain blocking sockets: ~1.2 seconds of CPU time for a single 0xFFFF-byte
frame delivered in one segment, matching the issue's reproduction steps
exactly.
after sending only the header.
sock.recv(1)on a socket whose peerhas shut down its write side returns
b''immediately on every call(not an exception) - so
requestnever grows, the loop condition neverchanges, and this spins at 100% CPU forever. I confirmed this manually:
the connection never closed within a 15-second wall-clock timeout, and
it doesn't recover on its own - not even
gevent.Timeoutinside the sameprocess can rescue it, since the tight CPU-bound loop never yields to
gevent's hub, so no other callback in the process ever runs again,
including the timer backing that Timeout.
Since Conpot runs every protocol (HTTP, S7, Modbus, ...) as greenlets on one
shared event loop, both variants stall or fully freeze every other emulated
service for as long as the malicious connection is being processed - matching
the issue's description and its noted use as a timing-based fingerprinting
vector.
Fix
any body at all. 254 isn't arbitrary: a conforming Modbus/TCP frame is at
most a 1-byte unit id + up to 253 bytes of PDU, the size limit inherited
from serial Modbus (256-byte ADU minus 1-byte address minus 2-byte CRC).
No legitimate client needs more.
sock.recv(remaining)for whatever's left (now capped at 254), and treat an empty read as
end-of-stream (
break) instead of looping on it.Testing
test_oversized_length_is_rejected_without_reading_body, whichreproduces the half-close hang. Deliberately not a wall-clock timing
assertion, since the underlying bug is a genuine infinite loop rather than
a slow path - see the test's docstring for why even
gevent.Timeoutcan'tsafely bound it against the pre-fix code (confirmed manually: hangs the
whole process rather than failing cleanly, so I didn't want a test that
can hang CI indefinitely on a future regression - the docstring explains
the manual reproduction instead).
conpot/tests/test_modbus_server.pypass with the fix(
python -m pytest conpot/tests/test_modbus_server.py -v).fix: (a) plain blocking sockets outside gevent, isolating the O(n²)
byte-copy cost at ~1.2s for a 64KB frame; (b) a full gevent StreamServer
with a second "victim" connection sampled continuously during the attack,
showing a 739.9ms max latency spike (vs <1ms baseline) pre-fix and 39.1ms
post-fix.
black --checkpasses on both changed files.Note (unrelated to this fix)
conpot/tests/test_logger_taxii.pyfails to even import on Python 3.13(
libtaxiiimports the removed stdlibcgimodule), and running the fullsuite with a fresh env needs
setuptools<81pinned explicitly sincepkg_resourceswas dropped from newer setuptools before the<84upperbound in
pyproject.tomlcatches it. Neither is related to this change -flagging in case it's useful, happy to open separate issues if wanted.