BudgetScan

UpdateBytes leaves inconsistent URI state on parse failure

low swesweep-fasthttp · uri.go · updateBytes · run #4

Description

For an absolute newURI, u.Parse(nil, newURI) may fail after u.Reset() and partial scheme/host mutation. updateBytes returns nil and UpdateBytes stores u.requestURI = nil without an error return. A crafted Location or redirect value can leave the URI partially initialized, causing malformed FullURI/RequestURI output or host confusion in callers that do not detect the failed update.

Triage

Not triaged.

Raw scan report for this file

## Security analysis

I treated this file as a library component whose public surface is callable by untrusted code or by a host application passing network-derived data. The main attack surface is:

- `URI.Parse` / `URI.parse`
- `URI.Update` / `URI.UpdateBytes`
- `URI.SetPathBytes`
- `URI.SetHostBytes`
- `URI.SetQueryStringBytes`
- `URI.SetHashBytes`
- `URI.FullURI` / `URI.RequestURI`

The primary data flow is:

```text
host/uri -> splitHostURI -> parseHost/unescape -> normalizePath -> URI fields
newURI   -> updateBytes  -> Parse or SetQueryStringBytes/SetHashBytes
```

### Memory-safety review

Because this is Go, the classic C-style stack/heap overflow classes do not apply directly. I reviewed the dangerous-looking in-place slice operations:

- `unescape()` writes back into the input slice, but it only shortens the slice and uses `append` into `s[:0]`.
- `parseHost()` unescapes IPv6 prefix, zone, and suffix as subslices of the same backing array and then concatenates them. The overlapping `append()` calls can alias, but because unescaping only reduces length and Go append/memmove handles overlapping source/destination, I did not find an out-of-bounds write or deterministic memory corruption here.
- `normalizePath()` performs in-place copying with `copy()`. I did not find an index calculation that can go out of bounds for reachable inputs, but the logic has timing and canonicalization issues.
- The fixed-size `var b [32]byte` in `updateBytes()` is not filled from attacker data; it is effectively unused.
- `hex2intTable` is indexed by `byte`, so there is no out-of-range lookup.

### Nil / API-misuse issues

Some methods dereference caller-supplied pointers without checks, for example `CopyTo(dst)` and `WriteTo(w)`. I deprioritized these because they are caller misuse rather than attacker-controlled input through the parsing/redirect surface.

### Most important real issues

1. **Quadratic-time DoS in `normalizePath()`**  
   The duplicate-slash, `/./`, and `/../` removal loops repeatedly scan and copy the remaining buffer. A crafted long path can make this O(n²), which is a practical CPU denial-of-service issue for a network-facing library.

2. **Windows backslash dot-segment normalization is not fully canonical**  
   On Windows, mixed slash/backslash parent-directory removal uses `bytes.LastIndexByte(b[:n], '/')` to find the previous separator. If the previous separator is a backslash or no slash exists, the fallback to index 0 can drop the leading slash or produce a backslash-rooted path. This can bypass downstream path-prefix or canonicalization checks.

3. **`UpdateBytes()` can leave the URI in a partially mutated state**  
   Absolute URIs are handled by calling `u.Parse(nil, newURI)`. If parsing fails after partial mutation, `updateBytes()` returns `nil` and `UpdateBytes()` stores `u.requestURI = nil` with no error return. A malicious `Location` or redirect value can leave an inconsistent URI object.

4. **`DisablePathNormalizing` exposes raw dot segments**  
   When enabled, `RequestURI()` uses `PathOriginal()` without normalization. If a host application uses this mode for untrusted paths, encoded or literal traversal segments can be forwarded as-is.

5. **Setters bypass validation performed during parsing**  
   `Parse()` rejects control bytes in the URI and validates the host via `parseHost()`, but direct setters such as `SetHostBytes`, `SetPathBytes`, `SetQueryStringBytes`, and `SetHashBytes` accept raw bytes. If untrusted data is passed to those setters and later emitted in request/response material, CRLF, NUL, or otherwise invalid URI data may be forwarded.

6. **Port validation accepts unbounded numeric length**  
   `validOptionalPort()` accepts any number of digits after `:`. This may be safe in this file, but downstream conversion to 16-bit ports or fixed-size integers can overflow or behave unexpectedly.

```json
[
  {
    "severity": "medium",
    "title": "Quadratic-time denial of service in path normalization",
    "function": "normalizePath",
    "description": "The in-place duplicate-slash, /./, and /../ removal loops repeatedly scan and copy the remaining buffer. An attacker-controlled path containing a long sequence of //, /./, or /../ supplied through URI.Parse, URI.Update, URI.UpdateBytes, or RequestURI can cause O(n^2) CPU work and exhaust the host process."
  },
  {
    "severity": "low",
    "title": "Windows backslash dot-segment normalization can produce non-canonical paths",
    "function": "normalizePath",
    "description": "When filepath.Separator is a backslash, mixed slash/backslash parent-directory removal uses bytes.LastIndexByte(b[:n], '/') to locate the previous separator. If no ASCII slash exists before the segment, it falls back to index 0 and can overwrite the leading slash with a backslash or truncate to a root-like path. Downstream path-prefix or canonicalization checks may then behave differently or be bypassed."
  },
  {
    "severity": "low",
    "title": "UpdateBytes leaves inconsistent URI state on parse failure",
    "function": "updateBytes",
    "description": "For an absolute newURI, u.Parse(nil, newURI) may fail after u.Reset() and partial scheme/host mutation. updateBytes returns nil and UpdateBytes stores u.requestURI = nil without an error return. A crafted Location or redirect value can leave the URI partially initialized, causing malformed FullURI/RequestURI output or host confusion in callers that do not detect the failed update."
  },
  {
    "severity": "low",
    "title": "Raw dot segments can be emitted when path normalization is disabled",
    "function": "RequestURI",
    "description": "When DisablePathNormalizing is true, RequestURI appends PathOriginal without decoding or dot-segment removal. If an application enables this mode for untrusted URI paths and later uses the resulting path for authorization, file resolution, or prefix checks, values such as /a/../../b are forwarded as-is, creating a path traversal primitive in the host application."
  },
  {
    "severity": "low",
    "title": "Setters bypass URI validation used by Parse",
    "function": "SetHostBytes/SetPathBytes/SetQueryStringBytes/SetHashBytes",
    "description": "Parse rejects control bytes in the URI and validates the host via parseHost, but direct setters accept raw bytes without control-character or escape validation. If untrusted values are passed to these setters and later emitted in FullURI, RequestURI, or HTTP headers, CRLF, NUL, or otherwise invalid host/path/query/hash data can be forwarded."
  },
  {
    "severity": "low",
    "title": "Host port validation accepts unbounded numeric port length",
    "function": "validOptionalPort",
    "description": "The port portion is accepted if it starts with ':' and contains only ASCII digits, with no length or numeric range check. An attacker can provide a very long port such as example.com:999999999999999999999999999999; downstream conversion to 16-bit or fixed-size integers may overflow, error, or behave inconsistently."
  }
]
```