fix(http): bounds-check proxy Content-Length and URI/string helpers#17
fix(http): bounds-check proxy Content-Length and URI/string helpers#17andypost wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request hardens the HTTP/URI parser and string helper bounds by adding checks for Content-Length underflow, handling invalid upstream Content-Length values, and fixing an off-by-one error in URI encoding detection. Feedback suggests that extra body bytes should be truncated before being passed downstream to avoid protocol violations. Additionally, reviewers recommended handling zero-length inputs in nxt_rmemstrn to prevent underflow and improving the safety of size comparisons to avoid signed integer overflow on 64-bit systems.
| if (nxt_slow_path((nxt_off_t) length > h1p->remainder)) { | ||
| /* | ||
| * Upstream sent more body bytes than its Content-Length | ||
| * declared. Avoid signed underflow of h1p->remainder and | ||
| * surface the inconsistency immediately instead of waiting | ||
| * for connection close. | ||
| */ | ||
| nxt_log(task, NXT_LOG_WARN, | ||
| "upstream sent %uz body bytes past Content-Length " | ||
| "(remainder %O)", length, h1p->remainder); | ||
|
|
||
| peer->request->inconsistent = 1; | ||
| h1p->remainder = 0; | ||
| nxt_buf_chain_add(&out, nxt_http_buf_last(peer->request)); | ||
| peer->closed = 1; |
There was a problem hiding this comment.
When the upstream sends more data than specified in the Content-Length header (length > h1p->remainder), the code correctly identifies the inconsistency and flags the request. However, the buffer chain out is passed to the next stage without being truncated to the expected remainder. This results in the extra bytes being sent to the downstream client, which violates the Content-Length previously sent in the response headers. This can lead to protocol errors or client-side confusion, even if the connection is closed immediately after.
| if (nxt_slow_path(length > (size_t) (end - s))) { | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
The nxt_rmemstrn function does not handle the case where length is 0. If length is 0, the subsequent length-- at line 326 (in the full file) will cause an underflow to SIZE_MAX. This leads to an out-of-bounds memory comparison in memcmp at line 332 and an out-of-bounds read at line 329 (c1 = *s1 where s1 is end). The bounds check should be updated to reject length == 0 or handle it appropriately.
if (nxt_slow_path(length == 0 || length > (size_t) (end - s))) {| h1p->remainder -= length; | ||
|
|
||
| if (h1p->remainder == 0) { | ||
| if (nxt_slow_path((nxt_off_t) length > h1p->remainder)) { |
There was a problem hiding this comment.
The comparison (nxt_off_t) length > h1p->remainder involves casting an unsigned size_t to a signed nxt_off_t. On 64-bit systems where both types are 64-bit, if length is extremely large (specifically > 2^63 - 1), the cast will result in a negative value. Since h1p->remainder is positive, the comparison negative > positive will be false, and the overflow check will be bypassed. A safer approach is to check for (nxt_off_t) length < 0 as well to handle potential overflow.
if (nxt_slow_path((nxt_off_t) length < 0
|| (nxt_off_t) length > h1p->remainder))
Audit-driven HTTP/URI parser and string-helper bounds pass (PR-F from security-audit.md / PR #10). Four findings, no protocol or config-surface changes. V1 [High] Proxy response Content-Length underflow (src/nxt_h1proto.c:2881). When an upstream sent more body bytes than its Content-Length declared, h1p->remainder (signed nxt_off_t) silently underflowed and subsequent `remainder > 0` checks dropped excess data, masking the inconsistency. Guard the subtraction on the slow path: log the violation, set request->inconsistent immediately, clamp remainder to zero, and close the peer cleanly. Happy path stays branch-predicted. V1 [Medium] Proxy Content-Length > NXT_OFF_T_MAX (src/nxt_http_proxy.c:417). nxt_off_t_parse() returns -2 on overflow and -1 on parse error; the previous `n >= 0` guard collapsed both into "leave content_length_n at -1", indistinguishable from "header absent". Now log at NXT_LOG_WARN and mark r->inconsistent — symmetric with the underflow path above and matches the upstream-side handling in nxt_http_request.c. V13 [Medium] nxt_is_complex_uri_encoded() off-by-one (src/nxt_string.c:718). The bounds check `end - src < 2` is wrong: the two pre-increments below it read src+1 and src+2, so three bytes must remain. A URI ending in "%X" (one hex digit) read one byte past end. Bumped to `< 3` with a short comment explaining the pre-increment shape. V13 [Low] nxt_rmemstrn() length underflow (src/nxt_string.c:319). `s1 = end - length` had no defensive check; all current callers validate upstream but a future caller could trigger pointer underflow. Added the audit's recommended guard at function entry. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
andypost
force-pushed
the
claude/audit-pr-f-http-uri-bounds
branch
from
1851081 to
f61be03
Compare
|
All three Gemini findings addressed in
Force-pushed; build clean. |
1 participant
Summary
Audit-driven HTTP/URI parser & string-helper bounds pass (PR-F from
security-audit.md / PR #10). Four findings; small, no protocol
or config-surface changes.
Findings addressed
Test plan
./configure --openssl && make -j unitdcleanpytest-3 --collect-only test/test_proxy.py test/test_proxy_chunked.py test/test_http_header.pycollects 54 testspytest-3 test/test_proxy.pyrequires root; not run in worktreeContent-Length: 99999999999999999999is logged and flags request inconsistent%X(one hex digit) is rejected without OOB readUpstream
Same fixes apply to `freeunitorg/freeunit`; will forward after merge.
Generated by Claude Code