Suricata

Reported by Communications Security Establishment (CSE):

## Summary

The SSLv2 record parser in `SSLv2Decode()` consumes six fixed `CLIENT_HELLO` body bytes
guarded only by the *buffer*-remaining length, not by the *record*-remaining length.
When an attacker sends an SSLv2 record whose declared `record_length` is between 1 and
6, `bytes_processed` overruns `record_length + record_lengths_length`, and the
subsequent `uint32_t diff = record_length + record_lengths_length - bytes_processed`
underflows to a value near `0xFFFFFFFA`. The wild `diff` is added to the `input` pointer
— undefined behaviour per C11 §6.5.6/8 — and the truncated return value causes
`SSLDecode()` to rewind and re-parse already-consumed body bytes as a fresh record
header. The bug is remotely reachable with a single 9-byte client→server TCP segment on
any TLS-enabled port and requires no special configuration.

## Affected Piece of Code

- **File:** `src/app-layer-ssl.c`
- **Function / Location:** `SSLv2Decode()` ~L2262-2376 (esp. L2263-2278 and L2359-2367)
- **Subsystem:** al-ssl — SSL/TLS handshake parser (record layer, extensions,
  certificates)

```c
/* src/app-layer-ssl.c */

2262        case SSLV2_MT_CLIENT_HELLO:
2263            if (input_len < 6) {
2264                SSLSetEvent(ssl_state, TLS_DECODER_EVENT_INVALID_SSL_RECORD);
2265                return SSL_DECODER_ERROR(-1);
2266            }
...
2275            uint16_t session_id_length = (input[5]) | (uint16_t)(input[4] << 8);
2276            input += 6;
2277            input_len -= 6;
2278            ssl_state->curr_connp->bytes_processed += 6;
...
2359    if (input_len + ssl_state->curr_connp->bytes_processed >=
2360            (ssl_state->curr_connp->record_length +
2361            ssl_state->curr_connp->record_lengths_length)) {
2362
2363        /* looks like we have another record after this */
2364        uint32_t diff = ssl_state->curr_connp->record_length +
2365                ssl_state->curr_connp->record_lengths_length + -
2366                ssl_state->curr_connp->bytes_processed;
2367        input += diff;
2368        SSLParserReset(ssl_state);
...
2376    return SSL_DECODER_OK((input - initial_input));
```

## The Bug

### Root cause

When an SSLv2 record carries `msg_type == SSLV2_MT_CLIENT_HELLO`, the parser at
L2262-2278 unconditionally reads six additional body bytes (client `version` (2),
`cipher_spec_length` (2), `session_id_length` (2)) and advances `input`, `input_len` and
`bytes_processed` by 6. The only guard is `if (input_len < 6)` at L2263, where
`input_len` is the number of bytes remaining in the **stream-slice buffer**, not the
number of bytes remaining inside the **current SSLv2 record**. Nothing checks that the
record itself is large enough to contain those six bytes.

After the header has been parsed by `SSLv2ParseRecord()` (L2098),
`bytes_processed == record_lengths_length + 1` (the length header plus the one
`msg_type` byte). Adding the six `CLIENT_HELLO` bytes yields
`bytes_processed == record_lengths_length + 7`. Control then falls through to L2359,
which is intended to advance `input` to the end of the current record so the next loop
iteration can start cleanly:

```c
uint32_t diff = record_length + record_lengths_length - bytes_processed;
input += diff;
```

This expression silently assumes
`bytes_processed <= record_length + record_lengths_length`. If the attacker sets
`record_length` to any value in `1..6` (e.g., bytes `80 01 01 ...` giving
`record_length = 1`), the right-hand side becomes negative before the implicit
conversion to `uint32_t`. With `record_length = 1`, `record_lengths_length = 2`,
`bytes_processed = 9`, we get `diff = 1 + 2 - 9 = (uint32_t)-6 = 0xFFFFFFFA`. The
statement `input += diff;` then forms a pointer roughly 4 GiB beyond the 9-byte input
buffer — undefined behaviour under C11 §6.5.6/8 even though the pointer is never
dereferenced.

The existing sanity checks do **not** catch this:

- The "have full record" check at L2227 compares `record_lengths_length + record_length`
  (`2 + 1 = 3`) against `input_len + bytes_processed` (`6 + 3 = 9`); `3 > 9` is false,
  so parsing continues.
- The `record_length == 0` check at L2241 is bypassed because `record_length == 1`.
- Neither check compares `record_length` against the six bytes about to be consumed in
  the `CLIENT_HELLO` case.

### Downstream effect — parser desynchronisation

