| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
Previously, when alias was used in a location with Dav COPY or MOVE
enabled, and the destination URI was shorter than the alias, integer
underflow could happen in ngx_http_map_uri_to_path(), which could
result in heap buffer overwrite, followed by a possible segfault.
With some implementations of memcpy(), the segfault could be avoided
and the overwrite could result in a change of the source or destination
file names to be outside of the location root.
Reported by Calif.io in collaboration with Claude and Anthropic Research.
|
|
Previously, a 32-bit overflow could happen while validating atom entries
count. This allowed processing of an invalid atom with entrires beyond
its boundaries with reads and writes outside of the allocated mp4 buffer.
Reported by Prabhav Srinath (sprabhav7).
|
|
Previously, data validation checks did not cover the cases when the output
contained empty buffers. Such buffers are considered illegal and produce
"zero size buf in output" alerts. The change rejects the mp4 files which
produce such alerts.
Also, the change fixes possible buffer overread and overwrite that could
happen while processing empty stco and co64 atoms, as reported by
Pavel Kohout (Aisle Research) and Tim Becker.
|
|
Per ISO 14496-12 Section 8.6.2, sync sample numbers must be 1-based.
A zero-valued stss entry caused ngx_http_mp4_seek_key_frame() to
return a key_prefix exceeding the samples consumed in the forward
stts pass, which led the backward loop in ngx_http_mp4_crop_stts_data()
to walk past the beginning of the stts data buffer.
The fix validates each stss entry in ngx_http_mp4_seek_key_frame()
and returns an error if a zero sync sample is encountered. The
function signature is changed to return ngx_int_t so it can signal
errors to the caller.
|
|
Previously, when connecting to a backend, if the read event handler was
called before the write event handler, and the received response triggered
a next upstream condition, then ngx_http_upstream_reinit() was not called
to clean up the old upstream context. This had multiple implications.
For all proxy modules, since the last upstream response was not cleaned up,
it was mixed with the next upstream response. This could result in ignoring
the second response status code, duplicate response headers or reporting
old upstream header errors.
With ngx_http_grpc_module and ngx_http_proxy_v2_module, ctx->connection
was left dangling since the object it referenced was allocated from the
last upstream connection pool, which was deleted when freeing last upstream.
This lead to use-after-free when trying to reuse this object for the next
upstream.
|
|
When connecting to a backend, the connection write event is triggered
first in most cases. However if a response arrives quickly enough, both
read and write events can be triggered together within the same event loop
iteration. In this case the read event handler is called first and the
write event handler is called after it.
SSL initialization for backend connections happens only in the write event
handler since SSL handshake starts with sending Client Hello. Previously,
if a backend sent a quick plain text response, it could be parsed by the
read event handler prior to starting SSL handshake on the connection.
The change adds protection against parsing such responses on SSL-enabled
connections.
|
|
If request URI was shorter than location prefix, as after replacement
with try_files, location length was used to copy the remaining URI part
leading to buffer overread.
The fix is to replace full request URI in this case. In the following
configuration, request "/123" is changed to "/" when sent to backend.
location /1234 {
try_files /123 =404;
proxy_pass http://127.0.0.1:8080/;
}
Closes #983 on GitHub.
|
|
In addition to moving memcpy() under the length condition in 15bf6d8cc,
which addressed a reported UB due to string function conventions, this
is repeated for advancing an input buffer, to make the resulting code
more clean and readable.
Additionally, although considered harmless for both string functions and
additive operators, as previously discussed in GitHub PR 866, this fixes
the main source of annoying sanitizer reports in the module.
Prodded by UndefinedBehaviorSanitizer (pointer-overflow).
|
|
|
|
Overflowing calculations are now aligned to the greatest positive integer
as specified in RFC 9111, Section 1.2.2.
|
|
|
|
Found by Coverity (CID 1662016).
|
|
Previously, it misused the Host header processing resulting in
400 (Bad Request) errors for a valid request that contains both
":authority" and Host headers with the same value, treating it
after 37984f0be as if client sent more than one Host header.
Such an overly strict handling violates RFC 9113.
The fix is to process ":authority" as a distinct header, similarly
to processing an authority component in the HTTP/1.x request line.
This allows to disambiguate and compare Host and ":authority"
values after all headers were processed.
With this change, the ngx_http_process_request_header() function
can no longer be used here, certain parts were inlined similar to
the HTTP/3 module.
To provide compatibility for misconfigurations that use $http_host
to return the value of the ":authority" header, the Host header,
if missing, is now reconstructed from ":authority".
|
|
No functional changes.
|
|
RFC 9114, Section 4.3.1. specifies a restriction for :authority and Host
coexistence in an HTTP/3 request:
: If both fields are present, they MUST contain the same value.
Previously, this restriction was correctly enforced only for portless
values. When Host contained a port, the request failed as if :authority
and Host were different, regardless of :authority presence.
This happens because the value of r->headers_in.server used for :authority
has port stripped. The fix is to use r->host_start / r->host_end instead.
|
|
This might happen for Huffman encoded string literals as the result
of length expansion. Notably, the maximum length of string literals
is already limited with the "large_client_header_buffers" directive,
so this was only possible with nonsensically large configured limits.
|
|
There were a few random places where 0 was being used as a null pointer
constant.
We have a NULL macro for this very purpose, use it.
There is also some interest in actually deprecating the use of 0 as a
null pointer constant in C.
This was found with -Wzero-as-null-pointer-constant which was enabled
for C in GCC 15 (not enabled with Wall or Wextra... yet).
Link: <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117059>
|
|
The functions ngx_http_merge_types() & ngx_conf_merge_path_value()
return either NGX_CONF_OK aka NULL aka ((void *)0) (probably) or
NGX_CONF_ERROR aka ((void *)-1).
They don't return an integer constant which is what NGX_OK aka (0) is.
Lets use the right thing in the function return check.
This was found with -Wzero-as-null-pointer-constant which was enabled
for C in GCC 15 (not enabled with Wall or Wextra... yet).
Link: <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117059>
|
|
After fixing ngx_http_v3_encode_varlen_int() in 400eb1b628,
NGX_HTTP_V3_VARLEN_INT_LEN retained the old value of 4, which is
insufficient for the values over 1073741823 (1G - 1).
The NGX_HTTP_V3_VARLEN_INT_LEN macro is used in ngx_http_v3_uni.c to
format stream and frame types. Old buffer size is enough for formatting
this data. Also, the macro is used in ngx_http_v3_filter_module.c to
format output chunks and trailers. Considering output_buffers and
proxy_buffer_size are below 1G in all realistic scenarios, the old buffer
size is enough here as well.
|
|
|
|
Previously, the expiration caused QUIC connection finalization even if
there are application-terminated streams finishing sending data. Such
finalization terminated these streams.
An easy way to trigger this is to request a large file from HTTP/3 over
a small MTU. In this case keepalive timeout expiration may abruptly
terminate the request stream.
|
|
Passwords were not preserved in optimized SSL contexts, the bug had
appeared in d791b4aab (1.23.1), as in the following configuration:
server {
proxy_ssl_password_file password;
proxy_ssl_certificate $ssl_server_name.crt;
proxy_ssl_certificate_key $ssl_server_name.key;
location /original/ {
proxy_pass https://u1/;
}
location /optimized/ {
proxy_pass https://u2/;
}
}
The fix is to always preserve passwords, by copying to the configuration
pool, if dynamic certificates are used. This is done as part of merging
"ssl_passwords" configuration.
To minimize the number of copies, a preserved version is then used for
inheritance. A notable exception is inheritance of preserved empty
passwords to the context with statically configured certificates:
server {
proxy_ssl_certificate $ssl_server_name.crt;
proxy_ssl_certificate_key $ssl_server_name.key;
location / {
proxy_pass ...;
proxy_ssl_certificate example.com.crt;
proxy_ssl_certificate_key example.com.key;
}
}
In this case, an unmodified version (NULL) of empty passwords is set,
to allow reading them from the password prompt on nginx startup.
As an additional optimization, a preserved instance of inherited
configured passwords is set to the previous level, to inherit it
to other contexts:
server {
proxy_ssl_password_file password;
location /1/ {
proxy_pass https://u1/;
proxy_ssl_certificate $ssl_server_name.crt;
proxy_ssl_certificate_key $ssl_server_name.key;
}
location /2/ {
proxy_pass https://u2/;
proxy_ssl_certificate $ssl_server_name.crt;
proxy_ssl_certificate_key $ssl_server_name.key;
}
}
|
|
It was possible to write outside of the buffer used to keep UTF-8
decoded values when parsing conversion table configuration.
Since this happened before UTF-8 decoding, the fix is to check in
advance if character codes are of more than 3-byte sequence. Note
that this is already enforced by a later check for ngx_utf8_decode()
decoded values for 0xffff, which corresponds to the maximum value
encoded as a valid 3-byte sequence, so the fix does not affect the
valid values.
Found with AddressSanitizer.
Fixes GitHub issue #529.
|
|
As uncovered by recent addition in slice.t, a partially initialized
context, coupled with HTTP 206 response from stub backend, might be
accessed in the next slice subrequest.
Found by bad memory allocator simulation.
|
|
It appears to be a relic from prototype locking removed in b0b7b5a35.
|
|
This makes it easier to understand why sessions may not be saved
in shared memory due to size.
|
|
All such transient buffers are converted to the single storage in BSS.
In preparation to raise the limit.
|
|
Previously, request might be left in inconsistent state in case of error,
which manifested in "http request count is zero" alerts when used by SSI
filter.
The fix is to reshuffle initialization order to postpone committing state
changes until after any potentially failing parts.
Found by bad memory allocator simulation.
|
|
In OpenSSL, session resumption always happens in the default SSL context,
prior to invoking the SNI callback. Further, unlike in TLSv1.2 and older
protocols, SSL_get_servername() returns values received in the resumption
handshake, which may be different from the value in the initial handshake.
Notably, this makes the restriction added in b720f650b insufficient for
sessions resumed with different SNI server name.
Considering the example from b720f650b, previously, a client was able to
request example.org by presenting a certificate for example.org, then to
resume and request example.com.
The fix is to reject handshakes resumed with a different server name, if
verification of client certificates is enabled in a corresponding server
configuration.
|
|
The directive sets a timeout during which a keepalive connection will
not be closed by nginx for connection reuse or graceful shutdown.
The change allows clients that send multiple requests over the same
connection without delay or with a small delay between them, to avoid
receiving a TCP RST in response to one of them. This excludes network
issues and non-graceful shutdown. As a side-effect, it also addresses
the TCP reset problem described in RFC 9112, Section 9.6, when the last
sent HTTP response could be damaged by a followup TCP RST. It is important
for non-idempotent requests, which cannot be retried by client.
It is not recommended to set keepalive_min_timeout to large values as
this can introduce an additional delay during graceful shutdown and may
restrict nginx from effective connection reuse.
|
|
Caching is enabled with proxy_ssl_certificate_cache and friends.
Co-authored-by: Aleksei Bavshin <a.bavshin@nginx.com>
|
|
A new directive "ssl_certificate_cache max=N [valid=time] [inactive=time]"
enables caching of SSL certificate chain and secret key objects specified
by "ssl_certificate" and "ssl_certificate_key" directives with variables.
Co-authored-by: Aleksei Bavshin <a.bavshin@nginx.com>
|
|
|
|
It now uses 5/4 times more memory for the pending buffer.
Further, a single allocation is now used, which takes additional 56 bytes
for deflate_allocs in 64-bit mode aligned to 16, to store sub-allocation
pointers, and the total allocation size now padded up to 128 bytes, which
takes theoretically 200 additional bytes in total. This fits though into
"4 * (64 + sizeof(void*))" additional space for ZALLOC used in zlib-ng
2.1.x versions. The comment was updated to reflect this.
|
|
Renaming a temporary file to an empty path ("") returns NGX_ENOPATH
with a subsequent ngx_create_full_path() to create the full path.
This function skips initial bytes as part of path separator lookup,
which causes out of bounds access on short strings.
The fix is to avoid renaming a temporary file to an obviously invalid
path, as well as explicitly forbid such syntax for literal values.
Although Coverity reports about potential type underflow, it is not
actually possible because the terminating '\0' is always included.
Notably, the run-time check is sufficient enough for Win32 as well.
Other short invalid values result either in NGX_ENOENT or NGX_EEXIST
and "MoveFile() .. failed" critical log messages, which involves a
separate error handling.
Prodded by Coverity (CID 1605485).
|
|
This simplifies merging protocol values after ea15896 and ebd18ec.
Further, as outlined in ebd18ec18, for libraries preceeding TLSv1.2+
support, only meaningful versions TLSv1 and TLSv1.1 are set by default.
While here, fixed indentation.
|
|
When cropping stsc atom, it's assumed that chunk index is never 0.
Based on this assumption, start_chunk and end_chunk are calculated
by subtracting 1 from it. If chunk index is zero, start_chunk or
end_chunk may underflow, which will later trigger
"start/end time is out mp4 stco chunks" error. The change adds an
explicit check for zero chunk index to avoid underflow and report
a proper error.
Zero chunk index is explicitly banned in ISO/IEC 14496-12, 8.7.4
Sample To Chunk Box. It's also implicitly banned in QuickTime File
Format specification. Description of chunk offset table references
"Chunk 1" as the first table element.
|
|
Currently an error is triggered if any of the chunk runs in stsc are
unordered. This however does not include the final chunk run, which
ends with trak->chunks + 1. The previous chunk index can be larger
leading to a 32-bit overflow. This could allow to skip the validity
check "if (start_sample > n)". This could later lead to a large
trak->start_chunk/trak->end_chunk, which would be caught later in
ngx_http_mp4_update_stco_atom() or ngx_http_mp4_update_co64_atom().
While there are no implications of the validity check being avoided,
the change still adds a check to ensure the final chunk run is ordered,
to produce a meaningful error and avoid a potential integer overflow.
|
|
A specially crafted mp4 file with an empty run of chunks in the stsc atom
and a large value for samples per chunk for that run, combined with a
specially crafted request, allowed to store that large value in prev_samples
and later in trak->end_chunk_samples while in ngx_http_mp4_crop_stsc_data().
Later in ngx_http_mp4_update_stsz_atom() this could result in buffer
overread while calculating trak->end_chunk_samples_size.
Now the value of samples per chunk specified for an empty run is ignored.
|
|
|
|
|
|
|
|
MSVC generates a compilation error in case #if/#endif is used in a macro
parameter.
|
|
Previously, all upstream DNS entries would be immediately re-resolved
on config reload. With a large number of upstreams, this creates
a spike of DNS resolution requests. These spikes can overwhelm the
DNS server or cause drops on the network.
This patch retains the TTL of previous resolutions across reloads
by copying each upstream's name's expiry time across configuration
cycles. As a result, no additional resolutions are needed.
|
|
The "resolver" and "resolver_timeout" directives can now be specified
directly in the "upstream" block.
|
|
After configuration is reloaded, it may take some time for the
re-resolvable upstream servers to resolve and become available
as peers. During this time, client requests might get dropped.
Such servers are now pre-resolved using the "cache" of already
resolved peers from the old shared memory zone.
|
|
|
|
Specifying the upstream server by a hostname together with the
"resolve" parameter will make the hostname to be periodically
resolved, and upstream servers added/removed as necessary.
This requires a "resolver" at the "http" configuration block.
The "resolver_timeout" parameter also affects when the failed
DNS requests will be attempted again. Responses with NXDOMAIN
will be attempted again in 10 seconds.
Upstream has a configuration generation number that is incremented each
time servers are added/removed to the primary/backup list. This number
is remembered by the peer.init method, and if peer.get detects a change
in configuration, it returns NGX_BUSY.
Each server has a reference counter. It is incremented by peer.get and
decremented by peer.free. When a server is removed, it is removed from
the list of servers and is marked as "zombie". The memory allocated by
a zombie peer is freed only when its reference count becomes zero.
Co-authored-by: Roman Arutyunyan <arut@nginx.com>
Co-authored-by: Sergey Kandaurov <pluknet@nginx.com>
Co-authored-by: Vladimir Homutov <vl@nginx.com>
|
|
TLSv1 and TLSv1.1 are formally deprecated and forbidden to negotiate due
to insufficient security reasons outlined in RFC 8996.
TLSv1 and TLSv1.1 are disabled in BoringSSL e95b0cad9 and LibreSSL 3.8.1
in the way they cannot be enabled in nginx configuration. In OpenSSL 3.0,
they are only permitted at security level 0 (disabled by default).
The support is dropped in Chrome 84, Firefox 78, and deprecated in Safari.
This change disables TLSv1 and TLSv1.1 by default for OpenSSL 1.0.1 and
newer, where TLSv1.2 support is available. For older library versions,
which do not have alternatives, these protocol versions remain enabled.
|
|
Starting from TLSv1.1 (as seen since draft-ietf-tls-rfc2246-bis-00),
the "certificate_authorities" field grammar of the CertificateRequest
message was redone to allow no distinguished names. In TLSv1.3, with
the restructured CertificateRequest message, this can be similarly
done by optionally including the "certificate_authorities" extension.
This allows to avoid sending DNs at all.
In practice, aside from published TLS specifications, all supported
SSL/TLS libraries allow to request client certificates with an empty
DN list for any protocol version. For instance, when operating in
TLSv1, this results in sending the "certificate_authorities" list as
a zero-length vector, which corresponds to the TLSv1.1 specification.
Such behaviour goes back to SSLeay.
The change relaxes the requirement to specify at least one trusted CA
certificate in the ssl_client_certificate directive, which resulted in
sending DNs of these certificates (closes #142). Instead, all trusted
CA certificates can be specified now using the ssl_trusted_certificate
directive if needed. A notable difference that certificates specified
in ssl_trusted_certificate are always loaded remains (see 3648ba7db).
Co-authored-by: Praveen Chaudhary <praveenc@nvidia.com>
|
