Unbounded query-string allocation and retained pool capacity

medium swesweep-fasthttp · args.go · Args.ParseBytes · run #3

Description

ParseBytes appends attacker-controlled query data into a.buf and allocates one argsKV for every non-empty argument without a bound on total length or argument count. Reset and ReleaseArgs do not release the enlarged a.buf, a.args, or argsKV.key/value backing arrays, so pooled Args objects can retain large attacker-controlled allocations across requests, enabling memory/resource exhaustion in host applications that pass untrusted query strings to the public API.

Triage

Verdict: in_progress
Model: qwen/qwen3.8-27b
Rounds: 1
Cost: –

The report claims that `Reset` and `ReleaseArgs` do not release enlarged backing arrays (`a.buf`, `a.args`, `argsKV.key/value`), leading to retained large allocations in pooled objects. I verified the code: `Reset` executes `a.args = a.args[:0]`, which sets the logical length to 0 but retains the underlying array's capacity (`cap(a.args)` remains high). Similarly, `Parse` uses `a.buf = append(a.buf[:0], s...)`, which reuses the existing capacity. `allocArg` and `appendArg` expand slices as needed during parsing, increasing capacity. Because `ReleaseArgs` returns the object to `sync.Pool` without resetting capacity to 0, a single request with a large query string (e.g., 100MB) will cause the pooled `Args` object to allocate ~100MB of memory. Subsequent smaller requests will reuse this large capacity, meaning the host application retains the 100MB allocation indefinitely (until the pool is cleared or GC reclaims the underlying arrays if the pool is idle, but `sync.Pool` tends to keep objects if they are reused). This constitutes a resource exhaustion vulnerability (DoS) reachable via the public API (`RequestCtx.QueryArgs()` or `Args.Parse`). An attacker can craft a request with a massive query string to force the server to allocate and retain large memory buffers, eventually crashing the server or causing performance degradation due to memory pressure. This is a valid security issue under the threat model of a library receiving untrusted input.

CRUX: Reset() sets len to 0 but does not shrink cap, so pooled Args objects retain large memory allocations from previous large inputs.
GREP: a.args = a.args[:0]

All rounds

[
  {
    "finding_title": "Unbounded query-string allocation and retained pool capacity",
    "verdict": "VALID",
    "reasoning": "The report claims that `Reset` and `ReleaseArgs` do not release enlarged backing arrays (`a.buf`, `a.args`, `argsKV.key/value`), leading to retained large allocations in pooled objects. I verified the code: `Reset` executes `a.args = a.args[:0]`, which sets the logical length to 0 but retains the underlying array's capacity (`cap(a.args)` remains high). Similarly, `Parse` uses `a.buf = append(a.buf[:0], s...)`, which reuses the existing capacity. `allocArg` and `appendArg` expand slices as needed during parsing, increasing capacity. Because `ReleaseArgs` returns the object to `sync.Pool` without resetting capacity to 0, a single request with a large query string (e.g., 100MB) will cause the pooled `Args` object to allocate ~100MB of memory. Subsequent smaller requests will reuse this large capacity, meaning the host application retains the 100MB allocation indefinitely (until the pool is cleared or GC reclaims the underlying arrays if the pool is idle, but `sync.Pool` tends to keep objects if they are reused). This constitutes a resource exhaustion vulnerability (DoS) reachable via the public API (`RequestCtx.QueryArgs()` or `Args.Parse`). An attacker can craft a request with a massive query string to force the server to allocate and retain large memory buffers, eventually crashing the server or causing performance degradation due to memory pressure. This is a valid security issue under the threat model of a library receiving untrusted input.\n\nCRUX: Reset() sets len to 0 but does not shrink cap, so pooled Args objects retain large memory allocations from previous large inputs.\nGREP: a.args = a.args[:0]",
    "elapsed": 206.6,
    "tokens": 7415,
    "tokens_in": 7038,
    "tokens_out": 377,
    "cost_usd": 0.004086960000000001,
    "file": "args.go",
    "round": 1
  }
]

Raw scan report for this file


## Security analysis of `args.go` (fasthttp `Args`)

I traced the untrusted-input path: `Parse(s)` → `a.buf` → `ParseBytes(b)` → `argsScanner.next(kv)` → `decodeArgAppend(dst, src)`, where the query-string bytes flow into `kv.key`/`kv.value` and into the `hex2intTable` index.

### Core parser is memory-safe

