Security #8853
Updated by Jason Ish 16 days ago
Reported by Communications Security Establishment (CSE): <pre> ## 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 <code>|01 00 02|</code>) | | `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(dst=TARGET)/TCP(dport=443, flags='PA', seq=..., ack=...)/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. </pre>