Panic on negative int in writeHexInt / AppendUint (DoS if reachable)
low swesweep-fasthttp · bytesconv.go · writeHexInt, AppendUint · run #4
Description
Both functions call panic() if given a negative n. If an upstream parser bug or a future code change allowed a negative value to reach these functions via attacker-controlled input (e.g., a malformed Content-Length or chunk size), this would crash the entire process. The panic is labeled as a 'developer sanity-check' and current call sites validate non-negativity, but it converts what should be a recoverable error into a fatal crash.
Triage
Not triaged.
Raw scan report for this file
# Security Analysis of `bytesconv.go`
## Step-by-Step Tracing
### 1. `AppendHTMLEscape` / `AppendHTMLEscapeBytes`
- Iterates over `s` byte-by-byte, appends to `dst` via `append` (dynamic).
- No fixed-size buffer, no indexing into `s` beyond its length.
- **Verdict:** Safe.
### 2. `AppendIPv4`
- `ip.To4()` returns either a 4-byte slice or `nil`. Nil is checked.
- `ip[0]..ip[3]` only accessed after nil check.
- **Verdict:** Safe.
### 3. `ParseIPv4` → `parseIPv4Octet`
- Length check on `ipStr` and `dst` before use.
- `dst[i]` writes for `i ∈ [0,3]` into a guaranteed 4-byte slice.
- `parseIPv4Octet`: `k := c - '0'` where `c` is `byte` (uint8). For any char < `'0'` (e.g. `.` = 0x2E), `k` wraps to a large value (≥ 240), which is > 9 → correctly rejected. For chars > `'9'` (≥ 0x3A), `k` ≥ 10 → rejected. Only `'0'`–`'9'` pass.
- Octet bounds: `if octet > 25 || (octet == 25 && k > 5)` fires **before** the multiply, so `octet` never exceeds 255.
- `parsed` (int) can overflow on 32-bit with 10+ digit inputs, but is only used in an error message string — not a size/index.
- **Verdict:** Safe. No OOB, no exploitable overflow.
### 4. `AppendHTTPDate`
- `AppendFormat` produces ≥ 29 bytes. `copy(dst[len(dst)-3:], strGMT)` writes 3 bytes into a valid tail region.
- **Verdict:** Safe.
### 5. `ParseHTTPDate` → `parseRFC1123DateGMT`
- Hard length check `len(b) != 29` before any indexed access `b[0..28]`.
- All sub-parsers (`parse2Digits`, `parse4Digits`, `parseMonth3`, `isWeekday3`) validate digit/char ranges before arithmetic.
- **Verdict:** Safe.
### 6. `ParseUint` → `parseUintBuf`
- Overflow guard: `vNew := 10*v + int(k); if vNew < v { return errTooLongInt }`. Since `v ≥ 0`, any signed wrap makes `vNew` negative → detected.
- Non-digit chars: same unsigned-byte-subtraction logic as `parseIPv4Octet` → correctly rejected.
- **Verdict:** Safe.
### 7. `ParseUfloat`
- Calls `strconv.ParseFloat`. Checks `res < 0` **before** `err != nil`.
- If `ParseFloat` returns `(0, err)` for garbage input, `0 < 0` is false → falls through to error return. Correct.
- If input is `"-inf"`, `ParseFloat` returns `(-Inf, nil)` → caught by `res < 0`. Correct.
- Minor: check ordering is unconventional but not exploitable.
- **Verdict:** Safe (minor style issue).
### 8. `readHexInt`
- `hex2intTable[c]`: `c` is `byte` (0–255), table is 256 entries. No OOB.
- `k == 16` is the sentinel for non-hex. Handled.
- `i >= maxHexIntChars` (7 on 32-bit, 15 on 64-bit) limits total hex digits. Max `n` after shift: `2^28-1` (32-bit) or `2^60-1` (64-bit) — both fit in `int`.
- **Verdict:** Safe.
### 9. `writeHexInt`
- Panics on `n < 0` (developer sanity-check).
- Buffer: `make([]byte, maxHexIntChars+1)` → 8 bytes (32-bit) or 16 bytes (64-bit).
- Loop writes `buf[i]` with `i` starting at `len(buf)-1` and decrementing. Max iterations = number of 4-bit groups in `n` ≤ `maxHexIntChars+1` (for the max positive `int`). `i` never goes below 0.
- **Verdict:** Safe. Buffer is exactly sized for the max positive `int`.
### 10. `AppendQuotedArg` / `appendQuotedPath`
- `quotedArgShouldEscapeTable[int(c)]`: `c` is `byte` → index 0–255. Table is 256 entries. Safe.
- `upperhex[c>>4]` and `upperhex[c&0xf]`: indices 0–15 into a 16-char string. Safe.
- **Verdict:** Safe.
### 11. `lowercaseBytes`
- In-place, index `i` in range `b`. `toLowerTable` is 256 entries. Safe.
### 12. Concurrency
- `hexIntBufPool` (`sync.Pool`) used in standard get/put pattern in `writeHexInt`. No shared mutable state outside the pool.
- **Verdict:** No race.
---
## Summary
This file is **well-defended**. All attacker-reachable paths (parsing IP strings, hex ints, uints, dates, floats) have proper bounds validation. The unsigned-byte arithmetic in `parseIPv4Octet` and `parseUintBuf` correctly rejects non-digit characters via wrap-around producing values > 9. Fixed-size buffers (`writeHexInt`'s pool buffer, `ParseIPv4`'s 4-byte `dst`, `parseRFC1123DateGMT`'s 29-byte check) are all sized to accommodate the maximum valid input.
The only notable observations are minor:
```json
[
{
"severity": "low",
"title": "ParseUfloat checks negative value before parse error",
"function": "ParseUfloat",
"description": "The check 'if res < 0' executes before 'if err != nil'. If strconv.ParseFloat ever returned a negative value alongside a non-nil error, the misleading 'negative input is invalid' error would be returned instead of the actual parse error. In practice ParseFloat returns 0 for unparseable input, so this is not currently exploitable, but the ordering is fragile."
},
{
"severity": "low",
"title": "Panic on negative int in writeHexInt / AppendUint (DoS if reachable)",
"function": "writeHexInt, AppendUint",
"description": "Both functions call panic() if given a negative n. If an upstream parser bug or a future code change allowed a negative value to reach these functions via attacker-controlled input (e.g., a malformed Content-Length or chunk size), this would crash the entire process. The panic is labeled as a 'developer sanity-check' and current call sites validate non-negativity, but it converts what should be a recoverable error into a fatal crash."
},
{
"severity": "info",
"title": "parseIPv4Octet 'parsed' field can overflow on 32-bit with long leading-zero input",
"function": "parseIPv4Octet",
"description": "The 'parsed' int accumulates all digits (e.g., 20+ leading zeros before '1'). On 32-bit platforms, 'parsed = parsed*10 + k' can silently overflow after ~10 digits. The value is only used in the error message string ('parsed %d'), never as a size or index, so there is no memory-safety impact. On 64-bit the overflow threshold is ~19 digits. Informational only."
}
]
```