From: Vladimír Čunát <vladimir.cunat@nic.cz>
Date: Thu, 28 Mar 2019 07:34:26 +0000 (+0100)
Subject: validate nitpick fix: unsupported algo edge case
X-Git-Tag: v4.0.0~13^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2bd31a489688ebdba4ba97782aacc44afc8a8b52;p=thirdparty%2Fknot-resolver.git

validate nitpick fix: unsupported algo edge case

kr_dnskeys_trusted() semantics is changed, but I do NOT consider that
a part of public API.

Go insecure due to algorithm support even if DNSKEY is NODATA.
I can't see how that's relevant to practical usage, but I think this new
behavior makes more sense.  We still do try to fetch the DNSKEY even
though we have information about its un-usability beforehand.
I'd consider fixing that a premature optimization.
We'll still be affected if the DNSKEY query SERVFAILs or something.

Thanks to PowerDNS people for catching this!
---

diff --git a/NEWS b/NEWS
index f6a025441..33b8e6859 100644
--- a/NEWS
+++ b/NEWS
@@ -37,6 +37,7 @@ Bugfixes
 - fix flushing of messages to logs in some cases (!781)
 - fix fallback when SERVFAIL or REFUSED is received from upstream (!784)
 - fix crash when dealing with unknown TA key algorhitm (#449)
+- go insecure due to algorithm support even if DNSKEY is NODATA (!798)
 
 Module API changes
 ------------------
diff --git a/lib/dnssec.c b/lib/dnssec.c
index 4f8ad8a26..efcdb1fb8 100644
--- a/lib/dnssec.c
+++ b/lib/dnssec.c
@@ -268,8 +268,10 @@ static int kr_rrset_validate_with_key(kr_rrset_validation_ctx_t *vctx,
 	return vctx->result;
 }
 
-static bool kr_ds_algo_support(const knot_rrset_t *ta)
+bool kr_ds_algo_support(const knot_rrset_t *ta)
 {
+	assert(ta && ta->type == KNOT_RRTYPE_DS && ta->rclass == KNOT_CLASS_IN);
+	/* Check if at least one DS has a usable algorithm pair. */
 	knot_rdata_t *rdata_i = ta->rrs.rdata;
 	for (uint16_t i = 0; i < ta->rrs.count;
 			++i, rdata_i = knot_rdataset_next(rdata_i)) {
@@ -293,12 +295,6 @@ int kr_dnskeys_trusted(kr_rrset_validation_ctx_t *vctx, const knot_rrset_t *ta)
 		return kr_error(EINVAL);
 	}
 
-	/* Check if at least one DS has a usable algorithm pair. */
-	if (!kr_ds_algo_support(ta)) {
-		/* See RFC6840 5.2. */
-		return vctx->result = kr_error(DNSSEC_INVALID_DS_ALGORITHM);
-	}
-
 	/* RFC4035 5.2, bullet 1
 	 * The supplied DS record has been authenticated.
 	 * It has been validated or is part of a configured trust anchor.
diff --git a/lib/dnssec.h b/lib/dnssec.h
index b54908c18..8383048b2 100644
--- a/lib/dnssec.h
+++ b/lib/dnssec.h
@@ -81,13 +81,17 @@ typedef struct kr_rrset_validation_ctx kr_rrset_validation_ctx_t;
 int kr_rrset_validate(kr_rrset_validation_ctx_t *vctx,
 			const knot_rrset_t *covered);
 
+/**
+ * Return true iff the RRset contains at least one usable DS.  See RFC6840 5.2.
+ */
+KR_EXPORT KR_PURE
+bool kr_ds_algo_support(const knot_rrset_t *ta);
+
 /**
  * Check whether the DNSKEY rrset matches the supplied trust anchor RRSet.
  * @param vctx  Pointer to validation context.
  * @param ta    Trust anchor RRSet against which to validate the DNSKEY RRSet.
- * @return      0 or error code, same as vctx->result.  In particular,
- * 		DNSSEC_INVALID_DS_ALGORITHM if *each* DS records is unusable
- * 		due to unimplemented DNSKEY or DS algorithm.
+ * @return      0 or error code, same as vctx->result.
  */
 int kr_dnskeys_trusted(kr_rrset_validation_ctx_t *vctx, const knot_rrset_t *ta);
 
diff --git a/lib/layer/validate.c b/lib/layer/validate.c
index 55c3ad774..16e339602 100644
--- a/lib/layer/validate.c
+++ b/lib/layer/validate.c
@@ -972,11 +972,8 @@ static int validate(kr_layer_t *ctx, knot_pkt_t *pkt)
 	}
 
 	if (knot_wire_get_aa(pkt->wire) && qtype == KNOT_RRTYPE_DNSKEY) {
-		ret = validate_keyset(req, pkt, has_nsec3);
-		if (ret == kr_error(EAGAIN)) {
-			VERBOSE_MSG(qry, ">< cut changed, needs revalidation\n");
-			return KR_STATE_YIELD;
-		} else if (ret == kr_error(DNSSEC_INVALID_DS_ALGORITHM)) {
+		const knot_rrset_t *ds = qry->zone_cut.trust_anchor;
+		if (ds && !kr_ds_algo_support(ds)) {
 			VERBOSE_MSG(qry, ">< all DS entries use unsupported algorithm pairs, going insecure\n");
 			/* ^ the message is a bit imprecise to avoid being too verbose */
 			qry->flags.DNSSEC_WANT = false;
@@ -984,6 +981,12 @@ static int validate(kr_layer_t *ctx, knot_pkt_t *pkt)
 			rank_records(ctx, KR_RANK_INSECURE, qry->zone_cut.name);
 			mark_insecure_parents(qry);
 			return KR_STATE_DONE;
+		}
+
+		ret = validate_keyset(req, pkt, has_nsec3);
+		if (ret == kr_error(EAGAIN)) {
+			VERBOSE_MSG(qry, ">< cut changed, needs revalidation\n");
+			return KR_STATE_YIELD;
 		} else if (ret != 0) {
 			VERBOSE_MSG(qry, "<= bad keys, broken trust chain\n");
 			qry->flags.DNSSEC_BOGUS = true;