BudgetScan

Unbounded growth of global filesLockMap sync.Map

low swesweep-fasthttp · fs.go · getFileLock / compressAndOpenFSFile · run #4

Description

getFileLock stores a *sync.Mutex in a package-level sync.Map keyed by the absolute path of every compressed file ever created. The map is never pruned. In a long-running server serving a large directory tree with compression enabled, the map grows monotonically for the life of the process. The growth is bounded by the number of unique files in the tree (not directly attacker-controlled), so impact is limited to slow memory creep, but it is a latent resource-leak in a hot code path.

Triage

Not triaged.

Raw scan report for this file

## Security Analysis: `fs.go` (fasthttp static file serving)

I traced every attacker-controlled input (`ctx.Path()`, `Range` header, `Host` header, `Accept-Encoding`, caller-supplied `path` string) through the full request pipeline: `handleRequest` → `pathToFilePath` → `openFSFile`/`createDirIndex` → reader creation → `ParseByteRange`/`UpdateByteRange` → response body. Below is a walkthrough of the key data flows and the vulnerabilities found.

---

### 1. Attacker-controlled path → `pathToFilePath` → `os.Open`

`ctx.Path()` (decoded URI path) is appended to `h.root` in `pathToFilePath`. For the `osFS` backend the result is passed to `os.Open`. Path traversal (`..`) is blocked in two layers:

