Bug #8851
Updated by Jason Ish 23 days ago
Reported by Communications Security Establishment (CSE): <pre> ## Summary `AppLayerExpectationCreate()` obtains a locked and reference-counted `IPPair` object via `IPPairGetIPPairFromHash()`, but on the error path taken when the per-pair `ExpectationList` allocation fails it jumps to `error:` without ever unlocking or releasing that `IPPair`. The mutex on the IP pair therefore remains held forever and its use-count is leaked. Any subsequent worker thread that looks up the same source/destination IP pair — for example `AppLayerExpectationHandle()` during new-flow setup, or another FTP `RETR`/`STOR` between the same two hosts — will block indefinitely in `SCMutexLock()`, taking that packet-processing thread out of service. The trigger is network-reachable through the FTP parser, but requires a ~24-byte `SCCalloc` to fail, so the process must already be at hard OOM for the leak to occur. ## Affected Piece of Code - **File:** `src/app-layer-expectation.c` - **Function / Location:** `AppLayerExpectationCreate()` ~L238–279 (specifically L258–260 → L276) - **Subsystem:** al-core — App-layer core dispatch, protocol detection, frames, expectations ```c src/app-layer-expectation.c: 238 ipp = IPPairGetIPPairFromHash(&ip_src, &ip_dst); 239 if (ipp == NULL) 240 goto error; 241 242 exp_list = SCIPPairGetStorageById(ipp, g_ippair_expectation_id); 243 if (exp_list) { ... 257 } else { 258 exp_list = SCCalloc(1, sizeof(*exp_list)); 259 if (exp_list == NULL) 260 goto error; /* <-- ipp still LOCKED + refcounted */ ... 273 IPPairUnlock(ipp); 274 return 0; 275 276 error: 277 SCFree(exp); /* <-- no IPPairRelease/IPPairUnlock */ 278 return -1; 279 } ``` ## The Bug **Vulnerability class:** network-reachable lock-leak / denial of service. `IPPairGetIPPairFromHash()` at L238 returns an `IPPair` that is both *locked* and *use-counted*: the implementation in `src/ippair.c` (L491/499 and L599–600/608–609) takes `SCMutexLock(&h->m)` and calls `IPPairIncrUsecnt(h)` before handing the object back to the caller. The caller is therefore responsible for dropping both. On the normal success paths `AppLayerExpectationCreate()` does this correctly — it calls `IPPairRelease(ipp)` at L254 (when an existing list is present and the entry was appended) or `IPPairUnlock(ipp)` at L273 (when a new list was created and attached as storage). The defect is in the else-branch starting at L257. When no `ExpectationList` has yet been associated with this IP pair, the function attempts to allocate one with `SCCalloc(1, sizeof(*exp_list))` at L258. If that allocation fails, control transfers via `goto error` at L260 directly to the `error:` label at L276, which performs only `SCFree(exp); return -1;`. Neither `IPPairUnlock(ipp)` nor `IPPairRelease(ipp)` is called. The `IPPair`'s mutex consequently remains held for the lifetime of the process, and its use-count is permanently elevated so the entry can never be recycled. There is no preceding guard or short-circuit that would prevent this path from being reached once the lookup at L238 has succeeded. The downstream effect is a hard hang: any later thread that calls `IPPairLookupIPPairFromHash()` or `IPPairGetIPPairFromHash()` for the same `(src-IP, dst-IP)` tuple — which happens in `AppLayerExpectationHandle()` for every new flow between those two hosts during protocol detection, and again in `AppLayerExpectationCreate()` if another FTP data-channel expectation is set up — will block forever inside `SCMutexLock(&h->m)`, permanently removing that worker thread from packet processing. **Call chain from the wire to the defect site:** 1. Network packet → `FlowWorker()` [`src/flow-worker.c:562`] 2. → `StreamTcp()` / `StreamTcpReassembleAppLayer()` [`src/stream-tcp-reassemble.c:1286/1359/1413`] 3. → `AppLayerHandleTCPData()` [`src/app-layer.c:710`] 4. → `AppLayerParserParse()` [`src/app-layer-parser.c:1316`] dispatches `ALPROTO_FTP` / `STREAM_TOSERVER` 5. → `FTPParseRequest()` [`src/app-layer-ftp.c:421`] Inside `FTPParseRequest()`, after a prior `PORT`/`PASV` exchange has populated `state->dyn_port != 0`, a client line such as `"RETR x\r\n"` or `"STOR x\r\n"` (with `line.len >= 6`) reaches the `FTP_COMMAND_RETR` / `FTP_COMMAND_STOR` case at `src/app-layer-ftp.c:512–560`. That handler allocates an `FtpTransferCmd` and calls: ```c AppLayerExpectationCreate(f, direction, 0, state->dyn_port, ALPROTO_FTPDATA, data); ``` at `src/app-layer-ftp.c:544`. Execution then enters `AppLayerExpectationCreate()` [`src/app-layer-expectation.c:217`]. `IPPairGetIPPairFromHash(&ip_src, &ip_dst)` at L238 returns the locked, use-counted pair as described above. Because this is the *first* expectation ever created for this particular `(src-IP, dst-IP)` pair, `SCIPPairGetStorageById()` at L242 returns `NULL` and the else-branch at L257 is taken. `SCCalloc(1, sizeof(ExpectationList))` — roughly 24 bytes — at L258 must then return `NULL` (i.e. the process is at hard OOM). Control jumps to `error:` (L276–278), which only frees `exp` and returns `-1`, leaking the lock and the reference. ## Reproduction Results **Status:** analytical only. A deterministic network-side trigger was not constructed because step 5 below requires an `SCCalloc` of ~24 bytes to fail, meaning the Suricata process must already be at its hard OOM limit; that condition cannot be forced solely through this code path and is non-deterministic from the attacker's vantage point. **Required Suricata config:** default `suricata.yaml` (`app-layer.protocols.ftp` is enabled by default). No detection rules required. **Protocol message sequence** (TCP, client `10.0.0.2:50000` ↔ server `10.0.0.1:21`, full three-way handshake first): 1. **S→C:** `"220 FTP ready\r\n"` — server banner; not strictly required for parsing but helps drive protocol detection. 2. **C→S:** `"USER anonymous\r\n"` (hex: `55 53 45 52 20 61 6e 6f 6e 79 6d 6f 75 73 0d 0a`) — establishes `ALPROTO_FTP` via pattern detection. 3. **C→S:** `"PASV\r\n"` (hex: `50 41 53 56 0d 0a`) — sets `state->command = FTP_COMMAND_PASV`. 4. **S→C:** `"227 Entering Passive Mode (10,0,0,1,200,10)\r\n"` — `FTPParseResponse()` → `FTPParsePassiveResponse()` sets `state->dyn_port = 200*256 + 10 = 51210`, `state->active = false`. *Alternative active-mode path:* C→S `"PORT 10,0,0,2,200,10\r\n"` followed by S→C `"200 PORT OK\r\n"`; `FTPParseResponse()` parses the stored `port_line` and sets `dyn_port`. 5. **C→S:** `"RETR a\r\n"` (hex: `52 45 54 52 20 61 0d 0a`, `line.len = 6`) — `FTPParseRequest()` enters the `FTP_COMMAND_RETR` case (`app-layer-ftp.c:512`); `state->dyn_port == 51210 != 0` and `line.len >= 6`, so it allocates an `FtpTransferCmd` and calls `AppLayerExpectationCreate(f, STREAM_TOCLIENT, 0, 51210, ALPROTO_FTPDATA, data)`. - **Pre-condition for the bug branch:** this must be the *first* expectation for the `(10.0.0.2, 10.0.0.1)` IP pair so that `SCIPPairGetStorageById()` returns `NULL` and the else-branch at L257 is taken. - **Trigger condition (NOT attacker-deterministic):** `SCCalloc(1, sizeof(ExpectationList))` at L258 returns `NULL`. In a test harness this can be simulated by `LD_PRELOAD`'ing a `calloc` that fails on the Nth invocation, or by setting `RLIMIT_AS` so the process is at its address-space limit when step 5's packet is processed. 6. **After the leak:** send any new TCP `SYN` from `10.0.0.2` to `10.0.0.1:51210` (or repeat steps 3–5 on a fresh control connection between the same two IPs). The worker thread handling that packet will call `IPPairLookupIPPairFromHash()` / `IPPairGetIPPairFromHash()` for `(10.0.0.2, 10.0.0.1)` and block forever in `SCMutexLock(&h->m)`. **Blocker for a pure-network PoC:** the ~24-byte `calloc` failure cannot be reliably induced remotely; the process would already be in an OOM-induced DoS state before the lock leak adds incremental harm. ## Severity **LOW.** The defect is a permanent mutex hold plus use-count leak on an `IPPair` object. Once leaked, any subsequent packet-processing thread that looks up the same source/destination IP pair — `AppLayerExpectationHandle()` during new-flow setup, or another FTP `RETR`/`STOR` between the same hosts — deadlocks in `SCMutexLock`, removing that worker from service. If repeated across multiple IP pairs this degrades to a full traffic-processing stall (denial of service / detection bypass). However, the precondition is a failed ~24-byte `calloc`, which essentially requires the process to already be at hard OOM — a state that is itself a denial of service. The incremental attacker-controlled impact beyond the pre-existing OOM condition is therefore small, and the rating is kept at LOW. ## Suggested Fix Release the `IPPair` (dropping both the lock and the use-count, since nothing was attached to it) before jumping to the error label when the `ExpectationList` allocation fails: ```diff --- a/src/app-layer-expectation.c +++ b/src/app-layer-expectation.c @@ -256,8 +256,10 @@ int AppLayerExpectationCreate(Flow *f, int direction, Port src, Port dst, } } else { exp_list = SCCalloc(1, sizeof(*exp_list)); - if (exp_list == NULL) + if (exp_list == NULL) { + IPPairRelease(ipp); goto error; + } exp_list->length = 0; CIRCLEQ_INIT(&exp_list->list); CIRCLEQ_INSERT_HEAD(&exp_list->list, exp, entries); ``` Equivalently, initialise `ipp = NULL` at the top of the function and add `if (ipp) IPPairRelease(ipp);` in the `error:` label so that any future error path introduced after L238 is also covered automatically. </pre>