Don't attempt to resolve DNS responses for intermediate results. This
may create multiple refreshes and can cause a crash.
One scenario is where for the query there is a CNAME and canonical
answer in cache that are both stale. This will trigger a refresh of
the RRsets because we encountered stale data and we prioritized it over
the lookup. It will trigger a refresh of both RRsets. When we start
recursing, it will detect a recursion loop because the recursion
parameters will eventually be the same. In 'dns_resolver_destroyfetch'
the sanity check fails, one of the callers did not get its event back
before trying to destroy the fetch.
Move the call to 'query_refresh_rrset' to 'ns_query_done', so that it
is only called once per client request.
Another scenario is where for the query there is a stale CNAME in the
cache that points to a record that is also in cache but not stale. This
will trigger a refresh of the RRset (because we encountered stale data
and we prioritized it over the lookup).
We mark RRsets that we add to the message with
DNS_RDATASETATTR_STALE_ADDED to prevent adding a duplicate RRset when
a stale lookup and a normal lookup conflict with each other. However,
the other non-stale RRset when following a CNAME chain will be added to
the message without setting that attribute, because it is not stale.
This is a variant of the bug in #2594. The fix covered the same crash
but for stale-answer-client-timeout > 0.
Fix this by clearing all RRsets from the message before refreshing.
This requires the refresh to happen after the query is send back to
the client.
When used with OpenSSL v3.0.0+, the `openssldh_compare()`,
`openssldh_paramcompare()`, and `openssldh_todns()` functions
fail to cleanup the used memory on some error paths.
Use `DST_RET` instead of `return`, when there is memory to be
released before returning from the functions.
when the compression buffer was reused for multiple statistics
requests, responses could grow beyond the correct size. this was
because the buffer was not cleared before reuse; compressed data
was still written to the beginning of the buffer, but then the size
of used region was increased by the amount written, rather than set
to the amount written. this caused responses to grow larger and
larger, potentially reading past the end of the allocated buffer.
Limit the amount of database lookups that can be triggered in
fctx_getaddresses() (i.e. when determining the name server addresses to
query next) by setting a hard limit on the number of NS RRs processed
for any delegation encountered. Without any limit in place, named can
be forced to perform large amounts of database lookups per each query
received, which severely impacts resolver performance.
The limit used (20) is an arbitrary value that is considered to be big
enough for any sane DNS delegation.
It is possible to bypass Response Rate Limiting (RRL)
`responses-per-second` limitation using specially crafted wildcard
names, because the current implementation, when encountering a found
DNS name generated from a wildcard record, just strips the leftmost
label of the name before making a key for the bucket.
While that technique helps with limiting random requests like
<random>.example.com (because all those requests will be accounted
as belonging to a bucket constructed from "example.com" name), it does
not help with random names like subdomain.<random>.example.com.
The best solution would have been to strip not just the leftmost
label, but as many labels as necessary until reaching the suffix part
of the wildcard record from which the found name is generated, however,
we do not have that information readily available in the context of RRL
processing code.
Fix the issue by interpreting all valid wildcard domain names as
the zone's origin name concatenated to the "*" name, so they all will
be put into the same bucket.
Commit b69e783164 inadvertently caused
builds using the --disable-doh switch to fail, by putting the
declaration of the isc__nm_async_settlsctx() function inside an #ifdef
block that is only evaluated when DNS-over-HTTPS support is enabled.
This results in the following compilation errors being triggered:
netmgr/netmgr.c:2657:1: error: no previous prototype for 'isc__nm_async_settlsctx' [-Werror=missing-prototypes]
2657 | isc__nm_async_settlsctx(isc__networker_t *worker, isc__netievent_t *ev0) {
| ^~~~~~~~~~~~~~~~~~~~~~~
Fix by making the declaration of the isc__nm_async_settlsctx() function
in lib/isc/netmgr/netmgr-int.h visible regardless of whether
DNS-over-HTTPS support is enabled or not.
Remove unnecessary != NULL checks
*** CID 352809: Null pointer dereferences (REVERSE_INULL) /lib/dns/message.c: 4654 in dns_message_buildopt()
4648 if (rdata != NULL) {
4649 dns_message_puttemprdata(message, &rdata);
4650 }
4651 if (rdataset != NULL) {
4652 dns_message_puttemprdataset(message, &rdataset);
4653 }
>>> CID 352809: Null pointer dereferences (REVERSE_INULL)
>>> Null-checking "rdatalist" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
4654 if (rdatalist != NULL) {
4655 dns_message_puttemprdatalist(message, &rdatalist);
4656 }
4657 return (result);
4658 }
4659
The isc_nm_listentlsdns() function erroneously calls
isc__nm_tcpdns_stoplistening() instead of isc__nm_tlsdns_stoplistening()
when something goes wrong, which can cause an assertion failure.
The dnstap query_message field was in some cases being filled in
with response messages, along with the response_message field.
The query_message field should only be used when logging requests,
and the response_message field only when logging responses.
Implement the configuration option with its checking and parsing parts.
The option should be later used by BIND to set an extended error
code (EDE) for the queries modified in the result of RPZ processing.
When we are closing the listening sockets, there's a time window in
which the TCP connection could be accepted although the respective
stoplistening function has already returned to control to the caller.
Clear the accept callback function early, so it doesn't get called when
we are not interested in the incoming connections anymore.
Previously:
* applications were using isc_app as the base unit for running the
application and signal handling.
* networking was handled in the netmgr layer, which would start a
number of threads, each with a uv_loop event loop.
* task/event handling was done in the isc_task unit, which used
netmgr event loops to run the isc_event calls.
In this refactoring:
* the network manager now uses isc_loop instead of maintaining its
own worker threads and event loops.
* the taskmgr that manages isc_task instances now also uses isc_loopmgr,
and every isc_task runs on a specific isc_loop bound to the specific
thread.
* applications have been updated as necessary to use the new API.
* new ISC_LOOP_TEST macros have been added to enable unit tests to
run isc_loop event loops. unit tests have been updated to use this
where needed.
* isc_timer was rewritten using the uv_timer, and isc_timermgr_t was
completely removed; isc_timer objects are now directly created on the
isc_loop event loops.
* the isc_timer API has been simplified. the "inactive" timer type has
been removed; timers are now stopped by calling isc_timer_stop()
instead of resetting to inactive.
* isc_manager now creates a loop manager rather than a timer manager.
* modules and applications using isc_timer have been updated to use the
new API.
This commit introduces new APIs for applications and signal handling,
intended to replace isc_app for applications built on top of libisc.
* isc_app will be replaced with isc_loopmgr, which handles the
starting and stopping of applications. In isc_loopmgr, the main
thread is not blocked, but is part of the working thread set.
The loop manager will start a number of threads, each with a
uv_loop event loop running. Setup and teardown functions can be
assigned which will run when the loop starts and stops, and
jobs can be scheduled to run in the meantime. When
isc_loopmgr_shutdown() is run from any the loops, all loops
will shut down and the application can terminate.
* signal handling will now be handled with a separate isc_signal unit.
isc_loopmgr only handles SIGTERM and SIGINT for application
termination, but the application may install additional signal
handlers, such as SIGHUP as a signal to reload configuration.
* new job running primitives, isc_job and isc_async, have been added.
Both units schedule callbacks (specifying a callback function and
argument) on an event loop. The difference is that isc_job unit is
unlocked and not thread-safe, so it can be used to efficiently
run jobs in the same thread, while isc_async is thread-safe and
uses locking, so it can be used to pass jobs from one thread to
another.
* isc_tid will be used to track the thread ID in isc_loop worker
threads.
* unit tests have been added for the new APIs.
There is one case in 'dns_nsec3_activex()' where it returns but forgets
to detach the db node. Add the missing 'dns_db_detachnode()' call.
This case only triggers if 'sig-signing-type' (privatetype) is set to 0
(which by default is not), or if the function is called with 'complete'
is set to 'true' (which at this moment do not exist).
When doing a dnssec-policy reconfiguration from a zone with NSEC only
keys to a zone that uses NSEC3, figure out to wait with building the
NSEC3 chain.
Previously, BIND 9 would attempt to sign such a zone, but failed to
do so because the NSEC3 chain conflicted with existing DNSKEY records
in the zone that were not compatible with NSEC3.
There exists logic for detecting such a case in the functions
dnskey_sane() (in lib/dns/zone.c) and check_dnssec() (in
lib/ns/update.c). Both functions look very similar so refactor them
to use the same code and call the new function (called
dns_zone_check_dnskey_nsec3()).
Also update the dns_nsec_nseconly() function to take an additional
parameter 'diff' that, if provided, will be checked whether an
offending NSEC only DNSKEY will be deleted from the zone. If so,
this key will not be considered when checking the zone for NSEC only
DNSKEYs. This is needed to allow a transition from an NSEC zone with
NSEC only DNSKEYs to an NSEC3 zone.
When the HTTP request has a body part after the HTTP headers, it is
not getting processed and is being prepended to the next request's data,
which results in an error when trying to parse it.
Improve the httpd.c:process_request() function with the following
additions:
1. Require that HTTP POST requests must have Content-Length header.
2. When Content-Length header is set, extract its value, and make sure
that it is valid and that the whole request's body is received before
processing the request.
3. Discard the request's body by consuming Content-Length worth of data
in the buffer.
After an earlier code cleanup, `dns_rdatalist_tordataset()` always
succeeds, so the `RETERR` error handling macro below the function
call was removed. After that change the `dynbuf` variable can never
be `NULL` in the error handling code path under the `failure` label.
*** CID 355779: Null pointer dereferences (REVERSE_INULL)
/lib/dns/tkey.c: 997 in buildquery()
991 dns_message_puttempname(msg, &aname);
992 }
993 if (question != NULL) {
994 dns_rdataset_disassociate(question);
995 dns_message_puttemprdataset(msg, &question);
996 }
>>> CID 355779: Null pointer dereferences (REVERSE_INULL)
>>> Null-checking "dynbuf" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
997 if (dynbuf != NULL) {
998 isc_buffer_free(&dynbuf);
999 }
1000 return (result);
1001 }
1002
Refactor the `buildquery()` function to simplify its error handling.
When dumping an ADB address entry associated with a name,
the name bucket lock was held, but the entry bucket lock was
not; this could cause data races when other threads were updating
address entry info. (These races are probably not operationally
harmful, but they triggered TSAN error reports.)
Clean up dns_rdatalist_tordataset() and dns_rdatalist_fromrdataset()
functions by making them return void, because they cannot fail.
Clean up other functions that subsequently cannot fail.
When checking if we should enable serve-stale, add an early out case
when the result is an error signalling a duplicate query or a query
that would be dropped.
In some circumstances generic TLS code could have resumed data reading
unexpectedly on the TCP layer code. Due to this, the behaviour of
isc_nm_pauseread() and isc_nm_resumeread() might have been
unexpected. This commit fixes that.
The bug does not seems to have real consequences in the existing code
due to the way the code is used. However, the bug could have lead to
unexpected behaviour and, at any rate, makes the TLS code behave
differently from the TCP code, with which it attempts to be as
compatible as possible.
Cumulative fetch limit logging happens on an event of a dropped
fetch if 60 seconds have been passed since the previous log message.
This change makes the log message different for the initial event
and for the later cumulative events to provide more useful information
to the system administrator.
When initially hitting the `fetches-per-zone` value, a log message
is being generated for the event of dropping the first fetch, then
any further log events occur only when another fetch is being dropped
and 60 seconds have been passed since the last logged message.
That logic isn't ideal because when the counter of the outstanding
fetches reaches zero, the structure holding the counters' values will
get deleted, and the information about the dropped fetches accumulated
during the last minute will not be logged.
Improve the fcount_logspill() function to makie sure that the final
values are getting logged before the counter object gets destroyed.
Sometimes tls_do_bio() might be called when there is no new data to
process (most notably, when resuming reads), in such a case internal
TLS session state will remain untouched and old value in 'errno' will
alter the result of SSL_get_error() call, possibly making it to return
SSL_ERROR_SYSCALL. This value will be treated as an error, and will
lead to closing the connection, which is not what expected.
Fedora 33 doesn't support RSASHA1 in future mode. There is no easy
check for this other than by attempting to perform a verification
using known good signatures. We don't attempt to sign with RSASHA1
as that would not work in FIPS mode. RSASHA1 is verify only.
The test vectors were generated using OpenSSL 3.0 and
util/gen-rsa-sha-vectors.c. Rerunning will generate a new set of
test vectors as the private key is not preserved.
e.g.
cc util/gen-rsa-sha-vectors.c -I /opt/local/include \
-L /opt/local/lib -lcrypto
The command 'rndc dumpdb -expired' will include expired RRsets in the
output, but only for the RBTDB_VIRTUAL time (of 5 minutes). This means
that if there is a cache cleaning problem and contents are not cleaned
up, the rndc command has little diagnostic value. Fix this by including
all RRsets in the dumpdb output if the '-expired' flag is set.
The "max-zone-ttl" option should now be configured as part of
"dnssec-policy". The option with the same name in "zone" and
"options" is hereby flagged as deprecated, and its functionality
will be removed in a future release.
These calls have not been needed since OpenSSL 0.9.7h.
This dates to commit 704d6eeab1, "Work
around non-reentrancy in openssl by disabling precomputation in keys".
This was in the bundled OpenSSL 0.9.3a era and made two changes. First,
it registered a locking callback because, in those days, OpenSSL needed
a callback to support locks. Second, it set flags to disable various
bits of cached state on DH, DSA, and RSA objects.
Looking back in OpenSSL 0.9.3a, that cached state was not protected by a
lock:
https://github.com/openssl/openssl/blob/OpenSSL_0_9_3a/crypto/rsa/rsa_eay.c#L137-L142
However, this was fixed in OpenSSL 0.9.7h:
6ec8e63af6
The other flags (DSA and RSA) have since fallen away, DSA with the
removal of DSA altogether (3994b1f9c2) and
RSA with 3a8d4a316e, "openssl 0.9.6a and
higher don't have the RSA locking bug [...] other algorithms still don't
do locking when performing precomputation [...]".
That seems to be referring to this OpenSSL change, which indeed fixed it
for RSA but not others:
bb617a9646
The 0.9.7h change above fixed it across the board, but there was never a
similar update to the workaround for DSA and DH. With such OpenSSL
versions long since out of support, the last remains of this workaround
can finally be removed.
When callback was NULL, bind9 would use BN_GENCB_set_old to set a NULL
callback because OpenSSL happened to allow a NULL "old" callback, but
not a NULL "new" callback. Instead, the way to turn off the callback is
to pass a NULL BN_GENCB itself.
Switch to doing that.
The BUFSIZ value varies between platforms, it could be 8K on Linux and
512 bytes on mingw. Make sure the buffers are always big enough for the
output data to prevent truncation of the output by appropriately
enlarging or sizing the buffers.
When a thread calls dns_dispatch_connect() on an unconnected TCP socket
it sets `tcpstate` from `DNS_DISPATCHSTATE_NONE` to `_CONNECTING`.
Previously, it then INSISTed that there were no pending connections
before calling isc_nm_tcpdnsconnect().
If a second thread called dns_dispatch_connect() during that window
of time, it could add a pending connection to the list, and trigger
an assertion failure.
This commit removes the INSIST since the condition is actually
harmless.
The STATID_CONNECT and STATID_CONNECTFAIL statistics were used
incorrectly. The STATID_CONNECT was incremented twice (once in
the *_connect_direct() and once in the callback) and STATID_CONNECTFAIL
would not be incremented at all if the failure happened in the callback.
Closes: #3452
