In some rare cases, graceful shutdown may happen while initializing an HTTP/2
connection. Previously, such a connection ignored the shutdown and remained
active. Now it is gracefully closed prior to processing any streams to
eliminate the shutdown delay.
Previously handlers were mandatory. However they are not always needed.
For example, a server configured with ssl_reject_handshake does not need a
handler. Such servers required a fake handler to pass the check. Now handler
absence check is moved to runtime. If handler is missing, the connection is
closed with 500 code.
Previously the last chain field of ngx_quic_buffer_t could still reference freed
chains and buffers after calling ngx_quic_free_buffer(). While normally an
ngx_quic_buffer_t object should not be used after freeing, resetting last_chain
field would prevent a potential use-after-free.
Sending handshake-level CRYPTO frames after the client's Finished message could
lead to memory disclosure and a potential segfault, if those frames are sent in
one packet with the Finished frame.
While inserting a new entry into the dynamic table, first the entry is added,
and then older entries are evicted until table size is within capacity. After
the first step, the number of entries may temporarily exceed the maximum
calculated from capacity by one entry, which previously caused table overflow.
The easiest way to trigger the issue is to keep adding entries with empty names
and values until first eviction.
The issue was introduced by 987bee4363d1.
Previously a decoder stream was created on demand for sending Section
Acknowledgement, Stream Cancellation and Insert Count Increment. If conditions
for sending any of these instructions never happen, a decoder stream is not
created at all. These conditions include client not using the dynamic table and
no streams abandoned by server (RFC 9204, Section 2.2.2.2). However RFC 9204,
Section 4.2 defines only one condition for not creating a decoder stream:
An endpoint MAY avoid creating a decoder stream if its decoder sets
the maximum capacity of the dynamic table to zero.
The change enables pre-creation of the decoder stream at HTTP/3 session
initialization if maximum dynamic table capacity is not zero. Note that this
value is currently hardcoded to 4096 bytes and is not configurable, so the
stream is now always created.
Also, the change fixes a potential stack overflow when creating a decoder
stream in ngx_http_v3_send_cancel_stream() while draining a request stream by
ngx_drain_connections(). Creating a decoder stream involves calling
ngx_get_connection(), which calls ngx_drain_connections(), which will drain the
same request stream again. If client's MAX_STREAMS for uni stream is high
enough, these recursive calls will continue until we run out of stack.
Otherwise, decoder stream creation will fail at some point and the request
stream connection will be drained. This may result in use-after-free, since
this connection could still be referenced up the stack.
Previously chain links could sometimes be dropped instead of being reused,
which could result in increased memory consumption during long requests.
A similar chain link issue in ngx_http_gzip_filter_module was fixed in
da46bfc484ef (1.11.10).
Based on a patch by Sangmin Lee.
Using "long *" instead of "AO_t *" leads either to -Wincompatible-pointer-types
or -Wpointer-sign warnings, depending on whether long and size_t are compatible
types (e.g., ILP32 versus LP64 data models). Notably, -Wpointer-sign warnings
are enabled by default in Clang only, and -Wincompatible-pointer-types is an
error starting from GCC 14.
Signed-off-by: Edgar Bonet <bonet@grenoble.cnrs.fr>
Passing from udp was not possible for the most part due to preread buffer
restriction. Passing to udp could occasionally work, but the connection would
still be bound to the original listen rbtree, which prevented it from being
deleted on connection closure.
When loading certificate keys via ENGINE_load_private_key() in runtime,
it was possible to overwrite configuration on ENGINE_by_id() failure.
OpenSSL documention doesn't describe errors in details, the only reason
I found in the comment to example is when the engine is not available.
Previously, a request body larger than declared in Content-Length resulted in
a 413 status code, because Content-Length was mistakenly used as the maximum
allowed request body, similar to client_max_body_size. Following the HTTP/3
specification, such requests are now rejected with the 400 error as malformed.
The ngx_quic_run() function uses qc->close timer to limit the handshake
duration. Normally it is removed by ngx_quic_do_init_streams() which is
called once when we are done with initial SSL processing.
The problem happens when the client sends early data and streams are
initialized in the ngx_quic_run() -> ngx_quic_handle_datagram() call.
The order of set/remove timer calls is now reversed; the close timer is
set up and the timer fires when assigned, starting the unexpected connection
close process.
The fix is to skip setting the timer if streams were initialized during
handling of the initial datagram. The idle timer for quic is set anyway,
and stream-related timeouts are managed by application layer.
Notably, Apple Silicon CPUs have 128 byte cache line size,
which is twice the default configured for generic aarch64.
Signed-off-by: Piotr Sikora <piotr@aviatrix.com>
Previously, the response text wasn't initialized and the rewrite module
was sending response body set to NULL.
Found with UndefinedBehaviorSanitizer (pointer-overflow).
Signed-off-by: Piotr Sikora <piotr@aviatrix.com>
Previously, it could result when left-shifting signed integer due to implicit
integer promotion, such that the most significant bit appeared on the sign bit.
In practice, though, this results in the same left value as with an explicit
cast, at least on known compilers, such as GCC and Clang. The reason is that
in_addr_t, which is equivalent to uint32_t and same as "unsigned int" in ILP32
and LP64 data type models, has the same type width as the intermediate after
integer promotion, so there's no side effects such as sign-extension. This
explains why adding an explicit cast does not change object files in practice.
Found with UndefinedBehaviorSanitizer (shift).
Based on a patch by Piotr Sikora.
While copying ngx_http_variable_value_t structures to geo binary base
in ngx_http_geo_copy_values(), and similarly in the stream module,
uninitialized parts of these structures are copied as well. These
include the "escape" field and possible holes. Calculating crc32 of
this data triggers uninitialized memory access.
Found with MemorySanitizer.
Signed-off-by: Piotr Sikora <piotr@aviatrix.com>
In preparation for adding more parameters to the listen directive,
and to be in sync with the corresponding structure in the http module.
No functional changes.
Originally, the stream module was developed based on the mail module,
following the existing style. Then it was diverged to closely follow
the http module development. This change updates style to use sscf
naming convention troughout the stream module, which matches the http
module code style. No functional changes.
The module allows to pass connections from Stream to other modules such as HTTP
or Mail, as well as back to Stream. Previously, this was only possible with
proxying. Connections with preread buffer read out from socket cannot be
passed.
The module allows selective SSL termination based on SNI.
stream {
server {
listen 8000 default_server;
ssl_preread on;
...
}
server {
listen 8000;
server_name foo.example.com;
pass 127.0.0.1:8001; # to HTTP
}
server {
listen 8000;
server_name bar.example.com;
...
}
}
http {
server {
listen 8001 ssl;
...
location / {
root html;
}
}
}
Server name is taken either from ngx_stream_ssl_module or
ngx_stream_ssl_preread_module.
The change adds "default_server" parameter to the "listen" directive,
as well as the following directives: "server_names_hash_max_size",
"server_names_hash_bucket_size", "server_name" and "ssl_reject_handshake".
Previously, preread buffer was always read out from socket, which made it
impossible to terminate SSL on the connection without introducing additional
SSL BIOs. The following patches will rely on this.
Now, when possible, recv(MSG_PEEK) is used instead, which keeps data in socket.
It's called if SSL is not already terminated and if an egde-triggered event
method is used. For epoll, EPOLLRDHUP support is also required.
Stream connection cleanup handler ngx_quic_stream_cleanup_handler() calls
ngx_quic_shutdown_stream() after which it resets the pointer from quic stream
to the connection (sc->connection = NULL). Previously if this call failed,
sc->connection retained the old value, while the connection was freed by the
application code. This resulted later in a second attempt to close the freed
connection, which lead to allocator double free error.
The fix is to reset the sc->connection pointer in case of error.
Inspired by RFC 9001, Section 6.3, trial packet decryption with the current
keys is now used to avoid a timing side-channel signal. Further, this fixes
segfault while accessing missing next keys (ticket #2585).
Previously if an MTU probe send failed early in ngx_quic_frame_sendto()
due to allocation error or congestion control, the application level packet
number was not increased, but was still saved as MTU probe packet number.
Later when a packet with this number was acknowledged, the unsent MTU probe
was acknowledged as well. This could result in discovering a bigger MTU than
supported by the path, which could lead to EMSGSIZE (Message too long) errors
while sending further packets.
The problem existed since PMTUD was introduced in 58afcd72446f (1.25.2).
Back then only the unlikely memory allocation error could trigger it. However
in efcdaa66df2e congestion control was added to ngx_quic_frame_sendto() which
can now trigger the issue with a higher probability.
Now "fastopen", "backlog", "accept_filter", "deferred", and "so_keepalive"
parameters are not allowed with "quic" in the "listen" directive.
Reported by Izorkin.
