Project

General

Profile

Actions

Bug #9116

open
LS VJ

detect: memory leaks in the alert output

Bug #9116: detect: memory leaks in the alert output

Added by Lukas Sismis 8 days ago. Updated 1 day ago.

Status:
In Review
Priority:
Normal
Assignee:
Target version:
Affected Versions:
Effort:
Difficulty:
Label:

Description

Aggregate ticket for multiple memory leak bug reports relevant in the same/similar areas.

# D4 — alert-json-info-leak-suppressed

## Finding

- **File/line:** `src/detect-engine-alert.c`, `PacketAlertSetContext()` (lines 327–371), reached
  from `PacketAlertSet()` (line 386) → `AlertQueueAppend()` (line 412)
- **Bug class:** heap memory leak (per-alert, unbounded across packets)

`PacketAlertSetContext()` attaches JSON context to every alert enqueued in the per-thread alert
queue (`AlertQueueAppend`): for each `pcre` `alert:` capture belonging to the matching signature it
allocates a `struct PacketContextData` (`SCCalloc`) and `SCStrdup`s the captured JSON string into
`pa->json_info` (line 363).

Those allocations are only released for alerts that are actually copied into the packet's final
alert array: `PacketAlertRecycle()` (`src/decode.c:152`) frees `json_info` for
`p->alerts.alerts[0..cnt)`. `PacketAlertFinalizeProcessQueue()` (`src/detect-engine-alert.c:~676`)
walks the queue and drops entries without copying them in several cases:

- **threshold suppression** — `PacketAlertHandle()` returns 0/2 (`p->alerts.suppressed++`)
- discarded on queue/packet-alert overflow (`p->alerts.discarded++`)
- firewall skip (`skip_td`/`skip_fw`) paths
- pass-action break-outs that stop before copying
- non-alert actions (`(s->action & (ACTION_ALERT|ACTION_PASS)) == 0`)

In every one of those cases, the `json_info` chain allocated when the alert was appended is simply
abandoned when the queue slot is reset by the next packet (`det_ctx->alert_queue_size = 0` in
`src/detect.c:1017` never frees the structs). Traffic that keeps matching a pcre-capture rule while
its alerts are suppressed therefore leaks `sizeof(PacketContextData) + strlen(capture) + 1` bytes
per matching transaction, indefinitely.

## How it reproduces

1. A rule uses the pcre **alert-var capture** syntax — after the closing `/` of the regex, a
   comma-separated variable list with the `alert:` prefix:
   `pcre:"/^([a-z0-9.\-]+)/,alert:host_captured";` (documented in
   `doc/userguide/rules/payload-keywords.rst`, "PCRE extraction").
   Each match calls `DetectAlertStoreMatch()` (`src/detect-pcre.c:161`) which stashes the capture
   in `det_ctx->json_content[]`, and `PacketAlertSetContext()` then duplicates it into the queued
   alert.
2. The rule carries `threshold: type limit, track by_flow, count 1, seconds 3600;` — one alert
   fires, all further matches on the flow are suppressed.
3. The pcap is one established TCP flow with **200 HTTP requests** (distinct `Host:` values so the
   capture differs each time). 100 requests are parsed before the flow times out / is re-inspected
   at flow end; 1 alert fires, 99 are suppressed — and those 99 queue entries' `json_info` are
   never freed.

## Notes / caveats

- Suricata's own exit code is 1 (LSAN failure) — that is the signal, not a crash.
- Only 100 of the 200 requests show up in `alerts_suppressed` because HTTP tx inspection for this
  flow happens partly at flow-timeout reinspection; irrelevant to the leak.
- The same leak affects the other listed paths (discarded, firewall-skip, pass-break,
  non-alert-action); threshold suppression is just the easiest to trigger deterministically.
# D5 — `p->alerts.drop` json_info memory leak (drop action + pcre `alert:` capture)

## Finding

- **File:** `src/detect-engine-alert.c`, `AlertQueueAppend()`, lines ~397-401
- **Bug class:** memory leak (missing free path)
- **Affected allocation:** `PacketAlertSetContext()` (same file, ~line 326-370) — allocates a
  `struct PacketContextData` node (SCCalloc) and strdup's `json_string` for every pcre
  `alert:<name>` capture group matched by a signature.

### The bug

`AlertQueueAppend()` stores an extra copy of the alert in `p->alerts.drop` the first time a
`drop`-action signature matches:

```c
if (p->alerts.drop.action == 0 && s->action & ACTION_DROP) {
    p->alerts.drop = PacketAlertSet(det_ctx, s, tx_id, alert_flags);   /* line 399 */
}
```

`PacketAlertSet()` calls `PacketAlertSetContext()`, which populates `pa->json_info` with a freshly
allocated linked list when the signature has pcre `alert:` captures.

The free sites only ever look at the *regular* alert array:

- `PacketAlertRecycle()` (`src/decode.c:152`) is called from `PACKET_RECYCLE`/`PacketRecycle`
  (`src/packet.c:130-135`) **only on `p->alerts.alerts`**, and only after setting
  `p->alerts.drop.action = 0` — the `json_info` chain hanging off `p->alerts.drop` is never freed
  and its pointer is simply overwritten on the next drop.
