Suricata

Reported by Communications Security Establishment (CSE):

## Summary

`FlowRateStoreFlushRing()` in `src/util-flow-rate.c` is meant to wipe the per-flow
sliding-window ring buffer when a packet arrives after a gap longer than the configured
`interval`. It calls `memset(buf, 0, size)`, but `buf` is a `uint64_t *` allocated as
`size * sizeof(uint64_t)` bytes, so only the first 1/8th of the ring is actually cleared
while `sum` and `last_idx` are fully reset. On the next in-window packet that lands on
one of the surviving stale slots, `FlowRateClearSumInRange()` subtracts the stale byte
counts from the now-tiny `sum`, wrapping the unsigned 64-bit value to near `UINT64_MAX`.
`FlowRateIsExceeding()` then returns true for a flow that has carried only a few hundred
bytes, permanently flagging it `FLOW_IS_ELEPHANT_*` and causing `flow.elephant` rule
keywords to misfire.

## Affected Piece of Code

- **File:** `src/util-flow-rate.c`
- **Function / Location:** `FlowRateStoreFlushRing()` ~L181-190; interacts with
  `FlowRateClearSumInRange()` ~L138-147 and `FlowRateStoreUpdateCurrentRing()` ~L149-179
- **Subsystem:** util-pool-storage — Object pools, storage API, var name registry

```c
 118          frs->dir[i].buf = SCCalloc(frs->dir[i].size, sizeof(uint64_t));
 ...
 138  static inline void FlowRateClearSumInRange(
 139          FlowRateStore *frs, uint16_t start, uint16_t end, int direction)
 140  {
 141      for (uint16_t i = start; i <= end; i++) {
 142          uint64_t byte_count_at_i = frs->dir[direction].buf[i];
 143          frs->dir[direction].buf[i] = 0;
 144          DEBUG_VALIDATE_BUG_ON(frs->dir[direction].sum < byte_count_at_i);
 145          frs->dir[direction].sum -= byte_count_at_i;
 146      }
 147  }
 ...
 181  static inline void FlowRateStoreFlushRing(
 182          FlowRateStore *frs, SCTime_t p_ts, uint32_t pkt_len, int direction)
 183  {
 184      memset(frs->dir[direction].buf, 0, frs->dir[direction].size);
 185      frs->dir[direction].last_idx = 0;
 186      frs->dir[direction].start_ts = p_ts;
 187      frs->dir[direction].buf[0] = pkt_len;
 188      /* Overwrite the sum calculated so far */
 189      frs->dir[direction].sum = pkt_len;
 190  }
```

## The Bug

### Root cause

`FlowRateStoreFlushRing()` is invoked from `FlowRateStoreUpdate()` whenever a packet
arrives more than `size` seconds (the configured `interval`) after the current
sliding-window `start_ts`. Its job is to discard the entire ring and re-seed it with the
new packet. Line 184 attempts this with:

```c
memset(frs->dir[direction].buf, 0, frs->dir[direction].size);
```

However, `buf` is allocated at L118 as `SCCalloc(frs->dir[i].size, sizeof(uint64_t))` —
i.e. `size` *elements* of 8 bytes each. The third argument to `memset` is a *byte*
count, so the call zeroes only `size` bytes instead of the required
`size * sizeof(uint64_t)` bytes. For the shipped example configuration of
`interval: 10`, the ring is 80 bytes long but only the first 10 bytes are cleared: all
of slot 0 and the low 2 bytes of slot 1. Slots 2 through 9 retain whatever byte counts
were recorded before the gap.

Immediately after the partial memset, the function unconditionally resets the running
total: `sum = pkt_len`, `last_idx = 0`, `start_ts = p_ts`. The data structure is now
internally inconsistent — `sum` reflects an empty ring, but `buf[ceil(size/8)..size-1]`
still hold pre-flush values.

### How the inconsistency becomes an underflow

On a subsequent packet that arrives within the new window, `FlowRateStoreUpdate()` takes
the `FlowRateStoreUpdateCurrentRing()` path (L149). That helper computes
`idx = (p_ts.secs - start_ts.secs) % size`. If the new index skips ahead of the previous
one (`idx > last_idx + 1`), the code "ages out" the skipped slots by calling
`FlowRateClearSumInRange(frs, last_idx + 1, idx, direction)`. For each slot in that
range it reads the stored byte count, sets the slot to zero, and subtracts the stored
count from `sum` (L142-145).

