From e280a162e43e7106e6f3ae4fdd0c9e3833d45def Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Oto=20=C5=A0=C5=A5=C3=A1va?= <oto.stava@nic.cz>
Date: Fri, 17 Jun 2022 10:57:58 +0200
Subject: [PATCH] daemon/http: improve URI checks
The `check_uri()` function now only checks that the endpoint is either
`/doh` or `/dns-query`. Parameter checks were moved into
`process_uri_path()` so that the check only takes place for GET
requests. POST requests now do not care about parameters at all.
---
NEWS | 8 +++++
daemon/http.c | 72 +++++++++++++-------------------------
tests/config/doh2.test.lua | 51 +++++++++++++++++++++++++++
3 files changed, 83 insertions(+), 48 deletions(-)
diff --git a/NEWS b/NEWS
index babcb287a..19499650d 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,11 @@
+Knot Resolver 5.5.2 (2022-mm-dd)
+================================
+
+Bugfixes
+--------
+- daemon/http: improved URI checks to fix some proxies (#746, !1311)
+
+
Knot Resolver 5.5.1 (2022-06-14)
================================
diff --git a/daemon/http.c b/daemon/http.c
index 2e8aa2f00..3347aa6f4 100644
--- a/daemon/http.c
+++ b/daemon/http.c
@@ -172,25 +172,14 @@ static int send_data_callback(nghttp2_session *h2, nghttp2_frame *frame, const u
/*
* Check endpoint and uri path
*/
-static int check_uri(const char* uri_path)
+static int check_uri(const char* path)
{
- static const char key[] = "dns=";
- static const char *delim = "&";
static const char *endpoints[] = {"dns-query", "doh"};
- char *beg;
- char *end_prev;
ssize_t endpoint_len;
ssize_t ret;
- if (!uri_path)
- return kr_error(EINVAL);
-
- auto_free char *path = malloc(sizeof(*path) * (strlen(uri_path) + 1));
if (!path)
- return kr_error(ENOMEM);
-
- memcpy(path, uri_path, strlen(uri_path));
- path[strlen(uri_path)] = '\0';
+ return kr_error(EINVAL);
char *query_mark = strstr(path, "?");
@@ -208,35 +197,7 @@ static int check_uri(const char* uri_path)
break;
}
- if (ret) /* no endpoint found */
- return kr_error(ENOENT);
-
- /* FIXME This also passes for GET when no variables are provided.
- * Fixing it doesn't seem straightforward, since :method may not be
- * known by the time check_uri() is called... */
- if (endpoint_len == strlen(path) - 1) /* done for POST method */
- return 0;
-
- /* go over key:value pair */
- beg = strtok(query_mark + 1, delim);
- if (beg) {
- while (beg != NULL) {
- if (!strncmp(beg, key, 4)) { /* dns variable in path found */
- break;
- }
- end_prev = beg + strlen(beg);
- beg = strtok(NULL, delim);
- if (!beg || beg-1 != end_prev) { /* detect && */
- return -1;
- }
- }
-
- if (!beg) { /* no dns variable in path */
- return -1;
- }
- }
-
- return 0;
+ return (ret) ? kr_error(ENOENT) : kr_ok();
}
static kr_http_header_array_t *headers_dup(kr_http_header_array_t *src)
@@ -265,13 +226,26 @@ static int process_uri_path(struct http_ctx *ctx, const char* path, int32_t stre
return kr_error(EINVAL);
static const char key[] = "dns=";
- char *beg = strstr(path, key);
- char *end;
- size_t remaining;
+ static const char *delim = "&";
+ char *beg, *end;
uint8_t *dest;
+ uint32_t remaining;
- if (!beg) /* No dns variable in path. */
- return -1;
+ if (!path)
+ return kr_error(EINVAL);
+
+ char *query_mark = strstr(path, "?");
+ if (!query_mark || strlen(query_mark) == 0) /* no parameters in path */
+ return kr_error(EINVAL);
+
+ /* go over key:value pair */
+ for (beg = strtok(query_mark + 1, delim); beg != NULL; beg = strtok(NULL, delim)) {
+ if (!strncmp(beg, key, 4)) /* dns variable in path found */
+ break;
+ }
+
+ if (!beg) /* no dns variable in path */
+ return kr_error(EINVAL);
beg += sizeof(key) - 1;
end = strchr(beg, '&');
@@ -282,6 +256,7 @@ static int process_uri_path(struct http_ctx *ctx, const char* path, int32_t stre
remaining = ctx->buf_size - ctx->submitted - ctx->buf_pos;
dest = ctx->buf + ctx->buf_pos;
+ /* Decode dns message from the parameter */
int ret = kr_base64url_decode((uint8_t*)beg, end - beg, dest, remaining);
if (ret < 0) {
ctx->buf_pos = 0;
@@ -296,7 +271,8 @@ static int process_uri_path(struct http_ctx *ctx, const char* path, int32_t stre
.headers = headers_dup(ctx->headers)
};
queue_push(ctx->streams, stream);
- return 0;
+
+ return kr_ok();
}
static void refuse_stream(nghttp2_session *h2, int32_t stream_id)
diff --git a/tests/config/doh2.test.lua b/tests/config/doh2.test.lua
index 881515c7b..2360e7f48 100644
--- a/tests/config/doh2.test.lua
+++ b/tests/config/doh2.test.lua
@@ -139,6 +139,45 @@ else
same(pkt:arcount(), 0, desc .. ': ADDITIONAL is empty')
end
+ local function test_post_ignore_params_1()
+ local desc = 'valid POST ignores parameters (without ?dns)'
+ local req = req_templ:clone()
+ req.headers:upsert(':method', 'POST')
+ req.headers:upsert(':path', '/doh?something=asdf&aaa=bbb')
+ req:set_body(basexx.from_base64( -- noerror.test. A
+ 'vMEBAAABAAAAAAAAB25vZXJyb3IEdGVzdAAAAQAB'))
+ local headers, pkt = check_ok(req, desc)
+ if not (headers and pkt) then
+ return
+ end
+ -- HTTP TTL is minimum from all RRs in the answer
+ same(headers:get('cache-control'), 'max-age=300', desc .. ': TTL 900')
+ same(pkt:rcode(), kres.rcode.NOERROR, desc .. ': rcode matches')
+ same(pkt:ancount(), 3, desc .. ': ANSWER is present')
+ same(pkt:nscount(), 1, desc .. ': AUTHORITY is present')
+ same(pkt:arcount(), 0, desc .. ': ADDITIONAL is empty')
+ end
+
+ local function test_post_ignore_params_2()
+ local desc = 'valid POST ignores parameters (with ?dns)'
+ local req = req_templ:clone()
+ req.headers:upsert(':method', 'POST')
+ req.headers:upsert(':path', '/dns-query?dns=' -- servfail.test. A
+ .. 'FZUBAAABAAAAAAAACHNlcnZmYWlsBHRlc3QAAAEAAQ')
+ req:set_body(basexx.from_base64( -- noerror.test. A
+ 'vMEBAAABAAAAAAAAB25vZXJyb3IEdGVzdAAAAQAB'))
+ local headers, pkt = check_ok(req, desc)
+ if not (headers and pkt) then
+ return
+ end
+ -- HTTP TTL is minimum from all RRs in the answer
+ same(headers:get('cache-control'), 'max-age=300', desc .. ': TTL 900')
+ same(pkt:rcode(), kres.rcode.NOERROR, desc .. ': rcode matches')
+ same(pkt:ancount(), 3, desc .. ': ANSWER is present')
+ same(pkt:nscount(), 1, desc .. ': AUTHORITY is present')
+ same(pkt:arcount(), 0, desc .. ': ADDITIONAL is empty')
+ end
+
local function test_post_nxdomain()
local desc = 'valid POST query which ends with NXDOMAIN'
local req = req_templ:clone()
@@ -252,6 +291,15 @@ else
same(pkt:nscount(), 1, desc .. ': AUTHORITY is present')
end
+ local function test_get_multiple_amps()
+ local desc = 'GET query with consecutive ampersands'
+ local req = req_templ:clone()
+ req.headers:upsert(':method', 'GET')
+ req.headers:upsert(':path',
+ '/doh?other=something&another=something&&&&dns=vMEBAAABAAAAAAAAB25vZXJyb3IEdGVzdAAAAQAB')
+ check_ok(req, desc)
+ end
+
local function test_get_other_params_before_dns()
local desc = 'GET query with other parameters before dns is valid'
local req = req_templ:clone()
@@ -446,6 +494,8 @@ else
start_server,
test_post_servfail,
test_post_noerror,
+ test_post_ignore_params_1,
+ test_post_ignore_params_2,
test_post_nxdomain,
test_huge_answer,
test_post_short_input,
@@ -455,6 +505,7 @@ else
test_get_servfail,
test_get_noerror,
test_get_nxdomain,
+ test_get_multiple_amps,
test_get_other_params_before_dns,
test_get_other_params_after_dns,
test_get_other_params,
--
GitLab