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.
The USE_OPENSSL constraint in dh_test does not seems to be necessary
anymore, the test runs with PKCS#11 as well.
(cherry picked from commit c341e7f740)
Make sure lib/dns/tests/dnstap_test returns an exit code that indicates
a skipped test when dnstap is not enabled.
(cherry picked from commit c286341703)
BIND 9.17 changed exit code of skipped test to meet Automake
expectations in fa505bfb0e. 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).
(cherry picked from commit fa505bfb0e)
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.
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.
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.
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);
(cherry picked from commit c40133d840)
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
(cherry picked from commit fd8d1337a5)
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)
(cherry picked from commit aabdedeae3)
*** 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
(cherry picked from commit 456d53d1fb)
Three small cleanups:
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).
(cherry picked from commit 189d9a2d21)
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.
(cherry picked from commit 291bcc3721)
The previous commit changed the function definition of
'keymgr_key_is_successor()', this commit updates the code where
this function is called.
In 'keymgr_key_exists_with_state()' the logic is also updated slightly
to become more readable. First handle the easy cases:
- If the key does not match the state, continue with the next key.
- If we found a key with matching state, and there is no need to
check the successor relationship, return (true).
- Otherwise check the successor relationship.
In 'keymgr_key_has_successor()' it is enough to check if a key has
a direct successor, so instead of calling 'keymgr_key_is_successor()',
we can just check 'keymgr_direct_dep()'.
In 'dns_keymgr_run()', we want to make sure that there is no
dependency on the keys before retiring excess keys, so replace
'keymgr_key_is_successor()' with 'keymgr_dep()'.
(cherry picked from commit 600915d1b2)
So far the key manager could only deal with two keys in a rollover,
because it used a simplified version of the successor relationship
equation from "Flexible and Robust Key Rollover" paper. The simplified
version assumes only two keys take part in the key rollover and it
for that it is enough to check the direct relationship between two
keys (is key x the direct predecessor of key z and is key z the direct
successor of key x?).
But when a third key (or more keys) comes into the equation, the key
manager would assume that one key (or more) is redundant and removed
it from the zone prematurely.
Fix by implementing Equation(2) correctly, where we check for
dependencies on keys:
z ->T x: Dep(x, T) = ∅ ∧
(x ∈ Dep(z, T) ∨
∃ y ∈ Dep(z, T)(y != z ∧ y ->T x ∧ DyKyRySy = DzKzRzSz))
This says: key z is a successor of key x if:
- key x depends on key z if z is a direct successor of x,
- or if there is another key y that depends on key z that has identical
key states as key z and key y is a successor of key x.
- Also, key x may not have any other keys depending on it.
This is still a simplified version of Equation(2) (but at least much
better), because the paper allows for a set of keys to depend on a
key. This is defined as the set Dep(x, T). Keys in the set Dep(x, T)
have a dependency on key x for record type T. The BIND implementation
can only have one key in the set Dep(x, T). The function
'keymgr_dep()' stores this key in 'uint32_t *dep' if there is a
dependency.
There are two scenarios where multiple keys can depend on a single key:
1. Rolling keys is faster than the time required to finish the
rollover procedure. This scenario is covered by the recursive
implementation, and checking for a chain of direct dependencies
will suffice.
2. Changing the policy, when a zone is requested to be signed with
a different key length for example. BIND 9 will not mark successor
relationships in this case, but tries to move towards the new
policy. Since there is no successor relationship, the rules are
even more strict, and the DNSSEC reconfiguration is actually slower
than required.
Note: this commit breaks the build, because the function definition
of 'keymgr_key_is_successor' changed. This will be fixed in the
following commit.
(cherry picked from commit cc38527b63)
Since keys now have their goals initialized in 'keymgr_key_init()',
remove this redundant piece of code in 'keymgr_key_run()'.
(cherry picked from commit 82632fa6d9)
The 'key_init()' function is used to initialize a state file for keys
that don't have one yet. This can happen if you are migrating from a
'auto-dnssec' or 'inline-signing' to a 'dnssec-policy' configuration.
It did not look at the "Inactive" and "Delete" timing metadata and so
old keys left behind in the key directory would also be considered as
a possible active key. This commit fixes this and now explicitly sets
the key goal to OMNIPRESENT for keys that have their "Active/Publish"
timing metadata in the past, but their "Inactive/Delete" timing
metadata in the future. If the "Inactive/Delete" timing metadata is
also in the past, the key goal is set to HIDDEN.
If the "Inactive/Delete" timing metadata is in the past, also the
key states are adjusted to either UNRETENTIVE or HIDDEN, depending on
how far in the past the metadata is set.
(cherry picked from commit 76cf72e65a)
This commit fix a race that could happen when two or more threads have
failed to refresh the same RRset, the threads could simultaneously
attempt to update the header->last_refresh_fail_ts field in
check_stale_header, a field used to implement stale-refresh-time.
By making this field atomic we avoid such race.
(cherry picked from commit c75575e350)
First of all, there was a flaw in the code related to the
'stale-refresh-time' option. If stale answers are enabled, and we
returned stale data, then it was assumed that it was because we were
in the 'stale-refresh-time' window. But now we could also have returned
stale data because of a 'stale-answer-client-timeout'. To fix this,
introduce a rdataset attribute DNS_RDATASETATTR_STALE_WINDOW to
indicate whether the stale cache entry was returned because the
'stale-refresh-time' window is active.
Second, remove the special case handling when the result is
DNS_R_NCACHENXRRSET. This can be done more generic in the code block
when dealing with stale data.
Putting all stale case handling in the code block when dealing with
stale data makes the code more easy to follow.
Update documentation to be more verbose and to match then new code
flow.
(cherry picked from commit fa0c9280d2)
The general logic behind the addition of this new feature works as
folows:
When a client query arrives, the basic path (query.c / ns_query_recurse)
was to create a fetch, waiting for completion in fetch_callback.
With the introduction of stale-answer-client-timeout, a new event of
type DNS_EVENT_TRYSTALE may invoke fetch_callback, whenever stale
answers are enabled and the fetch took longer than
stale-answer-client-timeout to complete.
When an event of type DNS_EVENT_TRYSTALE triggers fetch_callback, we
must ensure that the folowing happens:
1. Setup a new query context with the sole purpose of looking up for
stale RRset only data, for that matters a new flag was added
'DNS_DBFIND_STALEONLY' used in database lookups.
. If a stale RRset is found, mark the original client query as
answered (with a new query attribute named NS_QUERYATTR_ANSWERED),
so when the fetch completion event is received later, we avoid
answering the client twice.
. If a stale RRset is not found, cleanup and wait for the normal
fetch completion event.
2. In ns_query_done, we must change this part:
/*
* If we're recursing then just return; the query will
* resume when recursion ends.
*/
if (RECURSING(qctx->client)) {
return (qctx->result);
}
To this:
if (RECURSING(qctx->client) && !QUERY_STALEONLY(qctx->client)) {
return (qctx->result);
}
Otherwise we would not proceed to answer the client if it happened
that a stale answer was found when looking up for stale only data.
When an event of type DNS_EVENT_FETCHDONE triggers fetch_callback, we
proceed as before, resuming query, updating stats, etc, but a few
exceptions had to be added, most important of which are two:
1. Before answering the client (ns_client_send), check if the query
wasn't already answered before.
2. Before detaching a client, e.g.
isc_nmhandle_detach(&client->reqhandle), ensure that this is the
fetch completion event, and not the one triggered due to
stale-answer-client-timeout, so a correct call would be:
if (!QUERY_STALEONLY(client)) {
isc_nmhandle_detach(&client->reqhandle);
}
Other than these notes, comments were added in code in attempt to make
these updates easier to follow.
(cherry picked from commit 171a5b7542)
Since it takes a couple lines of code to check whether stale answers
are enabled for a given view, code was extracted out to a proper
function.
(cherry picked from commit 74840ec50b)
This is a minor performance improvement, we store the result of the
first call to strlcat to use as an offset in the next call when
constructing fctx->info string.
(cherry picked from commit 49c40827f6)
When 'opensslecdsa_parse()' encounters a label tag in the private key
file, load the private key with 'opensslecdsa_fromlabel()'. Otherwise
load it from the private structure.
This was attempted before with 'load_privkey()' and 'uses_engine()',
but had the same flaw as 'opensslecdsa_fromlabel()' had previously,
that is getting the private and public key separately, juggling with
pointers between EC_KEY and EVP_PKEY, did not create a valid
cryptographic key that could be used for signing.
(cherry picked from commit 57ac70ad46)
The 'opensslecdsa_fromlabel()' function does not need to get the
OpenSSL engine twice to load the private and public key. Also no need
to call 'dst_key_to_eckey()' as the EC_KEY can be derived from the
loaded EVP_PKEY's.
Add some extra checks to ensure the key has the same base id and curve
(group nid) as the dst key.
Since we already have the EVP_PKEY, no need to call 'finalize_eckey()',
instead just set the right values in the key structure.
(cherry picked from commit 393052d6ff)
The 'ecdsa_check()' function tries to correctly set the public key
on the eckey, but this should be skipped if the public key is
retrieved via the private key.
(cherry picked from commit 06b9724152)
The 'opensslecdsa_tofile()' function tags the label as an RSA label,
that is a copy paste error and should be of course an ECDSA label.
(cherry picked from commit 46afeca8bf)
The functions 'load_pubkey_from_engine()' and
'load_privkey_from_engine()' did not correctly store the pointers.
Update both functions to add 'EC_KEY_set_public_key()' and
'EC_KEY_set_private_key()' respectively, so that the pointers to
the public and private keys survive the "load from engine" functions.
(cherry picked from commit 01239691a1)
The 'function load_pubkey_from_engine()' made a call to the libssl
function 'ENGINE_load_private_key'. This is a copy paste error and
should be 'ENGINE_load_public_key'.
(cherry picked from commit 370285a62d)
- change name of 'bytes' to 'xfrsize' in dns_db_getsize() parameter list
and related variables; this is a more accurate representation of what
the function is doing
- change the size calculations in dns_db_getsize() to more accurately
represent the space needed for a *XFR message or journal file to contain
the data in the database. previously we returned the sizes of all
rdataslabs, including header overhead and offset tables, which
resulted in the database size being reported as much larger than the
equivalent *XFR or journal.
- map files caused a particular problem here: the fullname can't be
determined from the node while a file is being deserialized, because
the uppernode pointers aren't set yet. so we store "full name length"
in the dns_rbtnode structure while serializing, and clear it after
deserialization is complete.
the call initailizing a journal iterator can now optionally return
to the caller the size in bytes of an IXFR message (not including
DNS header overhead, signatures etc) containing the differences from
the beginning to the ending serial number.
this is calculated by scanning the journal transaction headers to
calculate the transfer size. since journal file records contain a length
field that is not included in IXFR messages, we subtract out the length
of those fields from the overall transaction length.
this necessitated adding an "RR count" field to the journal transaction
header, so we know how many length fields to subract. NOTE: this will
make existing journal files stop working!
The BIND 9 libraries are considered to be internal only and hence the
API and ABI changes a lot. Keeping track of the API/ABI changes takes
time and it's a complicated matter as the safest way to make everything
stable would be to bump any library in the dependency chain as in theory
if libns links with libdns, and a binary links with both, and we bump
the libdns SOVERSION, but not the libns SOVERSION, the old libns might
be loaded by binary pulling old libdns together with new libdns loaded
by the binary. The situation gets even more complicated with loading
the plugins that have been compiled with few versions old BIND 9
libraries and then dynamically loaded into the named.
We are picking the safest option possible and usable for internal
libraries - instead of using -version-info that has only a weak link to
BIND 9 version number, we are using -release libtool option that will
embed the corresponding BIND 9 version number into the library name.
That means that instead of libisc.so.1608 (as an example) the library
will now be named libisc-9.16.10.so.
(cherry picked from commit c605d75ea5)
as "type primary" is preferred over "type master" now, it makes
sense to make "primaries" available as a synonym too.
added a correctness check to ensure "primaries" and "masters"
cannot both be used in the same zone.
(cherry picked from commit 16e14353b1)
It is possible to have two threads destroying an rbtdb at the same
time when detachnode() executes and removes the last reference to
a node between exiting being set to true for the node and testing
if the references are zero in maybe_free_rbtdb(). Move NODE_UNLOCK()
to after checking if references is zero to prevent detachnode()
changing the reference count too early.
(cherry picked from commit 859d2fdad6)
While fixing #2359, 'report()' was changed so that it would print the
newline.
Newlines were missing from the output of 'dnssec-signzone'
and 'dnssec-verify' because change
664b8f04f5 moved the printing from
newlines to the library.
This had to be reverted because this also would print redundant
newlines in logfiles.
While doing the revert, some newlines in 'lib/dns/zoneverify.c'
were left in place, now making 'dnssec-signzone' and 'dnssec-verify'
print too many newlines.
This commit removes those newlines, so that the output looks nice
again.
(cherry picked from commit 18c62a077e)
The keymgr prevented zones from going to insecure mode. If we
have a policy with an empty key list this is a signal that the zone
wants to go back to insecure mode. In this case allow one extra state
transition to be valid when checking for DNSSEC safety.
(cherry picked from commit 9134100069)
Check if zone is transitioning from secure to insecure. If so,
delete the CDS/CDNSKEY records, otherwise make sure they are not
part of the RRset.
(cherry picked from commit 68d715a229)
Configure "none" as a builtin policy. Change the 'cfg_kasp_fromconfig'
api so that the 'name' will determine what policy needs to be
configured.
When transitioning a zone from secure to insecure, there will be
cases when a zone with no DNSSEC policy (dnssec-policy none) should
be using KASP. When there are key state files available, this is an
indication that the zone once was DNSSEC signed but is reconfigured
to become insecure.
If we would not run the keymgr, named would abruptly remove the
DNSSEC records from the zone, making the zone bogus. Therefore,
change the code such that a zone will use kasp if there is a valid
dnssec-policy configured, or if there are state files available.
(cherry picked from commit cf420b2af0)
