From: Oto Šťáva Date: Fri, 1 Apr 2022 08:42:36 +0000 (+0200) Subject: daemon/http: return 400 on failed packet_parse + improved stream handling X-Git-Tag: v5.5.1~11^2~5 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c4991edef46b00c7ad50efe7b0f6ba447394b0aa;p=thirdparty%2Fknot-resolver.git daemon/http: return 400 on failed packet_parse + improved stream handling --- diff --git a/daemon/http.c b/daemon/http.c index d3f8d2cd6..47dc75389 100644 --- a/daemon/http.c +++ b/daemon/http.c @@ -57,6 +57,7 @@ enum http_status { HTTP_STATUS_BAD_REQUEST = 400, HTTP_STATUS_NOT_FOUND = 404, HTTP_STATUS_PAYLOAD_TOO_LARGE = 413, + HTTP_STATUS_UNSUPPORTED_MEDIA_TYPE = 415, HTTP_STATUS_REQUEST_HEADER_FIELDS_TOO_LARGE = 431, HTTP_STATUS_NOT_IMPLEMENTED = 501, }; @@ -74,6 +75,8 @@ typedef array_t(nghttp2_nv) nghttp2_array_t; static int http_send_response(struct http_ctx *ctx, int32_t stream_id, nghttp2_data_provider *prov, enum http_status status); +static int http_send_response_rst_stream(struct http_ctx *ctx, int32_t stream_id, + nghttp2_data_provider *prov, enum http_status status); /* * Write HTTP/2 protocol data to underlying transport layer. @@ -219,7 +222,7 @@ static int check_uri(const char* uri_path) } end_prev = beg + strlen(beg); beg = strtok(NULL, delim); - if (beg-1 != end_prev) { /* detect && */ + if (!beg || beg-1 != end_prev) { /* detect && */ return -1; } } @@ -368,7 +371,7 @@ static int header_callback(nghttp2_session *h2, const nghttp2_frame *frame, kr_log_debug(DOH, "[%p] stream %d: header too large (%zu B), refused\n", (void *)h2, stream_id, valuelen); - return http_send_response(ctx, stream_id, NULL, + return http_send_response_rst_stream(ctx, stream_id, NULL, HTTP_STATUS_REQUEST_HEADER_FIELDS_TOO_LARGE); } @@ -389,9 +392,11 @@ static int header_callback(nghttp2_session *h2, const nghttp2_frame *frame, if (!strcasecmp(":path", (const char *)name)) { int uri_result = check_uri((const char *)value); if (uri_result == kr_error(ENOENT)) { - return http_send_response(ctx, stream_id, NULL, HTTP_STATUS_NOT_FOUND); + return http_send_response_rst_stream(ctx, stream_id, NULL, + HTTP_STATUS_NOT_FOUND); } else if (uri_result < 0) { - return http_send_response(ctx, stream_id, NULL, HTTP_STATUS_BAD_REQUEST); + return http_send_response_rst_stream(ctx, stream_id, NULL, + HTTP_STATUS_BAD_REQUEST); } kr_assert(ctx->uri_path == NULL); @@ -411,7 +416,15 @@ static int header_callback(nghttp2_session *h2, const nghttp2_frame *frame, ctx->current_method = HTTP_METHOD_HEAD; } else { ctx->current_method = HTTP_METHOD_NONE; - return http_send_response(ctx, stream_id, NULL, HTTP_STATUS_NOT_IMPLEMENTED); + return http_send_response_rst_stream(ctx, stream_id, NULL, + HTTP_STATUS_NOT_IMPLEMENTED); + } + } + + if (!strcasecmp("content-type", (const char *)name)) { + if (strcasecmp("application/dns-message", (const char *)value)) { + return http_send_response_rst_stream(ctx, stream_id, NULL, + HTTP_STATUS_UNSUPPORTED_MEDIA_TYPE); } } @@ -496,7 +509,7 @@ static int submit_to_wirebuffer(struct http_ctx *ctx) len = ctx->buf_pos - sizeof(uint16_t); if (len <= 0 || len > KNOT_WIRE_MAX_PKTSIZE) { kr_log_debug(DOH, "[%p] invalid dnsmsg size: %zd B\n", (void *)ctx->h2, len); - http_send_response(ctx, stream_id, NULL, (len <= 0) + http_send_response_rst_stream(ctx, stream_id, NULL, (len <= 0) ? HTTP_STATUS_BAD_REQUEST : HTTP_STATUS_PAYLOAD_TOO_LARGE); ret = -1; @@ -536,7 +549,8 @@ static int on_frame_recv_callback(nghttp2_session *h2, const nghttp2_frame *fram if (ctx->current_method == HTTP_METHOD_GET || ctx->current_method == HTTP_METHOD_HEAD) { if (process_uri_path(ctx, ctx->uri_path, stream_id) < 0) { /* End processing - don't submit to wirebuffer. */ - return http_send_response(ctx, stream_id, NULL, HTTP_STATUS_BAD_REQUEST); + return http_send_response_rst_stream(ctx, stream_id, NULL, + HTTP_STATUS_BAD_REQUEST); } } @@ -623,7 +637,9 @@ struct http_ctx* http_new(struct session *session, http_send_callback send_cb) queue_init(ctx->streams); ctx->stream_write_data = trie_create(NULL); ctx->incomplete_stream = -1; + ctx->submitted_stream = -1; ctx->submitted = 0; + ctx->streaming = true; ctx->current_method = HTTP_METHOD_NONE; ctx->uri_path = NULL; @@ -691,7 +707,8 @@ int http_send_bad_request(struct session *session) { struct http_ctx *ctx = session_http_get_server_ctx(session); if (ctx->submitted_stream >= 0) - return http_send_response(ctx, ctx->submitted_stream, NULL, HTTP_STATUS_BAD_REQUEST); + return http_send_response_rst_stream(ctx, ctx->submitted_stream, NULL, + HTTP_STATUS_BAD_REQUEST); return 0; } @@ -814,13 +831,27 @@ static int http_send_response(struct http_ctx *ctx, int32_t stream_id, return 0; } - if (status != HTTP_STATUS_OK) { - nghttp2_submit_rst_stream(h2, NGHTTP2_FLAG_NONE, stream_id, NGHTTP2_NO_ERROR); - } - return 0; } +/* + * Same as `http_send_response`, but resets the HTTP stream afterwards. Used + * for sending negative status messages. + */ +static int http_send_response_rst_stream(struct http_ctx *ctx, int32_t stream_id, + nghttp2_data_provider *prov, enum http_status status) +{ + int ret = http_send_response(ctx, stream_id, prov, status); + if (ret) + return ret; + + ctx->submitted_stream = -1; + nghttp2_submit_rst_stream(ctx->h2, NGHTTP2_FLAG_NONE, stream_id, NGHTTP2_NO_ERROR); + ret = nghttp2_session_send(ctx->h2); + return ret; +} + + /* * Send HTTP/2 stream data created from packet's wire buffer. * diff --git a/daemon/io.c b/daemon/io.c index ff2e25a50..30b3f453e 100644 --- a/daemon/io.c +++ b/daemon/io.c @@ -478,7 +478,7 @@ static void tcp_recv(uv_stream_t *handle, ssize_t nread, const uv_buf_t *buf) #if ENABLE_DOH2 if (session_flags(s)->has_http && streaming == 0 && ret == 0) { ret = http_send_bad_request(s); - if (ret < 0) { + if (ret) { /* An error has occurred, close the session. */ worker_end_tcp(s); } diff --git a/daemon/worker.c b/daemon/worker.c index b2e421abb..156335d31 100644 --- a/daemon/worker.c +++ b/daemon/worker.c @@ -1778,6 +1778,10 @@ int worker_submit(struct session *session, struct io_comm_data *comm, struct http_ctx *http_ctx = NULL; #if ENABLE_DOH2 http_ctx = session_http_get_server_ctx(session); + if (http_ctx && !is_outgoing && ret) { + http_send_bad_request(session); + return kr_error(EMSGSIZE); + } #endif if (!is_outgoing && http_ctx && queue_len(http_ctx->streams) <= 0) diff --git a/tests/config/doh2.test.lua b/tests/config/doh2.test.lua index 429963c9a..352ea4283 100644 --- a/tests/config/doh2.test.lua +++ b/tests/config/doh2.test.lua @@ -63,15 +63,15 @@ local function check_ok(req, desc) return headers, pkt end ---local function check_err(req, exp_status, desc) --- local headers, errmsg, errno = req:go(16) --- if errno then --- nok(errmsg, desc .. ': ' .. errmsg) --- return --- end --- local got_status = headers:get(':status') --- same(got_status, exp_status, desc) ---end +local function check_err(req, exp_status, desc) + local headers, errmsg, errno = req:go(16) + if errno then + nok(errmsg, desc .. ': ' .. errmsg) + return + end + local got_status = headers:get(':status') + same(got_status, exp_status, desc) +end -- check prerequisites local bound, port @@ -169,35 +169,39 @@ else end -- test an invalid DNS query using POST --- local function test_post_short_input() --- local req = assert(req_templ:clone()) --- req.headers:upsert(':method', 'POST') --- req:set_body(string.rep('0', 11)) -- 11 bytes < DNS msg header --- check_err(req, '400', 'too short POST finishes with 400') --- end --- + local function test_post_short_input() + local req = assert(req_templ:clone()) + req.headers:upsert(':method', 'POST') + req:set_body(string.rep('0', 11)) -- 11 bytes < DNS msg header + check_err(req, '400', 'too short POST finishes with 400') + end + -- local function test_post_long_input() --- local req = assert(req_templ:clone()) --- req.headers:upsert(':method', 'POST') --- req:set_body(string.rep('s', 1025)) -- > DNS msg over UDP --- check_err(req, '413', 'too long POST finishes with 413') --- end --- --- local function test_post_unparseable_input() --- local req = assert(req_templ:clone()) --- req.headers:upsert(':method', 'POST') --- req:set_body(string.rep('\0', 1024)) -- garbage --- check_err(req, '400', 'unparseable DNS message finishes with 400') --- end +-- -- FIXME: This test is broken in Lua. The connection times out +-- -- for some reason, but sending a request like this with `curl` +-- -- or PowerShell's `Invoke-RestMethod` provides correct results. -- --- local function test_post_unsupp_type() -- local req = assert(req_templ:clone()) -- req.headers:upsert(':method', 'POST') --- req.headers:upsert('content-type', 'application/dns+json') --- req:set_body(string.rep('\0', 12)) -- valid message --- check_err(req, '415', 'unsupported request content type finishes with 415') +-- req:set_body(string.rep('s', 1025)) -- > DNS msg over UDP +-- check_err(req, '400', 'too long POST finishes with 400') -- end + local function test_post_unparseable_input() + local req = assert(req_templ:clone()) + req.headers:upsert(':method', 'POST') + req:set_body(string.rep('\0', 1024)) -- garbage + check_err(req, '400', 'unparseable DNS message finishes with 400') + end + + local function test_post_unsupp_type() + local req = assert(req_templ:clone()) + req.headers:upsert(':method', 'POST') + req.headers:upsert('content-type', 'application/dns+json') + req:set_body(string.rep('\0', 12)) -- valid message + check_err(req, '415', 'unsupported request content type finishes with 415') + end + -- test a valid DNS query using GET local function test_get_servfail() local desc = 'valid GET query which ends with SERVFAIL' @@ -275,47 +279,47 @@ else check_ok(req, desc) end --- -- test an invalid DNS query using GET --- local function test_get_long_input() --- local req = assert(req_templ:clone()) --- req.headers:upsert(':method', 'GET') --- req.headers:upsert(':path', '/doh?dns=' .. basexx.to_url64(string.rep('\0', 1030))) --- check_err(req, '414', 'too long GET finishes with 414') --- end --- --- local function test_get_no_dns_param() --- local req = assert(req_templ:clone()) --- req.headers:upsert(':method', 'GET') --- req.headers:upsert(':path', '/doh?notdns=' .. basexx.to_url64(string.rep('\0', 1024))) --- check_err(req, '400', 'GET without dns parameter finishes with 400') --- end --- --- local function test_get_unparseable() --- local req = assert(req_templ:clone()) --- req.headers:upsert(':method', 'GET') --- req.headers:upsert(':path', '/doh??dns=' .. basexx.to_url64(string.rep('\0', 1024))) --- check_err(req, '400', 'unparseable GET finishes with 400') --- end --- --- local function test_get_invalid_b64() --- local req = assert(req_templ:clone()) --- req.headers:upsert(':method', 'GET') --- req.headers:upsert(':path', '/doh?dns=thisisnotb64') --- check_err(req, '400', 'GET with invalid base64 finishes with 400') --- end --- --- local function test_get_invalid_chars() --- local req = assert(req_templ:clone()) --- req.headers:upsert(':method', 'GET') --- req.headers:upsert(':path', '/doh?dns=' .. basexx.to_url64(string.rep('\0', 200)) .. '@#$%?!') --- check_err(req, '400', 'GET with invalid characters in b64 finishes with 400') --- end --- --- local function test_unsupp_method() --- local req = assert(req_templ:clone()) --- req.headers:upsert(':method', 'PUT') --- check_err(req, '405', 'unsupported method finishes with 405') --- end + -- test an invalid DNS query using GET + local function test_get_long_input() + local req = assert(req_templ:clone()) + req.headers:upsert(':method', 'GET') + req.headers:upsert(':path', '/doh?dns=' .. basexx.to_url64(string.rep('\0', 1030))) + check_err(req, '400', 'too long GET finishes with 400') + end + + local function test_get_no_dns_param() + local req = assert(req_templ:clone()) + req.headers:upsert(':method', 'GET') + req.headers:upsert(':path', '/doh?notdns=' .. basexx.to_url64(string.rep('\0', 1024))) + check_err(req, '400', 'GET without dns parameter finishes with 400') + end + + local function test_get_unparseable() + local req = assert(req_templ:clone()) + req.headers:upsert(':method', 'GET') + req.headers:upsert(':path', '/doh??dns=' .. basexx.to_url64(string.rep('\0', 1024))) + check_err(req, '400', 'unparseable GET finishes with 400') + end + + local function test_get_invalid_b64() + local req = assert(req_templ:clone()) + req.headers:upsert(':method', 'GET') + req.headers:upsert(':path', '/doh?dns=thisisnotb64') + check_err(req, '400', 'GET with invalid base64 finishes with 400') + end + + local function test_get_invalid_chars() + local req = assert(req_templ:clone()) + req.headers:upsert(':method', 'GET') + req.headers:upsert(':path', '/doh?dns=' .. basexx.to_url64(string.rep('\0', 200)) .. '@#$%?!') + check_err(req, '400', 'GET with invalid characters in b64 finishes with 400') + end + + local function test_unsupp_method() + local req = assert(req_templ:clone()) + req.headers:upsert(':method', 'PUT') + check_err(req, '501', 'unsupported method finishes with 501') + end local function test_dstaddr() local triggered = false @@ -438,29 +442,28 @@ else -- end -- plan tests - -- TODO: implement (some) of the error status codes local tests = { start_server, test_post_servfail, test_post_noerror, test_post_nxdomain, test_huge_answer, - --test_post_short_input, - --test_post_long_input, - --test_post_unparseable_input, - --test_post_unsupp_type, + test_post_short_input, +-- test_post_long_input, -- FIXME see the test function + test_post_unparseable_input, + test_post_unsupp_type, test_get_servfail, test_get_noerror, test_get_nxdomain, test_get_other_params_before_dns, test_get_other_params_after_dns, test_get_other_params, - --test_get_long_input, - --test_get_no_dns_param, - --test_get_unparseable, - --test_get_invalid_b64, - --test_get_invalid_chars, - --test_unsupp_method, + test_get_long_input, + test_get_no_dns_param, + test_get_unparseable, + test_get_invalid_b64, + test_get_invalid_chars, + test_unsupp_method, test_dstaddr, test_srcaddr, test_headers