Bug #8795
Updated by Victor Julien 19 days ago
Reported by Communications Security Establishment (CSE):
<pre>
## Summary
In the TLS ClientHello/ServerHello extension parser, the running counter `processed_len` is a `uint16_t` that is incremented by `ext_len + 4` each iteration. An attacker who supplies a single extension with `ext_len >= 0xFFFC`, or many smaller extensions whose cumulative `ext_len + 4` exceeds `0x10000`, wraps `processed_len`, causing the `while (processed_len < extensions_len)` loop to continue past the declared extensions block. Independently, several extension sub-parsers (`SNI`, `EllipticCurves`, `EllipticCurvePF`, `SigAlgorithms`, `SupportedVersions`) return the *actually consumed* byte count which can be `< ext_len`, while the loop always advances `processed_len` by `ext_len + 4`; this desynchronises `input` and `processed_len`. Both issues are *bounded* by the `HAS_SPACE` macro (which checks against the full handshake-message buffer, not `extensions_len`), so there is **no out-of-bounds read or write** — but the parser silently walks into bytes outside the declared extensions block and may misparse them as extensions, polluting JA3/JA4 fingerprints, ALPN list, supported-versions, etc. This is attacker-controlled (any TLS client/server) and the impact is parser-state confusion / detection bypass, not a crash.
## Affected code
- File: `src/app-layer-ssl.c`
- Function: `TLSDecodeHSHelloExtensions`
- Lines: 1287–1437 (at suricata commit 17dc06532)
```c
uint16_t extensions_len = (uint16_t)(*input << 8) | *(input + 1);
input += 2;
if (!(HAS_SPACE(extensions_len)))
goto invalid_length;
uint16_t processed_len = 0;
/* coverity[tainted_data] */
while (processed_len < extensions_len)
{
if (!(HAS_SPACE(2)))
goto invalid_length;
uint16_t ext_type = (uint16_t)(*input << 8) | *(input + 1);
input += 2;
if (!(HAS_SPACE(2)))
goto invalid_length;
uint16_t ext_len = (uint16_t)(*input << 8) | *(input + 1);
input += 2;
if (!(HAS_SPACE(ext_len))) /* checks vs handshake input_len, NOT vs extensions_len */
goto invalid_length;
switch (ext_type) {
case SSL_EXTENSION_SNI:
ret = TLSDecodeHSHelloExtensionSni(ssl_state, input, ext_len);
if (ret < 0) goto end;
input += ret; /* ret may be < ext_len */
break;
...
default:
input += ext_len;
break;
}
...
processed_len += ext_len + 4; /* uint16_t — wraps at 65536 */
}
```
## The bug
There are two coupled defects:
1. **`processed_len` is `uint16_t` and can wrap.** `ext_len` is also `uint16_t` (max 65535). The `HAS_SPACE(ext_len)` check at line 1309 validates `ext_len` against the *outer* `input_len` (the remaining bytes of the handshake message — which is bounded only by the 24-bit handshake `message_length`, up to 16 MB via the `hs_buffer` reassembly path), **not** against `extensions_len`. So a single extension may legally have `ext_len = 65535` even if `extensions_len` is small. After processing it, `processed_len += 65535 + 4` wraps to `3`, and the `while (processed_len < extensions_len)` test re-enters the loop body even though we have already consumed more than `extensions_len` bytes.
2. **`input` advance and `processed_len` advance disagree** for the `SNI`, `ELLIPTIC_CURVES`, `EC_POINT_FORMATS`, `SIGNATURE_ALGORITHMS`, and `SUPPORTED_VERSIONS` cases, which do `input += ret;` where `ret` is what the sub-parser actually consumed. The sub-parsers can return *less* than `ext_len` (e.g. `TLSDecodeHSHelloExtensionSni` only parses the first server-name entry; `TLSDecodeHSHelloExtensionEllipticCurves` consumes one fewer byte if `elliptic_curves_len` is odd; `TLSDecodeHSHelloExtensionSupportedVersions` returns `0` if neither client- nor server-hello flag is set). After this, `input` lags `processed_len` by `ext_len - ret` bytes, so the next iteration parses extension type/length from inside the previous extension's body.
Why this is **not** a memory-safety bug: every dereference of `input` is preceded by `HAS_SPACE(n)` which evaluates `(uint64_t)(input - initial_input) + (uint64_t)(n) <= (uint64_t)(input_len)`. `input_len` here is the remaining bytes of the (already buffered) handshake message, so all reads stay inside `hs_buffer` / the stream slice. The loop also cannot spin forever: `input` is monotonically advanced by at least 4 every iteration (the type+length header), so eventually `HAS_SPACE(2)` fails → `invalid_length`.
**Trigger path from wire bytes:**
1. TCP stream to port 443 (or any TLS-detected port), client→server.
2. TLS record: `0x16 0x03 0x01 <reclen>` (handshake, TLS 1.0). `record_length` ≤ 17 408 enforced at `app-layer-ssl.c:2473`.
3. Handshake header: type `0x01` (ClientHello), 24-bit `message_length` set to a large value (e.g. `0x010100` = 65 792). The handshake-fragment reassembly path (`hs_buffer`, lines 1701–1771 / 1812–1839) buffers data across multiple records until `message_length` bytes are available, so the parser ultimately calls `SSLv3ParseHandshakeType` → `TLSDecodeHandshakeHello` with `input_len = message_length`.
4. ClientHello body: version(2) + random(32) + session_id_len=0(1) + cipher_suites_len=2(2) + one cipher(2) + compression_len=1(1) + null compression(1) = 41 bytes consumed before extensions.
5. Extensions: `extensions_len = 0x0008`. First extension: type `0xeeee` (default branch), `ext_len = 0xFFFC`. The 65 532 body bytes are arbitrary. `processed_len += 0xFFFC + 4 = 0x10000` → wraps to `0`. Loop re-enters with `0 < 8`. `input` is now 65 538 bytes past `initial_input`; remaining `input_len - 65 538 ≈ 213` bytes are parsed as further extensions until exhausted.
## Validation results
- [x] Code-path traced manually from entry point — `SSLDecode` → `SSLv3Decode` → `SSLv3ParseHandshakeProtocol` (records loop, builds `hs_buffer` of `message_length` bytes) → `SSLv3ParseHandshakeType` → `TLSDecodeHandshakeHello` → `TLSDecodeHSHelloExtensions`. Confirmed `input_len` passed to the extensions parser is `message_length - 41` (≫ 65 538), so `HAS_SPACE(0xFFFC)` passes.
- [ ] Compiler/analyzer warning
- [ ] ASan/UBSan runtime hit
- [ ] Reproducer pcap/rule/input attached
- [x] Could NOT trigger a crash at runtime — by inspection, `HAS_SPACE` bounds every read. The defect is a logic/state bug only.
Confidence: high (the arithmetic is unambiguous). Severity: **low** — no memory corruption; impact limited to incorrect extension parsing → wrong/poisoned JA3/JA4/ALPN/SNI metadata, missed `TLS_DECODER_EVENT_HANDSHAKE_INVALID_LENGTH` event, possible detection bypass for rules keying on those buffers.
## Expected fix
1. Make `processed_len` a `uint32_t`.
2. Reject any extension whose `4 + ext_len` would push `processed_len` past `extensions_len` (i.e. enforce the inner length, not just `HAS_SPACE` against the outer buffer):
```c
if ((uint32_t)processed_len + 4 + ext_len > extensions_len)
goto invalid_length;
```
3. For the sub-parser cases that currently do `input += ret;`, advance by `ext_len` instead (the sub-parser already validated everything it needed inside `[input, input+ext_len)`), or have the sub-parsers always return `ext_len` on success.
</pre>