BudgetScan

Possible index-out-of-range panic in b2s for empty byte slices

low swesweep-fasthttp · args.go · AddBytesKV / SetBytesKV / DelBytes / PeekMultiBytes (via b2s) · run #4

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.

Triage

Not triaged.

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."
  }
]
```