When cache memory usage is over the configured cache size (overmem) and
we are cleaning unused entries, it might not be enough to clean just two
entries if the entries to be expired are smaller than the newly added
rdata. This could be abused by an attacker to cause a remote Denial of
Service by possibly running out of the operating system memory.
Currently, the addrdataset() tries to do a single TTL-based cleaning
considering the serve-stale TTL and then optionally moves to overmem
cleaning if we are in that condition. Then the overmem_purge() tries to
do another single TTL based cleaning from the TTL heap and then continue
with LRU-based cleaning up to 2 entries cleaned.
Squash the TTL-cleaning mechanism into single call from addrdataset(),
but ignore the serve-stale TTL if we are currently overmem.
Then instead of having a fixed number of entries to clean, pass the size
of newly added rdatasetheader to the overmem_purge() function and
cleanup at least the size of the newly added data. This prevents the
cache going over the configured memory limit (`max-cache-size`).
Additionally, refactor the overmem_purge() function to reduce for-loop
nesting for readability.
The number of clients per query is calculated using the pending
fetch responses in the list. The dns_resolver_createfetch() function
includes every item in the list when deciding whether the limit is
reached (i.e. fctx->spilled is true). Then, when the limit is reached,
there is another calculation in fctx_sendevents(), when deciding
whether it is needed to increase the limit, but this time the TRYSTALE
responses are not included in the calculation (because of early break
from the loop), and because of that the limit is never increased.
A single client can have more than one associated response/event in the
list (currently max. two), and calculating them as separate "clients"
is unexpected. E.g. if 'stale-answer-enable' is enabled and
'stale-answer-client-timeout' is enabled and is larger than 0, then
each client will have two events, which will effectively halve the
clients-per-query limit.
Fix the dns_resolver_createfetch() function to calculate only the
regular FETCHDONE responses/events.
Change the fctx_sendevents() function to also calculate only FETCHDONE
responses/events. Currently, this second change doesn't have any impact,
because the TRYSTALE events were already skipped, but having the same
condition in both places will help prevent similar bugs in the future
if a new type of response/event is ever added.
(cherry picked from commit 2ae5c4a674)
There is a lock-order-inversion (potential deadlock) in resolver.c,
because in dns_resolver_shutdown() a resolver bucket lock is locked
while the resolver lock itself is already locked, while in
fctx_sendevents() the resolver lock is locked while a bucket lock
is locked before calling that function in fctx__done_detach().
The resolver lock/unlock in dns_resolver_shutdown() was added back in
the 317e36d47e commit to make sure that
the function is finished before the resolver object is destroyed.
Since res->exiting is atomic, it should be possible to remove the
resolver locking in dns_resolver_shutdown() and add it to the
send_shutdown_events() function which requires it.
Also, since 'res->exiting' is now set while unlocked, the 'INSIST'
in spillattimer_countdown() is wrong, and is removed.
This counter indicates the number of the resolver's spilled
queries due to reaching the clients per query quota.
(cherry picked from commit 04648d7c2f)
This commit ensures that access to the TLS context cache within zone
manager is properly synchronised.
Previously there was a possibility for it to get unexpectedly
NULLified for a brief moment by a call to
dns_zonemgr_set_tlsctx_cache() from one thread, while being accessed
from another (e.g. from got_transfer_quota()). This behaviour could
lead to server abort()ing on configuration reload (under very rare
circumstances).
That behaviour has been fixed.
(cherry picked from commit 0b95cf74ff)
when a TCP dispatch times out, we call tcp_recv() with a result
value of ISC_R_TIMEDOUT; this cancels the oldest dispatch
entry in the dispatch's active queue, plus any additional entries
that have waited longer than their configured timeouts. if, at
that point, there were more dispatch entries still on the active
queue, it resumes reading, but until now it failed to restart
the timer.
this has been corrected: we now calculate a new timeout
based on the oldest dispatch entry still remaining. this
requires us to initialize the start time of each dispatch entry
when it's first added to the queue.
in order to ensure that the handling of timed-out requests is
consistent, we now calculate the runtime of each dispatch
entry based on the same value for 'now'.
incidentally also fixed a compile error that turned up when
DNS_DISPATCH_TRACE was turned on.
(cherry picked from commit 0e800467ee)
it was possible to add a TSIG key to more than one TSIG
keyring at a time, and this was in fact happening with the
session key, which was generated once and then added to the
keyrings for each view as it was configured.
this has been corrected and a REQUIRE added to dns_tsigkeyring_add()
to prevent it from happening again.
The zone_resigninc() function does not check the validity of
'zone->db', which can crash named if the zone was unloaded earlier,
for example with "rndc delete".
Check that 'zone->db' is not 'NULL' before attaching to it, like
it is done in zone_sign() and zone_nsec3chain() functions, which
can similarly be called by zone maintenance.
(cherry picked from commit fae0930eb8)
When retrying in the DNS dispatch, the local port would be forgotten on
ISC_R_ADDRINUSE, keep the configured source-port even when retrying.
Additionally, treat ISC_R_NOPERM same as ISC_R_ADDRINUSE.
Closes: #3986
(cherry picked from commit c8e8ccd026)
When OPTOUT was in use we didn't ensure that NSEC3 records
for orphaned empty-non-terminals where removed. Check if
there are orphaned empty-non-terminal NSEC3 even if there
wasn't an NSEC3 RRset to be removed in dns_nsec3_delnsec3.
(cherry picked from commit 27160c137f)
After the dns_xfrin was changed to use network manager, the maximum
global (max-transfer-time-in) and idle (max-transfer-idle-in) times for
incoming transfers were turned inoperational because of missing
implementation.
Restore this functionality by implementing the timers for the incoming
transfers.
(cherry picked from commit d2377f8e04)
In write_public_key() and write_key_state(), there were left-over checks
for result, that were effectively dead code after the last refactoring.
Remove those.
(cherry picked from commit 766366e934)
Instead of explicitly adding a reference to catzs (catalog zones) when
calling the update callback, attach the catzs to the catz (catalog zone)
object to keep it referenced for the whole time the catz exists.
(cherry picked from commit 2ded876db2)
The xfrin_connect_done() had several problems:
- it would not add the server to unreachable table in case of the
failure coming from the dispatch [GL #3989]
- if dns_dispatch_checkperm() disallowed the connection, the xfr would
be left undetached
- if xfrin_send_request() failed to send the request, the xfr would be
left undetached
All of these have been fixed in this commit.
(cherry picked from commit 536e439c79)
The req_response() function is using 'udpcount' variable to resend
the request 'udpcount' times on timeout even for TCP requests,
which does not make sense, as it would use the same connection.
Add a condition to use the resend logic only for UDP requests.
(cherry picked from commit edcdb881da)
The dns_request_createraw() function, unlike dns_request_create(), when
calculating the UDP timeout value, doesn't check that 'udpretries' is
not zero, and that is the more logical behavior, because the calculation
formula uses division to 'udpretries + 1', where '1' is the first try.
Change the dns_request_create() function to remove the 'udpretries != 0'
condition.
Add a 'REQUIRE(udpretries != UINT_MAX)' check to protect from a division
by zero.
Make the 'request->udpcount' field to represent the number of tries,
instead of the number of retries.
(cherry picked from commit 643abfbba7)
dns_rdata_fromstruct in dns_keytable_deletekey can potentially
fail with ISC_R_NOSPACE. Handle the error condition.
(cherry picked from commit b5df9b8591)
In selfsigned_dnskey only call dns_dnssec_verify if the signature's
key id matches a revoked key, the trust is pending and the key
matches a trust anchor. Previously named was calling dns_dnssec_verify
unconditionally resulted in busy work.
(cherry picked from commit e68fecbdaa)
The isc_fsaccess API was created to hide the implementation details
between POSIX and Windows APIs. As we are not supporting the Windows
APIs anymore, it's better to drop this API used in the DST part.
Moreover, the isc_fsaccess was setting the permissions in an insecure
manner - it operated on the filename, and not on the file descriptor
which can lead to all kind of attacks if unpriviledged user has read (or
even worse write) access to key directory.
Replace the code that operates on the private keys with code that uses
mkstemp(), fchmod() and atomic rename() at the end, so at no time the
private key files have insecure permissions.
(cherry picked from commit 263d232c79)
The dns__catz_update_cb() function was earlier updated (see
d2ecff3c4a) to use a separate
'dns_db_t' object ('catz->updb' instead of 'catz->db') to
avoid a race between the 'dns__catz_update_cb()' and
'dns_catz_dbupdate_callback()' functions, but the 'REQUIRE'
check there still checks the validity of the 'catz->db' object.
Fix the omission.
(cherry picked from commit a2817541b3)
When resquery_response() was called with ISC_R_SHUTTINDOWN, the region
argument would be NULL, but rctx_respinit() would try to pass
region->base and region->len to the isc_buffer_init() leading to
a NULL pointer dereference. Properly handle non-ISC_R_SUCCESS by
ignoring the provided region.
(cherry picked from commit 93259812dd)
This should delay the catalog zone from being destroyed during
shutdown, if the update process is still running.
Doing this should not introduce significant shutdown delays, as
the update function constantly checks the 'shuttingdown' flag
and cancels the process if it is set.
(cherry picked from commit dc2b8bb1c9)
If the zone already has existing NSEC/NSEC3 chains then zone_sign
needs to continue to use them. If there are no chains then use
kasp setting otherwise generate an NSEC chain.
(cherry picked from commit 4b55201459)
dns_dnssec_updatekeys's 'report' could be called with invalid arguments
which the compiler should be be able to detect.
(cherry picked from commit 7a0a2fc3e4)
As it is done in the RPZ module, use 'db' and 'dbversion' for the
database we are going to update to, and 'updb' and 'updbversion' for
the database we are working on.
Doing this should avoid a race between the 'dns__catz_update_cb()' and
'dns_catz_dbupdate_callback()' functions.
(cherry picked from commit d2ecff3c4a)
This reverts commit a719647023.
The commit introduced a data race, because dns_db_endload() is called
after unfreezing the zone.
(not cherry picked from commit 593dea871a)
There are leftovers from the previous refactoring effort, which left
some function declarations and comments in the header file unchanged.
Finish the renaming.
(cherry picked from commit 580ef2e18f)
When detaching from the previous version of the database, make sure
that the update-notify callback is unregistered, otherwise there is
an INSIST check which can generate an assertion failure in free_rbtdb(),
which checks that there are no outstanding update listeners in the list.
There is a similar code already in place for RPZ.
(cherry picked from commit cf79692a66)
The zone_postload() function can fail and unregister the callbacks.
Call dns_db_endload() only after calling zone_postload() to make
sure that the registered update-notify callbacks are not called
when the zone loading has failed during zone_postload().
Also, don't ignore the return value of zone_postload().
(cherry picked from commit ed268b46f1)
The dbiterator read-locks the whole zone and it stayed locked during
whole processing time when catz is being read. Pause the iterator, so
the updates to catz zone are not being blocked while processing the catz
update.
(cherry picked from commit 4e7187601f)
Instead of holding the catzs->lock the whole time we process the catz
update, only hold it for hash table lookup and then release it. This
should unblock any other threads that might be processing updates to
catzs triggered by extra incoming transfer.
(cherry picked from commit b1cd4a066a)
Offload catalog zone processing so that the network manager threads
are not interrupted by a large catalog zone update.
Introduce a new 'updaterunning' state alongside with 'updatepending',
like it is done in the RPZ module.
Note that the dns__catz_update_cb() function currently holds the
catzs->lock during the whole process, which is far from being optimal,
but the issue is going to be addressed separately.
(cherry picked from commit 0b96c9234f)
This change should make sure that catalog zone update processing
doesn't happen when the catalog zone is being shut down. This
should help avoid races when offloading the catalog zone updates
in the follow-up commit.
(cherry picked from commit 246b7084d6)
* Change 'dns_catz_new_zones()' function's prototype (the order of the
arguments) to synchronize it with the similar function in rpz.c.
* Rename 'refs' to 'references' in preparation of ISC_REFCOUNT_*
macros usage for reference tracking.
* Unify dns_catz_zone_t naming to catz, and dns_catz_zones_t naming to
catzs, following the logic of similar changes in rpz.c.
* Use C compound literals for structure initialization.
* Synchronize the "new zone version came too soon" log message with the
one in rpz.c.
* Use more of 'sizeof(*ptr)' style instead of the 'sizeof(type_t)' style
expressions when allocating or freeing memory for 'ptr'.
(cherry picked from commit 8cb79fec9d)
