BudgetScan

Nil handler in ServeConn causes panic on first request

low swesweep-fasthttp · server.go · ServeConn (package-level) · run #4

Description

The exported `ServeConn(c, handler)` assigns `s.Handler = handler` without a nil check. If `handler` is nil, the subsequent `s.Handler(ctx)` call inside `serveConn` invokes a nil function, panicking and crashing the connection goroutine. While this is a programmer error, it is reachable through the public API and results in an unhandled panic with no recovery.

Triage

Not triaged.

Raw scan report for this file

Looking at this file, I need to trace how untrusted data (HTTP requests from `net.Conn`) flows through the server's request-processing loop and identify where malformed input could trigger panics, resource exhaustion, or memory corruption.

**Key untrusted data flows:**
- Raw TCP bytes → `bufio.Reader` → `ctx.Request.Header.Read(br)` → `parseURI()` → `readLimitBody`/`readBodyStream` → `s.Handler(ctx)`
- Multipart form data → `ctx.MultipartForm()` / `ctx.FormValue()` / `ctx.FormFile()`
- Response writing → `writeResponse(ctx, bw)` → `br`/`bw` pools

**Function-by-function analysis:**

1. **`ServeConn` (top-level)**: `handler` parameter is assigned to `s.Handler` without nil check. If nil, `s.Handler(ctx)` in `serveConn` panics. Programmer error but reachable via public API.

2. **`serveConn`**: The main loop. `maxRequestBodySize` defaults to 4MB, bounding body reads. `br.Peek(1)` on keep-alive handles EOF. `s.Handler(ctx)` is called without panic recovery — a panic here kills the goroutine and leaks `br`, `bw`, `ctx` (no deferred cleanup for these). The `concurrency`/`open` counters are properly balanced via `defer s.serveConnCleanup()`.

3. **`FormFile`**: `mf.File[key]` returns `[]*FileHeader`. The nil check catches missing keys, but if the slice is non-nil yet empty (a parser invariant violation), `fhh[0]` panics with index-out-of-range.

4. **`FormValue` / `defaultFormValue`**: Falls through to `ctx.MultipartForm()` → `MultipartFormWithLimit(0)` — no size cap on multipart parsing. Body is bounded by `maxRequestBodySize`, but if the application sets `MaxRequestBodySize` to 0 or very large, an attacker can drive unbounded memory allocation through multipart form parsing.

5. **`TimeoutWithCodeHandler`**: Spawns a goroutine that does `<-concurrencyCh` after `h(ctx)` returns. If `h(ctx)` panics, the goroutine dies and the concurrency slot is never released — a permanent leak per panic.

6. **`acquireByteReader`**: `var b [1]byte; c.Read(b[:])` — bounded, safe. Panic on `n != 1` is a developer sanity check, not reachable with conforming `net.Conn`.

7. **`SaveMultipartFile`**: `fh.Open()` panics on nil `fh`. `path` is used directly in `os.Create`/`os.Rename` — documented as requiring server-trusted paths.

8. **`EarlyHints`**: Reads Link headers from `ctx.Response.Header` (handler-set, not attacker-set). Writes them to the connection. No untrusted data path.

9. **`writeErrorResponse`**: Ignores return values of `writeResponse` and `bw.Flush()` — errors are silently dropped, potentially leaving the connection in an inconsistent state.

10. **Pool type assertions** (`v.(*Server)`, `v.(*RequestCtx)`, `v.(*bufio.Reader)`, `v.(*atomic.Int64)`): Unchecked, but pools are internal and only populated by the server itself. Safe.

```json
[
  {
    "severity": "medium",
    "title": "Index out of range panic in FormFile on empty file header slice",
    "function": "RequestCtx.FormFile",
    "description": "After checking `fhh == nil`, the code returns `fhh[0]` without verifying `len(fhh) > 0`. If the multipart parser produces a non-nil empty `[]*FileHeader` for a given key (e.g., due to a crafted or edge-case multipart body), this triggers an index-out-of-range panic, crashing the connection goroutine. An attacker can attempt to trigger this by crafting a multipart form where a file field key maps to an empty header list."
  },
  {
    "severity": "medium",
    "title": "Unbounded multipart form memory allocation via default FormValue/MultipartForm (DoS)",
    "function": "RequestCtx.FormValue / RequestCtx.MultipartForm",
    "description": "The default `FormValue` implementation and `MultipartForm()` call `MultipartFormWithLimit(0)`, imposing no explicit cap on multipart parsing. While the initial body read is bounded by `MaxRequestBodySize` (4 MiB default), the multipart parser allocates additional memory for file headers, temporary files, and parsed values. If the application sets `MaxRequestBodySize` to 0 or a very large value, an attacker can send a large multipart body and force the server to allocate proportional (or greater) memory, leading to OOM. Even with the default 4 MiB limit, the parser's overhead amplifies per-request memory usage under high concurrency."
  },
  {
    "severity": "low",
    "title": "Concurrency slot leak when handler panics in TimeoutWithCodeHandler",
    "function": "TimeoutWithCodeHandler",
    "description": "The spawned goroutine executes `h(ctx); ch <- struct{}{}; <-concurrencyCh`. If `h(ctx)` panics (e.g., on malformed input reaching the handler), the goroutine terminates before `<-concurrencyCh` executes, permanently leaking one slot in the bounded `concurrencyCh`. Since fasthttp performs no panic recovery, each panicked request permanently reduces available concurrency. Repeated malformed requests can exhaust all concurrency slots, causing all subsequent requests to receive 429 Too Many Responses — a targeted DoS."
  },
  {
    "severity": "low",
    "title": "Nil handler in ServeConn causes panic on first request",
    "function": "ServeConn (package-level)",
    "description": "The exported `ServeConn(c, handler)` assigns `s.Handler = handler` without a nil check. If `handler` is nil, the subsequent `s.Handler(ctx)` call inside `serveConn` invokes a nil function, panicking and crashing the connection goroutine. While this is a programmer error, it is reachable through the public API and results in an unhandled panic with no recovery."
  },
  {
    "severity": "low",
    "title": "Resource leak (br/bw/ctx) on handler panic in serveConn",
    "function": "Server.serveConn",
    "description": "The `serveConn` method acquires `br`, `bw`, and `ctx` from sync.Pools but releases them only at the end of the function body (after the for-loop). If `s.Handler(ctx)` panics, the panic bypasses all cleanup code, leaking the pooled reader, writer, and context. The `defer s.serveConnCleanup()` handles counter bookkeeping but not pool object release. Under repeated panics (e.g., crafted malformed requests), pool objects accumulate, increasing memory pressure until OOM."
  }
]
```