Clarify how `max-delegation-servers` is used in the resolver, in
particular, the fact that it, in practice, caps the maximum outgoing
queries to resolve a name at a given delegation point.
Add a new system test which ensures that the `max-delegation-servers`
limit is correctly respected also in the case a domain has only NS names
(and no glues). In particular, this test when there are multiple NS
names and multiples IPs per names.
If the number of IP (even from the first picked NS name) reaches
`max-delegation-servers`, and the resolution is not a success, the
resolver won't attempt another NS name, as it already used all its
"credit".
Verify that the resolver selects authoritative servers in increasing
SRTT order. Four servers are configured with increasing response
delays. 100 queries are sent, expecting most to go to the fastest
server (ns2). Then ns2 stops responding, another 100 queries are
sent and should go to ns3 (the next fastest), and so on through
ns4 and ns5. Each query uses a unique name to avoid cache hits.
Test the resolver's behavior with self-pointed glue where each NS
has the same set of addresses. Verify that addresses are
deduplicated and each unique IP is only queried once.
Also test the NS processing limit (max-delegation-servers) and the
ADB address limit (adbaddrslimit), both individually and combined.
The SLIST (essentially `fctx->finds`, forwarders and dual-stack
alternatives aside) can have duplicate server addresses when multiple
in-domain nameservers share the same IP addresses:
sub.example. NS ns1.sub.example.
sub.example. NS ns2.sub.example.
ns1.sub.example. A 1.2.3.4
ns1.sub.example. A 5.6.7.8
ns2.sub.example. A 1.2.3.4
ns2.sub.example. A 5.6.7.8
If both 1.2.3.4 and 5.6.7.8 fail to return a valid answer, the resolver
would query each address twice.
The problem is fixed by replacing the two-phase server selection (sort
each find list by SRTT, sort finds by head SRTT) with a single linear
scan in nextaddress() that finds the lowest-SRTT unmarked, non-duplicate
address across all find lists.
The old approach had a correctness bug: after sorting, the resolver
picked the next address from the "current" find list rather than
globally. For example, with find lists [1, 15, 26] and [3, 4, 5], the
second pick would be SRTT 15 instead of the correct SRTT 3.
The new approach is both simpler and correct: each call to nextaddress()
walks all addresses, skips marked and duplicate entries, and returns the
one with the lowest SRTT. While this walk is repeated for each server
attempt, it operates on a small bounded list and is negligible compared
to the network I/O of querying the server.
The number of `dns_adbaddrfind_t` (NS address with metadata like SRTT)
returned from an ADB NS name lookup is now limited by the caller. The
default value (outside the resolver) uses `max-delegation-servers`, and
the resolver, for a given fetch, start with `max-delegation-servers` and
decrement it at each ADB fetch. This ensures that, for a given
delegation, no more than 13 nameservers will be contacted.
This is the same mechanism used when looking up `dns_adbaddrfind_t` from
a list of glues (addresses).
When an upstream server answers BADCOOKIE, no matter which transport is used,
the resolver resends the query using TCP. However, if the upstream
server responded with BADCOOKIE again over TCP, the resolver would keep
resending until the maximum query count was reached.
This is now fixed by no longer resending once the query has already been
sent over TCP.
See isc-projects/bind9#5804
Merge branch '5804-resend-loop-badcookie' into 'security-main'
See merge request isc-private/bind9!988
A bug during bad server handling could cause the resolver to enter an infinite loop, continuously sending queries to an upstream server with no exit condition, until the resolver query timeout was hit. This has been fixed.
ISC would like to thank Billy Baraja (BielraX) for bringing this issue to our attention.
Closesisc-projects/bind9#5804
Merge branch '5804-resend-loop' into 'security-main'
See merge request isc-private/bind9!985
Update the resend_loop_badcookie system test to ensure there is no
attempt to resend the query using TCP when getting BADCOOKIE from an
upstream server using this transport already.
Unauthorized clients could consume outgoing zone transfers quota
and block authorized zone transfer clients. This has been fixed.
Fixesisc-projects/bind9#3589
Merge branch '3859-security-xfrout-quota-fix' into 'security-main'
See merge request isc-private/bind9!971
Calls to `rctx_resend()` are done internally within the resolver, in
flow which are not supposed to happens more than once. For instance,
if some query fails, and a specific flag "F" wasn't set, then set the
flag and try again. This wouldn't occur more than once because if the
query fails the next attempt, the flag "F" would be set already, so the
resolver would move to the next server (or give up).
However, a subtle bug missing checking a flag, for instance, could lead
to an unbounded loop re-trying to query the same server. This is now
impossible as `rctx_resend()` also increment the query counters (so if
such case occurs, it would stop once the maximum limit is reached).
The dns_resstatscounter_retry are also only incremented if the
`fctx_query()` succeeds, similar to as is done in `fctx_try()`.
When an upstream server answers BADCOOKIE, no matter the transport used,
the resolver eventually resends the query using TCP. However, if the
upstream server responds with BADCOOKIE again over TCP, the resolver
would keep resending until the maximum query count is reached.
This is now fixed by stopping resending once the query has already been
sent over TCP.
Run malicious server: resend_loop/ans3/ans.py
Start BIND: ns4
Send single query to test.example
The resolver will repeatedly resend queries until the fetch timeout
expires, resulting in resulting in thousands of qrysent while the quota
counter remains 0.
When a validator is being shut down, the associated name
`val->name` is set to NULL. This could cause a crash if a worker
thread subsequently added an EDE code with `val->name` in the
extra text.
`validator_addede()` now checks whether the name is NULL before
trying to add it to the extra text.
Closes#5613
Merge branch 'each-validator-log-after-shutdown' into 'main'
See merge request isc-projects/bind9!11945
When a validator is being shut down, the associated name
`val->name` is set to NULL. This could cause a crash if a worker
thread subsequently added an EDE code to the response containing
val->name in the extra text.
`validator_addede()` now checks whether the name is NULL before
trying to add it to the extra text.
The :iscman:`named` process could terminate unexpectedly when
processing a catalog zone with an invalid ``allow-query`` or
``allow-transfer`` custom property (i.e. having a non-APL type)
coexisting with the valid property. This has been fixed.
Closes#5941
Merge branch '5941-catz-catz_process_apl-bug-fix' into 'main'
See merge request isc-projects/bind9!11954
The allow-transfer/allow-query catalog zone custom properties support
only APL RRtypes. All other types are correctly rejected by the
catz_process_apl() function. However, when an APL RRtype is processed
by that function, and another (non-APL) RRtype is then attempted to be
processed, there is an assertion failure happening in the prologue
of the function because `*aclbp != NULL` (i.e. an APL has been already
processed). Move the code to do type checking before the affected
REQUIRE assertion.
The :iscman:`named` process could leak small amounts of memory
when processing a catalog zone entry which had defined custom
primary servers with TSIG keys using both the regular ``primaries``
custom property syntax and the legacy alternative syntax (``masters``)
at the same time. This has been fixed.
Closes#5943
Merge branch '5943-catz-primaries-tsig-key-name-leak-fix' into 'main'
See merge request isc-projects/bind9!11951
An assertion failure could be triggered if both `dns64` and the `filter-aaaa` plugin were in use simultaneously. This happened if the plugin triggered a second recursion process, which then attempted to store DNS64 state information in a pointer that had already been set by the original recursion process. This has been fixed.
Closes#5854
Merge branch '5854-dns64-aaaaok' into 'main'
See merge request isc-projects/bind9!11949
The DNS64 state information stored in client->query.dns64_aaaaok
could cause an assertion failure in query_respond() if the server
was configured in such a way as to trigger a new recursion before
the query had been reset - for example, by using the filter-aaaa
plugin, which may need to recurse to find out whether an A record
exists.
This has been addressed by clearing DNS64 state information
immediately after the call to query_filter64().
In previous_closest_nsec(), a new qpreader was opened to search the NSEC
tree. It was possible for that to be used to update a QP iterator object
owned by the caller, and then be destroyed when the function returned.
This qpreader object isn't necessary anymore; since namespaces were
added to the QP trie in commit 15653c54a0, we can now just reuse the
existing reader for the main tree.
Closes#5942
Merge branch '5942-qpiter-fix' into 'main'
See merge request isc-projects/bind9!11955
In previous_closest_nsec(), a new qpreader was opened to search the NSEC
tree. It was possible for that to be used to update a QP iterator object
owned by the caller, and then be destroyed when the function returned.
This qpreader object isn't necessary anymore; since namespaces were
added to the QP trie in commit 15653c54a0, we can now just reuse the
existing reader for the main tree.
When named was reconfigured or shut down while a negative trust anchor
was being rechecked against authoritative servers, the in-flight recheck
could outlive the view that owned it and cause `named` to crash. This
has been fixed.
Closes#5938
Merge branch '5938-ref-ntatable' into 'main'
See merge request isc-projects/bind9!11948
Each dns__nta_t now references its parent ntatable in nta_create() and
releases it in dns__nta_destroy(). This avoids a use-after-free in
fetch_done() and other callbacks that dereference nta->ntatable: the
ntatable could otherwise be released by view destruction while an
in-flight resolver fetch still holds a reference to the NTA.
A DNSKEY record appearing before the first ';; KeySigningRequest'
header in a KSR file made dnssec-ksr abort on an internal assertion
instead of producing a structured error, killing pipelines that
fed it crafted or corrupted input. The tool now exits with a
fatal error naming the file and line.
Closes#5914
Merge branch '5914-dnssec-ksr-rdatalist-null-insist' into 'main'
See merge request isc-projects/bind9!11916
A DNSKEY record appearing before any ';; KeySigningRequest' header
in a KSR file made dnssec-ksr abort on INSIST(rdatalist != NULL),
which is the wrong tool for a malformed-input case. Issue a fatal()
naming the file and line instead so pipelines see a clean exit
status and an actionable message; the now-unreachable NULL check on
the rdatalist->ttl update goes away too.
Assisted-by: Claude:claude-opus-4-7
When BIND was asked to store a record set whose total size exceeds
what fits in a DNS message, it would allocate memory and build the
structure, then fail later at response time. Such oversized record
sets are now rejected at the time of storage with an error, avoiding
wasted work on data that can never be served.
Merge branch 'ondrej/harden-buflen-overflow' into 'main'
See merge request isc-projects/bind9!11963
makeslab(), makevec(), dns_rdatavec_merge() and dns_rdatavec_subtract()
summed per-record storage into an unsigned int with no upper-bound
check. An RRset whose total encoded size exceeds DNS_RDATA_MAXLENGTH
cannot fit in a DNS message and is unservable; building its in-memory
representation only burns memory on data that will fail at response
time, and at the upper bound the running sum could in theory wrap.
Cap the running total at DNS_RDATA_MAXLENGTH and return ISC_R_NOSPACE
when exceeded. Update the qpdb cache memory-purge test to use a
record size that fits within the new limit.
Assisted-by: Claude:claude-opus-4-7
KEY resource records originally defined NOAUTH, NOCONF, EXTENDED, and
ENTITY flags that were removed by RFC 3445 back in 2002. BIND still
carried code to parse and emit them, including the additional two-octet
flags field that followed when the EXTENDED bit was set. That handling
has been removed and the affected bit positions are now reserved.
Dropping the extended-flags handling also eliminates a possible crash
that could be reached when signing a zone containing an invalid key.
Closes#5900
Merge branch '5900-remove-keyflag-extended' into 'main'
See merge request isc-projects/bind9!11961
RFC 3445 also eliminated the DNS_KEYTYPE_NOAUTH, DNS_KEYTYPE_NOCONF,
and DNS_KEYOWNER_ENTITY flags. With NOAUTH and NOCONF gone, the
concept of NOKEY can no longer be expressed in KEY records.
DNS_KEYOWNER_ENTITY was already unused as of 22d688f656 but still
defined; that is now also removed.
The DNS_KEYFLAG_EXTENDED flag was only legitimate for type KEY
and was eliminated by RFC 3445. Dropping the extended-flags
handling in pub_compare() also fixes a possible crash when
signing a zone whose journal contains a crafted DNSKEY: a
6-byte record with the EXTENDED bit set produced a memmove()
length that underflowed and ran off a stack buffer.
With response rate limiting enabled, an attacker sending queries from many
spoofed source addresses could steer entries into the same slot of the
internal rate-limit table and slow down query processing on the affected
server. The table now uses a per-process keyed hash so the placement of
entries cannot be predicted or influenced from the network.
Closes#5906
Merge branch '5906-rrl-hash-collision-dos' into 'main'
See merge request isc-projects/bind9!11950
The previous hash_key() was a deterministic, unkeyed (<<1) + add over the
key words. An off-path attacker could invert it offline and submit
queries whose source /24, qname hash, and qtype map to a single bucket;
under chaining this turns every lookup into an O(N) walk under
rrl->lock and starves legitimate query processing on the very feature
deployed to mitigate DoS.
Replace it with isc_hash32(), which is HalfSipHash-2-4 keyed by a
per-process random seed, so collision sets cannot be precomputed.
Assisted-by: Claude:claude-opus-4-7
The name of the key to generate can be passed to `keygen()` as a
string; we don't need to pass it using argc/argv.
Merge branch 'each-keygen-cleanup' into 'main'
See merge request isc-projects/bind9!11942
Configuring the root zone as a signed primary with parental agents (or with
notify-on-cds-changes) caused named to exit on an internal assertion as soon
as the DS-publication machinery tried to look up the parent NS RRset — the root
has no parent. The lookup is now short-circuited cleanly.
Similar, a zone with no NS records in the parent caused named to exit in the same way.
Closes#5910
Merge branch '5910-nsfetch-start-root-domain-assertion' into 'main'
See merge request isc-projects/bind9!11909
Once the walk reaches the root, splitting one more label off would
trip an internal assertion and abort named. Stop cleanly with
ISC_R_NOTFOUND so the dispatcher cancels the fetch. Only reachable
through misconfiguration (root configured as a primary with parental
agents, or a parent zone that NODATAs its own NS).
Assisted-by: Claude:claude-opus-4-7
This is required to AXFR and verify the root zone and it makes no
difference for non-root zones (dnssec-verify takes FQDN or makes the
provided name absolute).
Add a test case where the root zone has dnssec-policy configured, with
checkds enabled. This is a silly case because the root does not have
any parent NS records, but it should not crash the server.
The same is true for zones that do not have parent NS records, but
eventually they will hit the same code path.
A rare crash has been observed in named while it is resolving upstream nameserver
addresses for a recursive query, surfacing as a segmentation fault with no immediate
clue as to the cause. This change adds internal consistency checks so that a future
occurrence of the same condition aborts named with a diagnostic message at the point
the inconsistency arises, rather than corrupting state and crashing later in
an unrelated location.
Closes#5602
Merge branch '5602-adb-find-sanity-checks' into 'main'
See merge request isc-projects/bind9!11943
The dns_adbfind_t lifetime model has no reference counting; storage
liveness is held together by find->lock and the FIND_EVENT_SENT
idempotency flag, plus an unwritten cross-module rule that all
non-trivial operations on a find run on find->loop. If a caller
violates that rule, the unlock-relock window in dns_adb_cancelfind
(and similar paths) becomes a use-after-free and we crash later
inside libpthread on a corrupted mutex.
Add REQUIREs at dns_adb_cancelfind, dns_adb_destroyfind and
find_sendevent so a violation aborts at the offending call site
rather than silently freeing storage another loop is still touching.
Also poison find->magic with ~DNS_ADBFIND_MAGIC in free_adbfind so
DNS_ADBFIND_VALID catches reuse-after-free at the next public entry
point instead of letting the dangling pointer reach the mutex code.
Assisted-by: Claude:claude-opus-4-7
dig's parser for EDNS options in a DNS reply now stops cleanly when an
option declares a length that runs past the end of the option data,
rather than trusting the upstream OPT-record validator to reject the
reply first. This is a defensive change; behavior is unchanged in
practice.
Merge branch 'ondrej/dig-process-opt-edns-optlen-oob' into 'main'
See merge request isc-projects/bind9!11937
process_opt() reads the per-option (optcode, optlen) header from the
OPT rdata and then advances the buffer by optlen, both for the COOKIE
branch (via process_cookie()) and for any other optcode. The walk
itself never compared optlen to the buffer remainder; the only reason
it cannot trip the isc_buffer_forward() REQUIRE today is that
fromwire_opt() (lib/dns/rdata/generic/opt_41.c) already validates each
option's length against the rdata bounds before the rdataset is
handed back, so process_opt() never sees a self-inconsistent rdata.
That upstream guarantee is fine, but it leaves the local walker
trusting an invariant established elsewhere. Add a defensive check
that just stops the walk when a future caller (a cached message, an
alternate parser, a refactor of the OPT validator) hands process_opt()
a buffer where optlen would run past the end.
Assisted-by: Claude:claude-opus-4-7
If a merge request is merged to an autorebased branch while it is
getting rebased, the "git push -f" command at the end of the autorebase
job will cause the contents of that merge request to be silently deleted
from Git history even though the merge request will still be (correctly)
shown as "merged" by GitLab.
Use "git push --force-with-lease" instead to prevent force-pushing the
rebased version of the branch if it is pushed to after its pre-rebase
version is fetched by the autorebase job. Report such an event
accordingly. For simplicity, no retries are attempted as the problem is
expected to be resolved by the next autorebase and the chances of this
scenario happening in practice are already low to begin with.
Merge branch 'michal/use-git-push-force-with-lease-for-autorebases' into 'main'
See merge request isc-projects/bind9!11939