- **When `PathRewrite` is set**: `hasDotDotPathSegment(path)` scans the final path for a `..` segment, handling both `/` and `\` separators (line ~910). A match returns 500.
- **When `PathRewrite` is nil** (default): the code relies on fasthttp's URI normalizer to have already collapsed `..` segments (comment: *"ctx.Path must normalize and sanitize the path"*). Null bytes are also rejected via `bytes.IndexByte(path, 0)`.

The `ServeFile`/`ServeFileBytes` family explicitly documents that user-supplied paths must **not** be passed in, and `normalizeServeFilePath` resolves relative paths to absolute — so a caller who passes `../../etc/shadow` gets path traversal by design (documented). The `FS`/`FSHandler` path is the safe path. No bypass found in this file.

### 2. `Range` header → `ParseByteRange` → `UpdateByteRange`

`ParseByteRange` parses `bytes=START-END` or `bytes=-SUFFIX`. Key checks:

- `startPos >= contentLength` → error (catches `bytes=0` on empty file).
- `endPos >= contentLength` → clamped to `contentLength-1`.
- `endPos < startPos` → error.
- Suffix range (`n==0`): `startPos = max(contentLength-v, 0)`, `endPos = contentLength-1`. If `contentLength==0` this yields `(0,-1,nil)`, but downstream `tailLen = endPos - startPos + 1 = 0` causes immediate EOF — no OOB.

In `bigFileReader.UpdateByteRange`, `lr.N = int64(endPos - startPos + 1)`. A negative `N` makes `io.LimitedReader` return EOF immediately. In `fsSmallFileReader`, `tailLen <= 0` returns EOF. No integer overflow is reachable because `contentLength` is validated against `int` range in `newFSFile`.

### 3. `Host` header → `NewVHostPathRewriter`

The `Host` value is concatenated into the request path. It is rejected if it contains `/`. The resulting path still passes through `hasDotDotPathSegment` and the null-byte check in `handleRequest`. No traversal vector found.

### 4. `Accept-Encoding` → compression path

`fileEncoding` is set to a fixed string (`"br"`, `"zstd"`, `"gzip"`) — never taken from the header value. Switch statements on `fileEncoding` have no `default` that would silently skip compression while setting `compressed: true`, because the encoding is always one of the three known values. Safe.

### 5. `contentLength` / `fileInfo.Size()` → `int` conversion

`newFSFile` does:
```go
n := fileInfo.Size()
contentLength := int(n)
if n != int64(contentLength) { return nil, fmt.Errorf("too big file") }
```
This correctly rejects files larger than `MaxInt` (relevant on 32-bit). No overflow.

### 6. Cache manager / `readersCount`

All reads and writes of `ff.readersCount` happen under `cacheManager.Lock()`. `cleanCacheNolock` runs with the lock held. `decReadersCount` acquires the lock. No data race.

### 7. `bigFileReader` pool

Pooled readers are returned via `Close()` → `Seek(0)` → append to `ff.bigFiles`. `ff.Release()` closes `ff.f` **and** every pooled `r.f` under `bigFilesLock`. `readersCount` gates whether `Release` can be called. No use-after-free / double-close path found.

### 8. `fsSmallFileReader` with `dirIndex`

`r.endPos` is set to `ff.contentLength` which equals `len(dirIndex)` for index pages. Slice operations `dirIndex[r.startPos:r.endPos]` and `dirIndex[r.startPos:]` stay in bounds. `WriteTo` with `ff.f == nil` uses the validated slice. Safe.

### 9. **Goroutine & memory leak in `serveFS`** ⚠️

This is the significant finding.

```go
func serveFS(ctx *RequestCtx, filesystem fs.FS, path string, literal bool) {
    f := &FS{ ... }          // NEW instance every call
    handler := f.NewRequestHandler()  // → initRequestHandler → newCacheManager
    ...
    handler(ctx)
}
```

`NewRequestHandler` → `initRequestHandler` → `newCacheManager(fs)` → `go instance.handleCleanCache(fs.CleanStop)`. Because `fs.CleanStop` is **nil** (never set in `serveFS`), the goroutine enters:

```go
for {
    time.Sleep(cm.cacheDuration / 2)  // 5 s
    clean()
}
```

…which **never terminates**. The `FS` instance, its four `map[string]*fsFile` caches, and the goroutine are all unreachable from the caller (the local `f` is discarded after `handler(ctx)` returns) but remain alive because the goroutine holds a reference. **Every single call to `ServeFS` or `ServeFSLiteral` leaks one goroutine and ~10 KB of heap** (four empty maps + struct + stack).

An attacker who can reach a handler that calls `ServeFS` per-request (a natural pattern for serving files from an `embed.FS` or `os.DirFS`) can exhaust the process:

| Requests | Leaked goroutines | ~Stack memory |
|----------|-------------------|---------------|
| 10 000   | 10 000            | ~80 MB        |
| 100 000  | 100 000           | ~800 MB       |
| 1 000 000| 1 000 000         | OOM / crash   |

The ticker-free loop also wakes every 5 s to iterate empty maps, adding sustained CPU load as the count grows.

Contrast with `ServeFile`, which uses a package-level `sync.Once` (`rootFSOnce`) so the handler (and its goroutine) is created exactly once.

### 10. `filesLockMap` (minor)

`getFileLock` stores a `*sync.Mutex` per absolute compressed-file path in a global `sync.Map` that is never pruned. Bounded by the number of files in the served tree, so not independently exploitable, but a long-lived server serving a large tree accumulates mutexes forever.

### 11. Other items examined and cleared

- `stripLeadingSlashes`: panics if path doesn't start with `/` — a "developer sanity check", not attacker-triggerable (fasthttp always produces paths starting with `/`).
- `NewPathPrefixStripper(negative)`: would panic, but `prefixSize` is developer-set.
- `readFileHeader`: `io.LimitedReader{N: 512}` bounds the read; seek-back-to-0 is correct.
- `compressFileNolock`: TOCTOU on the compressed file between `os.Stat` and `os.Rename` is guarded by `getFileLock`; a stale tmp file on error is a minor leak, not exploitable.
- `createDirIndex`: all user-visible strings pass through `html.EscapeString`; no XSS.
- `IfModifiedSince` / 304 path: `decReadersCount` is called exactly once; no double-decrement.
- Type assertions `r.(io.Closer)` and `r.(byteRangeUpdater)`: both concrete reader types implement both interfaces; no panic path.

---

```json
[
  {
    "severity": "high",
    "title": "Goroutine and memory leak in serveFS: every ServeFS/ServeFSLiteral call spawns a non-terminating cleanup goroutine",
    "function": "serveFS / ServeFS / ServeFSLiteral",
    "description": "serveFS allocates a fresh FS value on every call and invokes NewRequestHandler(), which starts an inMemoryCacheManager via newCacheManager(). Because FS.CleanStop is left nil, handleCleanCache() enters an infinite 'for { time.Sleep(5s); clean() }' loop that never returns. The local FS, its four map[string]*fsFile caches, and the goroutine are all unreachable from the caller yet kept alive by the goroutine's captured reference. An attacker who can trigger ServeFS or ServeFSLiteral once per HTTP request (e.g. a handler wrapping an embed.FS) causes linear, unbounded goroutine and heap growth (~8 KB stack + ~10 KB heap per call), leading to process OOM / DoS. The contrast with ServeFile — which reuses a single handler via package-level sync.Once — confirms this is an omission rather than intended design."
  },
  {
    "severity": "low",
    "title": "Unbounded growth of global filesLockMap sync.Map",
    "function": "getFileLock / compressAndOpenFSFile",
    "description": "getFileLock stores a *sync.Mutex in a package-level sync.Map keyed by the absolute path of every compressed file ever created. The map is never pruned. In a long-running server serving a large directory tree with compression enabled, the map grows monotonically for the life of the process. The growth is bounded by the number of unique files in the tree (not directly attacker-controlled), so impact is limited to slow memory creep, but it is a latent resource-leak in a hot code path."
  }
]
```