BudgetScan

Background DNS cache cleaner goroutine never terminates (goroutine leak)

low swesweep-fasthttp · tcpdialer.go · tcpAddrsClean · run #4

Description

once.Do in dial() starts 'go d.tcpAddrsClean()' which loops forever calling time.Sleep(1s) and cleanExpiredDNSEntries(). There is no Stop() method, no context.Context parameter, and no other cancellation path. If a caller creates a TCPDialer instance for a short-lived purpose (e.g., per-request in a high-churn service) and discards it, the goroutine leaks indefinitely, accumulating one leaked goroutine per discarded dialer. For the package-level defaultDialer this is benign (process lifetime), but for user-constructed dialers it is a resource leak that grows linearly with the number of created-and-discarded instances.

Triage

Not triaged.

Raw scan report for this file

## Detailed Security Analysis

I'll trace the data flow from the public API surface through each function, looking for exploitable issues.

### Data Flow

```
Dial/DialTimeout/DialDualStack/DialDualStackTimeout
  → TCPDialer.dial(addr, dualStack, timeout)
    → TCPDialer.getTCPAddrs(addr, dualStack, deadline)
      → resolveTCPAddrs(addr, dualStack, resolver, deadline)
        → net.SplitHostPort(addr) → host, portS
        → strconv.Atoi(portS) → port
        → resolver.LookupIPAddr(ctx, host) → ipaddrs
        → build []net.TCPAddr
    → TCPDialer.tryDial(network, addr, deadline, concurrencyCh)
      → net.Dialer.DialContext(ctx, network, addr)
```

Attacker-controlled: `addr` (host:port string), `timeout`, and if they construct their own `TCPDialer`: `Resolver`, `Concurrency`, `DNSCacheDuration`, `LocalAddr`, `DisableDNSResolution`.

---

### Function-by-Function Analysis

**`Dial` / `DialTimeout` / `DialDualStack` / `DialDualStackTimeout` (package-level)**
Thin wrappers delegating to `defaultDialer`. No independent logic. Safe.

**`TCPDialer.Dial*` methods**
All funnel into `d.dial(addr, dualStack, timeout)`. No validation of `addr` or `timeout` beyond what downstream functions do.

**`TCPDialer.dial`**
- `once.Do` initializes `concurrencyCh`, `DNSCacheDuration`, and starts the background cleaner. The `sync.Once` guarantees single initialization. Correct.
- `deadline := time.Now().Add(timeout)` — if `timeout` is negative or zero, `deadline` is at or before `time.Now()`. Downstream `tryDial` checks `time.Until(deadline) <= 0` and returns `ErrDialTimeout` immediately. Safe.
- `n := uint32(len(addrs))` — `addrs` comes from `getTCPAddrs` which guarantees `len(addrs) > 0` (enforced by `resolveTCPAddrs` returning `errNoDNSEntries` when empty). So `n > 0` and `idx % n` never divides by zero. Safe.
- The round-robin loop `for range n` iterates exactly `n` times. `idx` is a `uint32` from `atomic.AddUint32`; `idx % n` is well-defined for unsigned. Safe.
- **Key observation:** `getTCPAddrs` is called *before* any `tryDial`, and it does **not** acquire a slot from `concurrencyCh`. DNS resolution is therefore **not** bounded by the concurrency limiter.

**`TCPDialer.tryDial`**
- Checks `timeout <= 0` → immediate `ErrDialTimeout`. Safe.
- Concurrency slot acquisition with non-blocking + timed fallback. If the timed select times out, it returns *before* the `defer func() { <-concurrencyCh }()` line, so no slot is leaked. Correct.
- `dialer.DialContext` with a deadline context. Errors are wrapped via `wrapDialWithUpstream`. If `ctx.Err() == context.DeadlineExceeded`, returns `ErrDialTimeout`. Otherwise wraps the actual error. Both paths pass non-nil `err` to the wrapper. Safe.
- `d.LocalAddr` is nil-checked before assignment. Safe.

**`TCPDialer.getTCPAddrs`**
- **Race condition (TOCTOU):** Two goroutines can simultaneously `Load(addr)`, both find no entry (or an expired one), both set `e = nil`, both call `resolveTCPAddrs`, and both `Store` a new entry. The second `Store` overwrites the first. The first goroutine holds a reference to the now-orphaned entry, uses its `addrs`, and the orphaned entry is eventually GC'd. Impact: redundant DNS lookups and a brief window where two different `addrsIdx` counters exist for the same address. Not a memory safety issue, but a correctness concern.
- **`pending` flag logic:** When an entry is expired, only the goroutine that successfully `SwapInt32(&e.pending, 1)` from 0→1 re-resolves. Others fall through with the stale `e`. This is intentional (single-flight re-resolution) but means expired DNS entries are served to concurrent callers until the re-resolve completes and the map is updated.
- On resolution failure, it re-loads the entry and resets `pending` to 0. If the entry was *also* overwritten by a concurrent successful store in the meantime, it resets the *new* entry's `pending` — a minor logic flaw but not exploitable.

**`resolveTCPAddrs`**
- `net.SplitHostPort(addr)` returns an error for malformed input (missing port, multiple colons, etc.). Safe.
- `strconv.Atoi(portS)` returns an error for non-numeric or out-of-`int`-range values. A port like `"99999"` parses successfully but produces `net.TCPAddr{Port: 99999}`; the `net` package rejects this at `DialContext` time. Safe.
- `resolver.LookupIPAddr(ctx, host)` — `host` is caller-controlled. With `net.DefaultResolver`, this sends a DNS query. A very long or malformed host causes the resolver to return an error. The context deadline bounds the wait. Safe from a crash perspective, but see the concurrency-limit finding below.
- `!dualStack && ip.IP.To4() == nil` filters out IPv6 when not dual-stack. If all results are IPv6, `addrs` is empty → `errNoDNSEntries`. Safe.
- `n := len(ipaddrs)` then `make([]net.TCPAddr, 0, n)` — allocation is bounded by resolver output. Safe.