- **Percent-decoding (`decodeArgAppend` / `decodeArgAppendNoPlus`)**: The `hex2intTable[src[i+1]]` / `[src[i+2]]` lookups are safe. `src[i+1]` and `src[i+2]` are `byte` values (0–255) used as indices into a 256-entry table, so they can never go out of bounds. The guard `if i+2 >= len(src) { return append(dst, src[i:]...) }` ensures `i+1`/`i+2` are valid `src` indices before dereferencing. Invalid hex bytes map to `16`, causing the literal `%` to be preserved — no OOB, no crash. Decoding can only shrink or preserve length (each `%`/`+`/literal → 1 byte), so there is **no unbounded allocation** from crafted input.
- **Slice arithmetic in `next`**: `s.b[:i]`, `s.b[k:i]`, `s.b[i+1:]`, `s.b[k:]` are all provably in bounds because `=` always precedes the matching `&`/end-of-string, so `k <= i`. No out-of-range reads.
- **Sentinel/reuse logic in `ParseBytes`**: I traced cases like `a=1&=&b=2`, `=1&b=2`, `a=`, `=`, `&b=2` — empty key/value segments are correctly dropped and the sentinel `argsKV` is safely reused without cross-contamination. `releaseArg` always has `len>=1` (a sentinel is allocated before the loop), so `h[:len(h)-1]` cannot panic.
- **`b2s` aliasing in `Set*`/`Add*`**: `setArg`/`appendArg` copy bytes via `append(dst[:0], value...)` into the `argsKV`'s own backing array, so stored values do **not** alias the caller's slice or `a.buf`. No dangling-alias corruption.

### Findings worth reporting

1. **Algorithmic-complexity DoS in `delAllArgsStable`** (reachable from untrusted input). `Del`/`DelBytes` call `delAllArgsStable`, which performs `copy(args[i:], args[i+1:])` on *every* matching key. An attacker can flood a query string with many duplicate keys (`x=1&x=2&x=3&...` — bounded only by request size, so `N` can be large). A subsequent `Del("x")` then runs in `O(N²)`. This is a realistic amplification DoS for a high-throughput HTTP library.

2. **Potential nil-index panic via `b2s` on empty slices** (low confidence — `b2s` is not defined in this file). `AddBytesKV`, `SetBytesKV`, `DelBytes`, `PeekMultiBytes`, etc. pass byte slices through `b2s`. If `b2s` is implemented as `unsafe.String(&b[0], len(b))`, an *empty* slice panics on `&b[0]`. Empty values/keys are attacker-influenceable (e.g. `somekey=` yields an empty `PeekBytes` value that an app may feed back into `DelBytes`/`AddBytesV`), so this is an indirect crash vector, contingent on the `b2s` implementation.

### Considered and deprioritized (per threat model)
- **Pool reuse / data races** (`AcquireArgs`/`ReleaseArgs`): documented "MUST NOT be used concurrently"; programmer-error class.
- **`noCopy` not enforced**: copying an `Args` shares `args`/`buf` backing, but the API forbids it; programmer-error class.
- **`GetUint` returning signed `int`** (`-1` on missing): API oddity, not a memory/logic safety bug.
- **`PeekMulti` per-iteration `string(k)` allocations / `Sort` with invalid comparator**: performance/programmer-error, not attacker memory-safety issues.

```json
[
  {
    "severity": "low",
    "title": "Algorithmic-complexity DoS in Args.Del/DelBytes (O(n^2) delAllArgsStable)",
    "function": "delAllArgsStable / Del / DelBytes",
    "description": "delAllArgsStable executes copy(args[i:], args[i+1:]) for every arg whose key matches. An attacker can craft a query string containing many duplicate keys (e.g. x=1&x=2&...&x=N, bounded only by request size). If the host application later calls Del/DelBytes with that key, the deletion runs in O(N^2) time, enabling a CPU/DoS amplification in a high-throughput HTTP server. Reachable because the args are populated from untrusted input via Parse/ParseBytes."
  },
  {
    "severity": "low",
    "title": "Possible index-out-of-range panic in b2s for empty byte slices",
    "function": "AddBytesKV / SetBytesKV / DelBytes / PeekMultiBytes (via b2s)",
    "description": "Several exported methods convert their byte-slice arguments to strings via b2s. If b2s is implemented as unsafe.String(&b[0], len(b)) (b2s is not defined in this file, so this is implementation-dependent), an empty slice causes a &b[0] index-out-of-range panic. Empty keys/values are attacker-influenceable: for example an attacker sends 'somekey=', the app reads the empty value with PeekBytes and feeds it back into DelBytes/AddBytesV, triggering the panic. Low confidence because it depends on the actual b2s implementation and on host-application usage."
  }
]
```