BudgetScan

AddMissingPort fails to append default port for unbracketed IPv6 literals

low swesweep-fasthttp · client.go · AddMissingPort · run #4

Description

For an unbracketed IPv6 address like '::1' (no square brackets, no port), the function enters the IPv4 branch where LastIndexByte finds a colon at position > 0 and returns the address unchanged without appending :80 or :443. The resulting address lacks a port, causing dial failures or unintended port selection. While non-standard, such addresses can appear in user-supplied configuration or upstream host lists.

Triage

Not triaged.

Raw scan report for this file

# Security Analysis: `client.go` (fasthttp)

## Data Flow Tracing

I traced untrusted input through the public API surface:

- **`url`** → `hostnameFromURLString` → `host` → `c.hostClient` → `HostClient.Addr`
- **`req`** → `req.SetRequestURI` / `parseURI` → `req.URI().Host()` → connection target
- **`location`** (server response header) → `getRedirectURL` → `dst.UpdateBytes(location)` → next `url`
- **`postArgs`** → `req.BodyWriter`
- **Server response bytes** → `resp.Read` / `ReadLimitBody` → `dst` buffer

---

## Findings

### 1. Data race on `dst` buffer after timeout in `clientGetURLDeadline`

**Function:** `clientGetURLDeadline` (reachable via `GetDeadline`, `GetTimeout`, `Client.GetDeadline`, `Client.GetTimeout`, `HostClient.GetDeadline`, `HostClient.GetTimeout`)

**Flow:**
```
caller passes dst []byte
  → clientGetURLDeadline(dst, url, deadline, c)
    → spawns goroutine: doRequestFollowRedirectsBuffer(req, dst, url, c)
      → bodyBuf.B = dst          // response body written directly into dst
      → doRequestFollowRedirects → c.Do → transport.RoundTrip
        → resp.ReadLimitBody(br, maxBody)  // reads server body INTO dst
    → caller waits on select { ch | timer }
    → if timer fires: returns body = dst to caller
    → goroutine MAY STILL BE WRITING TO dst
```

The goroutine writes the response body directly into the caller-supplied `dst` slice (`bodyBuf.B = dst`). When the deadline timer fires, the main goroutine sets `timedout = true` and returns `body = dst` to the caller. The spawned goroutine is **not cancelled** — it continues executing `doRequestFollowRedirectsBuffer`, which is still reading the server's response body into `dst`. The code comment explicitly acknowledges this:

> "Note that the request continues execution on ErrTimeout until client-specific ReadTimeout exceeds."

The `sync.Mutex` only guards the `timedout`/`responded` flags and the channel send — it does **not** protect the `dst` buffer itself. This creates a concurrent read (by the caller) and write (by the goroutine) on the same `[]byte` without synchronization.

**Exploitability:** An attacker controlling the remote server can trigger this by:
1. Returning a response with a large `Content-Length` (or chunked body)
2. Sending body data slowly so the client's deadline expires mid-transfer
3. Continuing to send body data after the timeout

The caller receives a partially-filled `dst` that is still being mutated. If the caller parses `dst` (e.g., as JSON, form data, or a protocol message), the attacker can inject corrupted or malicious content. In the worst case, if `dst` is a fixed-size buffer and the goroutine writes beyond the expected boundary, this could corrupt adjacent memory.

**Severity: High** — memory integrity / data corruption, attacker-controllable via slow response.

---

### 2. Nil pointer dereference in public `Do*` methods

**Functions:** `Do`, `DoTimeout`, `DoDeadline`, `DoRedirects` (package-level and on `Client`/`HostClient`)

**Flow:**
```
Do(req, resp)
  → defaultClient.Do(req, resp)
    → uri := req.URI()              // PANIC: nil pointer dereference
```

```
DoTimeout(req, resp, timeout)
  → req.timeout = timeout           // PANIC: nil pointer dereference
```

```
DoDeadline(req, resp, deadline)
  → req.timeout = time.Until(deadline)  // PANIC: nil pointer dereference
```

```
DoRedirects(req, resp, n)
  → req.URI().DisablePathNormalizing = true  // PANIC: nil pointer dereference
```

Every public `Do*` entry point dereferences `req` without a nil check. If `req` is nil, the host process crashes with a runtime panic. While the documentation states "Request must contain at least non-zero RequestURI," the functions are exported library APIs. In a host application that processes untrusted data (e.g., a proxy, API gateway, or web crawler), a code path that produces a nil `*Request` (from a failed parse, a nil map lookup, etc.) will crash the entire process.

The `doNonNilReqResp` method contains an explicit debug panic for nil `req`/`resp`, confirming the developers are aware of this risk but chose to panic rather than return an error.

