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.

diff --git a/NEWS b/NEWS
index babcb287a356ba79ab652383a5f692e51757daa9..19499650d5f4e90b1469e705a578d4c0d86fcd0b 100644 (file)
--- 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 2e8aa2f00c003564179e7148b6f6033ee726550f..3347aa6f46fba23da73ca6b7bfa218de1cdad918 100644 (file)
--- 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 881515c7b2b52762ecc93f965708c53c9de12d8b..2360e7f48f60a958fc431bf3762b57f41e48ae6f 100644 (file)
--- 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,