From f61be03ff0755771633878bd13256cd7b6d94f49 Mon Sep 17 00:00:00 2001 From: Andy Postnikov Date: Fri, 8 May 2026 12:43:09 +0200 Subject: [PATCH] fix(http): bounds-check proxy Content-Length and URI/string helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGES | 6 +++++ src/nxt_h1proto.c | 53 ++++++++++++++++++++++++++++++++++++++++++-- src/nxt_http_proxy.c | 14 ++++++++++++ src/nxt_string.c | 11 ++++++++- 4 files changed, 81 insertions(+), 3 deletions(-) diff --git a/CHANGES b/CHANGES index 8ae835854..e662c60de 100644 --- a/CHANGES +++ b/CHANGES @@ -14,6 +14,12 @@ Changes with FreeUnit 1.35.4 xx xxx 2026 *) Bugfix: replace removed cgi module with email.parser in Python upload test fixture; fixes test suite compatibility with Python 3.13. + *) Bugfix: harden HTTP/URI parser and string helper bounds — proxy + response Content-Length underflow guarded in nxt_h1proto.c, oversized + upstream Content-Length values are now logged and flagged inconsistent + in nxt_http_proxy.c, nxt_is_complex_uri_encoded() off-by-one fixed for + trailing "%" sequences, and nxt_rmemstrn() rejects length > buffer. + Changes with FreeUnit 1.35.3 07 Apr 2026 *) Feature: migrate contrib package mirror from packages.nginx.org to diff --git a/src/nxt_h1proto.c b/src/nxt_h1proto.c index dda3900e3..331368faf 100644 --- a/src/nxt_h1proto.c +++ b/src/nxt_h1proto.c @@ -2880,11 +2880,60 @@ nxt_h1p_peer_body_process(nxt_task_t *task, nxt_http_peer_t *peer, } else if (h1p->remainder > 0) { length = nxt_buf_chain_length(out); - h1p->remainder -= length; - if (h1p->remainder == 0) { + /* + * Cast to nxt_off_t can wrap to negative on 64-bit if length + * exceeds NXT_OFF_T_MAX; check that explicitly as well as the + * overrun condition. + */ + if (nxt_slow_path((nxt_off_t) length < 0 + || (nxt_off_t) length > h1p->remainder)) + { + nxt_buf_t *b; + size_t trimmed; + + /* + * Upstream sent more body bytes than its Content-Length + * declared. Truncate the buf chain to remainder bytes so + * we never forward the excess past the Content-Length we + * already advertised downstream, then flag inconsistent + * and close. + */ + nxt_log(task, NXT_LOG_WARN, + "upstream sent %uz body bytes past Content-Length " + "(remainder %O)", length, h1p->remainder); + + trimmed = 0; + for (b = out; b != NULL; b = b->next) { + size_t bsz; + + if (nxt_buf_is_sync(b)) { + continue; + } + bsz = b->mem.free - b->mem.pos; + if (trimmed + bsz <= (size_t) h1p->remainder) { + trimmed += bsz; + continue; + } + /* Trim this buf to fit, drop everything after it. */ + b->mem.free = b->mem.pos + + (size_t) h1p->remainder - trimmed; + b->next = NULL; + break; + } + + peer->request->inconsistent = 1; + h1p->remainder = 0; nxt_buf_chain_add(&out, nxt_http_buf_last(peer->request)); peer->closed = 1; + + } else { + h1p->remainder -= length; + + if (h1p->remainder == 0) { + nxt_buf_chain_add(&out, nxt_http_buf_last(peer->request)); + peer->closed = 1; + } } } diff --git a/src/nxt_http_proxy.c b/src/nxt_http_proxy.c index 7f6ad6866..6ea87dcc1 100644 --- a/src/nxt_http_proxy.c +++ b/src/nxt_http_proxy.c @@ -418,6 +418,20 @@ nxt_http_proxy_content_length(void *ctx, nxt_http_field_t *field, if (nxt_fast_path(n >= 0)) { r->resp.content_length_n = n; + + } else { + /* + * n == -2 means the upstream Content-Length value overflows + * nxt_off_t; n == -1 is a generic parse error. Both are + * inconsistent with a usable response body length, so log and + * mark the response inconsistent rather than silently leaving + * content_length_n at -1. + */ + nxt_log(&r->task, NXT_LOG_WARN, + "upstream Content-Length \"%*s\" is invalid", + (size_t) field->value_length, field->value); + + r->inconsistent = 1; } return NXT_OK; diff --git a/src/nxt_string.c b/src/nxt_string.c index a23ee058f..551ba72b9 100644 --- a/src/nxt_string.c +++ b/src/nxt_string.c @@ -316,6 +316,10 @@ nxt_rmemstrn(const u_char *s, const u_char *end, const char *ss, size_t length) u_char c1, c2; const u_char *s1, *s2; + if (nxt_slow_path(length == 0 || length > (size_t) (end - s))) { + return NULL; + } + s1 = end - length; s2 = (u_char *) ss; c2 = *s2++; @@ -715,7 +719,12 @@ nxt_is_complex_uri_encoded(u_char *src, size_t length) if (nxt_uri_escape[ch >> 5] & (1U << (ch & 0x1f))) { if (ch == '%') { - if (end - src < 2) { + /* + * The two pre-increments below read src+1 and src+2, + * so at least three bytes ('%' plus two hex digits) + * must remain in the buffer. + */ + if (end - src < 3) { return 0; }