When a request was terminated due to an error via ngx_http_terminate_request()
while an AIO operation was running in a subrequest, various issues were
observed. This happened because ngx_http_request_finalizer() was only set
in the subrequest where ngx_http_terminate_request() was called, but not
in the subrequest where the AIO operation was running. After completion
of the AIO operation normal processing of the subrequest was resumed, leading
to issues.
In particular, in case of the upstream module, termination of the request
called upstream cleanup, which closed the upstream connection. Attempts to
further work with the upstream connection after AIO operation completion
resulted in segfaults in ngx_ssl_recv(), "readv() failed (9: Bad file
descriptor) while reading upstream" errors, or socket leaks.
In ticket #2555, issues were observed with the following configuration
with cache background update (with thread writing instrumented to
introduce a delay, when a client closes the connection during an update):
location = /background-and-aio-write {
proxy_pass ...
proxy_cache one;
proxy_cache_valid 200 1s;
proxy_cache_background_update on;
proxy_cache_use_stale updating;
aio threads;
aio_write on;
limit_rate 1000;
}
Similarly, the same issue can be seen with SSI, and can be caused by
errors in subrequests, such as in the following configuration
(where "/proxy" uses AIO, and "/sleep" returns 444 after some delay,
causing request termination):
location = /ssi-active-boom {
ssi on;
ssi_types *;
return 200 '
<!--#include virtual="/proxy" -->
<!--#include virtual="/sleep" -->
';
limit_rate 1000;
}
Or the same with both AIO operation and the error in non-active subrequests
(which needs slightly different handling, see below):
location = /ssi-non-active-boom {
ssi on;
ssi_types *;
return 200 '
<!--#include virtual="/static" -->
<!--#include virtual="/proxy" -->
<!--#include virtual="/sleep" -->
';
limit_rate 1000;
}
Similarly, issues can be observed with just static files. However,
with static files potential impact is limited due to timeout safeguards
in ngx_http_writer(), and the fact that c->error is set during request
termination.
In a simple configuration with an AIO operation in the active subrequest,
such as in the following configuration, the connection is closed right
after completion of the AIO operation anyway, since ngx_http_writer()
tries to write to the connection and fails due to c->error set:
location = /ssi-active-static-boom {
ssi on;
ssi_types *;
return 200 '
<!--#include virtual="/static-aio" -->
<!--#include virtual="/sleep" -->
';
limit_rate 1000;
}
In the following configuration, with an AIO operation in a non-active
subrequest, the connection is closed only after send_timeout expires:
location = /ssi-non-active-static-boom {
ssi on;
ssi_types *;
return 200 '
<!--#include virtual="/static" -->
<!--#include virtual="/static-aio" -->
<!--#include virtual="/sleep" -->
';
limit_rate 1000;
}
Fix is to introduce r->main->terminated flag, which is to be checked
by AIO event handlers when the r->main->blocked counter is decremented.
When the flag is set, handlers are expected to wake up the connection
instead of the subrequest (which might be already cleaned up).
Additionally, now ngx_http_request_finalizer() is always set in the
active subrequest, so waking up the connection properly finalizes the
request even if termination happened in a non-active subrequest.
Each AIO (thread IO) operation being run is now accompanied with 1-minute
timer. This timer prevents unexpected shutdown of the worker process while
an AIO operation is running, and logs an alert if the operation is running
for too long.
This fixes "open socket left" alerts during worker processes shutdown
due to pending AIO (or thread IO) operations while corresponding requests
have no timers. In particular, such errors were observed while reading
cache headers (ticket #2162), and with worker_shutdown_timeout.
When graceful shutdown was requested, and then nginx was forced to
do fast shutdown, it used to (incorrectly) complain about open sockets
left in connections which weren't yet closed when fast shutdown
was requested.
Fix is to avoid complaining about open sockets when fast shutdown was
requested after graceful one. Abnormal termination, if requested with
the WINCH signal, can still happen though.
OPENSSL_VERSION_NUMBER is now redefined to 0x1010000fL for LibreSSL 3.5.0
and above. Building with older LibreSSL versions, such as 2.8.0, may now
produce warnings (see cab37803ebb3) and may require appropriate compiler
options to suppress them.
Notably, this allows to start using SSL_get0_verified_chain() appeared
in OpenSSL 1.1.0 and LibreSSL 3.5.0, without additional macro tests.
Prodded by Ilya Shipitsin.
Similar to 7356:e3ba4026c02d, as long as SSL_OP_NO_CLIENT_RENEGOTIATION
is defined, it is the library responsibility to prevent renegotiation.
Additionally, this allows to raise LibreSSL version used to redefine
OPENSSL_VERSION_NUMBER to 0x1010000fL, such that this won't result in
attempts to dereference SSL objects made opaque in LibreSSL 3.4.0.
Patch by Maxim Dounin.
rand() is used on win32. RAND_MAX is implementation defined. win32's is
0x7fff.
Existing uses of ngx_random() rely upon 0x7fffffff range provided by
POSIX implementations of random().
On-packet acknowledgement is made path aware, as per RFC 9000, Section 9.4:
Packets sent on the old path MUST NOT contribute to congestion control
or RTT estimation for the new path.
To make this possible in a single congestion control context, the first packet
to be sent after the new path has been validated, which includes resetting the
congestion controller and RTT estimator, is now remembered in the connection.
Packets sent previously, such as on the old path, are not taken into account.
Note that although the packet number is saved per-connection, the added checks
affect application level packets only. For non-application level packets,
which are only processed prior to the handshake is complete, the remembered
packet number remains set to zero.
As per RFC 9000, Section 8.2.1:
When an endpoint is unable to expand the datagram size to 1200 bytes due
to the anti-amplification limit, the path MTU will not be validated.
To ensure that the path MTU is large enough, the endpoint MUST perform a
second path validation by sending a PATH_CHALLENGE frame in a datagram of
at least 1200 bytes.
Previously ngx_quic_frame_sendto() ignored congestion control and did not
contribute to in_flight counter.
Now congestion control window is checked unless ignore_congestion flag is set.
Also, in_flight counter is incremented and the frame is stored in ctx->sent
queue if it's ack-eliciting. This behavior is now similar to
ngx_quic_output_packet().
According to RFC 9000, an endpoint SHOULD NOT send multiple PATH_CHALLENGE
frames in a single packet. The change adds a check to enforce this claim to
optimize server behavior. Previously each PATH_CHALLENGE always resulted in a
single response datagram being sent to client. The effect of this was however
limited by QUIC flood protection.
Also, PATH_CHALLENGE is explicitly disabled in Initial and Handshake levels,
see RFC 9000, Table 3. However, technically it may be sent by client in 0-RTT
over a new path without actual migration, even though the migration itself is
prohibited during handshake. This allows client to coalesce multiple 0-RTT
packets each carrying a PATH_CHALLENGE and end up with multiple PATH_CHALLENGEs
per datagram. This again leads to suboptimal behavior, see above. Since the
purpose of sending PATH_CHALLENGE frames in 0-RTT is unclear, these frames are
now only allowed in 1-RTT. For 0-RTT they are silently ignored.
Previously, when using ngx_quic_frame_sendto() to explicitly send a packet with
a single frame, anti-amplification limit was not properly enforced. Even when
there was no quota left for the packet, it was sent anyway, but with no padding.
Now the packet is not sent at all.
This function is called to send PATH_CHALLENGE/PATH_RESPONSE, PMTUD and probe
packets. For all these cases packet send is retried later in case the send was
not successful.
By default packets with these frames are expanded to 1200 bytes. Previously,
if anti-amplification limit did not allow this expansion, it was limited to
whatever size was allowed. However RFC 9000 clearly states no partial
expansion should happen in both cases.
Section 8.2.1. Initiating Path Validation:
An endpoint MUST expand datagrams that contain a PATH_CHALLENGE frame
to at least the smallest allowed maximum datagram size of 1200 bytes,
unless the anti-amplification limit for the path does not permit
sending a datagram of this size.
Section 8.2.2. Path Validation Responses:
An endpoint MUST expand datagrams that contain a PATH_RESPONSE frame
to at least the smallest allowed maximum datagram size of 1200 bytes.
...
However, an endpoint MUST NOT expand the datagram containing the
PATH_RESPONSE if the resulting data exceeds the anti-amplification limit.
If URI is not fully parsed yet, some pointers are not set. As a result,
the calculation of "new + (ptr - old)" expression is flawed.
According to C11, 6.5.6 Additive operators, p.9:
: When two pointers are subtracted, both shall point to elements
: of the same array object, or one past the last element of the
: array object
Since "ptr" is not set, subtraction leads to undefined behaviour, because
"ptr" and "old" are not in the same buffer (i.e. array objects).
Prodded by GCC undefined behaviour sanitizer.
Neither r->port_start nor r->port_end were ever used.
The r->port_end is set by the parser, though it was never used by
the following code (and was never usable, since not copied by the
ngx_http_alloc_large_header_buffer() without r->port_start set).
Currently, packets generated by ngx_quic_frame_sendto() and
ngx_quic_send_early_cc() are not logged, thus making it hard
to read logs due to gaps appearing in packet numbers sequence.
At frames level, it is handy to see immediately packet number
in which they arrived or being sent.
As part of normal HTTP/2 processing, incomplete frames are saved in the
control state using a fixed size memcpy of NGX_HTTP_V2_STATE_BUFFER_SIZE.
For this matter, two state buffers are reserved in the HTTP/2 recv buffer.
As part of HTTP/2 auto-detection on plain TCP connections, initial data
is first read into a buffer specified by the client_header_buffer_size
directive that doesn't have state reservation. Previously, this made it
possible to over-read the buffer as part of saving the state.
The fix is to read the available buffer size rather than a fixed size.
Although memcpy of a fixed size can produce a better optimized code,
handling of incomplete frames isn't a common execution path, so it was
sacrificed for the sake of simplicity of the fix.
It is made local as it is only needed now when creating crypto context.
BoringSSL lacks EVP interface for ChaCha20, providing instead
a function for one-shot encryption, thus hp is still preserved.
Based on a patch by Roman Arutyunyan.
Previously it was possible to generate ACK frames using formally discarded
protection keys, in particular, when acknowledging a client Handshake packet
used to complete the TLS handshake and to discard handshake protection keys.
As it happens late in packet processing, it could be possible to generate ACK
frames after the keys were already discarded.
ACK frames are generated from ngx_quic_ack_packet(), either using a posted
push event, which envolves ngx_quic_generate_ack() as a part of the final
packet assembling, or directly in ngx_quic_ack_packet(), such as when there
is no room to add a new ACK range or when the received packet is out of order.
The added keys availability check is used to avoid generating late ACK frames
in both cases.
In addition to triggering alert, it ensures that such packets won't be sent.
With the previous change that marks server keys as discarded by zeroing the
key lengh, it is now an error to send packets with discarded keys. OpenSSL
based stacks tolerate such behaviour because key length isn't used in packet
protection, but BoringSSL will raise the UNSUPPORTED_KEY_SIZE cipher error.
It won't be possible to use discarded keys with reused crypto contexts as it
happens in subsequent changes.
Keys may be released by TLS stack in different times, so it makes sense
to check this independently as well. This allows to fine-tune what key
direction is used when checking keys availability.
When discarding, server keys are now marked in addition to client keys.
This improves nginx startup times significantly when using very large number
of locations due to computational complexity of the sorting algorithm being
used: insertion sort is O(n*n) on average, while merge sort is O(n*log(n)).
In particular, in a test configuration with 20k locations total startup
time is reduced from 8 seconds to 0.9 seconds.
Prodded by Yusuke Nojima,
https://mailman.nginx.org/pipermail/nginx-devel/2023-September/NUL3Y2FPPFSHMPTFTL65KXSXNTX3NQMK.html
In ngx_regex_cleanup() allocator wasn't configured when calling
pcre2_compile_context_free() and pcre2_match_data_free(), resulting
in no ngx_free() call and leaked memory. Fix is ensure that allocator
is configured for global allocations, so that ngx_free() is actually
called to free memory.
Additionally, ngx_regex_compile_context was cleared in
ngx_regex_module_init(). It should be either not cleared, so it will
be freed by ngx_regex_cleanup(), or properly freed. Fix is to
not clear it, so ngx_regex_cleanup() will be able to free it.
Reported by ZhenZhong Wu,
https://mailman.nginx.org/pipermail/nginx-devel/2023-September/3Z5FIKUDRN2WBSL3JWTZJ7SXDA6YIWPB.html
To ensure that attempts to flood servers with many streams are detected
early, a limit of no more than 2 * max_concurrent_streams new streams per one
event loop iteration was introduced. This limit is applied even if
max_concurrent_streams is not yet reached - for example, if corresponding
streams are handled synchronously or reset.
Further, refused streams are now limited to maximum of max_concurrent_streams
and 100, similarly to priority_limit initial value, providing some tolerance
to clients trying to open several streams at the connection start, yet
low tolerance to flooding attempts.
The error may be triggered in add_handhshake_data() by incorrect transport
parameter sent by client. The expected behaviour in this case is to close
connection complaining about incorrect parameter. Currently the connection
just times out.
Previously, the timer was never reset due to an explicit check. The check was
added in 36b59521a41c as part of connection close simplification. The reason
was to retain the earliest timeout. However, the timeouts are all the same
while QUIC handshake is in progress and resetting the timer for the same value
has no performance implications. After handshake completion there's only
application level. The change removes the check.
Now the session object is assigned to c->data while ngx_http_connection_t
object is referenced by its http_connection field, similar to
ngx_http_v2_connection_t and ngx_http_request_t.
The change allows to eliminate v3_session field from ngx_http_connection_t.
The field was under NGX_HTTP_V3 macro, which was a source of binary
compatibility problems when nginx/module is build with/without HTTP/3 support.
Postponing is essential since c->data should retain the reference to
ngx_http_connection_t object throughout QUIC handshake, because SSL callbacks
ngx_http_ssl_servername() and ngx_http_ssl_alpn_select() rely on this.
Instead, when worker is shutting down and handshake is not yet completed,
connection is terminated immediately.
Previously the callback could be called while QUIC handshake was in progress
and, what's more important, before the init() callback. Now it's postponed
after init().
This change is a preparation to postponing HTTP/3 session creation to init().
Previously QUIC did not have such parameter and handshake duration was
controlled by HTTP/3. However that required creating and storing HTTP/3
session on first client datagram. Apparently there's no convenient way to
store the session object until QUIC handshake is complete. In the followup
patches session creation will be postponed to init() callback.
As explained in BoringSSL change[1], levels were introduced in the original
QUIC API to draw a line between when keys are released and when are active.
In the new QUIC API they are released in separate calls when it's needed.
BoringSSL has then a consideration to remove levels API, hence the change.
If not available e.g. from a QUIC packet header, levels can be taken based on
keys availability. The only real use of levels is to prevent using app keys
before they are active in QuicTLS that provides the old BoringSSL QUIC API,
it is replaced with an equivalent check of c->ssl->handshaked.
This change also removes OpenSSL compat shims since they are no longer used.
The only exception left is caching write level from the keylog callback in
the internal field which is a handy equivalent of checking keys availability.
[1] https://boringssl.googlesource.com/boringssl/+/1e859054
As per RFC 9000, section 10.2.3, to ensure that peer successfully removed
packet protection, CONNECTION_CLOSE can be sent in multiple packets using
different packet protection levels.
Now it is sent in all protection levels available.
This roughly corresponds to the following paragraph:
* Prior to confirming the handshake, a peer might be unable to process 1-RTT
packets, so an endpoint SHOULD send a CONNECTION_CLOSE frame in both Handshake
and 1-RTT packets. A server SHOULD also send a CONNECTION_CLOSE frame in an
Initial packet.
In practice, this change allows to avoid sending an Initial packet when we know
the client has handshake keys, by checking if we have discarded initial keys.
Also, this fixes sending CONNECTION_CLOSE when using QuicTLS with old QUIC API,
where TLS stack releases application read keys before handshake confirmation;
it is fixed by sending CONNECTION_CLOSE additionally in a Handshake packet.
Status header with an empty reason-phrase, such as "Status: 404 ", is
valid per CGI specification, but looses the trailing space during parsing.
Currently, this results in "HTTP/1.1 404" HTTP status line in the response,
which violates HTTP specification due to missing trailing space.
With this change, only the status code is used from such short Status
header lines, so nginx will generate status line itself, with the space
and appropriate reason phrase if available.
Reported at:
https://mailman.nginx.org/pipermail/nginx/2023-August/EX7G4JUUHJWJE5UOAZMO5UD6OJILCYGX.html
Previously, a socket error on a path being validated resulted in validation
error and subsequent QUIC connection closure. Now the error is ignored and
path validation proceeds as usual, with several retries and a timeout.
When validating the old path after an apparent migration, that path may already
be unavailable and sendmsg() may return an error, which should not result in
QUIC connection close.
When validating the new path, it's possible that the new client address is
spoofed (See RFC 9000, 9.3.2. On-Path Address Spoofing). This address may
as well be unavailable and should not trigger QUIC connection closure.
Previously, original dcid was used to receive initial client packets in case
server initial response was lost. However, last dcid should be used instead.
These two are the same unless retry is used. In case of retry, client resends
initial packet with a new dcid, that is different from the original dcid. If
server response is lost, the client resends this packet again with the same
dcid. This is shown in RFC 9000, 7.3. Authenticating Connection IDs, Figure 8.
The issue manifested itself with creating multiple server sessions in response
to each post-retry client initial packet, if server response is lost.
Since at least f9fbeb4ee0de and certainly after 924882f42dea, which
TLS Key Update support predates, queued data output is deferred to a
posted push handler. To address timing signals after these changes,
generating next keys is now posted to run after the push handler.
Previously, NGX_AGAIN returned by ngx_quic_send() was treated by
ngx_quic_frame_sendto() as error, which triggered errors in its callers.
However, a blocked socket is not an error. Now NGX_AGAIN is passed as is to
the ngx_quic_frame_sendto() callers, which can safely ignore it.
When probe timeout expired while congestion window was exhausted, probe PINGs
could not be sent. As a result, lost packets could not be declared lost and
congestion window could not be freed for new packets. This deadlock
continued until connection idle timeout expiration.
Now PINGs are sent separately from the frame queue without congestion control,
as specified by RFC 9002, Section 7:
An endpoint MUST NOT send a packet if it would cause bytes_in_flight
(see Appendix B.2) to be larger than the congestion window, unless the
packet is sent on a PTO timer expiration (see Section 6.2) or when entering
recovery (see Section 7.3.2).
Previously, PTO handler analyzed the first packet in the sent queue for the
timeout expiration. However, the last sent packet should be analyzed instead.
An example is timeout calculation in ngx_quic_set_lost_timer().
Previously the field pnum of a potentially freed frame was accessed. Now the
value is copied to a local variable. The old behavior did not cause any
problems since the frame memory is not freed, but is moved to a free queue
instead.
In non-GSO mode, a datagram is sent if congestion window is not exceeded by the
time of send. The window could be exceeded by a small amount after the send.
In GSO mode, congestion window was checked in a similar way, but for all
concatenated datagrams as a whole. This could result in exceeding congestion
window by a lot. Now congestion window is checked for every datagram in GSO
mode as well.
Previously it was added to the tail as all other frames. However, if the
amount of queued data is large, it could delay the delivery of ACK, which
could trigger frames retransmissions and slow down the connection.
Previously ACK was not generated if max_ack_delay was not yet expired and the
number of unacknowledged ack-eliciting packets was less than two, as allowed by
RFC 9000 13.2.1-13.2.2. However this only makes sense to avoid sending ACK-only
packets, as explained by the RFC:
On the other hand, reducing the frequency of packets that carry only
acknowledgments reduces packet transmission and processing cost at both
endpoints.
Now ACK is delayed only if output frame queue is empty. Otherwise ACK is sent
immediately, which significantly improves QUIC performance with certain tests.
With this change, the NGX_OPENSSL_NO_CONFIG macro is defined when nginx
is asked to build OpenSSL itself. And with this macro automatic loading
of OpenSSL configuration (from the build directory) is prevented unless
the OPENSSL_CONF environment variable is explicitly set.
Note that not loading configuration is broken in OpenSSL 1.1.1 and 1.1.1a
(fixed in OpenSSL 1.1.1b, see https://github.com/openssl/openssl/issues/7350).
If nginx is used to compile these OpenSSL versions, configuring nginx with
NGX_OPENSSL_NO_CONFIG explicitly set to 0 might be used as a workaround.
Following OpenSSL 0.9.8f, OpenSSL tries to load application-specific
configuration section first, and then falls back to the "openssl_conf"
default section if application-specific section is not found, by using
CONF_modules_load_file(CONF_MFLAGS_DEFAULT_SECTION). Therefore this
change is not expected to introduce any compatibility issues with existing
configurations. It does, however, make it easier to configure specific
OpenSSL settings for nginx in system-wide OpenSSL configuration
(ticket #2449).
Instead of checking OPENSSL_VERSION_NUMBER when using the OPENSSL_init_ssl()
interface, the code now tests for OPENSSL_INIT_LOAD_CONFIG to be defined and
true, and also explicitly excludes LibreSSL. This ensures that this interface
is not used with BoringSSL and LibreSSL, which do not provide additional
library initialization settings, notably the OPENSSL_INIT_set_config_appname()
call.
Similarly to 6822:c045b4926b2c, environment variables introduced with
the "env" directive (and "NGINX_BPF_MAPS" added by QUIC) are now allocated
via ngx_alloc(), and explicitly freed by a cleanup handler if no longer used.
In collaboration with Sergey Kandaurov.
Previously used constant EVP_GCM_TLS_TAG_LEN had misleading name since it was
used not only with GCM, but also with CHACHAPOLY. Now a new constant
NGX_QUIC_TAG_LEN introduced. Luckily all AEAD algorithms used by QUIC have
the same tag length of 16.
Previously, computing rttvar used an updated smoothed_rtt value as per
RFC 9002, section 5.3, which appears to be specified in a wrong order.
A technical errata ID 7539 is reported.
Although it has better implementation status than HTTP/3 server push,
it remains of limited use, with adoption numbers seen as negligible.
Per IETF 102 materials, server push was used only in 0.04% of sessions.
It was considered to be "difficult to use effectively" in RFC 9113.
Its use is further limited by badly matching to fetch/cache/connection
models in browsers, see related discussions linked from [1].
Server push was disabled in Chrome 106 [2].
The http2_push, http2_push_preload, and http2_max_concurrent_pushes
directives are made obsolete. In particular, this essentially reverts
7201:641306096f5b and 7207:3d2b0b02bd3d.
[1] https://jakearchibald.com/2017/h2-push-tougher-than-i-thought/
[2] https://chromestatus.com/feature/6302414934114304
It has been deprecated since 7270:46c0c7ef4913 (1.15.0) in favour of
the "ssl" parameter of the "listen" directive, which has been available
since 2224:109849282793 (0.7.14).
The directive enables HTTP/2 in the current server. The previous way to
enable HTTP/2 via "listen ... http2" is now deprecated. The new approach
allows to share HTTP/2 and HTTP/0.9-1.1 on the same port.
For SSL connections, HTTP/2 is now selected by ALPN callback based on whether
the protocol is enabled in the virtual server chosen by SNI. This however only
works since OpenSSL 1.0.2h, where ALPN callback is invoked after SNI callback.
For older versions of OpenSSL, HTTP/2 is enabled based on the default virtual
server configuration.
For plain TCP connections, HTTP/2 is now auto-detected by HTTP/2 preface, if
HTTP/2 is enabled in the default virtual server. If preface is not matched,
HTTP/0.9-1.1 is assumed.
Previously, rec.level field was not uninitialized in SSL_provide_quic_data().
As a result, its value was always ssl_encryption_initial. Later in
ngx_quic_ciphers() such level resulted in resetting the cipher to
TLS1_3_CK_AES_128_GCM_SHA256 and using AES128 to encrypt the packet.
Now the level is initialized and the right cipher is used.
The layer is enabled as a fallback if the QUIC support is configured and the
BoringSSL API wasn't detected, or when using the --with-openssl option, also
compatible with QuicTLS and LibreSSL. For the latter, the layer is assumed
to be present if QUIC was requested, so it needs to be undefined to prevent
QUIC API redefinition as appropriate.
A previously used approach to test the TLSEXT_TYPE_quic_transport_parameters
macro doesn't work with OpenSSL 3.2 master branch where this macro appeared
with incompatible QUIC API. To fix the build there, the test is revised to
pass only for QuicTLS and LibreSSL.
Previously, ngx_quic_close_connection() could be called in a way that QUIC
connection was accessed after the call. In most cases the connection is not
closed right away, but close timeout is scheduled. However, it's not always
the case. Also, if the close process started earlier for a different reason,
calling ngx_quic_close_connection() may actually close the connection. The
connection object should not be accessed after that.
Now, when possible, return statement is added to eliminate post-close connection
object access. In other places ngx_quic_close_connection() is substituted with
posting close event.
Also, the new way of closing connection in ngx_quic_stream_cleanup_handler()
fixes another problem in this function. Previously it passed stream connection
instead of QUIC connection to ngx_quic_close_connection(). This could result
in incomplete connection shutdown. One consequence of that could be that QUIC
streams were freed without shutting down their application contexts. This could
result in another use-after-free.
Found by Coverity (CID 1530402).
The qsock->sockaddr field is a ngx_sockaddr_t union, and therefore can hold
any sockaddr (and union members, such qsock->sockaddr.sockaddr, can be used
to access appropriate variant of the sockaddr). It is better to set it via
qsock->sockaddr itself though, and not qsock->sockaddr.sockaddr, so static
analyzers won't complain about out-of-bounds access.
Prodded by Coverity (CID 1530403).
Previously, ngx_udp_rbtree_insert_value() was used for plain UDP and
ngx_quic_rbtree_insert_value() was used for QUIC. Because of this it was
impossible to initialize connection tree in ngx_create_listening() since
this function is not aware what kind of listening it creates.
Now ngx_udp_rbtree_insert_value() is used for both QUIC and UDP. To make
is possible, a generic key field is added to ngx_udp_connection_t. It keeps
client address for UDP and connection ID for QUIC.
The directive used to set the value of the "max_udp_payload_size" transport
parameter. According to RFC 9000, Section 18.2, the value specifies the size
of buffer for reading incoming datagrams:
This limit does act as an additional constraint on datagram size in
the same way as the path MTU, but it is a property of the endpoint
and not the path; see Section 14. It is expected that this is the
space an endpoint dedicates to holding incoming packets.
Current QUIC implementation uses the maximum possible buffer size (65527) for
reading datagrams.
HTTP and Stream variables $remote_addr and $binary_remote_addr rely on
constant client address, particularly because they are cacheable.
However, QUIC client may migrate to a new address. While there's no perfect
way to handle this, the proposed solution is to copy client address to QUIC
stream at stream creation.
The change also fixes truncated $remote_addr if migration happened while the
stream was active. The reason is addr_text string was copied to stream by
value.
The existing logic to evaluate multi header "$sent_http_*" variables,
such as $sent_http_cache_control, as previously introduced in 1.23.0,
doesn't take into account that one or more elements can be cleared,
yet still present in a linked list, pointed to by the next field.
Such elements don't contribute to the resulting variable length, an
attempt to append a separator for them ends up in out of bounds write.
This is not possible with standard modules, though at least one third
party module is known to override multi header values this way, so it
makes sense to harden the logic.
The fix restores a generic boundary check.
Previously, the value was not set and remained zero. While in nginx code the
value of c->sockaddr is accessed without taking c->socklen into account,
invalid c->socklen could lead to unexpected results in third-party modules.
Previously, the post-migration value of addr_text could be truncated, if
it was longer than the previous one. Also, the new value always included
port, which should not be there.
According to RFC 9000, 8.2.4. Failed Path Validation,
the following value is recommended as a validation timeout:
A value of three times the larger of the current PTO
or the PTO for the new path (using kInitialRtt, as
defined in [QUIC-RECOVERY]) is RECOMMENDED.
The change adds PTO of the new path to the equation as the lower bound.
Path validation packets containing PATH_CHALLENGE frames are sent separately
from regular frame queue, because of the need to use a decicated path and pad
the packets. The packets are sent periodically, separately from the regular
probe/lost detection mechanism. A path validation packet is resent up to 3
times, each time after PTO expiration, with increasing per-path PTO backoff.
The check is needed for clients in order to unblock a server due to
anti-amplification limits, and it seems to make no sense for servers.
See RFC 9002, A.6 and A.8 for a further explanation.
This makes max_ack_delay to now always account, notably including
PATH_CHALLENGE timers as noted in the last paragraph of 9000, 9.4,
unlike when it was only used when there are packets in flight.
While here, fixed nearby style.
Previously, ssl_encryption_application was hardcoded. Before 9553eea74f2a,
ngx_quic_frame_sendto() was used only for PATH_CHALLENGE/PATH_RESPONSE sent
at the application level only. Since 9553eea74f2a, ngx_quic_frame_sendto()
is also used for CONNECTION_CLOSE, which can be sent at initial level after
SSL handshake error or rejection. This resulted in packet encryption error.
Now level is copied from frame, which fixes the error.
Previously, before sending CONNECTION_CLOSE to client, all pending frames
were sent. This is redundant and could prevent CONNECTION_CLOSE from being
sent due to congestion control. Now pending frames are freed and
CONNECTION_CLOSE is sent without congestion control, as advised by RFC 9002:
Packets containing frames besides ACK or CONNECTION_CLOSE frames
count toward congestion control limits and are considered to be in flight.
Do not corrupt frame data chain pointer on ngx_quic_read_buffer() error.
The error leads to closing a QUIC connection where the frame may be used
as part of the QUIC connection tear down, which envolves writing pending
frames, including this one.
The rcf->studies list is unconditionally accessed by ngx_regex_cleanup(),
and this used to cause NULL pointer dereference if allocation
failed. Fix is to set cleanup handler only when allocation succeeds.
Previously, waiting on a shared connection was not allowed, because the only
type of such connection was plain UDP. However, QUIC stream connections are
also shared since they share socket descriptor with the listen connection.
Meanwhile, it's perfectly normal to wait on such connections.
The issue manifested itself with stream write errors when the amount of data
exceeded stream buffer size or flow control. Now no error is triggered
and Stream write module is allowed to wait for buffer space to become available.
When a stream is created by client, it's often the case that nginx will send
immediate response on that stream. An example is HTTP/3 request stream, which
in most cases quickly replies with at least HTTP headers.
QUIC stream init handlers are called from a posted event. Output QUIC
frames are also sent to client from a posted event, called the push event.
If the push event is posted before the stream init event, then output produced
by stream may trigger sending an extra UDP datagram. To address this, push
event is now re-posted when a new stream init event is posted.
An example is handling 0-RTT packets. Client typically sends an init packet
coalesced with a 0-RTT packet. Previously, nginx replied with a padded CRYPTO
datagram, followed by a 1-RTT stream reply datagram. Now CRYPTO and STREAM
packets are coalesced in one reply datagram, which saves bandwidth.
Other examples include coalescing 1-RTT first stream response, and
MAX_STREAMS/STREAM sent in response to ACK/STREAM.
It now uses custom alloc_aligned() wrapper for all allocations,
therefore all allocations are larger than expected by (64 + sizeof(void*)).
Further, they are seen as allocations of 1 element. Relevant calculations
were adjusted to reflect this, and state allocation is now protected
with a flag to avoid misinterpreting other allocations as the zlib
deflate_state allocation.
Further, it no longer forces window bits to 13 on compression level 1,
so the comment was adjusted to reflect this.
When establishing a connection to the backend, nginx blocks reading
from the client with ngx_mail_proxy_block_read(). Previously, such
events were lost, and in some cases this resulted in connection hangs.
Notably, this affected mail_imap_ssl.t on Windows, since the test
closes connections after requesting authentication, but without
waiting for any responses (so the connection close events might be
lost).
Fix is to post an event to read from the client after connecting to
the backend if there were blocked events.
SSL context is not present if the default server has neither certificates nor
ssl_reject_handshake enabled. Previously, this led to null pointer dereference
before it would be caught with configuration checks.
Additionally, non-default servers with distinct SSL contexts need to initialize
compatibility layer in order to complete a QUIC handshake.
This ensures that errors which happen during logging to syslog are logged
with proper context, such as "while logging to syslog" and the server name.
Prodded by Safar Safarly.
During initial startup the ngx_cycle->hostname is not available, and
previously this resulted in incorrect logging. Instead, hostname from the
configuration being parsed is now preserved in the syslog peer structure
and then used during logging.
Similarly, ngx_cycle->log might not match the configuration where the
syslog peer is defined if the configuration is not yet fully applied,
and previously this resulted in unexpected logging of syslog errors
and debug information. Instead, cf->cycle->new_log is now referenced
in the syslog peer structure and used for logging, similarly to how it
is done in other modules.
Similarly to ticket #274 (7354:1812f1d79d84), early request finalization
without calling ngx_http_run_posted_requests() resulted in a connection
hang (a socket leak) if the 400 (Bad Request) error was generated in
ngx_http_v2_state_process_header() due to invalid request headers and
"return 444" was used in error_page 400.
As tested with tlsfuzzer with LibreSSL 3.7.0, the following errors are
certainly client-related:
SSL_do_handshake() failed (SSL: error:14026073:SSL routines:ACCEPT_SR_CLNT_HELLO:bad packet length)
SSL_do_handshake() failed (SSL: error:1402612C:SSL routines:ACCEPT_SR_CLNT_HELLO:ssl3 session id too long)
SSL_do_handshake() failed (SSL: error:140380EA:SSL routines:ACCEPT_SR_KEY_EXCH:tls rsa encrypted value length is wrong)
Accordingly, the SSL_R_BAD_PACKET_LENGTH ("bad packet length"),
SSL_R_SSL3_SESSION_ID_TOO_LONG ("ssl3 session id too long"),
SSL_R_TLS_RSA_ENCRYPTED_VALUE_LENGTH_IS_WRONG ("tls rsa encrypted value
length is wrong") errors are now logged at the "info" level.
To further differentiate client-related errors and adjust logging levels
of various SSL errors, nginx was tested with tlsfuzzer with multiple
OpenSSL versions (3.1.0-beta1, 3.0.8, 1.1.1t, 1.1.0l, 1.0.2u, 1.0.1u,
1.0.0s, 0.9.8zh).
The following errors were observed during tlsfuzzer runs with OpenSSL 3.0.8,
and are clearly client-related:
SSL_do_handshake() failed (SSL: error:0A000092:SSL routines::data length too long)
SSL_do_handshake() failed (SSL: error:0A0000A0:SSL routines::length too short)
SSL_do_handshake() failed (SSL: error:0A000124:SSL routines::bad legacy version)
SSL_do_handshake() failed (SSL: error:0A000178:SSL routines::no shared signature algorithms)
Accordingly, the SSL_R_DATA_LENGTH_TOO_LONG ("data length too long"),
SSL_R_LENGTH_TOO_SHORT ("length too short"), SSL_R_BAD_LEGACY_VERSION
("bad legacy version"), and SSL_R_NO_SHARED_SIGNATURE_ALGORITHMS
("no shared signature algorithms", misspelled as "sigature" in OpenSSL 1.0.2)
errors are now logged at the "info" level.
Additionally, the following errors were observed with OpenSSL 3.0.8 and
with TLSv1.3 enabled:
SSL_do_handshake() failed (SSL: error:0A00006F:SSL routines::bad digest length)
SSL_do_handshake() failed (SSL: error:0A000070:SSL routines::missing sigalgs extension)
SSL_do_handshake() failed (SSL: error:0A000096:SSL routines::encrypted length too long)
SSL_do_handshake() failed (SSL: error:0A00010F:SSL routines::bad length)
SSL_read() failed (SSL: error:0A00007A:SSL routines::bad key update)
SSL_read() failed (SSL: error:0A000125:SSL routines::mixed handshake and non handshake data)
Accordingly, the SSL_R_BAD_DIGEST_LENGTH ("bad digest length"),
SSL_R_MISSING_SIGALGS_EXTENSION ("missing sigalgs extension"),
SSL_R_ENCRYPTED_LENGTH_TOO_LONG ("encrypted length too long"),
SSL_R_BAD_LENGTH ("bad length"), SSL_R_BAD_KEY_UPDATE ("bad key update"),
and SSL_R_MIXED_HANDSHAKE_AND_NON_HANDSHAKE_DATA ("mixed handshake and non
handshake data") errors are now logged at the "info" level.
Additionally, the following errors were observed with OpenSSL 1.1.1t:
SSL_do_handshake() failed (SSL: error:14094091:SSL routines:ssl3_read_bytes:data between ccs and finished)
SSL_do_handshake() failed (SSL: error:14094199:SSL routines:ssl3_read_bytes:too many warn alerts)
SSL_read() failed (SSL: error:1408F0C6:SSL routines:ssl3_get_record:packet length too long)
SSL_read() failed (SSL: error:14094085:SSL routines:ssl3_read_bytes:ccs received early)
Accordingly, the SSL_R_CCS_RECEIVED_EARLY ("ccs received early"),
SSL_R_DATA_BETWEEN_CCS_AND_FINISHED ("data between ccs and finished"),
SSL_R_PACKET_LENGTH_TOO_LONG ("packet length too long"), and
SSL_R_TOO_MANY_WARN_ALERTS ("too many warn alerts") errors are now logged
at the "info" level.
Additionally, the following errors were observed with OpenSSL 1.0.2u:
SSL_do_handshake() failed (SSL: error:1407612A:SSL routines:SSL23_GET_CLIENT_HELLO:record too small)
SSL_do_handshake() failed (SSL: error:1408C09A:SSL routines:ssl3_get_finished:got a fin before a ccs)
Accordingly, the SSL_R_RECORD_TOO_SMALL ("record too small") and
SSL_R_GOT_A_FIN_BEFORE_A_CCS ("got a fin before a ccs") errors are now
logged at the "info" level.
No additional client-related errors were observed while testing with
OpenSSL 3.1.0-beta1, OpenSSL 1.1.0l, OpenSSL 1.0.1u, OpenSSL 1.0.0s,
and OpenSSL 0.9.8zh.
In some cases there might be multiple errors in the OpenSSL error queue,
notably when a libcrypto call fails, and then the SSL layer generates
an error itself. For example, the following errors were observed
with OpenSSL 3.0.8 with TLSv1.3 enabled:
SSL_do_handshake() failed (SSL: error:02800066:Diffie-Hellman routines::invalid public key error:0A000132:SSL routines::bad ecpoint)
SSL_do_handshake() failed (SSL: error:08000066:elliptic curve routines::invalid encoding error:0A000132:SSL routines::bad ecpoint)
SSL_do_handshake() failed (SSL: error:0800006B:elliptic curve routines::point is not on curve error:0A000132:SSL routines::bad ecpoint)
In such cases it seems to be better to determine logging level based on
the last error in the error queue (the one added by the SSL layer,
SSL_R_BAD_ECPOINT in all of the above example example errors). To do so,
the ngx_ssl_connection_error() function was changed to use
ERR_peek_last_error().
An UTF-8 octet sequence cannot start with a 11111xxx byte (above 0xf8),
see https://datatracker.ietf.org/doc/html/rfc3629#section-3. Previously,
such bytes were accepted by ngx_utf8_decode() and misinterpreted as 11110xxx
bytes (as in a 4-byte sequence). While unlikely, this can potentially cause
issues.
Fix is to explicitly reject such bytes in ngx_utf8_decode().
Just a drive letter might not correctly represent file system being used,
notably when using symlinks (as created by "mklink /d"). As such, instead
of trying to call GetDiskFreeSpace() with just a drive letter, we now always
use GetDiskFreeSpace() with full path.
Further, it looks like the code to use just a drive letter never worked,
since it tried to test name[2] instead of name[1] to be ':'.
This ensures that ngx_win32_rename_file() will support non-ASCII names
when supported by the wrappers.
Notably, this is used by PUT requests in the dav module when overwriting
existing files with non-ASCII names (ticket #1433).
Previously, ngx_win32_rename_file() retried on all errors returned by
MoveFile() to a temporary name. It only make sense, however, to retry
when the destination file already exists, similarly to the condition
when ngx_win32_rename_file() is called. Retrying on other errors is
meaningless and might result in an infinite loop.
This makes it possible to create directories under prefix with non-ASCII
characters, as well as makes it possible to create directories with non-ASCII
characters when using the dav module (ticket #1433).
To ensure that the dav module operations are restricted similarly to
other file operations (in particular, short names are not allowed), the
ngx_win32_check_filename() function is used. It improved to support
checking of just dirname, and now can be used to check paths when creating
files or directories.
Notably, ngx_open_dir() now supports opening directories with non-ASCII
characters, and directory entries returned by ngx_read_dir() are properly
converted to UTF-8.
This is expected to help with clients using pipelining with some constant
depth, such as apt[1][2].
When downloading many resources, apt uses pipelining with some constant
depth, a number of requests in flight. This essentially means that after
receiving a response it sends an additional request to the server, and
this can result in requests arriving to the server at any time. Further,
additional requests are sent one-by-one, and can be easily seen as such
(neither as pipelined, nor followed by pipelined requests).
The only safe approach to close such connections (for example, when
keepalive_requests is reached) is with lingering. To do so, now nginx
monitors if pipelining was used on the connection, and if it was, closes
the connection with lingering.
[1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=973861#10
[2] https://mailman.nginx.org/pipermail/nginx-devel/2023-January/ZA2SP5SJU55LHEBCJMFDB2AZVELRLTHI.html
Since 4611:2b6cb7528409 responses from the gzip static, flv, and mp4 modules
can be used with subrequests, though empty files were not properly handled.
Empty gzipped, flv, and mp4 files thus resulted in "zero size buf in output"
alerts. While valid corresponding files are not expected to be empty, such
files shouldn't result in alerts.
Fix is to set b->sync on such empty subrequest responses, similarly to what
ngx_http_send_special() does.
Additionally, the static module, the ngx_http_send_response() function, and
file cache are modified to do the same instead of not sending the response
body at all in such cases, since not sending the response body at all is
believed to be at least questionable, and might break various filters
which do not expect such behaviour.
The "listen" directive in the http module can be used multiple times
in different server blocks. Originally, it was supposed to be specified
once with various socket options, and without any parameters in virtual
server blocks. For example:
server { listen 80 backlog=1024; server_name foo; ... }
server { listen 80; server_name bar; ... }
server { listen 80; server_name bazz; ... }
The address part of the syntax ("address[:port]" / "port" / "unix:path")
uniquely identifies the listening socket, and therefore is enough for
name-based virtual servers (to let nginx know that the virtual server
accepts requests on the listening socket in question).
To ensure that listening options do not conflict between virtual servers,
they were allowed only once. For example, the following configuration
will be rejected ("duplicate listen options for 0.0.0.0:80 in ..."):
server { listen 80 backlog=1024; server_name foo; ... }
server { listen 80 backlog=512; server_name bar; ... }
At some point it was, however, noticed, that it is sometimes convenient
to repeat some options for clarity. In nginx 0.8.51 the "ssl" parameter
was allowed to be specified multiple times, e.g.:
server { listen 443 ssl backlog=1024; server_name foo; ... }
server { listen 443 ssl; server_name bar; ... }
server { listen 443 ssl; server_name bazz; ... }
This approach makes configuration more readable, since SSL sockets are
immediately visible in the configuration. If this is not needed, just the
address can still be used.
Later, additional protocol-specific options similar to "ssl" were
introduced, notably "http2" and "proxy_protocol". With these options,
one can write:
server { listen 443 ssl backlog=1024; server_name foo; ... }
server { listen 443 http2; server_name bar; ... }
server { listen 443 proxy_protocol; server_name bazz; ... }
The resulting socket will use ssl, http2, and proxy_protocol, but this
is not really obvious from the configuration.
To emphasize such misleading configurations are discouraged, nginx now
warns as long as the "listen" directive is used with options different
from the options previously used if this is potentially confusing.
In particular, the following configurations are allowed:
server { listen 8401 ssl backlog=1024; server_name foo; }
server { listen 8401 ssl; server_name bar; }
server { listen 8401 ssl; server_name bazz; }
server { listen 8402 ssl http2 backlog=1024; server_name foo; }
server { listen 8402 ssl; server_name bar; }
server { listen 8402 ssl; server_name bazz; }
server { listen 8403 ssl; server_name bar; }
server { listen 8403 ssl; server_name bazz; }
server { listen 8403 ssl http2; server_name foo; }
server { listen 8404 ssl http2 backlog=1024; server_name foo; }
server { listen 8404 http2; server_name bar; }
server { listen 8404 http2; server_name bazz; }
server { listen 8405 ssl http2 backlog=1024; server_name foo; }
server { listen 8405 ssl http2; server_name bar; }
server { listen 8405 ssl http2; server_name bazz; }
server { listen 8406 ssl; server_name foo; }
server { listen 8406; server_name bar; }
server { listen 8406; server_name bazz; }
And the following configurations will generate warnings:
server { listen 8501 ssl http2 backlog=1024; server_name foo; }
server { listen 8501 http2; server_name bar; }
server { listen 8501 ssl; server_name bazz; }
server { listen 8502 backlog=1024; server_name foo; }
server { listen 8502 ssl; server_name bar; }
server { listen 8503 ssl; server_name foo; }
server { listen 8503 http2; server_name bar; }
server { listen 8504 ssl; server_name foo; }
server { listen 8504 http2; server_name bar; }
server { listen 8504 proxy_protocol; server_name bazz; }
server { listen 8505 ssl http2 proxy_protocol; server_name foo; }
server { listen 8505 ssl http2; server_name bar; }
server { listen 8505 ssl; server_name bazz; }
server { listen 8506 ssl http2; server_name foo; }
server { listen 8506 ssl; server_name bar; }
server { listen 8506; server_name bazz; }
server { listen 8507 ssl; server_name bar; }
server { listen 8507; server_name bazz; }
server { listen 8507 ssl http2; server_name foo; }
server { listen 8508 ssl; server_name bar; }
server { listen 8508; server_name bazz; }
server { listen 8508 ssl backlog=1024; server_name foo; }
server { listen 8509; server_name bazz; }
server { listen 8509 ssl; server_name bar; }
server { listen 8509 ssl backlog=1024; server_name foo; }
The basic idea is that at most two sets of protocol options are allowed:
the main one (with socket options, if any), and a shorter one, with options
being a subset of the main options, repeated for clarity. As long as the
shorter set of protocol options is used, all listen directives except the
main one should use it.
Now "listen" directve has a new "quic" parameter which enables QUIC protocol
for the address. Further, to enable HTTP/3, a new directive "http3" is
introduced. The hq-interop protocol is enabled by "http3_hq" as before.
Now application protocol is chosen by ALPN.
Previously used "http3" parameter of "listen" is deprecated.
A QUIC handshake failure breaks down into several cases:
- a handshake error which leads to a send_alert call
- an error triggered by the add_handshake_data callback
- internal errors (allocation etc)
Previously, in the first case, only error code was set in the send_alert
callback. Now the "handshake failed" reason phrase is set there as well.
In the second case, both code and reason are set by add_handshake_data.
In the last case, setting reason phrase is removed: returning NGX_ERROR
now leads to closing the connection with just INTERNAL_ERROR.
Reported by Jiuzhou Cui.
Previously, since 3550b00d9dc8, the token was allocated on stack, to get
rid of pool usage. Now the token is allocated by ngx_quic_copy_buffer()
in QUIC buffers, also used for STREAM, CRYPTO and ACK frames.
Previously, location prefix length in ngx_http_location_tree_node_t was
stored as "u_char", and therefore location prefixes longer than 255 bytes
were handled incorrectly.
Fix is to use "u_short" instead. With "u_short", prefixes up to 65535 bytes
can be safely used, and this isn't reachable due to NGX_CONF_BUFFER, which
is 4096 bytes.
In contrast to on-the-fly gzipping with gzip filter, static gzipped
representation as returned by gzip_static is persistent, and therefore
the same binary representation is available for future requests, making
it possible to use range requests.
Further, if a gzipped representation is re-generated with different
compression settings, it is expected to result in different ETag and
different size reported in the Content-Range header, making it possible
to safely use range requests anyway.
As such, ranges are now allowed for files returned by gzip_static.
