Commit graph

2 commits

Author SHA1 Message Date
Vadim Peretokin
c1d1f1aec8
fix: stop the telnet reader writing a NUL past the data it was given (#9677)
#### Brief overview of PR changes/additions

* `cTelnet::processSocketData()` terminated its input at
`in_buffer[amount + 1]`, one byte past the data it was given, and did so
*before* checking the `-1`/`0` returns from `QIODevice::read()`. It now
guards first, then terminates at `in_buffer[amount]`.
* The guard is `amount <= 0` rather than `== -1`, because
`loopbackTest()` narrows a `qsizetype` into an `int` and can produce a
negative that is not `-1`.
* Adds `cTelnetBufferTest` (5 slots) and drops the `reserve(size + 16)`
slack that three existing telnet tests carried purely to absorb the
stray write, which turns them into regression guards too.

#### Motivation for adding to Mudlet

The socket path survived this because `slot_socketReadyToBeRead()`
over-allocates its stack buffer, but the same function is reached from
Lua's `feedTelnet()` via `loopbackTest()`, which passes a `QByteArray`
sized exactly to its contents - so the stray NUL landed one byte past a
heap allocation. That is a real out-of-bounds write reachable from any
script, and the workarounds already sitting in our test suite show it
has been quietly worked around rather than fixed.

#### Other info (issues closed, discussion etc)

Closes #1065. Supersedes the closed #8438, which carried the same fix
under 19 commits of unrelated history and had CI red on a faulty
assertion in its own test.

**Test case:** reverting the `src/ctelnet.cpp` hunk makes 3 of the 5 new
slots fail and AddressSanitizer report `heap-buffer-overflow ... in
cTelnet::processSocketData(char*, int, bool)`; restoring it gives 7
passed / 0 failed, and the full suite is 72/72 serially.

One thing to flag for review: this adds `friend class
cTelnetBufferTest;` to `cTelnet`, since `processSocketData()` is private
and the public `loopbackTest()` cannot express a caller-laid-out buffer.
It sits beside the existing `friend class TelnetTlsPromptTest;`, so
there is precedent, but it is a test name in a shipped header and worth
a second opinion.

Three review findings were deliberately left out of scope. The MCCP
decompression path hit the same overflow, via the re-entry that passes
`remainingData`/`remainingAmount` back into `processSocketData()` - the
fix covers it, but nothing under `test/` exercises compression at all,
so it is fixed-but-unguarded and a dedicated MCCP test belongs in its
own PR. The other two are pre-existing and orthogonal: a read error
(`amount == -1`) is still silent, because `slot_socketError()` has been
commented out as unused since 2017; and `mDecompressionRecursionDepth`
is hand-balanced across four decrements rather than held by a scope
guard (verified balanced today, but fragile). Happy to do any of them as
a follow-up.

Assisted-by: Claude:claude-opus-5
2026-08-05 14:06:45 +02:00
Vadim Peretokin
282d774956
Fix: cap telnet subnegotiation length to bound memory use (#9440)
#### Brief overview of PR changes/additions
An `IAC SB` (subnegotiation begin) with no matching `IAC SE` grew the
subnegotiation buffer without bound across socket reads (the existing
missing-SE recovery only triggers on an embedded IAC byte). This aborts
a subnegotiation that exceeds 5MB - far larger than any real
GMCP/MSDP/ATCP payload - and recovers.

#### Motivation for adding to Mudlet
Prevents a hostile or broken game server from exhausting memory (a
denial of service).

#### Other info (issues closed, discussion etc)
Found via a source-code audit; no linked issue. The 5MB cap is easily
tunable.

---------

Signed-off-by: Vadim Peretokin <vadim.peretokin@mudlet.org>
2026-07-20 22:45:22 +02:00