Suricata

Reported by Communications Security Establishment (CSE):

## Summary

The Suricata RDP parser first extracts a complete, length-bounded T.123 TPKT payload and then runs nested MCS/CS sub-parsers over that fixed slice. Because the inner sub-parsers use the *streaming* variant of `nom8::bytes::take`, an attacker-controlled inner length field that overruns the bounded slice causes a `nom8::Err::Incomplete` to escape from a buffer that is in fact already complete. The caller in `rdp.rs` interprets this as "need one more byte" and asks the TCP reassembly layer for more data without consuming anything; since the TPKT is already fully present, the same `Incomplete` is reproduced on every subsequent invocation. The to-server RDP app-layer for that flow is permanently stalled — no further RDP transactions are decoded, logged to eve-log, or made available to detection — until the stream is truncated by `stream.reassembly.depth`.

## Affected Piece of Code

- **File:** `rust/src/rdp/parser.rs`
- **Function / Location:** `parse_t123_tpkt()` ~L440–475 in conjunction with `parse_mcs_connect()` / `parse_cs_client_core_data()` ~L742–1003
- **Subsystem:** rust-misc-proto — DHCP, SNMP, NTP, TFTP, Telnet, RFB/VNC, RDP, SIP, SDP, FTP (rust), BitTorrent DHT, SCTP parsers

```rust
// rust/src/rdp/parser.rs
 34   use nom8::bytes::streaming::{tag, take};
...
440   pub fn parse_t123_tpkt(input: &[u8]) -> IResult<&[u8], T123Tpkt, RdpError> {
441       let (i1, _version) = verify(be_u8, |&x| x == TpktVersion::T123 as u8).parse(input)?;
442       let (i2, _reserved) = be_u8(i1)?;
443       // less u8, u8, u16
444       let (i3, sz) = map_opt(be_u16, |x: u16| x.checked_sub(4)).parse(i2)?;
445       let (i4, data) = take(sz).parse(i3)?;
...
462       let opt3: Option<T123TpktChild> = match opt2 {
463           Some(x) => Some(x),
464           None => match opt(parse_x223_data_class_0).parse(data) {
465               Ok((_remainder, opt)) => opt.map(T123TpktChild::Data),
466               Err(e) => return Err(e),
467           },
...
809   fn parse_cs_client_core_data(input: &[u8]) -> IResult<&[u8], CsClientCoreData> {
810       let (i1, _typ) = verify(le_u16, |&x| x == CsType::Core as u16).parse(input)?;
811       // less u16, u16
812       let (i2, sz) = map_opt(le_u16, |x: u16| x.checked_sub(4)).parse(i1)?;
813       let (i3, data) = take(sz).parse(i2)?;
```

## The Bug

`parse_t123_tpkt()` is the outer framing parser for RDP. At L444–445 it reads the 16-bit TPKT length, subtracts the 4-byte header, and performs `take(sz)` on the network input. If this `take` succeeds, `data` is a *fully populated, bounded* slice containing the entire TPKT payload — no more bytes from the wire can ever be appended to it. The function then attempts three sub-parsers (`parse_x224_connection_request_class_0`, `parse_x224_connection_confirm_class_0`, `parse_x223_data_class_0`) on `data`, each wrapped in `nom8::combinator::opt()` so that a non-matching format falls through to the next candidate and ultimately to `T123TpktChild::Raw`.

The flaw is that `opt()` only swallows `nom8::Err::Error`; it explicitly *propagates* `nom8::Err::Incomplete`. Deep inside the third candidate, `parse_x223_data_class_0()` (L701) calls `parse_mcs_connect()` (L742), whose loop at L762 calls `parse_cs_client_core_data()` (L809). At L812 that function reads an attacker-controlled 16-bit little-endian length and at L813 invokes `nom8::bytes::streaming::take(sz)` (the streaming flavour imported at L34) on whatever remains of the already-bounded inner slice. If the declared CS_CORE length exceeds the bytes actually present — which is entirely under the sender's control — the streaming `take` returns `Err::Incomplete(Needed::Size(n))` rather than `Err::Error`. That `Incomplete` bubbles up unchanged through L797 → L722 → L466 and out of `parse_t123_tpkt()`.

