From: Markus Koschany Date: Mon, 11 Mar 2024 14:16:29 +0100 Subject: CVE-2019-10190 Bug-Debian: https://bugs.debian.org/932048 Origin: https://gitlab.labs.nic.cz/knot/knot-resolver/merge_requests/827 --- lib/layer/iterate.c | 2 +- lib/resolve.c | 96 +++++++++++++++++++++++------------------ lib/resolve.h | 2 +- modules/cookies/cookiemonster.c | 4 +- modules/hints/hints.c | 2 +- 5 files changed, 59 insertions(+), 47 deletions(-) diff --git a/lib/layer/iterate.c b/lib/layer/iterate.c index f7dd630..ee2607e 100644 --- a/lib/layer/iterate.c +++ b/lib/layer/iterate.c @@ -557,7 +557,7 @@ static int unroll_cname(knot_pkt_t *pkt, struct kr_request *req, bool referral, } else { int cnt_ = 0; state = update_nsaddr(rr, query->parent, &cnt_); - if (state == KR_STATE_FAIL) { + if (state & KR_STATE_FAIL) { return state; } } diff --git a/lib/resolve.c b/lib/resolve.c index 8718f25..b4b3657 100644 --- a/lib/resolve.c +++ b/lib/resolve.c @@ -458,7 +458,7 @@ static int answer_prepare(struct kr_request *req, knot_pkt_t *query) static int write_extra_records(const rr_array_t *arr, uint16_t reorder, knot_pkt_t *answer) { for (size_t i = 0; i < arr->len; ++i) { - int err = knot_pkt_put_rotate(answer, 0, arr->at[i], reorder, 0); + int err = knot_pkt_put_rotate(answer, 0, arr->at[i], reorder, KNOT_PF_NOTRUNC); if (err != KNOT_EOK) { return err == KNOT_ESPACE ? kr_ok() : kr_error(err); } @@ -548,42 +548,52 @@ static int answer_padding(struct kr_request *request) return kr_ok(); } -static int answer_fail(struct kr_request *request) +/* Make a clean SERVFAIL answer. */ +static void answer_fail(struct kr_request *request) { + /* Note: OPT in SERVFAIL response is still useful for cookies/additional info. */ knot_pkt_t *answer = request->answer; + knot_rrset_t *opt_rr = answer->opt_rr; /* it gets NULLed below */ int ret = kr_pkt_clear_payload(answer); knot_wire_clear_ad(answer->wire); knot_wire_clear_aa(answer->wire); knot_wire_set_rcode(answer->wire, KNOT_RCODE_SERVFAIL); - if (ret == 0 && answer->opt_rr) { - /* OPT in SERVFAIL response is still useful for cookies/additional info. */ + if (ret == 0 && opt_rr) { knot_pkt_begin(answer, KNOT_ADDITIONAL); answer_padding(request); /* Ignore failed padding in SERVFAIL answer. */ - ret = edns_put(answer, false); + answer->opt_rr = opt_rr; + edns_put(answer, false); } - return ret; } -static int answer_finalize(struct kr_request *request, int state) +static void answer_finalize(struct kr_request *request) { struct kr_rplan *rplan = &request->rplan; knot_pkt_t *answer = request->answer; - /* Always set SERVFAIL for bogus answers. */ - if (state == KR_STATE_FAIL && rplan->pending.len > 0) { - struct kr_query *last = array_tail(rplan->pending); - if ((last->flags.DNSSEC_WANT) && (last->flags.DNSSEC_BOGUS)) { - return answer_fail(request); - } - } - - struct kr_query *last = rplan->resolved.len > 0 ? array_tail(rplan->resolved) : NULL; + struct kr_query *const last = + rplan->resolved.len > 0 ? array_tail(rplan->resolved) : NULL; /* TODO ^^^^ this is slightly fragile */ + if (!last) { + /* Suspicious: no kr_query got resolved (not even from cache), + * so let's (defensively) SERVFAIL the request. + * ATM many checks below depend on `last` anyway, + * so this helps to avoid surprises. */ + answer_fail(request); + return; + } + /* TODO: clean this up in !660 or followup, and it isn't foolproof anyway. */ + if (last->flags.DNSSEC_BOGUS + || (rplan->pending.len > 0 && array_tail(rplan->pending)->flags.DNSSEC_BOGUS)) { + answer_fail(request); + return; + } + /* AD flag. We can only change `secure` from true to false. * Be conservative. Primary approach: check ranks of all RRs in wire. * Only "negative answers" need special handling. */ - bool secure = last != NULL && state == KR_STATE_DONE /*< suspicious otherwise */ + bool secure = last != NULL && request->state == KR_STATE_DONE /*< suspicious otherwise */ && knot_pkt_qtype(answer) != KNOT_RRTYPE_RRSIG; if (last && (last->flags.STUB)) { secure = false; /* don't trust forwarding for now */ @@ -604,7 +614,8 @@ static int answer_finalize(struct kr_request *request, int state) if (write_extra_ranked_records(&request->answ_selected, reorder, answer, &secure, &answ_all_cnames)) { - return answer_fail(request); + answer_fail(request); + return; } } @@ -614,25 +625,29 @@ static int answer_finalize(struct kr_request *request, int state) } if (write_extra_ranked_records(&request->auth_selected, reorder, answer, &secure, NULL)) { - return answer_fail(request); + answer_fail(request); + return; } /* Write additional records. */ knot_pkt_begin(answer, KNOT_ADDITIONAL); if (write_extra_records(&request->additional, reorder, answer)) { - return answer_fail(request); + answer_fail(request); + return; } /* Write EDNS information */ if (answer->opt_rr) { if (request->qsource.flags.tls) { if (answer_padding(request) != kr_ok()) { - return answer_fail(request); + answer_fail(request); + return; } } knot_pkt_begin(answer, KNOT_ADDITIONAL); int ret = knot_pkt_put(answer, KNOT_COMPR_HINT_NONE, answer->opt_rr, KNOT_PF_FREE); if (ret != KNOT_EOK) { - return answer_fail(request); + answer_fail(request); + return; } } @@ -667,8 +682,6 @@ static int answer_finalize(struct kr_request *request, int state) if (!secure) { knot_wire_clear_ad(answer->wire); } - - return kr_ok(); } static int query_finalize(struct kr_request *request, struct kr_query *qry, knot_pkt_t *pkt) @@ -852,7 +865,7 @@ static void update_nslist_score(struct kr_request *request, struct kr_query *qry { struct kr_context *ctx = request->ctx; /* On successful answer, update preference list RTT and penalise timer */ - if (request->state != KR_STATE_FAIL) { + if (!(request->state & KR_STATE_FAIL)) { /* Update RTT information for preference list */ update_nslist_rtt(ctx, qry, src); /* Do not complete NS address resolution on soft-fail. */ @@ -928,7 +941,7 @@ int kr_resolve_consume(struct kr_request *request, const struct sockaddr *src, k update_nslist_score(request, qry, src, packet); } /* Resolution failed, invalidate current NS. */ - if (request->state == KR_STATE_FAIL) { + if (request->state & KR_STATE_FAIL) { invalidate_ns(rplan, qry); qry->flags.RESOLVED = false; } @@ -1283,7 +1296,7 @@ static int zone_cut_check(struct kr_request *request, struct kr_query *qry, knot int state = KR_STATE_FAIL; do { state = ns_fetch_cut(qry, requested_name, request, packet); - if (state == KR_STATE_DONE || state == KR_STATE_FAIL) { + if (state == KR_STATE_DONE || (state & KR_STATE_FAIL)) { return state; } else if (state == KR_STATE_CONSUME) { requested_name = knot_wire_next_label(requested_name, NULL); @@ -1353,7 +1366,7 @@ int kr_resolve_produce(struct kr_request *request, struct sockaddr **dst, int *t /* Resolve current query and produce dependent or finish */ request->state = KR_STATE_PRODUCE; ITERATE_LAYERS(request, qry, produce, packet); - if (request->state != KR_STATE_FAIL && knot_wire_get_qr(packet->wire)) { + if (!(request->state & KR_STATE_FAIL) && knot_wire_get_qr(packet->wire)) { /* Produced an answer from cache, consume it. */ qry->secret = 0; request->state = KR_STATE_CONSUME; @@ -1541,7 +1554,7 @@ int kr_resolve_checkout(struct kr_request *request, const struct sockaddr *src, * don't affect the resolution or rest of the processing. */ int state = request->state; ITERATE_LAYERS(request, qry, checkout, packet, dst, type); - if (request->state == KR_STATE_FAIL) { + if (request->state & KR_STATE_FAIL) { request->state = state; /* Restore */ return kr_error(ECANCELED); } @@ -1589,25 +1602,24 @@ int kr_resolve_checkout(struct kr_request *request, const struct sockaddr *src, int kr_resolve_finish(struct kr_request *request, int state) { + request->state = state; /* Finalize answer and construct wire-buffer. */ ITERATE_LAYERS(request, NULL, answer_finalize); - if (request->state == KR_STATE_FAIL) { - state = KR_STATE_FAIL; - } else if (answer_finalize(request, state) != 0) { - state = KR_STATE_FAIL; - } + answer_finalize(request); - /* Error during processing, internal failure */ - if (state != KR_STATE_DONE) { - knot_pkt_t *answer = request->answer; - if (knot_wire_get_rcode(answer->wire) == KNOT_RCODE_NOERROR) { - knot_wire_clear_ad(answer->wire); - knot_wire_clear_aa(answer->wire); - knot_wire_set_rcode(answer->wire, KNOT_RCODE_SERVFAIL); + /* Defensive style, in case someone has forgotten. + * Beware: non-empty answers do make sense even with SERVFAIL case, etc. */ + if (request->state != KR_STATE_DONE) { + uint8_t *wire = request->answer->wire; + switch (knot_wire_get_rcode(wire)) { + case KNOT_RCODE_NOERROR: + case KNOT_RCODE_NXDOMAIN: + knot_wire_clear_ad(wire); + knot_wire_clear_aa(wire); + knot_wire_set_rcode(wire, KNOT_RCODE_SERVFAIL); } } - request->state = state; ITERATE_LAYERS(request, NULL, finish); #ifndef NOVERBOSELOG diff --git a/lib/resolve.h b/lib/resolve.h index 60d80bb..18372a3 100644 --- a/lib/resolve.h +++ b/lib/resolve.h @@ -308,7 +308,7 @@ int kr_resolve_checkout(struct kr_request *request, const struct sockaddr *src, * be destroyed, as it's owned by caller. * * @param request request state - * @param state either DONE or FAIL state + * @param state either DONE or FAIL state (to be assigned to request->state) * @return DONE */ KR_EXPORT diff --git a/modules/cookies/cookiemonster.c b/modules/cookies/cookiemonster.c index 7af7afa..6f00192 100644 --- a/modules/cookies/cookiemonster.c +++ b/modules/cookies/cookiemonster.c @@ -423,7 +423,7 @@ int check_request(kr_layer_t *ctx) /* Request has no server cookie. */ return_state = invalid_sc_status(return_state, false, ignore_badcookie, req, answer); - if (return_state == KR_STATE_FAIL) { + if (return_state & KR_STATE_FAIL) { return return_state; } goto answer_add_cookies; @@ -449,7 +449,7 @@ int check_request(kr_layer_t *ctx) /* Invalid server cookie. */ return_state = invalid_sc_status(return_state, true, ignore_badcookie, req, answer); - if (return_state == KR_STATE_FAIL) { + if (return_state & KR_STATE_FAIL) { return return_state; } goto answer_add_cookies; diff --git a/modules/hints/hints.c b/modules/hints/hints.c index 84e2e99..f55e85b 100644 --- a/modules/hints/hints.c +++ b/modules/hints/hints.c @@ -133,7 +133,7 @@ static int satisfy_forward(struct kr_zonecut *hints, knot_pkt_t *pkt, struct kr_ static int query(kr_layer_t *ctx, knot_pkt_t *pkt) { struct kr_query *qry = ctx->req->current_query; - if (!qry || ctx->state & (KR_STATE_FAIL)) { + if (!qry || (ctx->state & KR_STATE_FAIL)) { return ctx->state; }