Michal Nowak [Tue, 16 Feb 2021 10:33:58 +0000 (11:33 +0100)]
Prevent Git to expand $systest
CentOS 8 "git status" unexpectedly expands search directory "tsig" to
also search in the "tsiggss" directory, thus incorrectly identifying
files as "not removed" in the "tsig" directory:
$ git status -su --ignored tsig
$ touch tsiggss/ns1/{named.run,named.memstats}
$ git status -su --ignored tsig
!! tsiggss/ns1/named.memstats
!! tsiggss/ns1/named.run
Ondřej Surý [Thu, 7 Jan 2021 09:44:46 +0000 (10:44 +0100)]
Fix off-by-one bug in ISC SPNEGO implementation
The ISC SPNEGO implementation is based on mod_auth_kerb code. When
CVE-2006-5989 was disclosed, the relevant fix was not applied to the
BIND 9 codebase, making the latter vulnerable to the aforementioned flaw
when "tkey-gssapi-keytab" or "tkey-gssapi-credential" is set in
named.conf.
The original description of CVE-2006-5989 was:
Off-by-one error in the der_get_oid function in mod_auth_kerb 5.0
allows remote attackers to cause a denial of service (crash) via a
crafted Kerberos message that triggers a heap-based buffer overflow
in the component array.
Later research revealed that this flaw also theoretically enables remote
code execution, though achieving the latter in real-world conditions is
currently deemed very difficult.
This vulnerability was responsibly reported as ZDI-CAN-12302 ("ISC BIND
TKEY Query Heap-based Buffer Overflow Remote Code Execution
Vulnerability") by Trend Micro Zero Day Initiative.
Ondřej Surý [Thu, 11 Feb 2021 07:37:52 +0000 (08:37 +0100)]
Rollback setting IP_DONTFRAG option on the UDP sockets
In DNS Flag Day 2020, the development branch started setting the
IP_DONTFRAG option on the UDP sockets. It turned out, that this
code was incomplete leading to dropping the outgoing UDP packets.
Henceforth this commit rolls back this setting until we have a
proper fix that would send back empty response with TC flag set.
Michal Nowak [Mon, 18 Jan 2021 18:15:44 +0000 (19:15 +0100)]
Use BIND 9.17 preprocessor macro to skip unit test
BIND 9.17 changed exit code of skipped test to meet Automake
expectations in fa505bfb0e7623d7cfc94ae15a0246ae71000904. BIND 9.16 was
not rewritten to Automake, but for consistency reasons, the same
SKIPPED_TEST_EXIT_CODE preprocessor macro is used (though the actual
exit code differs from the one in BIND 9.17).
Michal Nowak [Wed, 30 Dec 2020 12:22:46 +0000 (13:22 +0100)]
Merge UNTESTED and SKIPPED system test results
Descriptions of UNTESTED and SKIPPED system test results are very
similar to one another and it may be confusing when to pick one and
when the other. Merging these two system test results removes the
confusion.
Mark Andrews [Fri, 22 Jan 2021 04:59:03 +0000 (15:59 +1100)]
Fix linking order for OpenSSL libraries
As libssl depends on libcrypto, -lssl needs to precede -lcrypto in
linker invocations or else the build will fail with static OpenSSL
libraries. Adjust m4/ax_check_openssl.m4 to prevent this issue from
getting triggered when pkg-config files for OpenSSL are not available.
Mark Andrews [Mon, 15 Feb 2021 03:46:08 +0000 (14:46 +1100)]
Stop including <gssapi.h> from <dst/gssapi.h> header
The only reason for including the gssapi.h from the dst/gssapi.h header
was to get the typedefs of gss_cred_id_t and gss_ctx_id_t. Instead of
using those types directly this commit introduces dns_gss_cred_id_t and
dns_gss_ctx_id_t types that are being used in the public API and
privately retyped to their counterparts when we actually call the gss
api.
This also conceals the gssapi headers, so users of the libdns library
doesn't have to add GSSAPI_CFLAGS to the Makefile when including libdns
dst API.
Mark Andrews [Mon, 15 Feb 2021 02:28:58 +0000 (13:28 +1100)]
Stop including <lmdb.h> from <dns/lmdb.h>
The lmdb.h header doesn't have to be included from the dns/lmdb.h
header as it can be separately included where used. This stops
exposing the inclusion of lmdb.h from the libdns headers.
Diego Fronza [Thu, 11 Feb 2021 14:32:20 +0000 (11:32 -0300)]
Fix dangling references to outdated views after reconfig
This commit fix a leak which was happening every time an inline-signed
zone was added to the configuration, followed by a rndc reconfig.
During the reconfig process, the secure version of every inline-signed
zone was "moved" to a new view upon a reconfig and it "took the raw
version along", but only once the secure version was freed (at shutdown)
was prev_view for the raw version detached from, causing the old view to
be released as well.
This caused dangling references to be kept for the previous view, thus
keeping all resources used by that view in memory.
Michal Nowak [Thu, 14 Jan 2021 11:09:04 +0000 (12:09 +0100)]
Add --enable-option-checking=fatal to ./configure in CI
The --enable-option-checking=fatal option prevents ./configure from
proceeding when an unknown option is used in the ./configure step in CI.
This change will avoid adding unsupported ./configure options or options
with typo or typo in pairwise testing "# [pairwise: ...]" marker.
Michal Nowak [Mon, 7 Dec 2020 17:08:53 +0000 (18:08 +0100)]
Lint manual pages
As we generate manual pages from reStructuredText sources, we don't have
absolute control on manual page output and therefore 'mandoc -Tlint' may
always report warnings we can't eliminate. In light of this some mandoc
warnings need to be ignored.
Mark Andrews [Wed, 27 Jan 2021 06:17:36 +0000 (17:17 +1100)]
Silence Insecure data handling (TAINTED_SCALAR)
Coverity assumes that the memory holding any value read using byte
swapping is tainted. As we store the NSEC3PARAM records in wire
form and iterations is byte swapped the memory holding the record
is marked as tainted. nsec3->salt_length is marked as tainted
transitively. To remove the taint the value need to be range checked.
For a correctly formatted record region.length should match
nsec3->salt_length and provides a convenient value to check the field
against.
*** CID 316507: Insecure data handling (TAINTED_SCALAR)
/lib/dns/rdata/generic/nsec3param_51.c: 241 in tostruct_nsec3param()
235 region.length = rdata->length;
236 nsec3param->hash = uint8_consume_fromregion(®ion);
237 nsec3param->flags = uint8_consume_fromregion(®ion);
238 nsec3param->iterations = uint16_consume_fromregion(®ion);
239
240 nsec3param->salt_length = uint8_consume_fromregion(®ion);
>>> CID 316507: Insecure data handling (TAINTED_SCALAR)
>>> Passing tainted expression "nsec3param->salt_length" to "mem_maybedup", which uses it as an offset.
241 nsec3param->salt = mem_maybedup(mctx, region.base,
242 nsec3param->salt_length);
243 if (nsec3param->salt == NULL) {
244 return (ISC_R_NOMEMORY);
245 }
246 isc_region_consume(®ion, nsec3param->salt_length);
Mark Andrews [Wed, 27 Jan 2021 06:11:52 +0000 (17:11 +1100)]
Silence Untrusted value as argument (TAINTED_SCALAR)
Coverity assumes that the memory holding any value read using byte
swapping is tainted. As we store the NSEC3 records in wire form
and iterations is byte swapped the memory holding the record is
marked as tainted. nsec3->salt_length and nsec3->next_length are
marked as tainted transitively. To remove the taint the values need
to be range checked. Valid values for these should never exceed
region.length so that is becomes a reasonable value to check against.
*** CID 316509: (TAINTED_SCALAR)
/lib/dns/rdata/generic/nsec3_50.c: 312 in tostruct_nsec3()
306 if (nsec3->salt == NULL) {
307 return (ISC_R_NOMEMORY);
308 }
309 isc_region_consume(®ion, nsec3->salt_length);
310
311 nsec3->next_length = uint8_consume_fromregion(®ion);
>>> CID 316509: (TAINTED_SCALAR)
>>> Passing tainted expression "nsec3->next_length" to "mem_maybedup", which uses it as an offset.
312 nsec3->next = mem_maybedup(mctx, region.base, nsec3->next_length);
313 if (nsec3->next == NULL) {
314 goto cleanup;
315 }
316 isc_region_consume(®ion, nsec3->next_length);
317
/lib/dns/rdata/generic/nsec3_50.c: 305 in tostruct_nsec3()
299 region.length = rdata->length;
300 nsec3->hash = uint8_consume_fromregion(®ion);
301 nsec3->flags = uint8_consume_fromregion(®ion);
302 nsec3->iterations = uint16_consume_fromregion(®ion);
303
304 nsec3->salt_length = uint8_consume_fromregion(®ion);
>>> CID 316509: (TAINTED_SCALAR)
>>> Passing tainted expression "nsec3->salt_length" to "mem_maybedup", which uses it as an offset.
305 nsec3->salt = mem_maybedup(mctx, region.base, nsec3->salt_length);
306 if (nsec3->salt == NULL) {
307 return (ISC_R_NOMEMORY);
308 }
309 isc_region_consume(®ion, nsec3->salt_length);
310
Test for Ed25519 and Ed448. If both algorithms are not supported, skip
test. If only one algorithm is supported, run test, skip the
unsupported algorithm. If both are supported, run test normally.
Create new ns3. This will test Ed448 specifically, while now ns2 only
tests Ed25519. This moves some files from ns2/ to ns3/.
The number of queries to use in the burst can be reduced, as we have
a very low fetch limit of 1.
The dig command in 'wait_for_fetchlimits()' should time out sooner as
we expect a SERVFAIL to be returned promptly.
Enabling serve-stale can be done before hitting fetch-limits. This
reduces the chance that the resolver queries time out and fetch count
is reset. The chance of that happening is already slim because
'resolver-query-timeout' is 10 seconds, but better to first let the
data become stale rather than doing that while attempting to resolve.
The 'query_usestale()' function was only called when in
'query_gotanswer()' and an unexpected error occurred. This may have
been "quota reached", and thus we were in some cases returning
stale data on fetch-limits (and if serve-stale enabled of course).
But we can also hit fetch-limits when recursing because we are
following a referral (in 'query_notfound()' and
'query_delegation_recurse()'). Here we should also check for using
stale data in case an error occurred.
Specifically don't check for using stale data when refetching a
zero TTL RRset from cache.
Move the setting of DNS_DBFIND_STALESTART into the 'query_usestale()'
function to avoid code duplication.
Matthijs Mekking [Thu, 28 Jan 2021 11:30:08 +0000 (12:30 +0100)]
Add test for serve-stale /w fetch-limits
Add a test case when fetch-limits are reached and we have stale data
in cache.
This test starts with a positive answer for 'data.example/TXT' in
cache.
1. Reload named.conf to set fetch limits.
2. Disable responses from the authoritative server.
3. Now send a batch of queries to the resolver, until hitting the
fetch limits. We can detect this by looking at the response RCODE,
at some point we will see SERVFAIL responses.
4. At that point we will turn on serve-stale.
5. Clients should see stale answers now.
6. An incoming query should not set the stale-refresh-time window,
so a following query should still get a stale answer because of a
resolver failure (and not because it was in the stale-refresh-time
window).
Matthijs Mekking [Wed, 27 Jan 2021 15:59:27 +0000 (16:59 +0100)]
Only start stale refresh window when resuming
If we did not attempt a fetch due to fetch-limits, we should not start
the stale-refresh-time window.
Introduce a new flag DNS_DBFIND_STALESTART to differentiate between
a resolver failure and unexpected error. If we are resuming, this
indicates a resolver failure, then start the stale-refresh-time window,
otherwise don't start the stale-refresh-time window, but still fall
back to using stale data.
(This commit also wraps some docstrings to 80 characters width)
Matthijs Mekking [Tue, 19 Jan 2021 08:04:29 +0000 (09:04 +0100)]
Use stale data also if we are not resuming
Before this change, BIND will only fallback to using stale data if
there was an actual attempt to resolve the query. Then on a timeout,
the stale data from cache becomes eligible.
This commit changes this so that on any unexpected error stale data
becomes eligble (you would still have to have 'stale-answer-enable'
enabled of course).
If there is no stale data, this may return in an error again, so don't
loop on stale data lookup attempts. If the DNS_DBFIND_STALEOK flag is
set, this means we already tried to lookup stale data, so if that is
the case, don't use stale again.
Mark Andrews [Wed, 3 Feb 2021 05:38:29 +0000 (16:38 +1100)]
Remove redundant 'version == NULL' check
*** CID 318094: Null pointer dereferences (REVERSE_INULL)
/lib/dns/rbtdb.c: 1389 in newversion()
1383 version->xfrsize = rbtdb->current_version->xfrsize;
1384 RWUNLOCK(&rbtdb->current_version->rwlock, isc_rwlocktype_read);
1385 rbtdb->next_serial++;
1386 rbtdb->future_version = version;
1387 RBTDB_UNLOCK(&rbtdb->lock, isc_rwlocktype_write);
1388
CID 318094: Null pointer dereferences (REVERSE_INULL)
Null-checking "version" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
1389 if (version == NULL) {
1390 return (result);
1391 }
1392
1393 *versionp = version;
1394
Mark Andrews [Wed, 3 Feb 2021 06:20:09 +0000 (17:20 +1100)]
Attempt to silence untrusted loop bound
Assign hit_len + key_len to len and test the result
rather than recomputing and letting the compiler simplify.
213 isc_region_consume(®ion, 2); /* hit length + algorithm */
9. tainted_return_value: Function uint16_fromregion returns tainted data. [show details]
10. tainted_data_transitive: Call to function uint16_fromregion with tainted argument *region.base returns tainted data.
11. tainted_return_value: Function uint16_fromregion returns tainted data.
12. tainted_data_transitive: Call to function uint16_fromregion with tainted argument *region.base returns tainted data.
13. var_assign: Assigning: key_len = uint16_fromregion(®ion), which taints key_len.
214 key_len = uint16_fromregion(®ion);
14. lower_bounds: Casting narrower unsigned key_len to wider signed type int effectively tests its lower bound.
15. Condition key_len == 0, taking false branch.
215 if (key_len == 0) {
216 RETERR(DNS_R_FORMERR);
217 }
16. Condition !!(_r->length >= _l), taking true branch.
17. Condition !!(_r->length >= _l), taking true branch.
218 isc_region_consume(®ion, 2);
18. lower_bounds: Casting narrower unsigned key_len to wider signed type int effectively tests its lower bound.
19. Condition region.length < (unsigned int)(hit_len + key_len), taking false branch.
219 if (region.length < (unsigned)(hit_len + key_len)) {
220 RETERR(DNS_R_FORMERR);
221 }
222
20. lower_bounds: Casting narrower unsigned key_len to wider signed type int effectively tests its lower bound.
21. Condition _r != 0, taking false branch.
223 RETERR(mem_tobuffer(target, rr.base, 4 + hit_len + key_len));
22. lower_bounds: Casting narrower unsigned key_len to wider signed type int effectively tests its lower bound.
23. var_assign_var: Compound assignment involving tainted variable 4 + hit_len + key_len to variable source->current taints source->current.
224 isc_buffer_forward(source, 4 + hit_len + key_len);
225
226 dns_decompress_setmethods(dctx, DNS_COMPRESS_NONE);
CID 281461 (#1 of 1): Untrusted loop bound (TAINTED_SCALAR)
24. tainted_data: Using tainted variable source->active - source->current as a loop boundary.
Ensure that tainted values are properly sanitized, by checking that their values are within a permissible range.
227 while (isc_buffer_activelength(source) > 0) {
228 dns_name_init(&name, NULL);
229 RETERR(dns_name_fromwire(&name, source, dctx, options, target));
230 }
1. Remove an unused keystr/dst_key_format.
2. Initialize a dst_key_state_t state with NA.
3. Update false comment about local policy (local policy only adds
barrier on transitions to the RUMOURED state, not the UNRETENTIVE
state).
There was a bug in function 'keymgr_ds_hidden_or_chained()'.
The funcion 'keymgr_ds_hidden_or_chained()' implements (3e) of rule2
as defined in the "Flexible and Robust Key Rollover" paper. The rules
says: All DS records need to be in the HIDDEN state, or if it is not
there must be a key with its DNSKEY and KRRSIG in OMNIPRESENT, and
its DS in the same state as the key in question. In human langauge,
if all keys have their DS in HIDDEN state you can do what you want,
but if a DS record is available to some validators, there must be
a chain of trust for it.
Note that the barriers on transitions first check if the current
state is valid, and then if the next state is valid too. But
here we falsely updated the 'dnskey_omnipresent' (now 'dnskey_chained')
with the next state. The next state applies to 'key' not to the state
to be checked. Updating the state here leads to (true) always, because
the key that will move its state will match the falsely updated
expected state. This could lead to the assumption that Key 2 would be
a valid chain of trust for Key 1, while clearly the presence of any
DS is uncertain.
The fix here is to check if the DNSKEY and KRRSIG are in OMNIPRESENT
state for the key that does not have its DS in the HIDDEN state, and
only if that is not the case, ensure that there is a key with the same
algorithm, that provides a valid chain of trust, that is, has its
DNSKEY, KRRSIG, and DS in OMNIPRESENT state.
The changes in 'keymgr_dnskey_hidden_or_chained()' are only cosmetical,
renaming 'rrsig_omnipresent' to 'rrsig_chained' and removing the
redundant initialization of the DST_KEY_DNSKEY expected state to NA.
projects / thirdparty / bind9.git / log
