When a primary cache hit has a Vary secondary_key_signature, the code calls
retain_entry() and shctx_row_detach() before performing the secondary lookup.
If get_secondary_entry() returns NULL (no stored variant matches), res is set
to NULL and the function falls through to return ACT_RET_CONT without calling
release_entry() or shctx_row_reattach(). Each such request leaks one refcount
and pins one shctx row permanently, eventually exhausting the cache.
In order to fix this we must do like when no secondary key could be built
and release everything. We only reattach to the row if we previously
detached.
The issue was introduced in 2.4 with commit 1785f3dd9 ("MEDIUM: cache: Add
the Vary header support"). The code changed a bit in 2.9 with commit
48f81ec09 ("MAJOR: cache: Delay cache entry delete in reserve_hot function"),
so in order to backport to 2.8 and older, the patch will have to be manually
applied (no test on detached).
In dict_insert(), when ebis_insert() returns an existing node n indicating
that another thread inserted the same key concurrently, the code freed its
own newly-allocated entry and returned the winner without bumping its
refcount. Both callers then held a reference with refcount=1 instead of 2,
so when one expires the other becomes a use-after-free or double-free.
The bug likely comes from the fact that new_dict_entry() creates an entry
with a refcount preset to 1 (saves an atomic op) and that because of this
there is no refcount increment upon a successful insertion in the tree,
resulting in requiring different code paths for collision and normal
insertion.
A simple fix consists in bumping the refcount under the lock and unlocking
only at the end, but this would mean performing two free() calls under a
lock, which we always try to avoid. The code was slightly rearranged so
that we can now bump the existing entry's refcount under the lock in case
of duplicate, or unlock immediately in the common case, so that the free()
call is done out of the lock.
The probably of the race is very low (at peers connection setup only),
reason why it's marked low. This should be backported to all versions.
In parse_log_message(), the first loop looks for '>' that finishes the
priority field, and unfortunately it stops once it has checked the first
byte after the end of the buffer. This means that a priority made only
of digits for the whole buffer would read one extra byte. In practice
since pools have a tag at the end this is only detectable when using ASAN,
but this should be fixed nevertheless.
This can be backported to all versions.
It's worth noting that RFC5424 now says that the PRI field is 1..3
digits only, so maybe at some point we could seriously limit the
length as well.
In 2.3, in preparation for log forwarding, commit 546488559 ("MEDIUM:
log/sink: re-work and merge of build message API.") extended the log
send API to be able to use metadata from an existing header. However
the month number is parsed from the passed meta-data and compared
against 11 but there's no check for negative values which could in
theory cause a negative monthname[] index.
It can be a problem when the date is received as RFC5424 and forced
to RFC3164 because certain characters in the month field could result
in a negative month value. Let's fix it by turning the month to unsigned
to make sure we only accept months 0..11.
This should be backported to all branches.
In 2.5, commit da0264a96 ("MINOR: sample: Add be2hex converter")
introduced the be2hex() converter, which reads input data of a given
chunk size, processes it as a big endian block and turns it to hex
output.
There's an issue if the configured chunk_size (2nd argument) is larger
than tune.bufsize/2, because the max_size calculation will underflow,
and the later loop will always match since it compares a size_t to an
int (BTW, compilers love to annoy us with useless warnings but I never
found how to see some for these ones). This can result in overflowing
the output trash if the input sample is at least as large as half a
buffer.
Let's add an explicit check for this, and change the max_size type to
size_t so that the comparison is always right. While we're at it, let's
ask the trash buffer to be twice as large, just like bin2hex() does, as
it may result in offering a larger buffer in 3.4. thanks to the large
buffers support.
Despite the risk, this is marked as minor because a config with that
large an argument in the converter makes absolutely no sense.
This should be backported to 2.6. The *2 for the trash allocation will
conflict and have to be dropped in stable versions, which is safe.
When the PRIORITY flag is present on a HEADERS frame, the frame must
contain a stream dependency and a weight, for a total of 5 bytes. The
length is checked after reading the stream dep field so theoretically
such a frame could cause up to 4-byte OOB read at the end of the buffer,
though in practice buffers allocated from pools never end on a page
boundary (one extra word at the end) and the anomaly is still detected
after reading the stream ID and the connection aborted with the glitch
count incremented. Thus while not technically correct, practically
speaking it's harmless.
This should be backported to all stable releases.
The previous fix 75f72c2eb ("BUG/MEDIUM: resolvers: Fix test on dn label
size in resolv_dn_label_to_str()") may still leave garbage from the input
buffer into the response: if a component length is passed as zero, it
should mark the end, but instead a dot will be emitted, and whatever
follows it in the input buffer would continue to be appended as extra
components. While having no direct consequences beyond the domain not
being properly decoded, it could at least complicate troubleshooting.
This should be backported where the fix above is backported.
The previous fix 75f72c2eb ("BUG/MEDIUM: resolvers: Fix test on dn label
size in resolv_dn_label_to_str()") can still be fooled by an input exactly
the size of str_len, in which case the trailing zero appended at the end
was not being accounted for. Let's add 1 to the condition to prepare for
it.
This needs to be backported wherever the fix above is backported.
When lf_expr_compile() fails in cfg_parse_log_profile, the code leaves
without freeing the previously strdup()'d strings in target_lf->str and
target_lf->conf.file. Let's add a call to lf_expr_deinit() there to
release it.
It was harmless anyway since the startup will abort when this happens,
but better clean it because with increasingly dynamic setups, one day
it could become a runtime leak.
No backport is needed.
In resolv_validate_dns_response(), when matching an additional A/AAAA
record to an SRV record, the code checked tmp_record->ar_item == NULL
then called pool_free(resolv_answer_item_pool, tmp_record->ar_item).
This is a copy-paste mistake from similar patterns elsewhere since
the pointer is confirmed to be NULL a few lines above, so let's just
drop the confusing pool_free.
Both boundary checks in the authority record parsing loop of
resolv_validate_dns_response() use >= bufend where they should use
> bufend, causing valid DNS responses with exactly enough bytes to be
rejected as invalid.
The first one, "reader + offset + 10 >= bufend" is too strict since it
prevents 10-byte responses from being accepted as valid while they
are. The second one, "reader + len >= bufend" has the same issue, when
exactly len bytes remain, the check rejects it even though dns_max_name()
already validated it. It may be backported though it is unlikely to ever
be noticed.
In resolv_validate_dns_response(), the second DNS record parsing path
manually constructs a 32-bit big-endian TTL value from four individual
bytes using the expression:
reader[0] * 16777216 + reader[1] * 65536 + reader[2] * 256 + reader[3]
We have read_n32() to do this, and it's more robust against unexpected
signedness surprises (which should not happen right here since reader is
unsigned char and we use -fwrapv so the result is defined). Also, let's
make the ttl an uint instead of an int. The TTL is only retrieved and not
used for now, so better clean it now.
acme_res_certificate() passes the httpclient response buffer to
ssl_sock_load_pem_into_ckch(), which will then call BIO_new_mem_buf(buf, -1).
The "-1" flag will make the OpenSSL PEM parser determine the length by
using strlen(). However, the httpclient populates the response buffer with
__b_putblk() without writing a trailing NUL to it. The byte at area[data]
is whatever data previously resided there in the memory pool.
Thus, a malicious or compromised ACME CA can perform an arbitrary-length
out-of-bounds read until hitting the first NULL byte past the response
body. The OpenSSL PEM loader will try to iterate to load the chain
certificates, thus the PEM-looking garbage found in freed memory chunks
can be erroneously loaded as additional intermediate certificates. The
presence of a single NUL inside the valid response body will result in
silent truncation of the certificate.
Make sure that the area[data] contains a terminating NULL before passing
the buffer to the parser. Fail on insufficient room for the NUL terminator.
No backport required: The ACME client has been added in 3.x and this
code path didn't exist in 2.x.
When the dedidacted buffer to store the command payload was added (c5ae0da62
"MEDIUM: cli: Make a buffer for the command payload"), an bug was
introduced. When the pattern finishing the command payload is found, it is
removed from the buffer. A NULL-bytes is added before it, skipping the
previous newline character.
It worked well in all cases before the commit above, because the commandline
was already parsed and was placed at the beginning of the cmdline buffer.
So, there is always a line before the payload.
Now, the payload is stored in a dedicated buffer. So there is nothing
preceeding it in a buffer. If the payload is empty, we cannot rewind to the
previous line to set the NULL-byte character. We must handle this case to
avoid integer underflow on the payload buffer length.
It is a 3.4-specific bug. No backport needed.
In hlua_socket_receive_yield(), when we try to get a line, the trailing CRLF is
stripped by decrementing the block length. The '\n' is first skipped, then,
possible a preceeding '\r'. But the block lenght is never checked. If an empty
line is returned, this leads to an integer underflow and most probably to a
crash because this length is used to copy data into a LUA string.
To fix the issue, the block length is now properly tested against 0 before
decrementing it.
This patch must be backported to all stable versions.
When parsing the agent-check reply, we first loop on the response to find
the newline character, to add a NULL-byte at the end of the line. However,
this loop is not bounded to the data available in the buffer. So it is
possible to read bytes outside the buffer and eventually write a NULL-byte
ouside the buffer.
So let's check for the end of the buffer when looping on the agent-check
reply.
This patch must be backported to all stable versions.
In dict_entry_unref(), the write lock on d->rwlock was only acquired after
decrementing the refcount. However, between the decrement and the lock,
another thread could increment it by calling dict_insert(). That could lead
to a UAF.
To fix the issue, the call to HA_ATOMIC_SUB_FETCH is moved inside the write
lock.
This patch must be backported to all stable versions.
In haproxy, when an Initial packet is received, a new connection may be
created and a DCID must be attributed. This CID is derived from the
original DCID used by the client in its first packet. This is an
optimization to avoid storing two CIDs values in the CID tree.
On CID lookup, if the DCID used is not found, derivation is performed
again. This should permit to retrieve the DCID node. However, this
operation is not performed as expected in quic_get_cid_tid(), as the
wrong value is used on the second lookup. Fix this function by using
derive CID for it. Note that retrieve_qc_conn_from_cid() performs the
same lookup but the bug was not present there.
The impact of this bug is relatively low as most clients send a single
Initial packet. Even in case of multiple packets in a single datagram,
this does not cause any issue as the current thread is assigned as
default.
This should be backported up to 2.8.
In ssl_sock_generate_certificate(), if the LRU cache for generated
certificates is used, the LRU tree is not unlocked on cache miss if the
certificate generation failed. So let's unlock it on error path.
The bug was introduced by the commit fbc98ebcd ("BUG/MEDIUM: ssl: fix error
path on generate-certificates"). So this patch must be backported with the
commit above, so to all stable versions.
In resolv_dn_label_to_str(), size for a dn label was stored into an integer
from a signed char without a cast to unsigned. So dn label with a size of
128 bytes or more become negative, skipping this way the copy loop and
desynchronizing input vs output.
In addition, the size of the destination string was only checked at the
begining, against the dn string length. But it must also be checked for
every dn label, to be sure. The dn string can be forged to copied more bytes
than expected.
This patch must be backported to all stable versions.
when appctx_rcv_buf() function was called to get data from the applet, but
to get zero bytes, nothing was performed and the function early
returned. However, we must at least take care to set SE_FL_WANT_ROOM if
necessary. Otherwise, if data are still blocked in the applet's output
buffer while the EOI/EOS are pending, the information can be reported to the
upper layer and remaining data can be lost.
Indeed, in such case, SE_FL_WANT_ROOM flag is here to specify the applet has
more data to deliver. Thanks to this flag, the stream will wait before
closing. But when appctx_rcv_buf() function is called, this flag is removed by
the stconn. It is the function responsibility to set it again when necessary.
This patch should fix second part of the issue #3366. It must be backported
to 3.0.
Completes qmux_parse_frm() to ensure every frames allowed by QMux
protocol are listed. For now, nothing is implemented except a CHECK_IF()
to report such events.
This is necessary to prevent a crash on abort. Frames not supported by
QMux should already have been rejected prior via qmux_is_frm_valid().
Handle reception of a MAX_STREAMS frame for unidirectional stream usage
when using QMux. This simply consists in using qcc_recv_max_streams() as
with QUIC protocol.
Ensure reception of STOP_SENDING via QMux protocol is properly handled.
This simply consists in using qcc_recv_stop_sending() which will update
the associated QCS if found.
The check on the OCSP response expire time is based on the "Next Update"
field of the response, converted by my_timegm function that returns a
time_t (signed long). It is then stored in the 'expire' field of the
certificate_ocsp structure which is typed as a signed long.
When loading an OCSP response, if the "Next Update" time is too far in
the future and we are running on a 32 bits machine, we might end up with
negative times ireturned by my_timegm, which make the comparison with
the current date fail and raises the "OCSP single response: no longer
valid." error message.
This problem typically happens in the ocsp_auto_update.vtc regtest since
the loaded OCSP response have a "Next Update" field in 2050.
This patch simply changes the type of the expire field to an unsigned
long since the 'my_timegm' function does not return '-1' in case of
error, contrary to the standard 'timegm' one.
Ths patch can be backported to all stable branches.
Use xprt_add_l6hs() at the end of connect_server() if selected MUX layer
relies on a temporary handshake prior to its initialization. This
functions is noop is SSL layer is active.
This change is necessary to support clear QMux on the backend side.
Recently defined <init_xprt> from mux_proto_list is used to render the
code as generic as possible.
Activates xprt_qmux layer if necessary via session_accept_fd(). This is
necessary to be able to support QMux in clear. This operation is noop if
SSL is active, as in this case xprt_qmux will be activated after the SSL
handshake completion.
To ensure MUX init is delayed when running with clear QMux, mask
CO_FL_WAIT_XPRT_L6 is added to test if the embryonic task must be
started instead.
Define a new connection flag mask CO_FL_WAIT_XPRT_L6. This will be used
to indicate that a XPRT layer is running on top of layer 6. For now,
only xprt_qmux implements this method of operation.
Extend get_alpn() for xprt_qmux layer. If lower layer does not implement
ALPN negotiation, return a statically default protocol value. Currently
this is set to "h3".
This change is required to support QMux in clear without SSL. In the
future, it could be useful to configure the default protocol, for
example by extending the syntax for the "proto" keyword.
When QMux protocol is used, xprt_qmux layer is setup after SSL handshake
completion but prior to the MUX initialization. Once transport
parameters exchange is successful, the layer is removed and the MUX is
started.
The layer setup operation was performed directly on ssl_sock_io_cb().
Simplify the code by extracting it in a dedicated function
xprt_add_l6hs(). The function is generic so the requested XPRT layer
must be passed as argument.
The code is mostly identical. One difference is that a check is
performed to ensure no SSL handshake is pending. If this is the case,
the function is a noop. This will become useful to support QMux
transparently both in clear or on top of SSL.
Another minor addition is that CO_FL_XPRT_READY flag is automatically
resetted by xprt_add_l6hs(). This allows the code to use
conn_xprt_start() standard function after XPRT init.
A recent patch has introduced <init_xprt> mux_proto_list member. This
allows to activate QMux on SSL handshake completion without explicit
"proto qmux" setting.
Thanks to this change, on SSL handshake completion it is not necessary
anymore to check for CO_FL_QMUX_* flags.
The various tcp_option_* converters rely on tcp_fullhdr_find_opt() to
find the option. However, the same bug as fixed in commit dbf471f99a
("BUG/MAJOR: net_helper: ip.fp infinite loop on malformed tcp options")
was also present there, by which an option of length 0 could be looped
over indefinitely. In practice this does not happen since such options
are not valid, but if passed encoded in an HTTP header for example, it
could possibly be passed.
While fixing it, let's check for length >1 in all 3 locations insteead
of only non-zero, since there's no point processing a malformed option
that wouldn't even be properly skipped.
This fix doesn't need to be backported, unless the ip.fp series is.
Thanks to @Vincent55 for reporting this issue.
When threads are disabled, "static __decl_spinlock(foo);" ends up as
"static;", causing a build warning when threads are disabled. We don't
need it to be static so let's drop "static" here. No backport is needed,
this is 3.4-only.
Instead of relying on malloc(n*size), we now pass array_size_or_fail(n,m)
so that it becomes possible to detect overflow. This is particularly
interesting for global settings that might be set large enough to cause
overflows on 32-bit systems for example, resulting in small values that
then cause trouble. Now the overflow will be detected at allocation time.
Around 25 locations were updated.
In stksess_trash_oldest(), and process_tables_expire(), avoid freeing
elements while holding two locks, as it could be very costly.
Instead, build a linked list of elements to be free'd, and do so once we
no longer hold any lock.
This may help with github issue #3380, and may be backported to 3.3.
Since support for large buffers was added, we must be careful when chunks
are allocated. Indeed, depending on the context a large chunks may be
required if data are copied from a large buffer.
In htx_replace_blk_value() function, when a defragmentation is necessary,
the data to be replaced are copied to a chunk before the
defragmentation. However, I forgot to get large chunk when necessary by
calling alloc_trash_chunk_sz() instead of alloc_trash_chunk(). Because of
this issue, it is possible to copy data to a too small chunk, leading to a
crash.
So let's fix the issue.
Thanks to Vincent55 for finding and reporting this.
No backport needed.
appctx_htx_snd_buf() function is relying on htx_xfer() function to transfer
HTX blocks when a swap of buffers is not possible. However, it was not
properly using this function.
Indeed, originally htx_xfer() was designed to transfer blocks with a limit,
the <count> parameter, which included the blocks payload and the
meta-data. It was aligned with all calls, except for the transfer of HTX
data to the applet, in appctx_htx_snd_buf() function. In that case, the
<count> parameter is the amount of data forwarded by the stream to the
applet. So meta-data are not included.
Thanks to the previous commit ("MEDIUM: htx: Improve htx_xfer API to not count
HTX meta-data"), it is now possible to instruct htx_xfer() function that
<count> parameter does not include the meta-data.
Because of this bug, crashes can be experienced when transferring HTX data
to an applet. At first glance, lua HTTP applets and the http client are
concerned.
Stable versions from 3.3 to 3.0 are also affected. But this patch cannot be
backported as is because htx_xfer() function does not exist on these
versions.
Thaks to Yon Harlicaj for finding and reporting this.
(https://x.com/nvmb3r - https://www.linkedin.com/in/eljon-harlicaj/)
This patch add the ability to the htx_xfer() function to transfer data
without acounting the meta-data. By default, the <count> variable includes
the meta-data. But by setting the flag HTX_XFER_NO_METADATA, It is possible
to transfer HTX blocks without count meta-data. In that case, <count> will
not contain the blocks meta-data and the return value will not include them.
Fix usage of stream.max-concurrent QUIC setting on the backend side.
Contrary to frontend connections, this limit must be enforced by QUIC
MUX directly. This is necessary as the peer may allow a larger number of
concurrent streams via its flow control.
First, QUIC TP initial max bidi streams value is now set to 0. This is
fine as only the HTTP/3 client is expected to open bidirectional
streams.
The most important changes is performed in qcm_avail_streams(). The
value first depends on the peer flow control. Now, it is further reduced
if necessary to not exceed the configured BE stream.max-concurrent.
Note that this new behavior may further increases current limitation on
QUIC BE reuse when a QCS instance is kept while its upper stream layer
is detached. In this case there is a risk that the connection is not
reinserted in the correct server pool, as an idle or avail one.
This is a breaking change as BE stream.max-concurrent keyword setting
meaning is changed in effect. However, this does not necessitate extra
warnings as the previous usage was in effect useless. Furthermore, QUIC
on the backend side is still considered as experimental.
This can be backported up to 3.3.
Commit 8aa854ab26a7daa613a17548f1fe1d0adb8cf61b made it so we'd store
the hash corresponding to the server parameters, so that we could detect
if we're still talking to the same server, and not use those parameters
if not.
However, when updating those parameters, we forgot to store the new
hash, which would result in the new parameters never be used, and
breakling 0RTT.
Fix that by properly update the hash when needed.
This should be backported when 8aa854ab26a7daa613a17548f1fe1d0adb8cf61b
is backported.
We're still regularly seeing insecure configs where chroot is missing.
Now that we have "chroot auto", there's no excuse for not knowing where
to chroot, so let's detect that we're starting as root, detect that the
process is allowed to chroot (i.e. no capability issue, or some hardened
containers), and if no chroot is set, let's emit a warning explaining how
to silence it, i.e. either "chroot auto" or "chroot /".
Most likely we'll start using "chroot auto" by default in 3.5 if no
usability issue is reported.
We'll recommend to use "chroot /" to explicitly disable chroot, however
there might be configurations where it would cause problems to just issue
the syscall (typically some hardened containers), so let's make sure that
"chroot /" is a nop in this case.
In case of XPRT_QMUX init failure on the frontend side, the connection
must immediately be released. This is not the case on the backend side
as a stream can supervize the connection lifetime.
This patch performs the connection free via conn_complete_session(). As
conn is flagged with CO_FL_ERROR, this will automatically fail and
invoke session_kill_embryonic(), which ensures the session and its
connection are both freed as wanted in this case.
No need to backport.
When we store the negociated server parameters, such as the ALPN, also
store the calculated hash with the connection. If it is different, as
can happen because the IP address is different because set-dst was used,
we certainly do not want to reuse the information in the cache,
otherwise we could end up using the wrong ALPN and mux.
That means we already have to calculate the hash in connect_server()
now, while before we would not do it for Websockets, if we could not do
connection reuse, as that's all the hash was used for.
This should fix Github issue #3386
This should be backported as far as 3.2.
qcc_release_remote_stream() is called each time a remote stream is
closed. Flow control accounting is updated and when necessary, a
MAX_STREAMS_BIDI frame is prepared to allow the peer to initiate new
streams.
This patch extends stream elasticity features with the QUIC bidirection
stream flow control mechanism. The announced value can now be possibly
reduced depending on conn_calc_max_streams().
The first step is to decrement closed streams from the global committed
extra streams total. This must be performed conn_calc_max_streams() to
ensure the calculation will be valid.
Then, there is two cases depending on conn_calc_max_streams() result. If
the value is less than the peer still remaining stream window, nothing
more is performed. If the opposite case, flow control must be increased
and a MAX_STREAMS_BIDI frame is prepared, with the value adjusted to not
exceed the stream elasticity limit. Global extra streams total is then
finally incremented.
This calcul also ensures that when all streams are closed, global extra
streams accounting operations are decremented by 1, as a connection
always has access to one stream which is excluded from the global total.
Note that if stream elasticity is not active, flow control increases
principle is unchanged and remains statically performed.
This patch is labelled as major as it complexifies bidirectional stream
flow control mechanisme. This is a sensitive operation as there is a
risk of connection freeze if flow control updates are inadvertently
skipped.
Add a new QCC member <ms_bidi_rel>. This represents the number of
concurrent streams advertised similarly to ms_bidi, but as a relative
value.
This patch does not introduce any functional change. For now,
<ms_bidi_rel> will be equal to <ms_bidi_init>. However, with the
implementation of stream elasticity and dynamic adjustment for
concurrent max-streams-bidi, the former will be required to keep the
last advertised value.
When stream elasticity is active, the maximum number of concurrent bidi
streams advertised via transport parameters is now reduced depending on
the connection load. This is implemented via conn_calc_max_streams()
which returns the value to use.
This is not applied on listeners with enabled 0-RTT. Indeed, for such
connections, clients are expected to reuse the previously seen transport
parameters. The server on the other hand must not decrease several
values on the newly advertised params, in particular for the maximum
number of concurrent bidi streams. The simplest way to prevent 0-RTT
failure is to not mix stream elasticity with it.
Note that the 0-RTT limitation is only applied for the initial value :
during the connection lifetime, stream elasticity can still be used by
the MUX to dynamically reduce the stream window. This will be
implemented in a future patch.