Because the flush left stale non-zero values in those slots while resetting `sum` to a
small `pkt_len`, the subtraction at L145 takes a small unsigned 64-bit value below zero.
The only guard is `DEBUG_VALIDATE_BUG_ON(frs->dir[direction].sum < byte_count_at_i)` at
L144, which `util-validate.h:109` compiles to a no-op in release builds. The result is
`sum` wrapping to a value close to `UINT64_MAX`.

`FlowRateIsExceeding()` (L227-229) then compares this wrapped `sum` against the
configured `bytes` threshold and returns true. Back in `FlowUpdateFlowRate()`
(`src/flow.c:374/382`) the flow is stamped `FLOW_IS_ELEPHANT_TOSERVER` /
`FLOW_IS_ELEPHANT_TOCLIENT`. From this point on, `DetectFlowElephantMatch()`
(`src/detect-flow-elephant.c:35-64`) will match the `flow.elephant` rule keyword on a
flow that in reality carried only a handful of small packets. Any rule whose logic
depends on the flow *not* being an elephant (e.g. a custom `pass`/`bypass` rule keyed on
`flow.elephant`) becomes a detection bypass for the attacker's own traffic.

### Network-to-bug call chain

```
Network packet
  → capture source → TM slot
  → FlowWorker()                [src/flow-worker.c:562]
  → FlowHandlePacket()          [src/flow.c:563]          assigns p->flow
  → FlowUpdate()                [src/flow-worker.c:214]
  → FlowHandlePacketUpdate()    [src/flow.c:424]
  → FlowUpdateFlowRate()        [src/flow.c:356, called at :460/:486]
  → FlowRateStoreUpdate()       [src/util-flow-rate.c:192]
      if (p->ts.secs - start_ts.secs >= size)
        → FlowRateStoreFlushRing()   [util-flow-rate.c:181]  ← buggy memset, L184
      else
        → FlowRateStoreUpdateCurrentRing() [L149]
            if (idx > last_idx + 1)
              → FlowRateClearSumInRange() [L138]  ← uint64 underflow, L145
  → FlowRateIsExceeding()       [util-flow-rate.c:227-229]   returns true
  → f->flags |= FLOW_IS_ELEPHANT_TOSERVER/TOCLIENT  [flow.c:374/382]
  → DetectFlowElephantMatch()   [src/detect-flow-elephant.c:35-64]  matches
```

### Required field values to trigger

All packets must share the same 5-tuple so they map to the same `Flow`. The sequence is:
(1) at least one packet that stores non-zero bytes in a ring slot whose index is ≥
`ceil(size/8)`; (2) silence of ≥ `interval` seconds but less than the flow timeout; (3)
one packet to trigger the flush; (4) a gap of ≥ 2 seconds; (5) one more packet whose
`(ts - flush_ts) % size` lands on a stale slot. Payload content is irrelevant — only
wire length and timestamps matter.

**Vulnerability class:** network-reachable logic-bypass.

## Reproduction Results

This is an **analytically-derived trigger**, traced line-by-line through the source; it
has not been executed against a live build during this audit, but every value below is
computed directly from the code paths cited above and is fully constructible from packet
timing alone. No blocker was encountered.

**Precondition** — `suricata.yaml` must enable the (default-off) feature. The values
below are exactly the commented example shipped at `suricata.yaml.in:1591`:

```yaml
flow:
  rate-tracking:
    bytes: 1GiB
    interval: 10        # ring size = 10 uint64 slots = 80 bytes; the buggy memset clears only 10 bytes (slot 0 + low 2 bytes of slot 1)
```

**Optional rule** to observe the result:

```
alert udp any any -> any any (msg:"ELEPHANT"; flow.elephant:to_server; sid:1;)
```

**PCAP / live-traffic sequence** — all packets are IPv4/UDP with the *same* 5-tuple
(e.g. `10.0.0.1:40000 → 10.0.0.2:9999`) so they hash to the same `Flow`. Payload content
is irrelevant; only wire length and pcap timestamp matter. The default UDP "new" 
flow-timeout is 30 s, so the 15 s gap below does **not** recycle the flow.

