Suricata

Reported by Communications Security Establishment (CSE):

## Summary

When EVE JSON logging is configured with `threaded: true` and either a `sensor-name` or an `outputs.eve-log.prefix` string is set, Suricata creates one child `LogFileCtx` per worker thread by structure-assigning the parent context. The shallow copy aliases the heap-allocated `prefix` and `sensor_name` pointers into every child without transferring or duplicating ownership. During output teardown, `LogFileFreeCtx()` is invoked on every child and then on the parent, each unconditionally calling `SCFree()` on those same two allocations, producing an (N_threads + 1)-way double/multi-free. The result is a glibc tcache double-free abort (or an ASAN double-free report) on shutdown, and — in the unix-socket pcap-processing runmode — mid-life termination of a daemon that was meant to keep running.

## Affected Piece of Code

- **File:** `src/util-logopenfile.c`
- **Function / Location:** `LogFileNewThreadedCtx()` ~L886-946 and `LogFileFreeCtx()` ~L952-1020
- **Subsystem:** util-path-conf — Path handling, config parsers (classification/reference/threshold), log file open

```c
src/util-logopenfile.c
 886  static bool LogFileNewThreadedCtx(LogFileCtx *parent_ctx, const char *log_path, const char *append,
 887          ThreadLogFileHashEntry *entry)
 888  {
 889      LogFileCtx *thread = SCCalloc(1, sizeof(LogFileCtx));
 ...
 895      *thread = *parent_ctx;                       /* shallow copy: prefix, sensor_name aliased */
 ...
 927      thread->threaded = false;
 928      thread->parent = parent_ctx;
 ...
 968      if (lf_ctx->threaded) {
 ...
 973          if (lf_ctx->threads->ht) {
 974              HashTableFree(lf_ctx->threads->ht);  /* -> ThreadLogFileHashFreeFunc -> LogFileFreeCtx(child) */
 975          }
 976          SCFree(lf_ctx->threads);
 977      } else {
 ...
 984      }
 985
 986      if (lf_ctx->prefix != NULL) {                /* no lf_ctx->parent guard -> freed by every child */
 987          SCFree(lf_ctx->prefix);
 988          lf_ctx->prefix_len = 0;
 989      }
 990
 991      if(lf_ctx->filename != NULL)
 992          SCFree(lf_ctx->filename);
 993
 994      if (lf_ctx->sensor_name)                     /* same: freed N+1 times */
 995          SCFree(lf_ctx->sensor_name);
```

## The Bug

`LogFileNewThreadedCtx()` (line 886) manufactures a per-thread child `LogFileCtx` by performing a full structure assignment from the parent: `*thread = *parent_ctx;` (line 895). This is a shallow copy. Every heap pointer held by the parent — in particular `prefix` and `sensor_name`, both `SCStrdup`'d earlier in `OutputJsonInitCtx()` — is duplicated by value into the child without a fresh allocation. The only field that is subsequently re-allocated is `filename`, and only in the `LOGFILE_TYPE_FILE` branch (line 909); for `LOGFILE_TYPE_FILETYPE` even `filename` remains aliased. The child is then marked `threaded = false` (line 927) and `parent = parent_ctx` (line 928), but no ownership flag is recorded for the aliased string members and nothing NULLs them on the child.

**Setup chain.** `SuricataInit → RunModeDispatch → RunModeInitializeOutputs → OutputJsonInitCtx()` (`src/output-json.c:1147`) reads the YAML keys `sensor-name` and `outputs.eve-log.prefix`, `SCStrdup`'s them into the parent `LogFileCtx` (lines 1176 and 1221), and sets `file_ctx->threaded = true` when `outputs.eve-log.threaded: true` is present (line 1233).

**Per-worker chain.** `TmThreadsSlotPktAcqLoop → ThreadInit callbacks → JsonLogThreadInit()` / `CreateEveThreadCtx()` (`src/output-json-common.c:41,107`) → `LogFileEnsureExists()` (`src/util-logopenfile.c:787`) → `LogFileNewThreadedCtx()` (line 886). The `*thread = *parent_ctx;` at line 895 shallow-copies the parent's `prefix` and `sensor_name` heap pointers into every child; only `filename` is re-`SCStrdup`'d, and only for `LOGFILE_TYPE_FILE` (line 909).

**Teardown chain.** `main() → SuricataShutdown()` (`src/suricata.c:3194`) → `PostRunDeinit()` (lines 2389/3200) → `RunModeShutDown()` (`src/runmodes.c:576`) → `RunOutputFreeList()` (line 524) → `output_ctx->DeInit == OutputJsonDeInitCtx()` (`src/output-json.c:1333`) → `LogFileFreeCtx(parent)` (`src/util-logopenfile.c:952`). The parent takes the `lf_ctx->threaded` branch (line 968) and calls `HashTableFree(threads->ht)` (line 974). The hash table's free callback, `ThreadLogFileHashFreeFunc()` (lines 376-392), invokes `LogFileFreeCtx()` on each child. Each child has `threaded == false` (set at line 927) and `parent != NULL` (set at line 928), so it skips the threaded branch and falls through to lines 986-995. Those lines have no `lf_ctx->parent == NULL` ownership guard, so every child executes `SCFree(lf_ctx->prefix)` and `SCFree(lf_ctx->sensor_name)` on the parent-owned allocations. After `HashTableFree()` returns, the parent's own `LogFileFreeCtx()` invocation continues past line 977 to lines 986-995 and frees the same two pointers once more. With N worker threads this is an (N + 1)-way multi-free of two heap chunks.

