1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
|
From: Markus Koschany <apo@debian.org>
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;
}
|