| Step | t (s)   | Packet                                        | State after `FlowRateStoreUpdate()` |
|------|---------|-----------------------------------------------|-------------------------------------|
| 1    | 0.000   | UDP, 100-byte payload (Eth14+IP20+UDP8+100 = 142 wire bytes; `GET_PKT_LEN`≈142) | `FlowInit()` runs `FlowRateStoreInit()`; `buf[0]=142`, `sum=142`, `start_ts=0` |
| 2    | 1.000   | same UDP, 100-byte payload                    | `buf[1]=142`, `sum=284` |
| 3    | 2.000   | same UDP, 100-byte payload                    | `buf[2]=142`, `sum=426`. Slot index 2 ≥ `ceil(10/8)=2`, so it lies beyond the 10 bytes the bad memset will later clear. |
| 4    | 3.000 … 14.999 | **SILENCE** — no packets on this 5-tuple | — |
| 5    | 15.000  | same UDP, 20-byte payload (wire ≈ 62 bytes)   | `p_ts.secs - start_ts.secs = 15 ≥ size(10)` → `FlowRateStoreFlushRing()`. `memset` clears bytes 0-9 only → `buf[0]` cleared then overwritten with 62; low 2 bytes of `buf[1]` cleared (142 = 0x8E → 0); `buf[2]..buf[9]` **unchanged** (`buf[2]` still 142). State: `sum=62`, `last_idx=0`, `start_ts=15`. |
| 6    | 17.000  | same UDP, 20-byte payload (wire ≈ 62 bytes)   | `idx = (17-15) % 10 = 2`. Since `idx(2) > last_idx+1(1)` → `FlowRateClearSumInRange(frs, 1, 2)`:<br>  `i=1`: `sum = 62 - 0 = 62`<br>  `i=2`: `sum = 62 - 142 = 0xFFFFFFFFFFFFFFB0` (uint64 wrap)<br>then `sum += 62` → `0xFFFFFFFFFFFFFFEE`.<br>`FlowRateIsExceeding()`: `0xFFFFFFFFFFFFFFEE >= 1 GiB` → **TRUE**.<br>`flow.c:374` sets `FLOW_IS_ELEPHANT_TOSERVER`; stats counter `flow.elephant` increments; `sid:1` alerts; eve.json flow record carries `"elephant"` flag — for a flow that moved < 600 bytes total. |

**Offline reproduction:** write the six frames above into a pcap with the stated
timestamps (libpcap header `ts` fields) and run:

```
suricata -c suricata.yaml -r repro.pcap -S elephant.rules -l ./out
```

Inspect `out/eve.json` and `out/stats.log` for the elephant alert/counter.

No blocker — the trigger is fully constructible from packet timing alone; no special
protocol fields are parsed.

## Severity

**LOW** — Logic / accounting error only — no OOB read/write, no crash, no info-leak.

Consequences:

- **(a)** False-positive elephant-flow classification of the attacker's own flow →
  spurious `flow.elephant` rule alerts, inflated `flow.elephant*` stats counters, and an
  incorrect `"elephant"` tag in EVE JSON flow records (alert-fatigue / log-pollution).
- **(b)** Potential detection-bypass **only** if an operator has written a custom
  `pass`/`bypass` rule that keys on `flow.elephant` (not shipped by default) — the
  attacker could then trick that rule into bypassing inspection for their own low-rate
  flow.

The feature is disabled in the shipped `suricata.yaml` (commented out at L1591), further
limiting exposure.

## Suggested Fix

In `src/util-flow-rate.c`, `FlowRateStoreFlushRing()`, pass the byte size to `memset`
instead of the element count:

```diff
--- a/src/util-flow-rate.c
+++ b/src/util-flow-rate.c
@@ -181,7 +181,8 @@
 static inline void FlowRateStoreFlushRing(
         FlowRateStore *frs, SCTime_t p_ts, uint32_t pkt_len, int direction)
 {
-    memset(frs->dir[direction].buf, 0, frs->dir[direction].size);
+    memset(frs->dir[direction].buf, 0,
+            frs->dir[direction].size * sizeof(*frs->dir[direction].buf));
     frs->dir[direction].last_idx = 0;
     frs->dir[direction].start_ts = p_ts;
     frs->dir[direction].buf[0] = pkt_len;
```

**Optional hardening:** in `FlowRateClearSumInRange()` (L145) and
`FlowRateStoreUpdateCurrentRing()` (L172), saturate instead of wrapping, e.g.:

```c
frs->dir[direction].sum = (frs->dir[direction].sum > byte_count_at_i)
        ? frs->dir[direction].sum - byte_count_at_i : 0;
```

so any future accounting bug degrades to "rate = 0" rather than "rate ≈ UINT64_MAX". A
regression unit test mirroring steps 1-6 above (`interval=10`, populate slot 2, flush,
then hit slot 2) should be added alongside `FlowRateTest04`.