Back in `rust/src/rdp/rdp.rs`, `RdpState::parse_ts()` (and symmetrically `parse_tc()`) handles the `Incomplete` arm at L248–253 by returning `AppLayerResult::incomplete(consumed = 0, needed = available.len() + 1)`. The validation in `app-layer-parser.c:1426` — `res.consumed > input_len || res.needed + res.consumed < input_len` — does not reject `(0, len+1)`, so the engine simply raises `ssn->client.data_required` by one byte and waits. When the next TCP segment arrives, the reassembly layer presents the *same* leading bytes plus the new data; `parse_t123_tpkt()` again succeeds at L445 (the TPKT was already complete the first time), again carves out the *identical* 14-byte `data` slice, and again hits the same `Incomplete` at L813. The parser asks for one more byte, forever. `app_progress` for the to-server direction never advances; `stream-tcp-reassemble.c:1363–1365` detects the lack of progress and breaks out of the per-packet loop, so each segment costs only a single O(1) re-parse of a ≤14-byte slice — there is no CPU spin — but the RDP app-layer for this half-stream is permanently wedged. All subsequent client→server RDP messages are buffered in the TCP reassembly store (up to `stream.reassembly.depth`) but are never decoded, never produce `RdpTransaction` objects, and never appear in eve-log.

**Call chain to the defect:**

```
TCP segment to port 3389 (client → server)
  → stream-tcp-reassemble.c : ReassembleUpdateAppLayer()
    → app-layer.c : AppLayerHandleTCPData()
      → app-layer-parser.c : AppLayerParserParse()
        → p->Parser[0] = rdp_parse_ts            (rust/src/rdp/rdp.rs:465)
          → RdpState::parse_ts()                 (rdp.rs:203)
            → parse_t123_tpkt()                  (rust/src/rdp/parser.rs:440)
               L444-445  take(sz) → COMPLETE bounded `data` slice
               L464      opt(parse_x223_data_class_0).parse(data)
                 → parse_x223_data_class_0()     (parser.rs:701)
                   → parse_mcs_connect()         (parser.rs:742)
                      L762 loop → parse_cs_client_core_data()  (parser.rs:809)
                         L812  sz = le_u16 - 4   (attacker = 0x00FF → 251)
                         L813  streaming take(251) on 0 remaining bytes
                               → Err::Incomplete   ← propagates through every opt()
          → rdp.rs:248-253  AppLayerResult::incomplete(0, available.len()+1)
```

A minimal 18-byte to-server trigger is: `03 00 00 12 02 f0 80 7f 65 44 75 63 61 04 01 c0 ff 00` — a CS_CORE block that claims `0x00FF` bytes inside a 4-byte container.

**Vulnerability class:** network-reachable parser stall / infinite-loop (per-flow, non-CPU-bound).

## Reproduction Results

The trigger below is **analytically derived** from source review of `rust/src/rdp/parser.rs` and `rust/src/rdp/rdp.rs`; it has not been executed against a live Suricata build in this audit, but every byte is accounted for against the parser's verify/tag/length combinators and the nom8 `opt`/`Incomplete` semantics, so confidence is high.

1. Use the default `suricata.yaml` (`app-layer.protocols.rdp.enabled: yes` is the default). No signature is required; the effect is visible in eve-log (`rdp` event type never fires for the affected direction) and via the per-flow `app_progress` remaining at 0.
2. Establish a normal TCP 3-way handshake to a sensor-monitored port 3389, client → server.
3. Send a single 18-byte TCP payload from client to server:

   ```
   03 00 00 12  02 f0 80  7f 65  44 75 63 61  04  01 c0 ff 00
   ```

   Field breakdown:

   | Bytes            | Meaning                                                                                                  |
   |------------------|----------------------------------------------------------------------------------------------------------|
   | `03`             | T.123 version — passes `rdp_probe_ts_tc` and the `verify` at L441                                        |
   | `00`             | reserved                                                                                                 |
   | `00 12`          | TPKT length = 18 → `sz` = 14 → `data` = 14-byte bounded slice (L444–445)                                 |
   | `02 f0 80`       | X.223 Data class-0 header: len=2, DT/ROA=0xF0, EOT=0x80 — passes L712–714                                |
   | `7f 65`          | BER application tag `0x7f`, T.125 Connect-Initial `0x65` — passes L743–750                               |
   | `44 75 63 61`    | ASCII `"Duca"` — consumed by `take_until_and_consume` (L753)                                             |
   | `04`             | PER length determinant = 4 → `length_data` yields a 4-byte inner slice (L755)                            |
   | `01 c0`          | `le_u16` = `0xC001` = `CsType::Core` — passes L810                                                       |
   | `ff 00`          | `le_u16` = `0x00FF` → `sz` = 0xFF − 4 = 251; streaming `take(251)` on 0 remaining bytes → `Err::Incomplete` (L812–813) |