**Alternate, non-process-exit trigger.** The unix-socket pcap runmode reaches the same teardown while the daemon is still expected to be alive: `UnixSocketPcapFilesCheck()` (`src/runmode-unix-socket.c:440`) → `PostRunDeinit(RUNMODE_PCAP_FILE)` (line 454) → `RunModeShutDown()` runs after each queued pcap finishes, so the double-free fires between jobs in a long-lived process rather than only at final exit.

**Exact field values needed.** YAML `sensor-name: <any non-empty string>` (global or under `eve-log`) and/or `outputs.eve-log.prefix: <any non-empty string>`, combined with `outputs.eve-log.threaded: true` and at least one worker thread. No specific packet bytes are required; the bug is purely a function of configuration plus normal teardown.

**Vulnerability class:** double-free.

## Reproduction Results

The trigger below is **analytically derived** from source review of the call chains cited above; it was not executed in this environment because a built Suricata binary was not available. The path is deterministic given the stated configuration — every line on the chain is unconditional once `threaded: true` and a non-NULL `prefix`/`sensor_name` are in place.

1. Create `suricata.yaml` containing:
   ```yaml
   sensor-name: testbox
   outputs:
     - eve-log:
         enabled: yes
         filetype: regular
         filename: eve.json
         threaded: true
         prefix: "@cee: " 
         types: [ alert ]
   ```
2. Run with at least two packet-processing threads, e.g.:
   ```
   suricata -c suricata.yaml -r any.pcap --runmode autofp
   ```
   (or `-i eth0` then send SIGINT). Each worker's `JsonLogThreadInit → LogFileEnsureExists → LogFileNewThreadedCtx` clones the parent ctx, aliasing `prefix` (`"@cee: "`) and `sensor_name` (`"testbox"`).
3. Let the pcap finish (or send SIGTERM/SIGINT for live mode). `PostRunDeinit → RunModeShutDown → OutputJsonDeInitCtx → LogFileFreeCtx(parent) → HashTableFree → ThreadLogFileHashFreeFunc → LogFileFreeCtx(child_1)` frees `prefix`/`sensor_name`; `LogFileFreeCtx(child_2)` frees them again → glibc `free(): double free detected in tcache 2` abort. Under ASAN: heap-use-after-free / double-free at `util-logopenfile.c:987` and `:995`.
4. Variant that crashes a still-running daemon (not just at exit): start `suricata -c suricata.yaml --unix-socket`, then via `suricatasc` send `pcap-file any.pcap /tmp/out`. When the pcap completes, `UnixSocketPcapFilesCheck → PostRunDeinit(RUNMODE_PCAP_FILE) → RunModeShutDown` executes the same teardown and the long-lived unix-socket daemon aborts.

No network packet content controls the freed data; the trigger is purely the documented config combination plus normal shutdown / pcap-job completion.

## Severity

**LOW** — Crash / DoS of the Suricata process during output teardown. With glibc tcache the second `SCFree()` of the same small chunk aborts the process; with ASAN it is a deterministic double-free report. In the normal IDS/IPS runmodes the frees occur after all packet and management threads have been joined (`suricata.c:2415-2428`), so the process was already on its way out — impact there is a dirty exit / core dump instead of a clean shutdown. In unix-socket pcap-processing mode (`runmode-unix-socket.c:454`) the same path runs between jobs while the daemon is intended to stay alive, so the abort terminates an otherwise long-running service. The freed buffers' contents (`sensor-name` / `prefix` strings from local YAML) are not attacker-controlled from the network, the frees happen back-to-back on a single thread with no intervening attacker-influenced allocations, and modern allocators detect the duplicate free immediately, so escalation beyond DoS is not realistic.

## Suggested Fix

Make the child contexts non-owning for the pointers that were shallow-copied from the parent. Two equivalent options:

**Option A — guard the frees on `parent`** (matches the existing pattern at line 1004):

```diff
--- a/src/util-logopenfile.c
+++ b/src/util-logopenfile.c
@@ -985,12 +985,15 @@ int LogFileFreeCtx(LogFileCtx *lf_ctx)
-
-    if (lf_ctx->prefix != NULL) {
-        SCFree(lf_ctx->prefix);
-        lf_ctx->prefix_len = 0;
-    }
-
-    if(lf_ctx->filename != NULL)
-        SCFree(lf_ctx->filename);
-
-    if (lf_ctx->sensor_name)
-        SCFree(lf_ctx->sensor_name);
+    /* Children created by LogFileNewThreadedCtx() share these pointers
+     * with the parent via struct assignment; only the parent owns them. */
+    if (lf_ctx->parent == NULL) {
+        if (lf_ctx->prefix != NULL) {
+            SCFree(lf_ctx->prefix);
+            lf_ctx->prefix_len = 0;
+        }
+        if (lf_ctx->sensor_name)
+            SCFree(lf_ctx->sensor_name);
+    }
+    if (lf_ctx->filename != NULL)
+        SCFree(lf_ctx->filename);   /* per-child SCStrdup for TYPE_FILE */
```

Additionally, for the `LOGFILE_TYPE_FILETYPE` branch in `LogFileNewThreadedCtx()` the child's `filename` is also the parent's pointer (no `SCStrdup` at line 909 for that branch); either NULL it after the struct copy (`thread->filename = NULL;` for non-FILE types) or include `filename` under the same `parent == NULL` guard.

**Option B — deep-copy in `LogFileNewThreadedCtx()`** right after line 895:

```c
    thread->prefix      = parent_ctx->prefix      ? SCStrdup(parent_ctx->prefix)      : NULL;
    thread->sensor_name = parent_ctx->sensor_name ? SCStrdup(parent_ctx->sensor_name) : NULL;
```

and bail to `error:` on allocation failure. Option A is simpler and avoids N redundant copies.