`SSLv2Decode()` returns via `SSL_DECODER_OK((input - initial_input))` at L2376. On a
64-bit target the pointer subtraction yields a `ptrdiff_t` of `0x100000003` (the genuine
9 bytes consumed plus the wild `0xFFFFFFFA` advance). `SSL_DECODER_OK` stores this into
a `uint32_t retval`, truncating it to `0x00000003`. Back in `SSLDecode()` (L2730) the
guard `r.retval < 0 || r.retval > input_len` evaluates `3 > 9` → false, so the caller
believes only **3** bytes were consumed, advances `input` by 3, and re-enters the
`while (input_len > 0)` loop with `input` now pointing at offset 3 of the original
payload. The six bytes that were already parsed as the `CLIENT_HELLO` body
(`00 02 00 00 00 00`) are now re-interpreted as the start of a brand-new SSLv2/TLS
record header. Suricata's view of the SSLv2 record boundaries has diverged from the wire
by six bytes — a deterministic parser desync that an attacker can use to feed Suricata a
different record stream from the one the peer endpoint sees.

### Call chain to the sink

A single client→server TCP segment on any port where `ALPROTO_TLS` is detected (default
`443`, plus the probing-parser port set) drives the following chain:

1. `RegisterSSLParsers()` (`src/app-layer-ssl.c:3219`) registers `SSLParseClientRecord`
   as the `STREAM_TOSERVER` handler.
2. `SSLParseClientRecord()` (L2807) → `SSLDecode()` (L2658).
3. The dispatch loop at L2696 inspects `input[0]`; because `input[0] & 0x80` is set
   (L2710), it assigns `curr_connp->version = SSL_VERSION_2` and calls `SSLv2Decode()`
   (L2728-2729) → `SSLv2Decode()` (L2182).
4. `SSLv2Decode()` calls `SSLv2ParseRecord()` (L2208 → L2098).

With the 9-byte payload `80 01 01 00 02 00 00 00 00`:

- `SSLv2ParseRecord()` sees `input[0] & 0x80`, sets `record_lengths_length = 2`,
  computes `record_length = (0x7F & 0x80) << 8 | 0x01 = 1`, sets `content_type = 0x01`
  (`SSLV2_MT_CLIENT_HELLO`), `bytes_processed = 3`, returns `3`. `input` advances to
  offset 3, `input_len = 6`.
- L2227: `2 + 1 > 6 + 3` → false → continue.
- L2241: `record_length == 0` → false → continue.
- L2262 `case SSLV2_MT_CLIENT_HELLO`: L2263 tests `input_len < 6` (buffer-remaining),
  `6 < 6` → false. Reads `version = 0x0002`, `session_id_length = 0`, then `input += 6`,
  `input_len = 0`, `bytes_processed = 9`.
- L2359: `0 + 9 >= 1 + 2` → true.
- L2364: `diff = 1 + 2 - 9 = 0xFFFFFFFA`; L2367: `input += 0xFFFFFFFA` (**UB**); L2368:
  `SSLParserReset()`.
- L2376: returns `SSL_DECODER_OK(0x100000003)` → `retval = 3`.
- Caller L2730: `3 <= 9`, accepted; `input += 3`, `input_len -= 3`; loop iteration 2 now
  parses offsets `3..8` as a new record.

Protocol detection is satisfied without any prior traffic: `SSLProbingParser()` (L2941)
accepts the input because `(input[0] & 0x80) && input[2] == 0x01`, and the
pattern-matcher signature `|01 00 02|` registered at offset 2 (L3012) also matches
`bytes[2..4] = 01 00 02`.

**Vulnerability class:** network-reachable integer underflow → undefined-behaviour
pointer arithmetic and record-stream desynchronisation.

## Reproduction Results

No special configuration or rule is needed; the default `suricata.yaml` with the TLS
app-layer parser enabled (the default) suffices.

1. Establish a TCP three-way handshake from any client IP/port to
   `<sensor-monitored-host>:443` (or any port in the configured `tls` detection-ports
   list).