- `PacketAlertFree()` (`src/decode.c:168`) frees the packet's alert array at pool teardown, not
  `p->alerts.drop`.

Result: every packet that (a) matches a `drop` rule with pcre `alert:` captures and (b) is the
first drop match on that packet, leaks `N` nodes + `N` strings where N = number of capture
groups. In a long-running IPS deployment with drop rules that extract captures this is an
unbounded, per-dropped-packet heap leak.

## Expected signal

- LeakSanitizer reports `ERROR: LeakSanitizer: detected memory leaks` on the drop run.
- Leak stack goes through `PacketAlertSetContext <- PacketAlertSet <- AlertQueueAppend`
  (`detect-engine-alert.c:399` — the `p->alerts.drop` assignment).
- The control run (alert action) reports **no** leak — the regular alert queue's `json_info` is
  freed by `PacketAlertRecycle()`.

## Notes

- The leak is 62 bytes for a single matching packet with two captures; it scales per dropped
  packet, so it is a slow but unbounded leak in production IPS mode.
- The alert is raised from the flow-timeout pseudo-packet path (`FlowWorkerFlowTimeout`) in this
  pcap because the detection runs on reassembled stream data; the leak path is identical for
  per-packet drop matches — what matters is that `AlertQueueAppend()` takes the drop branch.
- Suggested fix: free `p->alerts.drop.json_info` in `PacketRecycle()` (next to
  `p->alerts.drop.action = 0`, `src/packet.c:130`) — e.g. add a `PacketAlertDropReset()` that
  recycles the drop copy's `json_info` chain — or drop the `json_info` from the copy stored in
  `p->alerts.drop` if the drop logger does not need it.

# D6 — alert json_info leak in the pass-without-alert branch of `PacketAlertFinalizeProcessQueue`

> **Re-validated on 8.0.7 RELEASE (`d681600f3`, branch `release-8.0.7-260913122826`): still
> REPRODUCED — this repo's history briefly marked it "FIXED-UPSTREAM" after testing the wrong
> Suricata version (9.0.0-dev main, where fix commit `5b19508661` "detect/alert: fully add
> pass-only rules to alert queue" lives). That commit is NOT an ancestor of this branch's HEAD
> (`git merge-base --is-ancestor 5b19508661 HEAD` fails) — it was never backported to
> `release-8.0.7-260913122826`.

## Finding

- **File/line:** `src/detect-engine-alert.c`, `PacketAlertFinalizeProcessQueue()`, the pass branch at lines 727–750 (branch condition on line 732, the `break`/`continue` without `p->alerts.cnt++` at lines 734–739; the `cnt++` that gets skipped is line 740). Consequence lands in `src/packet.c:131-135` (`PacketReinit` → `PacketAlertRecycle`) which only frees json_info for indices `[0, p->alerts.cnt)`.
- **Bug class:** memory leak (unreachable heap object), security-adjacent: unbounded memory growth on a long-running sensor, triggerable by ordinary network traffic matching a crafted rule.
- **Allocations leaked:** `PacketContextData` (SCCalloc) + `json_string` (SCStrdup), both from `PacketAlertSetContext()` (`src/detect-engine-alert.c:327-371`).

## Mechanics

1. A rule with action `pass` and a pcre capture named `alert:<key>` matches a packet.
   `AlertQueueAppend()` → `PacketAlertSet()` → `PacketAlertSetContext()` allocates
   `json_info` for the captured group and attaches it to the queued `PacketAlert`.
2. In `PacketAlertFinalizeProcessQueue()` the alert passes thresholding, so it is
   stored: `p->alerts.alerts[p->alerts.cnt] = *pa;` (line 728).
3. The rule action is PASS without ALERT, so the branch at line 732 hits and the
   function `break`s (non-firewall mode) — **`p->alerts.cnt` is never incremented**.
4. When the pooled `Packet` is later recycled, `PacketReinit()` sees `cnt == 0`
   and never frees the json_info sitting at slot `[0]`.
5. The next packet reuses the pooled `Packet` and raises an alert at slot index 0,
   overwriting the `PacketAlert` (and the `json_info` pointer) with a fresh one.
   The old `json_info` becomes unreachable → leak. (At pool shutdown `PacketFree()`
   iterates all slots, so only slots overwritten *during* the run leak.)

Each matching packet leaks one `PacketContextData` (~16 B) plus one duplicated
string (capture-length dependent).

## Notes / caveats

- `--runmode single` is essential. With the default autofp runmode the capture
  thread hands packets to a separate detect thread via a queue; enough packets
  are in flight that each pass-rule packet gets a fresh pooled `Packet` whose
  slot is never overwritten before exit, so `PacketFree()` cleans up and no leak
  is visible. In single mode one thread recycles the same pooled packet
  immediately, so every subsequent packet overwrites the leaked slot.
- Suggested fix: increment `p->alerts.cnt` before the pass-branch `break`/
  `continue` (or recycle the just-written slot before breaking), so the
  recycler sees the json_info-bearing slot.
- Similar overwrite-without-recycle patterns exist for `p->alerts.drop`
  (`AlertQueueAppend`, set before queueing) — not covered here.

Actions

Also available in: PDF Atom