Backport guard-clause fixes from freenginx - #1696
Open
dekobon wants to merge 27 commits into
Open
Conversation
|
🎉 Thank you for your contribution! It appears you have not yet signed the F5 Contributor License Agreement (CLA), which is required for your changes to be incorporated into an F5 Open Source Software (OSS) project. Please kindly read the F5 CLA and reply on a new comment with the following text to agree: I have hereby read the F5 CLA and agree to its terms 2 out of 4 committers have signed the CLA. |
|
✅ The F5 CLA is not required for this PR. |
dekobon
force-pushed
the
cherry/freenginx-guard-clauses
branch
from
August 27, 2026 00:21
5c1d58e to
6b66b39
Compare
Request body reading indirectly uses the "do { c->recv() } while
(c->read->ready)" form, which is not really correct, as for example with
SSL c->read->ready may be still set when c->recv() returns NGX_AGAIN due
to SSL_ERROR_WANT_WRITE (see 7351:2b5528023f6b), and therefore this form
might be an infinite loop.
Added explicit NGX_AGAIN handling for the sake of correctness.
Signed-off-by: Elijah Zupancic <e.zupancic@f5.com>
Origin: <https://freenginx.org/hg/nginx/rev/392e8e2fd22a8461e75a67d5d33d85c07974e1e3>
dekobon
force-pushed
the
cherry/freenginx-guard-clauses
branch
from
August 27, 2026 02:29
6b66b39 to
70be726
Compare
Member
|
Minor nits:
|
dekobon
force-pushed
the
cherry/freenginx-guard-clauses
branch
from
August 27, 2026 17:51
70be726 to
b370679
Compare
Previously, arbitrary amounts of chunk extensions and trailer headers were accepted and skipped. Despite being under limit_conn / limit_req limits (if configured), this can be a DoS vector, so it is now limited by the client_max_body_size limit. Reported by Bartek Nowotarski. Adapted for nginx: the chunked parser state machine in nginx differs (bare LF is rejected and CR handling uses separate *_almost_done states), so the ctx->skipped increments were placed in the corresponding sw_chunk_extension, sw_last_chunk_extension, and sw_trailer_header states instead of applying verbatim. Additionally, ctx->skipped is now also incremented in the sw_trailer state when a trailer header name starts: without this, minimal trailer lines (such as "a" CRLF) were parsed through the sw_trailer, sw_trailer_header, and sw_trailer_header_almost_done states without ever incrementing ctx->skipped, bypassing the limit entirely. The original patch does not charge such lines against the limit. Co-authored-by: Elijah Zupancic <e.zupancic@f5.com> Signed-off-by: Elijah Zupancic <e.zupancic@f5.com> Origin: <https://freenginx.org/hg/nginx/rev/f3df785649aebf967a428b4d736b1317a94f815b>
Previously, r->headers_in.content_length_n was used, which is not 0 till the request body is fully discarded, and the request might hang. Signed-off-by: Elijah Zupancic <e.zupancic@f5.com> Origin: <https://freenginx.org/hg/nginx/rev/51e0dc7137843feda75b497773f598d3836c60a1>
If reading is not blocked, additional client activity can result in undefined behaviour, including segfaults, as seen with proxying with proxy_ignore_client_abort before 4072:cf334deeea66. While unlikely with low-level errors being returned when reading the request body, it might be the case, for example, when chunked transfer encoding is used, and error_page 400 (or 413) is proxied with proxy_ignore_client_abort. Signed-off-by: Elijah Zupancic <e.zupancic@f5.com> Origin: <https://freenginx.org/hg/nginx/rev/43fe0edddba1cb34eb4ca750d254ed6ec4d8e633>
We need to reset the imap tag to empty after an authentication attempt completes, otherwise if the next line parsed is incomplete with no tag (e.g. empty line) then we use the "tag" from the previous buffer which is now definitely wrong and has been partially overwritten with the most recently read data (e.g. CRLF). An example before this patch: S: * OK IMAP4 ready C: foobar login a b S: foobar NO Incorrect username or password. C: S: S: obar BAD invalid command Then with this patch: S: * OK IMAP4 ready C: foobar login a b S: foobar NO Incorrect username or password. C: S: * BAD invalid command Signed-off-by: Elijah Zupancic <e.zupancic@f5.com> Origin: <https://freenginx.org/hg/nginx/rev/46ecad404a296042c0088e699f275a92758e5ab9>
If an HTTP/2 connection opened before a graceful shutdown, but ngx_http_v2_init() is called after idle connections were closed, such a connection ended up being open till closed by the client (or up to keepalive_time), delaying shutdown. With this change, such connections are allowed to serve just one request, much like it happens in HTTP/1.x, and closed afterwards. Reported by Kasei Wang, https://freenginx.org/pipermail/nginx-devel/2024-May/000277.html Signed-off-by: Elijah Zupancic <e.zupancic@f5.com> Origin: <https://freenginx.org/hg/nginx/rev/4a0cd107c0f1d56cd455df39c37eb0fffc085760>
In ngx_http_source_charset(), name->data was left uninitialized, and only name->len was set. Since it is used in debug logging, this resulted in the following complaints from Valgrind on systems with musl libc: ==42== Conditional jump or move depends on uninitialised value(s) ==42== at 0x12BC66: memcpy (string.h:51) ==42== by 0x12BC66: ngx_sprintf_str (ngx_string.c:586) ==42== by 0x12C03C: ngx_vslprintf (ngx_string.c:255) ==42== by 0x127694: ngx_log_error_core (ngx_log.c:135) ==42== by 0x1B8795: ngx_http_charset_header_filter (ngx_http_charset_filter_module.c:252) Similarly, ngx_http_split_args() returned uninitialized arg->data, which was then copied to r->args, and also used in debug logging: ==42== Conditional jump or move depends on uninitialised value(s) ==42== at 0x12BC10: memcpy (string.h:50) ==42== by 0x12BC10: ngx_sprintf_str (ngx_string.c:586) ==42== by 0x12C03C: ngx_vslprintf (ngx_string.c:255) ==42== by 0x127694: ngx_log_error_core (ngx_log.c:135) ==42== by 0x184EFB: ngx_http_internal_redirect (ngx_http_core_module.c:2526) ==42== by 0x1D8CCC: ngx_http_try_files_handler (ngx_http_try_files_module.c:209) Fix is to initialize data to NULL. Note that, while memcpy(p, NULL, 0) is also formally undefined now, it is used in multiple places in the code, and expected to be allowed in C2y (see WG14 proposals N3177, N3261, "Allow zero length operations on null pointers"). Prodded by Valgrind. Signed-off-by: Elijah Zupancic <e.zupancic@f5.com> Origin: <https://freenginx.org/hg/nginx/rev/f53146df9a47c04ba3e0fce286acff1cebda33cf>
When draining a connection associated with an HTTP/3 stream, calling ngx_http_v3_send_cancel_stream() might result in an attempt to obtain a connection for the decoder stream. This in turn will trigger draining of the very same connection. Depending on the client settings, this might either lead to stack overflow or will end up in decoder stream creation error and destroying the connection at some point, potentially resulting in use-after-free on stack. Fix is to make sure that connection reuse is disabled in ngx_http_v3_reset_stream(), so the recursion in question won't happen regardless of what called functions do. Adapted for nginx: applied as a single-line addition at the start of ngx_http_v3_reset_stream(); the surrounding context differs (nginx does not have the freenginx max_table_capacity changes). Co-authored-by: Elijah Zupancic <e.zupancic@f5.com> Signed-off-by: Elijah Zupancic <e.zupancic@f5.com> Origin: <https://freenginx.org/hg/nginx/rev/d9fe808c1841b4d9b2c5884cb561a14ad2b504ee>
The s->passwd field might be set after previous (failed) authentication in the same session, and since EXTERNAL authentication did not touch it, it was sent to the auth server. Signed-off-by: Elijah Zupancic <e.zupancic@f5.com> Origin: <https://freenginx.org/hg/nginx/rev/f83cb031a4a4b43869f7a80c4265f580af3cf0d4>
Signed-off-by: Elijah Zupancic <e.zupancic@f5.com> Origin: <https://freenginx.org/hg/nginx/rev/ea0eef2dd12c2d41349d63c532e942cf95fc4d7b>
Following 3518:eb3aaf8bd2a9 (0.8.37), r->request_output is only set if there are data in the first buffer sent in the subrequest. As a result, following the change mentioned this flag cannot be used to prevent duplicate ngx_http_ssi_stub_output() calls, since it is not set if there was already some output, but the first buffer was empty. Still, when there are multiple subrequests, even an empty subrequest response might be delayed by the postpone filter, leading to a second call of ngx_http_ssi_stub_output() during finalization from ngx_http_writer() the subreqest buffers are released by the postpone filter. Since r->request_output is not set after the first call, this resulted in duplicate stub output. Additionally, checking only the first buffer might be wrong in some unusual cases. For example, the first buffer might be empty if $r->flush() is called before printing any data in the embedded Perl module. Depending on the postpone_output value and corresponding sizes, this issue can result in either duplicate or unexpected stub output, or "zero size buf in writer" alerts. Following 8124:f5515e727656 (1.23.4), it became slightly easier to reproduce the issue, as empty static files and empty cache items now result in a response with an empty buffer. Before the change, an empty proxied response can be used to reproduce the issue. Fix is check all buffers and set r->request_output if any non-empty buffers are sent. This ensures that all unusual cases of non-empty responses are covered, and also that r->request_output will be set after the first stub output, preventing duplicate output. Reported by Jan Gassen. Signed-off-by: Elijah Zupancic <e.zupancic@f5.com> Origin: <https://freenginx.org/hg/nginx/rev/5be23505292b718581a706be312a604237f91f8e>
When handling incorrect data in ngx_http_mp4_crop_stsc_data(), trak->end_chunk_samples might end up being arbitrary large, leading to reading before the buffer in ngx_http_mp4_update_stsz_atom(). Fix is to check that trak->end_chunk_samples corresponds to a memory within the stsz atom data. For consistency, trak->start_chunk_samples is checked similarly. Similarly, trak->end_chunk might end up being smaller than trak->start_chunk, leading to reading memory after the buffer in ngx_http_mp4_update_stco_atom() and ngx_http_mp4_update_co64_atom(). Corresponding checks are updated to explicitly test (trak->end_chunk - trak->start_chunk) instead of just checking trak->end_chunk and assuming it is larger than trak->start_chunk. This is generally in line with existing checks of (trak->end_sample - trak->start_sample) in ngx_http_mp4_update_stsz_atom(), where trak->end_sample might also become smaller than trak->start_sample when handling incorrect data in ngx_http_mp4_crop_stts_data(). Signed-off-by: Elijah Zupancic <e.zupancic@f5.com> Origin: <https://freenginx.org/hg/nginx/rev/d6f75dd66761c10d4bfb257ae70a212411b6a69b>
Current open file cache code cannot properly work on platforms without pread(), since file->sys_offset is not shared across files. Further, it is not set on file initialization after ngx_open_cached_file(), leading to incorrect value 0 instead of non-zero current offset for cached file descriptors. Since platforms without pread() are rather exotic nowadays, fix is to disable open_file_cache for them. Signed-off-by: Elijah Zupancic <e.zupancic@f5.com> Origin: <https://freenginx.org/hg/nginx/rev/5249bce09d1f1d62eaa6f6683da3d986c1cdecaf>
Previously, different disable_symlinks settings were respected when retrieving a file from open file cache when using periodic retest, but were ignored with using open_file_cache_events. Fix is to test disable_symlinks settings in all cases, and retest the file if requested settings are different from what we already have in the cache. Signed-off-by: Elijah Zupancic <e.zupancic@f5.com> Origin: <https://freenginx.org/hg/nginx/rev/116453ebb9003b5a8fe566864920fe51732888ee>
The code in ngx_http_postpone_filter_in_memory() used to assign r->headers_out.content_length_n to a size_t variable before comparison, which can lead to incorrect results on 32-bit platforms. Fix is to compare r->headers_out.content_length_n before conversion to size_t. Found with MSVC with C4244 warnings (conversion from 'type1' to 'type2', possible loss of data) enabled. Signed-off-by: Elijah Zupancic <e.zupancic@f5.com> Origin: <https://freenginx.org/hg/nginx/rev/f5928c2e47c5ba25b43a108906d44304d9b71290>
Previously, if ngx_pool_cleanup_add() failed, already incremented connection counter was left as is, eventually resulting in unexpected connection limiting or shared memory exhaustion. Fix is to decrement the counter by calling the cleanup handler with data on stack. Signed-off-by: Elijah Zupancic <e.zupancic@f5.com> Origin: <https://freenginx.org/hg/nginx/rev/722e252fcabe6413eb63555890273d7b0e7fc132>
Allocations for PING and SETTINGS frames were limited in 7379:57463f4e2fcd to prevent potential excessive memory usage due to misbehaving upstream servers. The limit as implemented applies to all interactions with all upstream servers within an upstream, that is, if the limit is reached, switching to the next upstream server is likely to hit the limit again on the next PING (or SETTINGS) frame. This is believed to be incorrect: other upstream servers shouldn't be responsible for misbehaviour of the previous one, and only allocations within a particular connection should be limited. Fix is to reset limits on request reinitialization. Adapted for nginx: context differs since nginx also resets ctx->in, ctx->busy, and ctx->out in ngx_http_grpc_reinit_request(); the ctx->pings and ctx->settings resets were appended to that block. Co-authored-by: Elijah Zupancic <e.zupancic@f5.com> Signed-off-by: Elijah Zupancic <e.zupancic@f5.com> Origin: <https://freenginx.org/hg/nginx/rev/47ecba793b8dab07747c5e45fcbdd523121fb193>
Previously, clcf->auto_redirect was only set (for locations ending with "/") when uwsgi_pass and scgi_pass were used without variables, but not with variables. For proxy_pass and fastcgi_pass this was fixed in 2989:dff9764eaca2 (0.8.7). It was not, however, fixed in ngx_http_uwsgi_module (as imported later, in 3541:21452748d165, 0.8.40) and ngx_http_scgi_module (introduced shortly after, in 3637:d656caa72ec9, 0.8.42). The fix is mostly identical to the one in 2989:dff9764eaca2 (0.8.7) and ensures that clcf->auto_redirect is set both with and without variables. Signed-off-by: Elijah Zupancic <e.zupancic@f5.com> Origin: <https://freenginx.org/hg/nginx/rev/6ea8697c07408cadbe10527aa00742ecb47e002a>
As of OpenSSL 4.0 alpha 1, errors during reading are remembered in the SSL connection structure, and further attempts to write to the connections are rejected with SSL_ERROR_SSL error and no additional details. While rejecting such attempts is probably correct, lack of the additional error details makes it hard to figure out what actually happened, and to do appropriate logging. In particular, "[crit] ... SSL_write() failed" errors were observed in the ssl_stapling.t test, where the socket is closed right after sending the request, leading to RST sent with TLSv1.3 in response to the tickets sent after the handshake, and often observed by the server while reading the request (but not yet processed). To make sure such errors are not reported as "[crit] ... SSL_write() failed", we now don't try to call SSL_write() after an error was detected by ngx_ssl_recv(). Signed-off-by: Elijah Zupancic <e.zupancic@f5.com> Origin: <https://freenginx.org/hg/nginx/rev/ff5becaf54440b628ea76af6ed27d7d5a85f303b>
If we need to be notified about further events, ngx_handle_write_event() needs to be called after a write event is processed. Without this, an event can be removed from the kernel and won't be reported again, notably when using oneshot event methods, such as eventport on Solaris. Signed-off-by: Elijah Zupancic <e.zupancic@f5.com> Origin: <https://freenginx.org/hg/nginx/rev/15ab3e70ab16fbaa6bdfb65a3803184d14657edc>
On Windows, in some cases a read event is reported by select() before a write event signals that a connection to a backend server is established. This results in c->write->ready not being set when the read handler is called, and subsequent c->send() returns NGX_AGAIN, as ngx_wsasend() does not do anything unless c->write->ready is set. And the connection establishment code assumes that short initial commands can be sent without blocking, further resulting in an internal error being reported to the client. It is not clear why this happens, though it was observed in practice when running tests on Windows 10 (22H2). The fix is to block read handlers from doing anything unless c->write->ready is set, much like we do when sending the PROXY protocol header. Signed-off-by: Elijah Zupancic <e.zupancic@f5.com> Origin: <https://freenginx.org/hg/nginx/rev/04172e7f25c62ee20690f369801a1a846ee04ad2>
If COPY or MOVE methods are enabled, the DAV module assumes that it is
configured for the whole server, and does not try validate that the
destination URI is within the particular location. This, however, does
not work well if it is in fact configured in a non-root location,
and this location uses the "alias" directive, such as in the following
configuration:
location /prefix/ {
dav_methods COPY;
alias /foo/;
}
In particular, if the destination URI is shorter than the aliased
location prefix, calling ngx_http_map_uri_to_path() with such
destination URI used to result in segmentation faults (CVE-2026-27654).
And if the destination URI is longer than the aliased location prefix,
but does not match it, calling ngx_http_map_uri_to_path() resulted in
unexpected path values, including ones above the "alias" directory
specified.
The fix is to check destination URI prefix if "alias" is used, similarly
to what we do with "try_files". Additionally, the
ngx_http_map_uri_to_path() function was updated with additional sanity
checking to prevent similar issues.
See also:
nginx@9739e75
Adapted for nginx: nginx already carried its own length-only
"Destination" check (duri.len < clcf->alias, from commit 9739e75);
it is replaced here by the stronger location prefix comparison. The
ngx_http_map_uri_to_path() sanity check applied as is.
Co-authored-by: Elijah Zupancic <e.zupancic@f5.com>
Signed-off-by: Elijah Zupancic <e.zupancic@f5.com>
Origin: <https://freenginx.org/hg/nginx/rev/aae28b083279ab1b73d08ba48a2496f0deb31cca>
The Destination URI check introduced for COPY and MOVE with "alias" compares the first clcf->alias bytes of r->uri, but r->uri can be shorter than the location prefix if it was changed by a rewrite. Make sure to reject such requests before calling ngx_filename_cmp(), much like the corresponding check in ngx_http_map_uri_to_path(). Signed-off-by: Elijah Zupancic <e.zupancic@f5.com> Adaptation-Commit-For: <https://freenginx.org/hg/nginx/rev/aae28b083279ab1b73d08ba48a2496f0deb31cca>
Previously, auth_request_set did not use the NGX_HTTP_VAR_WEAK flag,
and using auth_request_set with a prefix variable anywhere in the
configuration resulted in an empty value of the variable in other
contexts. For example, in the following configuration the $http_foo
variable was always empty in requests to "/", even if the "Foo" header
was present in requests:
location / {
return 200 $http_foo;
}
location /protected/ {
auth_request /auth;
auth_request_set $http_foo "set";
...
}
The fix is to use the NGX_HTTP_VAR_WEAK flag, much like the "set"
directive does.
Signed-off-by: Elijah Zupancic <e.zupancic@f5.com>
Origin: <https://freenginx.org/hg/nginx/rev/bb638269f1e3e0c7e1e311a3213f32fee932754d>
Previously, named captures unconditionally changed v->get_handler to
ngx_http_variable_not_found(), so merely defining a regular expression
with a named captures was enough to hide an existing variable. For
example, in the following configuration the $foo variable was always
empty in requests to "/":
map $uri $foo {
default "a value from map";
}
location / {
return 200 "value: $foo";
}
location ~ /re/(?<foo>.*) {
return 200 "value: $foo";
}
Similarly, for variables within an existing prefix, such as for
$http_foo, using a regular expression with a named captures was enough
to hide the original prefix variable. For example, in the following
configuration the $http_foo variable was always empty in requests to
"/", even if the "Foo" header was present in requests:
location / {
return 200 "value: $http_foo";
}
location ~ /re/(?<http_foo>.*) {
return 200 "value: $http_foo";
}
The fix is to avoid changing v->get_handler if it's already present, and
to use the NGX_HTTP_VAR_WEAK flag to avoid redefining get handler for
prefix variables, similarly to what the "set" directive does.
Signed-off-by: Elijah Zupancic <e.zupancic@f5.com>
Origin: <https://freenginx.org/hg/nginx/rev/623210753d47864b44f23ddfd250a83546c12b28>
Changeable variables can be updated by the "set" directive, and
to support this ngx_http_add_variable() does not generate an error
when a changeable variable is added again.
This, however, makes it possible to redefine variables to completely
different use, such as from a builtin variable to a map{}. And this
in turn might cause unexpected issues, such as when a variable with
set_handler is redefined to a map, which changes v->get_handler and
v->data, but not v->set_handler. Most notably, this caused segmentation
faults when trying to define a map to the $limit_rate variable before
7504:c19ca381b2e6 (https://trac.nginx.org/nginx/ticket/1238).
With this change, redefinition of variables with v->set_handler is not
allowed unless the NGX_HTTP_VAR_WEAK flag is also provided, which is
currently used by "set" and other similar directives.
Note that this change changes the meaning of the NGX_HTTP_VAR_WEAK flag.
Previously, it was used solely to preserve get handler from prefix
variables. Now the meaning of the flag becomes somewhat broader, and
now it means that we are defining a fallback variable to be used in
"set" or similar directives, and the caller is not going to redefine the
variable if it's already present.
Similar changes were made in the stream module.
Signed-off-by: Elijah Zupancic <e.zupancic@f5.com>
Origin: <https://freenginx.org/hg/nginx/rev/5930e96ebd5a3d93bd719210e837f7281fc4f6d9>
Previously, the gzip filter called ngx_pfree() on the ctx->preallocated pointer in ngx_http_gzip_filter_deflate_end() when compression is finished, but did not clear it. As a result, ngx_pfree() might be called again in the ngx_http_gzip_body_filter() error handling code path if an error happened either in ngx_http_gzip_filter_deflate_end() during allocation of a chain link, or anywhere in the next body filters when sending the last part of the response. Potentially, this might cause issues if the same address is used by a large pool allocation in the next body filters (unlikely in practice though, especially given that standard body filters running after the gzip filter don't do any large pool allocations). The fix is to clear ctx->preallocated after it is freed in ngx_http_gzip_filter_deflate_end(). Reported by Evan Hellman, freenginx/nginx#25 Signed-off-by: Elijah Zupancic <e.zupancic@f5.com> Origin: <https://freenginx.org/hg/nginx/rev/2dd3d84cfa0fd2d122c1b87c46b9b5f65a204aae>
dekobon
force-pushed
the
cherry/freenginx-guard-clauses
branch
from
August 27, 2026 18:32
b370679 to
36b71d0
Compare
dekobon
marked this pull request as ready for review
August 28, 2026 15:33
There was a problem hiding this comment.
Pull request overview
This PR backports a set of defensive guard-clause and state-hardening fixes from freenginx into nginx across HTTP, stream, mail, resolver, SSL/OCSP, and several HTTP modules. The changes primarily add bounds/consistency checks and reset logic to prevent concrete failure modes (crashes, hangs, leaks, memory-safety issues, and security bypasses) while preserving existing behavior when inputs/config are valid.
Changes:
- Add duplicate-variable rejection for variables with set-handlers, and mark certain internally-created variables as WEAK to avoid clobbering existing handlers (HTTP + stream).
- Harden request/body processing and parsing paths (chunked extensions/trailers accounting; read-event handling after errors; safer length/logging and request_output detection).
- Add targeted guards across subsystems/modules (limit_conn cleanup on allocation failure, DAV alias Destination constraints, OCSP partial-write event handling, OpenSSL post-error write guard, mp4 bounds checks, etc.).
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/stream/ngx_stream_variables.c | Rejects duplicate variables that already have set-handlers; uses WEAK variables for regex captures and preserves existing get-handlers. |
| src/stream/ngx_stream_limit_conn_module.c | Ensures shared-memory limit_conn counters are cleaned up if pool cleanup allocation fails. |
| src/mail/ngx_mail_proxy_module.c | Avoids mail proxy read-path progress when upstream write side isn’t ready (prevents stalled/busy loops). |
| src/mail/ngx_mail_handler.c | Clears password when using EXTERNAL auth to avoid stale credential state. |
| src/mail/ngx_mail_auth_http_module.c | Resets IMAP tag length on auth sleep re-entry to avoid stale tag reuse. |
| src/http/v3/ngx_http_v3_request.c | Disables connection reusability during stream reset to avoid unsafe reuse paths. |
| src/http/v2/ngx_http_v2.c | Forces keepalive termination behavior during graceful exit to avoid lingering HTTP/2 connections. |
| src/http/ngx_http.h | Adds skipped accounting field to chunked parsing context for extension/trailer size limiting. |
| src/http/ngx_http_variables.c | Mirrors stream duplicate-variable/set-handler guard; uses WEAK variables for regex captures and preserves existing get-handlers. |
| src/http/ngx_http_request_body.c | Hardens request-body read state transitions and enforces limits on chunk extensions/trailers via client_max_body_size. |
| src/http/ngx_http_postpone_filter_module.c | Fixes subrequest in-memory sizing checks and logging to use correct types/format. |
| src/http/ngx_http_parse.c | Prevents stale args pointer usage; accounts skipped bytes in chunk extensions/trailers. |
| src/http/ngx_http_core_module.c | Adds alias sanity check in map_uri_to_path; makes open_file_cache platform support explicit with warnings. |
| src/http/ngx_http_copy_filter_module.c | Detects request output across the full input chain rather than just the first buffer. |
| src/http/modules/ngx_http_uwsgi_module.c | Ensures auto_redirect is set consistently before URL parsing/validation. |
| src/http/modules/ngx_http_scgi_module.c | Ensures auto_redirect is set consistently before URL parsing/validation. |
| src/http/modules/ngx_http_proxy_module.c | Forces internal body length to 0 when discarding body to avoid incorrect upstream request framing. |
| src/http/modules/ngx_http_mp4_module.c | Adds bounds checks to prevent out-of-range reads on crafted mp4 atom data. |
| src/http/modules/ngx_http_limit_conn_module.c | Ensures shared-memory limit_conn counters are cleaned up if pool cleanup allocation fails. |
| src/http/modules/ngx_http_gzip_filter_module.c | Prevents double-free by nulling preallocated after ngx_pfree(). |
| src/http/modules/ngx_http_grpc_module.c | Resets additional gRPC state on request reinit to avoid stale counters/flags. |
| src/http/modules/ngx_http_dav_module.c | Hardens DAV COPY/MOVE Destination validation under alias to prevent escaping the location prefix. |
| src/http/modules/ngx_http_charset_filter_module.c | Clears charset string pointer when charset is OFF to avoid stale pointer reuse. |
| src/http/modules/ngx_http_auth_request_module.c | Uses WEAK variable creation for auth_request_set to avoid clobbering existing handlers. |
| src/event/ngx_event_openssl.c | Prevents further SSL writes after a recorded fatal error by marking write as not-ready/error. |
| src/event/ngx_event_openssl_stapling.c | Ensures OCSP write handler rearms write events and errors out cleanly on failure. |
| src/core/ngx_resolver.c | Adjusts DNS response flag validation mask (accepts additional valid response bit patterns). |
| src/core/ngx_open_file_cache.c | Ensures cached-file reuse respects disable_symlinks settings even when open_file_cache events are enabled. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR backports 26 defensive guard-clause fixes from freenginx (plus one
follow-up hardening commit), each a small conditional that prevents a concrete
malfunction: a crash, hang, memory-safety issue, resource leak, DoS vector, or
security bypass.
It is the result of a full audit of every freenginx changeset since the fork
(345 changesets, nginx 1.25.4 / Feb 2024 through freenginx 1.31.4). Changes were
included only when they (a) prevent a concrete failure mode, (b) are absent from
current nginx (including semantically equivalent fixes), (c) guard code that
exists in nginx (fixes for freenginx-only regressions were excluded), and (d)
apply with at most trivial adaptation.
Each commit is one upstream changeset with the original author, date, and commit
message preserved, an
Origin:trailer linking the freenginx changeset, and thebackporter's
Signed-off-by:. Commits whose patches required adaptationadditionally carry a
Co-authored-by:trailer and document the deviation in thecommit body — see Changes not present in freenginx below for the explicit
list.
Highlights
client_max_body_size(reported by Bartek Nowotarski)since 1.5.13); HTTP/3 recursion -> stack overflow / stack use-after-free during
connection reuse; gzip double
ngx_pfree(); uninitialized pointer usedisable_symlinksignored for cached entries withopen_file_cache_events; DAV COPY/MOVEDestinationescaping thealiasdirectory (strengthens the check from 9739e75) plus a
ngx_http_map_uri_to_path()sanity checklimit_connshared-memory counter leak on allocation errors; stalled OCSPrequests after partial writes; HTTP/2 connections lingering through graceful
shutdown
set_handlerrejected (the
map $x $limit_ratesegfault class, ticket Add config generation number to process titles #1238), with theauth_request_setand named-captures companions includedChanges not present in freenginx
Two commits contain functional changes beyond the freenginx originals. Both
address flaws that are still present in freenginx itself and are candidates for
an upstream report there:
4f039db74Dav: fixed out-of-bounds read in destination validation. Entirely
original work (
Adaptation-Commit-For:trailer, no freenginx counterpart).The backported Destination check compares the first
clcf->aliasbytes ofr->uri, butr->urican be shorter than the location prefix if it waschanged by a rewrite; such requests are now rejected before
ngx_filename_cmp()is called.c0df646b1Request body: limited chunk extensions and trailer headers. Beyond
adapting the patch to nginx's chunked parser states, this commit adds a
ctx->skippedincrement in thesw_trailerstate. The original freenginxpatch never charges minimal trailer lines (
aCRLF) against the new limit,so an endless stream of them bypasses the cap entirely; with this change
every trailer line costs at least one byte of budget.
Three further commits are mechanical adaptations with no intended behavioral
difference from their freenginx changesets, each documented in its commit
body:
e9abcb601HTTP/3: protection from recursion during connection reuse. — the
one-line fix was placed by hand; the surrounding context differs (nginx does
not carry the freenginx
max_table_capacitychanges).58ea3d04egRPC: reinitialization of ping and settings limits. — the
ctx->pings/ctx->settingsresets were appended to nginx's largerngx_http_grpc_reinit_request()block.8b6386be6Dav: destination validation for COPY and MOVE with "alias". — replaces
nginx's own earlier length-only Destination check (from 9739e75) with
the stronger location prefix comparison.
The remaining 22 backports are byte-for-byte identical to the freenginx
changesets in their added/removed lines.
Compatibility note
Configs that redefine variables having a set handler (
$limit_rate,$args)via map-like directives (
map,geo,split_clients, ...) now fail at startupwith
the duplicate "..." variable. Such configs were latently broken - theget/set handler state they produced was inconsistent.
setandauth_request_setcontinue to work.Known limitations (faithful to upstream freenginx)
client_max_body_sizeand does not extend to the discard-body path.fastcgi/uwsgi/scgi/grpc are follow-up candidates.
SSL_write()guard does not cover theSSL_sendfile()(kTLS)path.
Testing
-Werrorbuild with SSL, HTTP/2, HTTP/3/QUIC, mail, stream, mp4, dav,threads enabled during the audit; the rewritten branch was rebuilt after the
two mail pipelining backports were removed.
backports were removed.
Tests: backport guard-clause fix tests from freenginx nginx-tests#112 (4 commits on a matching
cherry/freenginx-guard-clausesbranch: chunked extensions/trailers, IMAPEXTERNAL auth, SSI stub, proxy SSL password inheritance); all four files
pass against this branch (75/75). As a
deviation from the freenginx originals, their fork version guards were
retargeted to mainline versioning rather than removed, so the new
assertions TODO/skip instead of failing hard when the suite is run against
an nginx without these fixes. Notably, the suite caught a real integration
gap during the audit (
auth_request_set+ duplicate-variable rejection),which is why the
auth_request_setcompanion changeset is included.