**`cleanExpiredDNSEntries` / `tcpAddrsClean`**
- `sync.Map.Range` is safe for concurrent modification. The type assertion `v.(*tcpAddrEntry)` is guarded by `ok`. Safe.
- `expireDuration := 2 * d.DNSCacheDuration` — if `DNSCacheDuration` is `0` (before `once.Do` sets it), `expireDuration` is `0`, and every entry is immediately expired. But `tcpAddrsClean` is started *inside* `once.Do` after `DNSCacheDuration` is set, so this can't happen in practice. Safe.
- **Goroutine leak:** `tcpAddrsClean` loops forever with `time.Sleep(1s)`. There is no `Stop()` method, no `context.Context`, and no other cancellation mechanism. If a `TCPDialer` instance is short-lived (e.g., created per-request in a server), the goroutine leaks. For the package-level `defaultDialer` this is benign (process lifetime), but for user-created dialers it's a resource leak.

**`FlushDNSCache`**
- `sync.Map.Range` + `Delete` inside. Safe.

**`ErrDialWithUpstream.Error()`**
- `e.wrapErr.Error()` would panic if `wrapErr` is nil. All internal call sites pass non-nil errors. However, the struct is exported, so a caller *could* construct `&ErrDialWithUpstream{}` with a nil `wrapErr` and trigger a panic. This is an API robustness issue, not an externally-triggerable vulnerability.

---

### Findings

The most significant externally-triggerable issue is the **lack of any bound on DNS cache entries and on concurrent DNS resolution**. An attacker who controls the `addr` parameter (e.g., in a reverse-proxy or API-gateway that passes user-supplied hostnames to `fasthttp.Dial`) can:

1. Supply millions of unique `host:port` strings → each triggers a `tcpAddrsMap.Store` → unbounded memory growth until the 2-minute cleaner runs.
2. Make many concurrent `dial` calls with unique addrs → `getTCPAddrs` calls `resolveTCPAddrs` → each performs an unrestricted `resolver.LookupIPAddr` → the `concurrencyCh` limiter is **not** consulted during DNS resolution → unlimited concurrent DNS queries → exhaustion of the system's DNS resolver, network stack, or upstream DNS server.

The goroutine leak is lower severity but worth noting for long-lived services that create and discard `TCPDialer` instances.

```json
[
  {
    "severity": "medium",
    "title": "Unbounded DNS cache growth allows memory exhaustion",
    "function": "getTCPAddrs / dial",
    "description": "The tcpAddrsMap (sync.Map) has no maximum entry limit. Each unique addr string triggers a DNS lookup and a Store into the map. An attacker supplying many unique host:port values (e.g., via user-controlled hostnames in a proxy) can create an unbounded number of cache entries within the 2-minute expiry window (2 x DefaultDNSCacheDuration). Each entry holds a []net.TCPAddr. While the background cleaner (tcpAddrsClean) removes entries older than 2*DNSCacheDuration, entries created in a burst persist until then. Combined with the absence of a concurrency limit on DNS resolution (see next finding), an attacker can rapidly fill the map and exhaust process memory."
  },
  {
    "severity": "medium",
    "title": "No concurrency limit on DNS resolution — unlimited concurrent resolver queries",
    "function": "dial / getTCPAddrs",
    "description": "The concurrencyCh limiter (default capacity 1000) is only enforced in tryDial for the TCP connection phase. getTCPAddrs, which calls resolveTCPAddrs and ultimately resolver.LookupIPAddr, is invoked before any tryDial call and does NOT acquire a concurrency slot. An attacker issuing many concurrent Dial/DialTimeout calls with distinct addr values triggers unbounded concurrent DNS lookups. With net.DefaultResolver this floods the system DNS resolver; with a custom in-process Resolver it can trigger unbounded CPU/memory. The Concurrency field's documented guarantee ('maximum number of concurrent Dials') does not cover the DNS resolution phase."
  },
  {
    "severity": "low",
    "title": "Background DNS cache cleaner goroutine never terminates (goroutine leak)",
    "function": "tcpAddrsClean",
    "description": "once.Do in dial() starts 'go d.tcpAddrsClean()' which loops forever calling time.Sleep(1s) and cleanExpiredDNSEntries(). There is no Stop() method, no context.Context parameter, and no other cancellation path. If a caller creates a TCPDialer instance for a short-lived purpose (e.g., per-request in a high-churn service) and discards it, the goroutine leaks indefinitely, accumulating one leaked goroutine per discarded dialer. For the package-level defaultDialer this is benign (process lifetime), but for user-constructed dialers it is a resource leak that grows linearly with the number of created-and-discarded instances."
  },
  {
    "severity": "low",
    "title": "TOCTOU race in getTCPAddrs causes redundant resolution and orphaned cache entries",
    "function": "getTCPAddrs",
    "description": "Two goroutines can simultaneously Load(addr), both find the entry missing or expired, both set e=nil, both call resolveTCPAddrs, and both Store a new *tcpAddrEntry. The second Store overwrites the first, orphaning the first entry. The orphaned entry is still referenced by the first goroutine (so it is used for that request's round-robin) but is no longer in the map, so the background cleaner will never delete it — it is only reclaimed by GC after the goroutine releases its reference. Impact: redundant DNS lookups (amplifying resolver load) and a brief window where two independent addrsIdx counters exist for the same address, slightly skewing round-robin distribution. Not a memory-safety issue but a correctness and load-amplification concern."
  }
]
```