2. Send one client→server TCP data segment whose payload is exactly these 9 bytes (hex):

   ```
   80 01 01 00 02 00 00 00 00
   ```

   Field meaning as Suricata parses it:

   | Bytes  | Meaning |
   |--------|---------|
   | `80 01` | SSLv2 2-byte length header, high bit set, `record_length = 0x0001` |
   | `01`    | `msg_type = SSLV2_MT_CLIENT_HELLO` |
   | `00 02` | client version (read at L2272; value `0x0002` also satisfies PM pattern &#124;01 00 02&#124;) |
   | `00 00` | `cipher_spec_length` (unused on this path) |
   | `00 00` | `session_id_length = 0` |

	
3. Suricata's app-layer dispatch calls `SSLParseClientRecord` → `SSLDecode` →
   `SSLv2Decode` with `input_len = 9`.
4. Inside `SSLv2Decode()` the underflow occurs at L2364-2367:
   `diff = 1 + 2 - 9 = 0xFFFFFFFA` (`uint32_t`), then `input += 0xFFFFFFFA`.
5. Observable evidence without a debugger:
   - On a build compiled with `-fsanitize=undefined` / `-fsanitize=pointer-overflow`,
     the process aborts at `app-layer-ssl.c:2367` with a diagnostic of the form
     "pointer index expression ... overflowed".
   - On a release build, `SSLv2Decode()` returns `retval = 3` (not `9`); `SSLDecode()`
     then re-enters the loop with `input` pointing at offset 3 (`00 02 00 00 00 00`) and
     parses those body bytes as a fresh record — i.e., the SSLv2 record stream is
     desynchronised by six bytes.


	
Variant: any `record_length` in `1..6` triggers the same underflow; e.g.,
`80 03 01 00 02 00 00 00 00` gives `diff = 3 + 2 - 9 = 0xFFFFFFFC` and `retval = 5`.


	
Pcap/scapy one-liner for the step-2 payload (after completing the handshake):


	
```python
send(IP/TCP/bytes.fromhex('800101000200000000'))
```


	
or simply:


	
```sh
printf '\x80\x01\x01\x00\x02\x00\x00\x00\x00' | ncat TARGET 443
```


	
Status of reproduction: This is an analytically-derived trigger. Every value in
the call chain above was traced statically through the current source; the arithmetic is
deterministic and depends on no compiler-defined behaviour prior to the UB at L2367. The
trigger has not been exercised against a live Suricata instance as part of this audit,
but no guard in the code path can reject the payload before the underflow is reached.


	

	
Severity
	


	
LOW


	
Two effects flow from the bug:


	
1. Undefined behaviour. `input += 0xFFFFFFFA` forms a pointer roughly 4 GiB outside
   the `StreamSlice` buffer. The pointer is never dereferenced, but its mere formation
   is UB; a UBSan build aborts here, and an aggressively-optimising compiler is
   permitted to assume the subtraction at L2364 cannot underflow, which could in
   principle license unexpected transformations of the surrounding code.
2. Detection bypass / parser desync. Because the truncated `retval`
   (`record_length + record_lengths_length`, e.g. `3`) is smaller than the 9 bytes
   actually consumed, `SSLDecode()` rewinds and re-parses six attacker-controlled body
   bytes as a new SSLv2/TLS record header. Suricata's view of record boundaries diverges
   from the wire, so subsequent record-level inspection (frames, events, keyword
   matching) operates on a stream the peer never sent.


	
Mitigating factors keep this at LOW:


	
- The dispatch loop in `SSLDecode()` is bounded by `max_records` (L2695-2703), so there
  is no infinite loop or hang.
- No out-of-bounds read or write occurs; the wild pointer is used only for a difference
  computation.
- Practical evasion value is limited: a real TLS server would reject an SSLv2 record
  with `record_length = 1` that carries 7+ body bytes, so this cannot cloak a working
  handshake. The primary concern is the formal UB and localised intra-slice parser
  confusion rather than a viable end-to-end TLS-inspection bypass.


	

	
Suggested Fix
	


	
Validate that the SSLv2 record actually contains the six fixed `CLIENT_HELLO` bytes
before consuming them. The `msg_type` byte has already been counted against
`record_length`, so at least 7 record bytes are required. This guarantees
`bytes_processed` can never exceed `record_length + record_lengths_length`, which in
turn makes the subtraction at L2364 safe.


	
```diff
--- a/src/app-layer-ssl.c
+++ b/src/app-layer-ssl.c
@ -2260,7 +2260,9 @ static struct SSLDecoderResult SSLv2Decode(uint8_t direction, SSLState *ssl_sta
             break;


	
case SSLV2_MT_CLIENT_HELLO:
-            if (input_len < 6) {
+            /* we already consumed 1 byte (msg_type) of record_length; need 6 more
+             * (version(2) + cipher_spec_len(2) + session_id_len(2)) inside the record */
+            if (input_len < 6 || ssl_state->curr_connp->record_length < 7) {
                 SSLSetEvent(ssl_state, TLS_DECODER_EVENT_INVALID_SSL_RECORD);
                 return SSL_DECODER_ERROR(-1);
             }
```


	
Optional defence-in-depth at the sink: before computing `diff` at L2364, bail out if
`bytes_processed > record_length + record_lengths_length` (set
`TLS_DECODER_EVENT_INVALID_SSL_RECORD` and return `SSL_DECODER_ERROR(-1)`). This ensures
any future code path that over-consumes record bytes cannot reintroduce the underflow.