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.

diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c
index 88f47142ae9e683e5d6adef5906e6cdd827ad38a..6eba231ca65cad743275549d9c5501be848a23c7 100644 (file)
--- a/lib/dns/resolver.c
+++ b/lib/dns/resolver.c
@@ -8859,16 +8859,6 @@ rctx_answer_cname(respctx_t *rctx) {
                return ISC_R_COMPLETE;
        }
 
-       if (rctx->type == dns_rdatatype_rrsig ||
-           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 482909b33d4232aeff09f5f906ba8ef3299b0e22..3034a4478aa6514de783e81576621a1cf632243b 100644 (file)
--- a/lib/dns/validator.c
+++ b/lib/dns/validator.c
@@ -963,26 +963,52 @@ notfound:
 }
 
 /*%
- * 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 && cur->rdataset->attributes.chaining)
                {
+                       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");
@@ -1008,8 +1034,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;
        }