Detect non-advancing alias chains in the validator

The resolver turned a CNAME response to an RRSIG or NSEC query into
FORMERR inside rctx_answer_cname().  That is redundant -- every caller
already copes with a DNS_R_CNAME or DNS_R_DNAME result -- and it is the
wrong layer, because the resolver cannot tell a legitimate alias from a
broken one.  Drop it; a CNAME for one of these types now flows back as
an ordinary alias.

The case that must be stopped lives in the validator.  While proving an
unsigned CNAME insecure, proveunsecure() fetches the DS for the CNAME's
own name; because fetches are shared, that fetch re-enters and stalls on
the in-flight fetch the validator is waiting for, deadlocking for about
twelve seconds (GL#5878).  Unlike the resolver, the validator knows it
is validating an alias, so check_chaining() now aborts a fetch whose
name matches the chaining rdataset's owner: it cannot advance the chain
and would only self-join.

(cherry picked from commit d0c6219d66e9b5742d0d7d8cb66752828880d74f)

diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c
index ab6843659a6f81b90f61fcdbafebc087dd266493..176de96b132822eb3d2aaedfd9f5fe799b767db0 100644 (file)
--- a/lib/dns/resolver.c
+++ b/lib/dns/resolver.c
@@ -8954,16 +8954,6 @@ rctx_answer_cname(respctx_t *rctx) {
                return ISC_R_COMPLETE;
        }
 
-       if (rctx->type == dns_rdatatype_rrsig ||
-           rctx->type == dns_rdatatype_key || rctx->type == dns_rdatatype_nsec)
-       {
-               char buf[DNS_RDATATYPE_FORMATSIZE];
-               dns_rdatatype_format(rctx->type, buf, sizeof(buf));
-               log_formerr(fctx, "CNAME response for %s RR", buf);
-               rctx->result = DNS_R_FORMERR;
-               return ISC_R_COMPLETE;
-       }
-
        if (!is_answertarget_allowed(fctx, fctx->name, rctx->cname,
                                     rctx->crdataset, NULL))
        {

diff --git a/lib/dns/validator.c b/lib/dns/validator.c
index dc7b990fc1dce97a2966be88b7095e7e7ba943b9..0fafb68c82652b6b66924701de88e84dfa537a86 100644 (file)
--- a/lib/dns/validator.c
+++ b/lib/dns/validator.c
@@ -960,27 +960,54 @@ notfound:
        return result;
 }
 
+#define CHAINING(r) (((r)->attributes & DNS_RDATASETATTR_CHAINING) != 0)
+
 /*%
- * Checks to make sure we are not going to loop.  As we use a SHARED fetch
- * the validation process will stall if looping was to occur.
+ * Returns true if proceeding would stall the SHARED fetch: either the
+ * fetch cannot advance an alias chain, or an ancestor is already
+ * resolving the same (name, type).
  */
 static bool
 check_deadlock(dns_validator_t *val, dns_name_t *name, dns_rdatatype_t type,
               dns_rdataset_t *rdataset, dns_rdataset_t *sigrdataset) {
-       dns_validator_t *parent;
-
-       for (parent = val; parent != NULL; parent = parent->parent) {
-               if (parent->type == type &&
-                   dns_name_equal(parent->name, name) &&
-                   /*
-                    * As NSEC3 records are meta data you sometimes
-                    * need to prove a NSEC3 record which says that
-                    * itself doesn't exist.
-                    */
-                   (parent->type != dns_rdatatype_nsec3 || rdataset == NULL ||
-                    sigrdataset == NULL || parent->message == NULL ||
-                    parent->rdataset != NULL || parent->sigrdataset != NULL))
+       for (dns_validator_t *cur = val; cur != NULL; cur = cur->parent) {
+               if (!dns_name_equal(cur->name, name)) {
+                       continue;
+               }
+
+               /*
+                * Validating a CNAME/DNAME ("chaining" rdataset): a fetch at
+                * the alias's own name cannot advance the chain (the type we
+                * need, e.g. DS/DNSKEY for an insecurity proof, cannot live at
+                * an alias) and would only self-join the in-flight fetch.
+                */
+               if (cur->rdataset != NULL && CHAINING(cur->rdataset)) {
+                       validator_log(
+                               val, ISC_LOG_DEBUG(3),
+                               "fetch would not advance the alias chain: "
+                               "aborting validation");
+                       return true;
+               }
+
+               /*
+                * Not a loop: NSEC3 is meta data, so proving a name's
+                * nonexistence can need the NSEC3 RRset that proves it
+                * validated at its own owner name.  Allow that when we hold a
+                * concrete RRset and the ancestor runs a message-driven
+                * nonexistence proof.
+                */
+               if (cur->type == dns_rdatatype_nsec3 && rdataset != NULL &&
+                   sigrdataset != NULL && cur->message != NULL &&
+                   cur->rdataset == NULL && cur->sigrdataset == NULL)
                {
+                       continue;
+               }
+
+               /*
+                * An ancestor at this name is already validating this type; a
+                * shared fetch would block on itself.
+                */
+               if (cur->type == type) {
                        validator_log(val, ISC_LOG_DEBUG(3),
                                      "continuing validation would lead to "
                                      "deadlock: aborting validation");
@@ -990,6 +1017,8 @@ check_deadlock(dns_validator_t *val, dns_name_t *name, dns_rdatatype_t type,
        return false;
 }
 
+#undef CHAINING
+
 /*%
  * Start a fetch for the requested name and type.
  */
@@ -1002,8 +1031,6 @@ create_fetch(dns_validator_t *val, dns_name_t *name, dns_rdatatype_t type,
        disassociate_rdatasets(val);
 
        if (check_deadlock(val, name, type, NULL, NULL)) {
-               validator_log(val, ISC_LOG_DEBUG(3),
-                             "deadlock found (create_fetch)");
                return ISC_R_DEADLOCK;
        }