From 703d918a27b08e88e2e995afebd6c7395311fd91 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Vladim=C3=ADr=20=C4=8Cun=C3=A1t?= Date: Thu, 2 Jul 2020 17:04:08 +0200 Subject: [PATCH] validator: bottom->up chase DS if RRSIG(s) are missing This is about situations when validator *thinks* it's in a signed zone but an unsigned answer comes in. The assumption was that RRSIGs didn't make it through some middle-boxes and it retried with explicit QTYPE=RRSIG. There were two issues with that. 1. It seems that in most cases the cause of the situation is that we skipped over a zone cut that transitioned to insecure state, so the signatures correctly don't exist. 2. An explicit RRSIG query appears to be more trouble than worth; it seems reasonable for servers not to answer it (fully); see RFC 8482 sect. 7. The new approach simply tries to find a proof that the name is insecure, by spawning a QTYPE=DS sub-query on that name. That fixes some real-life cases; usually this happens in iteration mode where one IP address serves zones on both sides of a cut that transitions to insecure. For details see new comments in that rrsig_not_found() function. The change resulted in the iterator fallback not making sense anymore so it was removed. --- NEWS | 1 + lib/layer/iterate.c | 34 +----------------- lib/layer/validate.c | 74 +++++++++++++++++++-------------------- lib/resolve.h | 2 +- tests/integration/deckard | 2 +- 5 files changed, 41 insertions(+), 72 deletions(-) diff --git a/NEWS b/NEWS index 0dc56dfdd..0a00c5e92 100644 --- a/NEWS +++ b/NEWS @@ -13,6 +13,7 @@ Bugfixes - tls: fix compilation to support net.tls_sticket_secret() (!1021) - validator: ignore bogus RRSIGs present in insecure domains (!1022, #587) - build if libsystemd version isn't detected as integer (#592, !1029) +- validator: more robust reaction when RRSIGs are missing unexpectedly (!1020) Knot Resolver 5.1.2 (2020-07-01) diff --git a/lib/layer/iterate.c b/lib/layer/iterate.c index 780e9d567..8b8bc2d20 100644 --- a/lib/layer/iterate.c +++ b/lib/layer/iterate.c @@ -986,36 +986,6 @@ static int resolve_badmsg(knot_pkt_t *pkt, struct kr_request *req, struct kr_que #endif } -static int resolve_notimpl(knot_pkt_t *pkt, struct kr_request *req, struct kr_query *qry) -{ - if (qry->stype == KNOT_RRTYPE_RRSIG && qry->parent != NULL) { - /* RRSIG subquery have got NOTIMPL. - * Possible scenario - same NS is autoritative for child and parent, - * but child isn't signed. - * We got delegation to parent, - * then NS responded as NS for child zone. - * Answer contained record been requested, but no RRSIGs, - * Validator issued RRSIG query then. If qname is zone name, - * we can get NOTIMPL. Ask for DS to find out security status. - * TODO - maybe it would be better to do this in validator, when - * RRSIG revalidation occurs. - */ - struct kr_rplan *rplan = &req->rplan; - struct kr_query *next = kr_rplan_push(rplan, qry->parent, qry->sname, - qry->sclass, KNOT_RRTYPE_DS); - if (!next) { - return KR_STATE_FAIL; - } - kr_zonecut_set(&next->zone_cut, qry->parent->zone_cut.name); - kr_zonecut_copy(&next->zone_cut, &qry->parent->zone_cut); - kr_zonecut_copy_trust(&next->zone_cut, &qry->parent->zone_cut); - next->flags.DNSSEC_WANT = true; - qry->flags.RESOLVED = true; - return KR_STATE_DONE; - } - return resolve_badmsg(pkt, req, qry); -} - /** Resolve input query or continue resolution with followups. * * This roughly corresponds to RFC1034, 5.3.3 4a-d. @@ -1101,11 +1071,9 @@ static int resolve(kr_layer_t *ctx, knot_pkt_t *pkt) } /* fall through */ case KNOT_RCODE_FORMERR: - VERBOSE_MSG("<= rcode: %s\n", rcode ? rcode->name : "??"); - return resolve_badmsg(pkt, req, query); case KNOT_RCODE_NOTIMPL: VERBOSE_MSG("<= rcode: %s\n", rcode ? rcode->name : "??"); - return resolve_notimpl(pkt, req, query); + return resolve_badmsg(pkt, req, query); default: VERBOSE_MSG("<= rcode: %s\n", rcode ? rcode->name : "??"); return resolve_error(pkt, req); diff --git a/lib/layer/validate.c b/lib/layer/validate.c index d36860920..11299ec8c 100644 --- a/lib/layer/validate.c +++ b/lib/layer/validate.c @@ -551,57 +551,57 @@ static const knot_dname_t *signature_authority(struct kr_request *req) return signer_name; } -static int rrsig_not_found(kr_layer_t *ctx, const knot_rrset_t *rr) +static int rrsig_not_found(kr_layer_t *ctx, const knot_pkt_t *pkt, const knot_rrset_t *rr) { + /* Signatures are missing. There might be a zone cut that we've skipped + * and transitions to insecure. That can commonly happen when iterating + * and both sides of that cut are served by the same IP address(es). + * We'll try proving that the name truly is insecure - by spawning + * a DS sub-query on a suitable QNAME. + */ struct kr_request *req = ctx->req; struct kr_query *qry = req->current_query; - /* Parent-side record, so don't ask for RRSIG. - * We won't receive it anyway. */ - if (qry->stype == KNOT_RRTYPE_DS) { + if (qry->flags.FORWARD || qry->flags.STUB) { + /* Undiscovered signed cuts can't happen in the current forwarding + * algorithm, so this function shouldn't be able to help. */ return KR_STATE_FAIL; } - struct kr_zonecut *cut = &qry->zone_cut; - const knot_dname_t *cut_name_start = qry->zone_cut.name; - bool use_cut = true; - if (knot_dname_in_bailiwick(rr->owner, cut_name_start) < 0) { - int zone_labels = knot_dname_labels(qry->zone_cut.name, NULL); - int matched_labels = knot_dname_matched_labels(qry->zone_cut.name, rr->owner); - int skip_labels = zone_labels - matched_labels; - while (skip_labels--) { - cut_name_start = knot_wire_next_label(cut_name_start, NULL); - } - /* try to find the name wanted among ancestors */ - use_cut = false; - while (cut->parent) { - cut = cut->parent; - if (knot_dname_is_equal(cut_name_start, cut->name)) { - use_cut = true; + /* Find cut_next: the name at which to try finding the "missing" zone cut. */ + const knot_dname_t *cut_next = rr->owner; + const knot_dname_t * const cut_top = qry->zone_cut.name; + const int next_depth = knot_dname_in_bailiwick(rr->owner, cut_top); + if (next_depth <= 0) { + return KR_STATE_FAIL; // shouldn't happen, I think + } else if (next_depth > 1) { + /* Try finding SOA. That can speed up things in deeper zones. */ + const knot_pktsection_t *sec = knot_pkt_section(pkt, KNOT_AUTHORITY); + for (int i = sec->pos; i < sec->pos + sec->count; ++i) { + if (pkt->rr[i].type != KNOT_RRTYPE_SOA) continue; + const knot_dname_t *owner = pkt->rr[i].owner; + if (knot_dname_in_bailiwick(owner, cut_top) >= 1 + && knot_dname_in_bailiwick(cut_next, owner) >= 1) { + cut_next = owner; break; } - }; + } } - struct kr_rplan *rplan = &req->rplan; - struct kr_query *next = kr_rplan_push(rplan, qry, rr->owner, rr->rclass, KNOT_RRTYPE_RRSIG); + + /* Spawn that DS sub-query. */ + struct kr_query *next = kr_rplan_push(&req->rplan, qry, cut_next, + rr->rclass, KNOT_RRTYPE_DS); if (!next) { return KR_STATE_FAIL; } - kr_zonecut_init(&next->zone_cut, cut_name_start, &req->pool); - if (use_cut) { - kr_zonecut_copy(&next->zone_cut, cut); - kr_zonecut_copy_trust(&next->zone_cut, cut); - } else { - next->flags.AWAIT_CUT = true; - } - if (qry->flags.FORWARD) { - next->flags.AWAIT_CUT = false; - } + kr_zonecut_init(&next->zone_cut, qry->zone_cut.name, &req->pool); + kr_zonecut_copy(&next->zone_cut, &qry->zone_cut); + kr_zonecut_copy_trust(&next->zone_cut, &qry->zone_cut); next->flags.DNSSEC_WANT = true; return KR_STATE_YIELD; } -static int check_validation_result(kr_layer_t *ctx, ranked_rr_array_t *arr) +static int check_validation_result(kr_layer_t *ctx, const knot_pkt_t *pkt, ranked_rr_array_t *arr) { int ret = KR_STATE_DONE; struct kr_request *req = ctx->req; @@ -654,7 +654,7 @@ static int check_validation_result(kr_layer_t *ctx, ranked_rr_array_t *arr) VERBOSE_MSG(qry, ">< cut changed (new signer), needs revalidation\n"); ret = KR_STATE_YIELD; } else if (kr_rank_test(invalid_entry->rank, KR_RANK_MISSING)) { - ret = rrsig_not_found(ctx, rr); + ret = rrsig_not_found(ctx, pkt, rr); } else if (!kr_rank_test(invalid_entry->rank, KR_RANK_SECURE)) { qry->flags.DNSSEC_BOGUS = true; ret = KR_STATE_FAIL; @@ -1118,13 +1118,13 @@ static int validate(kr_layer_t *ctx, knot_pkt_t *pkt) } /* check validation state and spawn subrequests */ if (!req->answ_validated) { - ret = check_validation_result(ctx, &req->answ_selected); + ret = check_validation_result(ctx, pkt, &req->answ_selected); if (ret != KR_STATE_DONE) { return ret; } } if (!req->auth_validated) { - ret = check_validation_result(ctx, &req->auth_selected); + ret = check_validation_result(ctx, pkt, &req->auth_selected); if (ret != KR_STATE_DONE) { return ret; } diff --git a/lib/resolve.h b/lib/resolve.h index 3a55fefee..bf75b18f9 100644 --- a/lib/resolve.h +++ b/lib/resolve.h @@ -90,7 +90,7 @@ enum kr_rank { KR_RANK_INDET = 4, /**< Unable to determine whether it should be secure. */ KR_RANK_BOGUS, /**< Ought to be secure but isn't. */ KR_RANK_MISMATCH, - KR_RANK_MISSING, /**< Unable to obtain a good signature. */ + KR_RANK_MISSING, /**< No RRSIG found for that owner+type combination. */ /** Proven to be insecure, i.e. we have a chain of trust from TAs * that cryptographically denies the possibility of existence diff --git a/tests/integration/deckard b/tests/integration/deckard index 594d3b878..fe30083ea 160000 --- a/tests/integration/deckard +++ b/tests/integration/deckard @@ -1 +1 @@ -Subproject commit 594d3b8784f843beb8f1c095f5b9878fc1e84519 +Subproject commit fe30083ea07122125062d53a4b055b967b39794c -- 2.47.2