Partial header state corruption after trailer parse error

[
  {
    "finding_title": "Partial header state corruption after trailer parse error",
    "verdict": "VALID",
    "reasoning": "The report claims that `tryReadTrailer` corrupts the header state `h.h` when parsing a trailer fails. I examined the code for `tryReadTrailer` in `header.go`. The function calls `parseTrailer`, which returns a new slice `hh` if successful, or the original `dest` (which is `h.h`) along with an error if it fails early (specifically before any `appendArgBytes` calls, such as on invalid key or forbidden trailer). \n\nHowever, the key lies in the caller `ResponseHeader.ReadTrailer` or the internal flow. Looking at `tryReadTrailer`:\n```go\nhh, headersLen, errParse := parseTrailer(b, h.h, h.disableNormalizing)\nh.h = hh\nif errParse != nil {\n    ...\n    return headerError(...)\n}\n```\nThe report correctly notes that `h.h = hh` happens *before* the error check. If `parseTrailer` processes some valid trailer lines (appending them to `h.h`) and *then* encounters a malformed line, `parseTrailer` returns the modified `h.h` slice (as `dest`) and an error. Consequently, `h.h` retains the partially parsed trailer data.\n\nIn contrast, `ResponseHeader.Read` calls `resetSkipNormalize` on error, which clears `h.h` (among other things). `RequestHeader.Read` also calls `resetSkipNormalize`. The `ReadTrailer` path does *not* call `resetSkipNormalize` on error. \n\nTherefore, if an attacker sends a chunked response with a valid trailer entry followed by a malformed header, the `ResponseHeader` object will retain the valid trailer entry in `h.h`. A caller who ignores the error (or only checks for specific errors) and continues to use the `ResponseHeader` (e.g., calling `VisitAll` or `ContentLength`) will observe this corrupted state. Since `ReadTrailer` is a public API reachable via network input (client-side or server-side handling of trailers), and the state corruption leads to incorrect application behavior (data integrity), this is a valid security finding under the library threat model (correct behaviour on malformed input).\n\nCRUX: tryReadTrailer assigns h.h = hh before checking errParse, and parseTrailer mutates the destination slice before returning an error on malformed input, leaving partial state if the caller does not reset.\nGREP: func (h *header) tryReadTrailer",
    "elapsed": 85.0,
    "tokens": 32323,
    "tokens_in": 31779,
    "tokens_out": 544,
    "cost_usd": 0.01497918,
    "file": "header.go",
    "round": 1,
    "grep_used": true,
    "grep_results": "GREP `func (h *header) tryReadTrailer (simplified to: tryReadTrailer)`:\n```\nheader.go:2199:\t\terr := h.tryReadTrailer(r, n)\nheader.go:2210:func (h *header) tryReadTrailer(r *bufio.Reader, n int) error {\n```"
  },
  {
    "finding_title": "Partial header state corruption after trailer parse error",
    "verdict": "INVALID",
    "reasoning": "The report claims that a parsing error in `ReadTrailer` leaves partial data in `h.h`, leading to 'corrupted header state' and security impact. I must evaluate if this is a security vulnerability under the provided threat model.\n\n1. **Code Behavior Verification**: \n   - `tryReadTrailer` calls `parseTrailer(b, h.h, ...)`. \n   - `parseTrailer` appends valid keys to `dest` (which is `h.h`) as it parses. If it encounters a malformed line (e.g., invalid byte), it returns `dest` (the modified slice) and an error.\n   - `tryReadTrailer` assigns `h.h = hh` (the modified slice) *before* checking the error. \n   - On error, `tryReadTrailer` returns the error. It does *not* call `resetSkipNormalize()`. \n   - Therefore, `h.h` does indeed retain the partial parsed trailers.\n\n2. **Security Impact Analysis**:\n   - The threat model defines vulnerabilities as causing 'meaningful harm (crash, code execution, data corruption, auth bypass)' or 'memory safety and integrity' issues. It explicitly states that 'correct behaviour on malformed input' is an asset, but the verdict rules specify: 'A crash, memory corruption or unbounded resource use reachable through the public API with a crafted payload is VALID.'\n   - The finding is about *logic* inconsistency: the header object is in a partially modified state after a failed read. \n   - Does this lead to memory corruption? No. The slices are managed by Go's runtime. \n   - Does this lead to a crash? No. \n   - Does this lead to code execution? No. \n   - Does this lead to auth bypass? Unlikely. Trailers are metadata. Even if a trailer like `Authorization` were partially parsed, `isBadTrailer` blocks `Authorization` (and other sensitive headers) from being parsed into trailers anyway. The headers allowed in trailers are non-sensitive (like `Checksum`). \n   - The 'harm' described is 'unexpected headers in VisitAll, incorrect Content-Length resolution'. `Content-Length` cannot be a trailer (blocked by `isBadTrailer`). So `ContentLength()` logic wouldn't be affected by trailer data in `h.h` in a way that breaks framing, because `h.contentLength` is a separate integer field set during `parseHeaders` (the main header parse), not `parseTrailer`. `parseTrailer` only modifies `h.h`. `ContentLength()` reads `h.contentLength`. Thus, the claim of 'incorrect Content-Length resolution' is factually incorrect for the `ResponseHeader.ContentLength()` method which relies on `h.contentLength`.\n   - Even if `h.h` contains a stray key-value pair, this is a state inconsistency bug, not a memory safety or denial of service issue. The caller is expected to handle errors. If the caller ignores the error, they misuse the API. The library's contract is that on error, the state is undefined or should be reset. However, failing to reset on error is a code quality/robustness issue, not a security vulnerability in the context of 'memory safety' or 'crash'. \n   - The threat model rules state: 'Missing parameter validation in non-public... functions... is not a vulnerability'. While this is a public function, the impact is not security-relevant (no crash, no memory corruption, no bypass). It is 'correct behaviour' which is listed as an asset, but the standard for VALID is usually higher (crash/corruption). A logic bug that results in stale/partial data on error is typically a bug report, not a security patch item, unless it allows an attacker to inject valid-looking data that bypasses checks. Since `isBadTrailer` blocks sensitive headers, the partial data cannot be a sensitive header. \n   - Therefore, the impact is negligible from a security perspective. It is a code quality issue (inconsistent state on error) rather than a security vulnerability (crash/exploit).\n\nCRUX: The partial state corruption in h.h after a trailer parse error does not lead to memory safety issues, crashes, or security bypasses, as sensitive headers are blocked from being parsed as trailers and Content-Length is stored in a separate field.\nGREP: isBadTrailer",
    "elapsed": 107.4,
    "tokens": 33474,
    "tokens_in": 32529,
    "tokens_out": 945,
    "cost_usd": 0.01649718,
    "file": "header.go",
    "round": 2,
    "grep_used": true,
    "grep_results": "GREP `isBadTrailer`:\n```\nheader.go:511:\t\tif !isValidTrailerKey(key) || isBadTrailer(key) {\nheader.go:2729:\t\tif isBadTrailer(s.key) {\nheader.go:2741:func isBadTrailer(key []byte) bool {\n```"
  },
  {
    "finding_title": "Partial header state corruption after trailer parse error",
    "verdict": "INVALID",
    "reasoning": "The report claims that a partial parse error in `ReadTrailer` leaves corrupted state in the header object, specifically causing 'incorrect Content-Length resolution'. I need to verify if this claimed impact is real.\n\n1. **Code Flow**: `tryReadTrailer` calls `parseTrailer(b, h.h, ...)`. If `parseTrailer` succeeds in parsing some valid trailer lines but fails on a subsequent malformed line, it returns the modified `h.h` slice (with the valid lines appended) and an error. `tryReadTrailer` assigns `h.h = hh` (the modified slice) before checking the error. It then returns the error without calling `resetSkipNormalize()`. Thus, `h.h` retains the partial trailer data. This part of the report is correct: state inconsistency exists on error.\n\n2. **Impact Analysis - Content-Length**: The report claims 'incorrect Content-Length resolution'. \n   - `ResponseHeader.ContentLength()` returns `h.contentLength`.\n   - `h.contentLength` is an `int` field in the `header` struct.\n   - `parseTrailer` appends key-value pairs to the `dest` slice (`h.h`). It does NOT modify `h.contentLength`.\n   - `h.contentLength` is set during the main header parsing phase (`parseHeaders`), where `Content-Length` is treated as a special header and its value is parsed into the `contentLength` integer. It is explicitly excluded from `h.h` if processed as a special header (or blocked from being a trailer by `isBadTrailer`).\n   - Even if a `Content-Length` key somehow ended up in `h.h` (which `isBadTrailer` prevents, but let's assume for the sake of argument), `ContentLength()` does not look at `h.h`; it looks at the `contentLength` integer. \n   - Therefore, the claim that `ReadTrailer` failure leads to 'incorrect Content-Length resolution' is **factually incorrect**. The `Content-Length` value is isolated in a separate field.\n\n3. **Impact Analysis - Other Headers**: \n   - The partial data in `h.h` would be non-sensitive headers allowed as trailers (e.g., `X-Checksum`). \n   - Sensitive headers like `Authorization`, `Cookie`, `Set-Cookie`, `Host`, etc., are blocked by `isBadTrailer`. \n   - A caller who ignores the error and inspects `h.h` via `VisitAll` or `Peek` might see these non-sensitive headers. \n   - Does this lead to security harm? \n     - **Crash/DoS**: No. Go slices are safe. Iterating over extra key-value pairs does not cause a crash.\n     - **Auth Bypass**: No. Sensitive headers are filtered out.\n     - **Data Corruption**: The 'data' is just extra header entries in memory. It doesn't corrupt other data.\n     - **Misuse**: The API contract implies that on error, the state might be invalid. A robust caller should check the error and discard/reset the header object. If a caller ignores the error, they are violating the API's expected usage. This is a code quality/robustness issue (state not cleared on error) but not a security vulnerability that an attacker can exploit for meaningful harm (crash, execution, bypass). The attacker controls the input, but the 'exploit' only results in the header object having a few extra, non-sensitive, expected-looking header entries. The application would likely just ignore them or fail to find the expected specific trailer data, but not suffer security compromise.\n\n4. **Threat Model**: The threat model defines VALID as 'crash, code execution, data corruption, auth bypass'. This finding is a state inconsistency bug that does not meet the bar for security vulnerability. It is a 'correct behaviour on malformed input' issue, but the harm is negligible. It is effectively a logic bug where the object isn't reset on error, which is a common coding pattern issue but rarely a security hole unless it leads to something worse. Here, it doesn't.\n\nTherefore, the finding is INVALID.\n\nCRUX: The claimed impact of 'incorrect Content-Length resolution' is false because Content-Length is stored in a separate integer field (h.contentLength) that is not modified by parseTrailer, and the remaining partial state consists of non-sensitive headers that do not enable crash, bypass, or memory corruption.\nGREP: func (h *ResponseHeader) ContentLength()",
    "elapsed": 76.1,
    "tokens": 34483,
    "tokens_in": 33507,
    "tokens_out": 976,
    "cost_usd": 0.01700094,
    "file": "header.go",
    "round": 3,
    "grep_used": true,
    "grep_results": "GREP `func (h *ResponseHeader) ContentLength() (simplified to: ResponseHeader)`:\n```\nheader_timing_test.go:93:func BenchmarkResponseHeaderRead(b *testing.B) {\nheader_timing_test.go:95:\t\tvar h ResponseHeader\nheader_timing_test.go:138:func BenchmarkResponseHeaderWrite(b *testing.B) {\nheader_timing_test.go:140:\t\tvar h ResponseHeader\nheader_timing_test.go:185:func BenchmarkResponseHeaderPeekBytesSpecialHeader(b *testing.B) {\nheader_timing_test.go:187:\t\tvar h ResponseHeader\nheader_timing_test.go:199:func BenchmarkResponseHeaderPeekBytesNonSpecialHeader(b *testing.B) {\nheader_timing_test.go:201:\t\tvar h ResponseHeader\nstreaming_test.go:232:\tvar respH ResponseHeader\nstreaming_test.go:255:\t\t\t\tvar respH ResponseHeader\nREADME.md:389:  - [w.Header()](https://pkg.go.dev/net/http#ResponseWriter) **➜** [ctx.Response.Header](https://pkg.go.dev/github.com/valyala/fasthttp#ResponseHeader)\nREADME.md:390:  - [w.Header().Set()](https://pkg.go.dev/net/http#Header.Set) **➜** [ctx.Response.Header.Set()](https://pkg.go.dev/github.com/valyala/fasthttp#ResponseHeader.Set)\nREADME.md:392:  - [w.Header().Set(\"Set-Cookie\")](https://pkg.go.dev/net/http#Header.Set) **➜** [ctx.Response.Header.SetCookie()](https://pkg.go.dev/github.com/valyala/fasthttp#ResponseHeader.SetCookie)\nserver_test.go:729:\t\tvar respH ResponseHeader\nserver_test.go:3660:\tverifyResponseHeader(t, &resp.Header, 200, 0, string(defaultContentType), \"\")\nserver_test.go:4041:\t\tverifyResponseHeaderConnection(t, &resp.Header, \"\")\nserver_test.go:4103:\t\tverifyResponseHeaderConnection(t, &resp.Header, \"close\")\nserver_test.go:4781:\tverifyResponseHeader(t, &resp.Header, expectedStatusCode, len(resp.Body()), expectedContentType, \"\")\nheader_test.go:17:func TestResponseHeaderAddContentType(t *testing.T) {\nheader_test.go:20:\tvar h ResponseHeader\nheader_test.go:39:func TestResponseHeaderAddContentEncoding(t *testing.T) {\nheader_test.go:42:\tvar h ResponseHeader\nheader_test.go:61:func TestResponseHeaderFirstLineSettersSanitizeNewlines(t *testing.T) {\nheader_test.go:66:\t\tset           func(*ResponseHeader)\nheader_test.go:67:\t\tvalue         func(*ResponseHeader) []byte\nheader_test.go:73:\t\t\tset: func(h *ResponseHeader) {\nheader_test.go:76:\t\t\tvalue:         func(h *ResponseHeader) []byte { return h.StatusMessage() },\nheader_test.go:82:\t\t\tset: func(h *ResponseHeader) {\nheader_test.go:85:\t\t\tvalue:         func(h *ResponseHeader) []byte { return h.Protocol() },\nheader_test.go:95:\t\t\tvar h ResponseHeader\n```"
  },
  {
    "finding_title": "Partial header state corruption after trailer parse error",
    "verdict": "INVALID",
    "reasoning": "The bug pattern described is real: `tryReadTrailer` assigns `h.h = hh` before checking `errParse`, and `parseTrailer` mutates `h.h` iteratively, leaving partial state on error. However, under the provided threat model (library), a security vulnerability requires 'meaningful harm' such as crash, code execution, data corruption, or authentication bypass. The reported impact is 'corrupted header state' leading to 'unexpected headers in VisitAll' or 'incorrect Content-Length resolution'. 1) Content-Length is stored in a separate integer field `h.contentLength` which is not modified by `parseTrailer`; thus the claimed Content-Length impact is factually incorrect. 2) Sensitive headers (Authorization, Cookie, Host, etc.) are explicitly blocked by `isBadTrailer` from being parsed into trailers, preventing authentication bypass or session hijacking. 3) The remaining partial state consists of non-sensitive headers (e.g., X-Checksum) which, if retained in `h.h` after an error, do not cause memory safety issues, crashes, or logical security bypasses in the host process. A caller ignoring the error and reading these stale headers is a code quality/robustness issue (violation of expected error handling) but does not constitute a security vulnerability exploitable by an attacker to cause meaningful harm.\n\nCRUX: The partial state corruption is limited to non-sensitive trailer headers because sensitive headers are blocked by isBadTrailer, and Content-Length is unaffected, so no security-relevant impact (crash, bypass, corruption) is possible.\nGREP: isBadTrailer",
    "elapsed": 273.0,
    "tokens": 35542,
    "tokens_in": 35191,
    "tokens_out": 351,
    "cost_usd": 0.01583322,
    "file": "header.go",
    "round": 4,
    "grep_used": true,
    "grep_results": "GREP `isBadTrailer`:\n```\nheader.go:511:\t\tif !isValidTrailerKey(key) || isBadTrailer(key) {\nheader.go:2729:\t\tif isBadTrailer(s.key) {\nheader.go:2741:func isBadTrailer(key []byte) bool {\n```"
  },
  {
    "finding_title": "Partial header state corruption after trailer parse error",
    "verdict": "INVALID",
    "reasoning": "The reported bug is a state consistency issue: if `ReadTrailer` encounters a parse error after processing some valid trailer lines, `h.h` retains those partially parsed headers because `resetSkipNormalize` is not called on error (unlike `Read`). However, under the provided threat model, this is not a security vulnerability. 1) There is no memory safety impact (crash, OOB access, corruption) as Go's runtime manages the slices and the data is within bounds. 2) There is no authentication bypass or privilege escalation because `isBadTrailer` explicitly blocks sensitive headers (Authorization, Cookie, Host, etc.) from being parsed as trailers. The attacker can only inject non-sensitive metadata (e.g., X-Checksum). 3) The claim of 'incorrect Content-Length resolution' is factually incorrect; `ContentLength()` reads from the integer field `h.contentLength`, which is set during the main header parsing phase and is not modified by `parseTrailer`. 4) The result is merely stale, non-sensitive data in the header struct if a caller ignores the returned error. This is a code quality/robustness defect (failure to reset state on error) but does not lead to the 'meaningful harm' (crash, code execution, data corruption, auth bypass) required for a VALID verdict in this threat model.\n\nCRUX: The partial state corruption only affects non-sensitive headers (blocked sensitive ones by isBadTrailer) and does not impact Content-Length (separate int field), resulting in no crash, memory corruption, or auth bypass.\nGREP: isBadTrailer",
    "elapsed": 52.5,
    "tokens": 35304,
    "tokens_in": 34955,
    "tokens_out": 349,
    "cost_usd": 0.015728100000000002,
    "file": "header.go",
    "round": 5,
    "grep_used": true,
    "grep_results": "GREP `isBadTrailer`:\n```\nheader.go:511:\t\tif !isValidTrailerKey(key) || isBadTrailer(key) {\nheader.go:2729:\t\tif isBadTrailer(s.key) {\nheader.go:2741:func isBadTrailer(key []byte) bool {\n```"
  },
  {
    "verdict": "INVALID",
    "reasoning": "[ARBITER] The report describes a state inconsistency bug where `h.h` retains partial data if `ReadTrailer` fails after parsing some valid trailer lines. However, under the provided threat model, this does not constitute a security vulnerability. 1) There is no memory safety issue (crash, OOB, etc.) as Go manages memory safely. 2) The partial data consists of non-sensitive headers; sensitive headers (like Content-Length, Host, Cookie, etc.) are explicitly blocked from being parsed as trailers by the `isBadTrailer` function and `parseTrailer` logic, or are stored in separate fields not affected by `h.h` mutation during trailer parsing (e.g., `h.contentLength`). 3) Even if a caller ignores the error, the resulting state only contains valid, non-sensitive header key-value pairs that do not enable authentication bypass, injection, or data corruption of critical state.",
    "round": 6,
    "file": "header.go",
    "finding_title": "Partial header state corruption after trailer parse error",
    "elapsed": 48.8,
    "tokens_in": 32393,
    "tokens_out": 203,
    "cost_usd": 0.014214059999999999
  }
]