**Severity: Medium** — process crash (DoS). Not directly attacker-controllable unless the host application has a bug that produces nil requests from untrusted data.

---

### 3. `AddMissingPort` mishandles unbracketed IPv6 literals

**Function:** `AddMissingPort`

```go
func AddMissingPort(addr string, isTLS bool) string {
	...
	isIP6 := addr[0] == '['
	if isIP6 {
		// handles [::1] correctly
	} else { // IPv4
		columnPos := strings.LastIndexByte(addr, ':')
		if columnPos > 0 {
			return addr  // BUG: treats unbracketed IPv6 as having a port
		}
	}
	...
}
```

For an unbracketed IPv6 address like `::1` (no port, no brackets), `addr[0]` is `:` (not `[`), so it enters the "IPv4" branch. `strings.LastIndexByte("::1", ':')` returns `2`, which is `> 0`, so the function returns the address as-is **without appending a port**. The dialer then receives `::1` with no port, which will fail to dial or dial an unintended address.

While unbracketed IPv6 literals are non-standard, they can appear in configuration files or user-supplied addresses. This is a correctness bug rather than a direct security vulnerability, but it could cause connection failures or, in edge cases, connection to an unintended port.

**Severity: Low** — incorrect port handling for non-standard input.

---

## Functions Reviewed and Found Safe

- **`stripSensitiveHeadersOnRedirect` / `shouldStripSensitiveHeadersOnRedirect` / `isDomainOrSubdomainBytes`**: The subdomain check correctly uses `EqualFold` and requires a `.` separator. No bypass via encoding tricks. Sensitive headers (Authorization, Cookie, etc.) are properly stripped on cross-domain redirects.
- **`splitHostPortBytes` / `hostnameFromHostPortBytes`**: Correctly handles bracketed IPv6. The unbracketed IPv6 edge case in `splitHostPortBytes` (multiple colons → no port split) is consistent with `AddMissingPort` behavior.
- **`hostClient`**: Double-checked locking with `mLock` is correct. `mOnce.Do` guarantees map initialization before use.
- **`AcquireConn` / `ReleaseConn` / `decConnsCount`**: Connection pool operations are protected by `connsLock`. The `wantConnQueue` is properly managed.
- **`connsCleaner` / `mCleaner`**: Goroutine lifecycle is managed correctly with proper lock acquisition.
- **`PipelineClient.Do` overflow handling**: Correctly evicts oldest work item and returns `ErrPipelineOverflow`.
- **`transport.RoundTrip`**: Properly handles timeouts, retries, and connection lifecycle. `MaxResponseBodySize` bounds body reads.
- **`tlsServerName` / `newClientTLSConfig`**: The `InsecureSkipVerify = true` path is only triggered by malformed addresses set by the application developer, not by attacker input.
- **`dialHostHard`**: Attempts all configured addresses before giving up. Proper deadline enforcement.

```json
[
  {
    "severity": "high",
    "title": "Data race on caller-supplied dst buffer after request timeout",
    "function": "clientGetURLDeadline",
    "description": "When the deadline timer fires, the function returns dst to the caller while the spawned goroutine may still be writing the server response body into dst via doRequestFollowRedirectsBuffer (bodyBuf.B = dst). The sync.Mutex only guards the timedout/responded flags and channel send, not the dst buffer. An attacker controlling the remote server can trigger this by sending a slow, large response body that causes the client to time out mid-transfer. The caller then reads a buffer that is concurrently being mutated, leading to data corruption, injection of attacker-controlled content into the caller's processing pipeline, or potential memory corruption."
  },
  {
    "severity": "medium",
    "title": "Nil pointer dereference panics in all public Do* entry points",
    "function": "Do, DoTimeout, DoDeadline, DoRedirects (package-level, Client, HostClient)",
    "description": "All public Do* methods dereference req without a nil check: req.URI() in Do, req.timeout assignment in DoTimeout/DoDeadline, and req.URI().DisablePathNormalizing in DoRedirects. Passing a nil *Request causes an unrecoverable runtime panic that crashes the host process. In library consumers that process untrusted data (proxies, API gateways, crawlers), a nil request produced by a failed parse or lookup leads to denial of service. doNonNilReqResp confirms awareness via an explicit debug panic but provides no error return at the public API boundary."
  },
  {
    "severity": "low",
    "title": "AddMissingPort fails to append default port for unbracketed IPv6 literals",
    "function": "AddMissingPort",
    "description": "For an unbracketed IPv6 address like '::1' (no square brackets, no port), the function enters the IPv4 branch where LastIndexByte finds a colon at position > 0 and returns the address unchanged without appending :80 or :443. The resulting address lacks a port, causing dial failures or unintended port selection. While non-standard, such addresses can appear in user-supplied configuration or upstream host lists."
  }
]
```