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