From: Petr Špaček <petr.spacek@nic.cz>
Date: Tue, 7 Apr 2020 13:55:52 +0000 (+0200)
Subject: validator: use rank BOGUS where appropriate instead of MISSING
X-Git-Tag: v5.1.0~13^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=cce8d9355;p=thirdparty%2Fknot-resolver.git

validator: use rank BOGUS where appropriate instead of MISSING

MISSING triggers re-query to auth in attempt to find missing RRSIGs.
It causes reduntant queries and also puts some BOGUS RRsets in answers.
(It sounds bad but we were correctly setting rcode=SERVFAIL and AD=0
even before this commit.)

Formerly RRSIG ranks did not reflect results of validation.
Now we mark them as BOGUS and upgrade them to SECURE if they validate.

New validator phase answer_finalize prevents BOGUS RRsets from being
put even into SERVFAIL answers.

Closes: #396
---

diff --git a/NEWS b/NEWS
index 355efbd95..9bedf6710 100644
--- a/NEWS
+++ b/NEWS
@@ -16,6 +16,7 @@ Bugfixes
 - fix a strict aliasing problem that might've lead to "miscompilation" (!962)
 - lua resolve(): correctly include EDNS0 in the virtual packet (!963)
   Custom modules might have been confused by that.
+- do not leak bogus data into SERVFAIL answers (#396)
 
 Incompatible changes
 --------------------
diff --git a/lib/cache/api.c b/lib/cache/api.c
index 9962fcc78..df5c574ab 100644
--- a/lib/cache/api.c
+++ b/lib/cache/api.c
@@ -477,6 +477,13 @@ static ssize_t stash_rrset(struct kr_cache *cache, const struct kr_query *qry,
 		const knot_rrset_t *rr, const knot_rrset_t *rr_sigs, uint32_t timestamp,
 		uint8_t rank, trie_t *nsec_pmap, bool *needs_pkt)
 {
+	if (kr_rank_test(rank, KR_RANK_BOGUS)) {
+		WITH_VERBOSE(qry) {
+			auto_free char *type_str = kr_rrtype_text(rr->type);
+			VERBOSE_MSG(qry, "=> skipping bogus RR set %s\n", type_str);
+		}
+		return kr_ok();
+	}
 	assert(stash_rrset_precond(rr, qry) > 0);
 	if (!cache) {
 		assert(!EINVAL);
diff --git a/lib/dnssec.c b/lib/dnssec.c
index 43b0405a9..50b53f2e0 100644
--- a/lib/dnssec.c
+++ b/lib/dnssec.c
@@ -208,6 +208,7 @@ static int kr_rrset_validate_with_key(kr_rrset_validation_ctx_t *vctx,
 			if (knot_rrsig_type_covered(rdata_j) != covered->type) {
 				continue;
 			}
+			kr_rank_set(&vctx->rrs->at[i]->rank, KR_RANK_BOGUS); /* defensive style */
 			vctx->rrs_counters.matching_name_type++;
 			int retv = validate_rrsig_rr(&val_flgs, covered_labels, rdata_j,
 			                      keys->owner, key_rdata, keytag,
@@ -265,6 +266,7 @@ static int kr_rrset_validate_with_key(kr_rrset_validation_ctx_t *vctx,
 
 			kr_dnssec_key_free(&created_key);
 			vctx->result = kr_ok();
+			kr_rank_set(&vctx->rrs->at[i]->rank, KR_RANK_SECURE); /* upgrade from bogus */
 			return vctx->result;
 		}
 	}
diff --git a/lib/layer/validate.c b/lib/layer/validate.c
index 5f01d2326..c60858b43 100644
--- a/lib/layer/validate.c
+++ b/lib/layer/validate.c
@@ -115,7 +115,8 @@ static int validate_section(kr_rrset_validation_ctx_t *vctx, const struct kr_que
 				vctx->err_cnt += 1;
 				break;
 			}
-			kr_rank_set(&entry->rank, KR_RANK_OMIT);
+			if (!kr_rank_test(entry->rank, KR_RANK_BOGUS))
+				kr_rank_set(&entry->rank, KR_RANK_OMIT);
 			continue;
 		}
 
@@ -137,7 +138,8 @@ static int validate_section(kr_rrset_validation_ctx_t *vctx, const struct kr_que
 			 * to MISMATCH on revalidation, e.g. in test val_referral_nods :-/
 			 */
 
-		} else if (validation_result == kr_error(ENOENT)) {
+		} else if (validation_result == kr_error(ENOENT)
+				&& vctx->rrs_counters.matching_name_type == 0) {
 			/* no RRSIGs found */
 			kr_rank_set(&entry->rank, KR_RANK_MISSING);
 			vctx->err_cnt += 1;
@@ -1110,11 +1112,40 @@ static int validate(kr_layer_t *ctx, knot_pkt_t *pkt)
 	return KR_STATE_DONE;
 }
 
+/** Hide RRsets which did not validate from clients. */
+static int hide_bogus(kr_layer_t *ctx) {
+	if (knot_wire_get_cd(ctx->req->qsource.packet->wire)) {
+		return ctx->state;
+	}
+	/* We don't want to send bogus answers to clients, not even in SERVFAIL
+	 * answers, but we cannot drop whole sections. If a CNAME chain
+	 * SERVFAILs somewhere, the steps that were OK should be put into
+	 * answer.
+	 *
+	 * There is one specific issue: currently we follow CNAME *before*
+	 * we validate it, because... iterator comes before validator.
+	 * Therefore some rrsets might be added into req->*_selected before
+	 * we detected failure in validator.
+	 * TODO: better approach, probably during work on parallel queries.
+	 */
+	const ranked_rr_array_t *sel[] = kr_request_selected(ctx->req);
+	for (knot_section_t sect = KNOT_ANSWER; sect <= KNOT_ADDITIONAL; ++sect) {
+		 for (size_t i = 0; i < sel[sect]->len; ++i) {
+			  ranked_rr_array_entry_t *e = sel[sect]->at[i];
+			  if (kr_rank_test(e->rank, KR_RANK_BOGUS)) {
+				   e->to_wire = false;
+			  }
+		 }
+	}
+	return ctx->state;
+}
+
 /** Module implementation. */
 int validate_init(struct kr_module *self)
 {
 	static const kr_layer_api_t layer = {
 		.consume = &validate,
+		.answer_finalize = &hide_bogus,
 	};
 	self->layer = &layer;
 	return kr_ok();
diff --git a/tests/integration/deckard b/tests/integration/deckard
index 84d16f4bf..5f50864d6 160000
--- a/tests/integration/deckard
+++ b/tests/integration/deckard
@@ -1 +1 @@
-Subproject commit 84d16f4bf169e1365e6e2bc68efd44592a7e37a1
+Subproject commit 5f50864d6f4ca79ca77f91d65eea4b23c18736c4