4. Observe Suricata return `AppLayerResult{status: 1, consumed: 0, needed: 19}`. Now send any further client→server bytes (a real MCS Connect-Initial, padding, anything). On each new segment the parser is re-invoked, re-derives the identical 14-byte `data`, and again returns `Incomplete` with `needed = current_buffer_len + 1`. No `rdp` eve-log records are emitted for the to-server direction of this flow for the remainder of the connection; `app_progress` for `ssn->client` stays at 0 until `stream.reassembly.depth` truncates the stream.
5. A symmetric to-client variant exists via `parse_tc` / `parse_mcs_connect_response`, but the to-server CS_CORE path above is the simplest trigger.

Equivalent unit-test trigger (no network required):

```rust
let bytes = [
    0x03,0x00,0x00,0x12, 0x02,0xf0,0x80, 0x7f,0x65,
    0x44,0x75,0x63,0x61, 0x04, 0x01,0xc0,0xff,0x00,
];
// BUG: the TPKT is fully present, this should NOT be Incomplete
assert!(matches!(parse_t123_tpkt(&bytes), Err(nom8::Err::Incomplete(_))));
```

## Severity

**LOW** — The impact is a per-flow detection/logging bypass for the RDP app-layer: eve-log `rdp` events for the affected direction are suppressed for the remainder of the connection, and up to `stream.reassembly.depth` bytes are retained in the TCP reassembly buffer for that single half-stream. There is **no** infinite CPU loop: `stream-tcp-reassemble.c:1363–1365` breaks out as soon as it sees `app_progress` unchanged, so each new TCP segment triggers exactly one O(1) re-parse of a ≤14-byte slice. RDP currently exposes no detection keywords (only `rust/src/rdp/log.rs` consumes the transactions), and raw/content stream inspection uses its own progress tracker and is unaffected. An attacker who already controls one RDP endpoint could achieve equivalent evasion simply by not sending a recognisable handshake, so the marginal security gain from this bug is small. Memory growth is bounded by the existing reassembly-depth cap. No crash, no RCE, no cross-flow effect.

## Suggested Fix

Once `take(sz)` at `parser.rs:445` has succeeded, the TPKT payload is by construction fully present, and no sub-parser running on that bounded slice should ever be allowed to report `Incomplete` to the caller. Wrap each sub-parser invocation on `data` in `nom8::combinator::complete` so that `Err::Incomplete` is converted to `Err::Error`, which `opt()` then swallows, causing fall-through to `T123TpktChild::Raw` (the existing "unknown payload" path) and normal consumption of the TPKT:

```diff
--- a/rust/src/rdp/parser.rs
+++ b/rust/src/rdp/parser.rs
@@
-use nom8::combinator::{map, map_opt, map_res, opt, verify};
+use nom8::combinator::{complete, map, map_opt, map_res, opt, verify};
@@ parse_t123_tpkt
-        match opt(parse_x224_connection_request_class_0).parse(data) {
+        match opt(complete(parse_x224_connection_request_class_0)).parse(data) {
@@
-        None => match opt(parse_x224_connection_confirm_class_0).parse(data) {
+        None => match opt(complete(parse_x224_connection_confirm_class_0)).parse(data) {
@@
-        None => match opt(parse_x223_data_class_0).parse(data) {
+        None => match opt(complete(parse_x223_data_class_0)).parse(data) {
```

Optionally the same `complete()` wrapping can be applied inside `parse_x223_data_class_0` at L720/L727 and inside the `parse_mcs_connect` loop at L762/L769/L777, or the inner-slice `take()` calls at L813, L1011 and L1042 can be switched to `nom8::bytes::complete::take`. However, the three-line change above in `parse_t123_tpkt` is sufficient on its own because it sits at the exact boundary where the slice is known to be complete.