Bug #8826
Updated by Jason Ish 4 days ago
Reported by Communications Security Establishment (CSE): <pre> ## 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`. </pre>