Suricata

Reported by Communications Security Establishment (CSE):

## 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.