From: Artem Boldariev Date: Wed, 19 May 2021 15:03:11 +0000 (+0300) Subject: Verify HTTP paths both in incoming requests and in config file X-Git-Tag: v9.17.17~21^2~1 X-Git-Url: http://git.ipfire.org/gitweb/?a=commitdiff_plain;h=954240467da26b1ec99130987326099b2cf642a4;p=thirdparty%2Fbind9.git Verify HTTP paths both in incoming requests and in config file This commit adds the code (and some tests) which allows verifying validity of HTTP paths both in incoming HTTP requests and in BIND's configuration file. --- diff --git a/bin/tests/system/checkconf/bad-doh-badpath-1.conf b/bin/tests/system/checkconf/bad-doh-badpath-1.conf new file mode 100644 index 00000000000..ef1839aa7dd --- /dev/null +++ b/bin/tests/system/checkconf/bad-doh-badpath-1.conf @@ -0,0 +1,19 @@ +/* + * Copyright (C) Internet Systems Consortium, Inc. ("ISC") + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + * + * See the COPYRIGHT file distributed with this work for additional + * information regarding copyright ownership. + */ + +# bad HTTP location +http local-http-server { + endpoints { "dns-query"; }; +}; + +options { + listen-on port 8080 tls none http local-http-server { 10.53.0.1; }; +}; diff --git a/bin/tests/system/checkconf/bad-doh-badpath-2.conf b/bin/tests/system/checkconf/bad-doh-badpath-2.conf new file mode 100644 index 00000000000..6432ac1939c --- /dev/null +++ b/bin/tests/system/checkconf/bad-doh-badpath-2.conf @@ -0,0 +1,19 @@ +/* + * Copyright (C) Internet Systems Consortium, Inc. ("ISC") + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + * + * See the COPYRIGHT file distributed with this work for additional + * information regarding copyright ownership. + */ + +# bad HTTP location +http local-http-server { + endpoints { "//"; }; +}; + +options { + listen-on port 8080 tls none http local-http-server { 10.53.0.1; }; +}; diff --git a/bin/tests/system/checkconf/bad-doh-badpath-3.conf b/bin/tests/system/checkconf/bad-doh-badpath-3.conf new file mode 100644 index 00000000000..57c179b19d9 --- /dev/null +++ b/bin/tests/system/checkconf/bad-doh-badpath-3.conf @@ -0,0 +1,19 @@ +/* + * Copyright (C) Internet Systems Consortium, Inc. ("ISC") + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + * + * See the COPYRIGHT file distributed with this work for additional + * information regarding copyright ownership. + */ + +# bad HTTP location +http local-http-server { + endpoints { "/dns-query?dns="; }; +}; + +options { + listen-on port 8080 tls none http local-http-server { 10.53.0.1; }; +}; diff --git a/doc/arm/reference.rst b/doc/arm/reference.rst index 0f14eeb17ee..299718e2a74 100644 --- a/doc/arm/reference.rst +++ b/doc/arm/reference.rst @@ -4766,10 +4766,11 @@ cause ``named`` to listen for incoming requests over HTTPS. The following options can be specified in an ``http`` statement: ``endpoints`` - A list of HTTP query paths on which to listen. A typical path - is "/dns-query". + A list of HTTP query paths on which to listen. This is the portion + of an :rfc:`3986`-compliant URI following the hostname; it must be + an absolute path, beginning with "/". A typical endpoint is "/dns-query". -for example, the following configuration enables DNS-over-HTTPS queries on +For example, the following configuration enables DNS-over-HTTPS queries on all local addresses: :: diff --git a/lib/bind9/check.c b/lib/bind9/check.c index c27de55476b..e97983cce61 100644 --- a/lib/bind9/check.c +++ b/lib/bind9/check.c @@ -1949,6 +1949,94 @@ bind9_check_parentalagentlists(const cfg_obj_t *cctx, isc_log_t *logctx, return (result); } +#if HAVE_LIBNGHTTP2 +static isc_result_t +bind9_check_httpserver(const cfg_obj_t *http, isc_log_t *logctx, + isc_symtab_t *symtab, isc_mem_t *mctx) { + isc_result_t result, tresult; + const char *name = cfg_obj_asstring(cfg_map_getname(http)); + char *tmp = isc_mem_strdup(mctx, name); + const cfg_obj_t *eps = NULL; + const cfg_listelt_t *elt = NULL; + isc_symvalue_t symvalue; + + /* Check for duplicates */ + symvalue.as_cpointer = http; + result = isc_symtab_define(symtab, tmp, 1, symvalue, + isc_symexists_reject); + if (result == ISC_R_EXISTS) { + const char *file = NULL; + unsigned int line; + + tresult = isc_symtab_lookup(symtab, tmp, 1, &symvalue); + RUNTIME_CHECK(tresult == ISC_R_SUCCESS); + + line = cfg_obj_line(symvalue.as_cpointer); + file = cfg_obj_file(symvalue.as_cpointer); + if (file == NULL) { + file = ""; + } + + cfg_obj_log(http, logctx, ISC_LOG_ERROR, + "http '%s' is duplicated: " + "also defined at %s:%u", + name, file, line); + } + isc_mem_free(mctx, tmp); + + /* Check endpoints are valid */ + tresult = cfg_map_get(http, "endpoints", &eps); + RUNTIME_CHECK(tresult == ISC_R_SUCCESS); + for (elt = cfg_list_first(eps); elt != NULL; elt = cfg_list_next(elt)) { + const cfg_obj_t *ep = cfg_listelt_value(elt); + const char *path = cfg_obj_asstring(ep); + if (!isc_nm_http_path_isvalid(path)) { + cfg_obj_log(eps, logctx, ISC_LOG_ERROR, + "endpoint '%s' is not a " + "valid absolute HTTP path", + path); + if (result == ISC_R_SUCCESS) { + result = ISC_R_FAILURE; + } + } + } + + return (result); +} + +static isc_result_t +bind9_check_httpservers(const cfg_obj_t *config, isc_log_t *logctx, + isc_mem_t *mctx) { + isc_result_t result, tresult; + const cfg_obj_t *obj = NULL; + const cfg_listelt_t *elt = NULL; + isc_symtab_t *symtab = NULL; + + result = isc_symtab_create(mctx, 100, NULL, NULL, false, &symtab); + if (result != ISC_R_SUCCESS) { + return (result); + } + + result = cfg_map_get(config, "http", &obj); + if (result != ISC_R_SUCCESS) { + result = ISC_R_SUCCESS; + goto done; + } + + for (elt = cfg_list_first(obj); elt != NULL; elt = cfg_list_next(elt)) { + obj = cfg_listelt_value(elt); + tresult = bind9_check_httpserver(obj, logctx, symtab, mctx); + if (result == ISC_R_SUCCESS) { + result = tresult; + } + } + +done: + isc_symtab_destroy(&symtab); + return (result); +} +#endif /* HAVE_LIBNGHTTP2 */ + static isc_result_t get_remotes(const cfg_obj_t *cctx, const char *list, const char *name, const cfg_obj_t **ret) { @@ -5207,6 +5295,12 @@ bind9_check_namedconf(const cfg_obj_t *config, bool check_plugins, result = ISC_R_FAILURE; } +#if HAVE_LIBNGHTTP2 + if (bind9_check_httpservers(config, logctx, mctx) != ISC_R_SUCCESS) { + result = ISC_R_FAILURE; + } +#endif /* HAVE_LIBNGHTTP2 */ + (void)cfg_map_get(config, "view", &views); if (views != NULL && options != NULL) { diff --git a/lib/isc/include/isc/netmgr.h b/lib/isc/include/isc/netmgr.h index 23605e29284..b494426e25a 100644 --- a/lib/isc/include/isc/netmgr.h +++ b/lib/isc/include/isc/netmgr.h @@ -504,11 +504,14 @@ isc_nm_listenhttp(isc_nm_t *mgr, isc_sockaddr_t *iface, int backlog, isc_result_t isc_nm_http_endpoint(isc_nmsocket_t *sock, const char *uri, isc_nm_recv_cb_t cb, void *cbarg, size_t extrahandlesize); -#endif bool isc_nm_is_http_handle(isc_nmhandle_t *handle); +bool +isc_nm_http_path_isvalid(const char *path); +#endif /* HAVE_LIBNGHTTP2 */ + void isc_nm_bad_request(isc_nmhandle_t *handle); /*%< diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index a51dbf233eb..a7de43aeafa 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -1695,6 +1695,13 @@ server_handle_path_header(isc_nmsocket_t *socket, const uint8_t *value, } socket->h2.request_path = isc_mem_strndup( socket->mgr->mctx, (const char *)value, vlen + 1); + + if (!isc_nm_http_path_isvalid(socket->h2.request_path)) { + isc_mem_free(socket->mgr->mctx, socket->h2.request_path); + socket->h2.request_path = NULL; + return (ISC_HTTP_ERROR_BAD_REQUEST); + } + handler = find_server_request_handler(socket->h2.request_path, socket->h2.session->serversocket); if (handler != NULL) { @@ -2476,7 +2483,7 @@ isc_nm_http_endpoint(isc_nmsocket_t *sock, const char *uri, isc_nm_recv_cb_t cb, REQUIRE(VALID_NMSOCK(sock)); REQUIRE(sock->type == isc_nm_httplistener); - REQUIRE(uri != NULL && *uri != '\0'); + REQUIRE(isc_nm_http_path_isvalid(uri)); httpcbarg = isc_mem_get(sock->mgr->mctx, sizeof(isc_nm_httpcbarg_t)); *httpcbarg = (isc_nm_httpcbarg_t){ .cb = cb, .cbarg = cbarg }; @@ -3008,6 +3015,7 @@ typedef struct isc_httpparser_state { #define MATCH(ch) (st->str[0] == (ch)) #define MATCH_ALPHA() isalpha((unsigned char)(st->str[0])) +#define MATCH_DIGIT() isdigit((unsigned char)(st->str[0])) #define MATCH_ALNUM() isalnum((unsigned char)(st->str[0])) #define MATCH_XDIGIT() isxdigit((unsigned char)(st->str[0])) #define ADVANCE() st->str++ @@ -3182,3 +3190,194 @@ rule_percent_charcode(isc_httpparser_state_t *st) { return (true); } + +/* + * DoH URL Location Verifier. Based on the following grammar (EBNF/WSN + * notation): + * + * S = path_absolute. + * path_absolute = '/' [ segments ] '\0'. + * segments = segment_nz { slash_segment }. + * slash_segment = '/' segment. + * segment = { pchar }. + * segment_nz = pchar { pchar }. + * pchar = unreserved | pct_encoded | sub_delims | ':' | '@'. + * unreserved = ALPHA | DIGIT | '-' | '.' | '_' | '~'. + * pct_encoded = '%' XDIGIT XDIGIT. + * sub_delims = '!' | '$' | '&' | '\'' | '(' | ')' | '*' | '+' | + * ',' | ';' | '='. + * + * The grammar is extracted from RFC 3986. It is slightly modified to + * aid in parser creation, but the end result is the same + * (path_absolute is defined slightly differently - split into + * multiple productions). + * + * https://datatracker.ietf.org/doc/html/rfc3986#appendix-A + */ + +typedef struct isc_http_location_parser_state { + const char *str; +} isc_http_location_parser_state_t; + +static bool +rule_loc_path_absolute(isc_http_location_parser_state_t *); + +static bool +rule_loc_segments(isc_http_location_parser_state_t *); + +static bool +rule_loc_slash_segment(isc_http_location_parser_state_t *); + +static bool +rule_loc_segment(isc_http_location_parser_state_t *); + +static bool +rule_loc_segment_nz(isc_http_location_parser_state_t *); + +static bool +rule_loc_pchar(isc_http_location_parser_state_t *); + +static bool +rule_loc_unreserved(isc_http_location_parser_state_t *); + +static bool +rule_loc_pct_encoded(isc_http_location_parser_state_t *); + +static bool +rule_loc_sub_delims(isc_http_location_parser_state_t *); + +static bool +rule_loc_path_absolute(isc_http_location_parser_state_t *st) { + if (MATCH('/')) { + ADVANCE(); + } else { + return (false); + } + + (void)rule_loc_segments(st); + + if (MATCH('\0')) { + ADVANCE(); + } else { + return (false); + } + + return (true); +} + +static bool +rule_loc_segments(isc_http_location_parser_state_t *st) { + if (!rule_loc_segment_nz(st)) { + return (false); + } + + while (rule_loc_slash_segment(st)) { + /* zero or more */; + } + + return (true); +} + +static bool +rule_loc_slash_segment(isc_http_location_parser_state_t *st) { + if (MATCH('/')) { + ADVANCE(); + } else { + return (false); + } + + return (rule_loc_segment(st)); +} + +static bool +rule_loc_segment(isc_http_location_parser_state_t *st) { + while (rule_loc_pchar(st)) { + /* zero or more */; + } + + return (true); +} + +static bool +rule_loc_segment_nz(isc_http_location_parser_state_t *st) { + if (!rule_loc_pchar(st)) { + return (false); + } + + while (rule_loc_pchar(st)) { + /* zero or more */; + } + + return (true); +} + +static bool +rule_loc_pchar(isc_http_location_parser_state_t *st) { + if (rule_loc_unreserved(st)) { + return (true); + } else if (rule_loc_pct_encoded(st)) { + return (true); + } else if (rule_loc_sub_delims(st)) { + return (true); + } else if (MATCH(':') || MATCH('@')) { + ADVANCE(); + return (true); + } + + return (false); +} + +static bool +rule_loc_unreserved(isc_http_location_parser_state_t *st) { + if (MATCH_ALPHA() | MATCH_DIGIT() | MATCH('-') | MATCH('.') | + MATCH('_') | MATCH('~')) + { + ADVANCE(); + return (true); + } + return (false); +} + +static bool +rule_loc_pct_encoded(isc_http_location_parser_state_t *st) { + if (!MATCH('%')) { + return (false); + } + ADVANCE(); + + if (!MATCH_XDIGIT()) { + return (false); + } + ADVANCE(); + + if (!MATCH_XDIGIT()) { + return (false); + } + ADVANCE(); + + return (true); +} + +static bool +rule_loc_sub_delims(isc_http_location_parser_state_t *st) { + if (MATCH('!') | MATCH('$') | MATCH('&') | MATCH('\'') | MATCH('(') | + MATCH(')') | MATCH('*') | MATCH('+') | MATCH(',') | MATCH(';') | + MATCH('=')) + { + ADVANCE(); + return (true); + } + + return (false); +} + +bool +isc_nm_http_path_isvalid(const char *path) { + isc_http_location_parser_state_t state = { 0 }; + + REQUIRE(path != NULL); + + state.str = path; + + return (rule_loc_path_absolute(&state)); +} diff --git a/lib/isc/tests/doh_test.c b/lib/isc/tests/doh_test.c index 2aec55fda57..7b6ccb1cc35 100644 --- a/lib/isc/tests/doh_test.c +++ b/lib/isc/tests/doh_test.c @@ -1964,6 +1964,41 @@ doh_base64_to_base64url(void **state) { } } +static void +doh_path_validation(void **state) { + UNUSED(state); + + assert_true(isc_nm_http_path_isvalid("/")); + assert_true(isc_nm_http_path_isvalid(DOH_PATH)); + assert_false(isc_nm_http_path_isvalid("laaaa")); + assert_false(isc_nm_http_path_isvalid("")); + assert_false(isc_nm_http_path_isvalid("//")); + assert_true(isc_nm_http_path_isvalid("/lala///")); + assert_true(isc_nm_http_path_isvalid("/lalaaaaaa")); + assert_true(isc_nm_http_path_isvalid("/lalaaa/la/la/la")); + assert_true(isc_nm_http_path_isvalid("/la/a")); + assert_true(isc_nm_http_path_isvalid("/la+la")); + assert_true(isc_nm_http_path_isvalid("/la&la/la*la/l-a_/la!/la\'")); + assert_true(isc_nm_http_path_isvalid("/la/(la)/la")); + assert_true(isc_nm_http_path_isvalid("/la,la,la")); + assert_true(isc_nm_http_path_isvalid("/la-'la'-la")); + assert_true(isc_nm_http_path_isvalid("/la:la=la")); + assert_true(isc_nm_http_path_isvalid("/l@l@l@")); + assert_false(isc_nm_http_path_isvalid("/#lala")); + assert_true(isc_nm_http_path_isvalid("/lala;la")); + assert_false( + isc_nm_http_path_isvalid("la&la/laalaala*lala/l-al_a/lal!/")); + assert_true(isc_nm_http_path_isvalid("/Lal/lAla.jpg")); + + /* had to replace ? with ! because it does not verify a query string */ + assert_true(isc_nm_http_path_isvalid("/watch!v=oavMtUWDBTM")); + assert_false(isc_nm_http_path_isvalid("/watch?v=dQw4w9WgXcQ")); + assert_true(isc_nm_http_path_isvalid("/datatracker.ietf.org/doc/html/" + "rfc2616")); + assert_true(isc_nm_http_path_isvalid("/doc/html/rfc8484")); + assert_true(isc_nm_http_path_isvalid("/123")); +} + int main(void) { const struct CMUnitTest tests_short[] = { @@ -1975,6 +2010,8 @@ main(void) { NULL), cmocka_unit_test_setup_teardown(doh_base64_to_base64url, NULL, NULL), + cmocka_unit_test_setup_teardown(doh_path_validation, NULL, + NULL), cmocka_unit_test_setup_teardown(doh_noop_POST, nm_setup, nm_teardown), cmocka_unit_test_setup_teardown(doh_noop_GET, nm_setup, @@ -1994,6 +2031,8 @@ main(void) { NULL), cmocka_unit_test_setup_teardown(doh_base64_to_base64url, NULL, NULL), + cmocka_unit_test_setup_teardown(doh_path_validation, NULL, + NULL), cmocka_unit_test_setup_teardown(doh_noop_POST, nm_setup, nm_teardown), cmocka_unit_test_setup_teardown(doh_noop_GET, nm_setup, diff --git a/lib/isccfg/namedconf.c b/lib/isccfg/namedconf.c index 555c7d4a65e..7d283b0e4a6 100644 --- a/lib/isccfg/namedconf.c +++ b/lib/isccfg/namedconf.c @@ -90,7 +90,7 @@ static cfg_type_t cfg_type_bracketed_dscpsockaddrlist; static cfg_type_t cfg_type_bracketed_namesockaddrkeylist; static cfg_type_t cfg_type_bracketed_netaddrlist; static cfg_type_t cfg_type_bracketed_sockaddrnameportlist; -static cfg_type_t cfg_type_bracketed_qstring_list; +static cfg_type_t cfg_type_bracketed_http_endpoint_list; static cfg_type_t cfg_type_controls; static cfg_type_t cfg_type_controls_sockaddr; static cfg_type_t cfg_type_destinationlist; @@ -3891,15 +3891,17 @@ static cfg_type_t cfg_type_optional_tls = { /* http and https */ -static cfg_type_t cfg_type_bracketed_qstring_list = { "bracketed_qstring_list", - cfg_parse_bracketed_list, - cfg_print_bracketed_list, - cfg_doc_bracketed_list, - &cfg_rep_list, - &cfg_type_qstring }; +static cfg_type_t cfg_type_bracketed_http_endpoint_list = { + "bracketed_http_endpoint_list", + cfg_parse_bracketed_list, + cfg_print_bracketed_list, + cfg_doc_bracketed_list, + &cfg_rep_list, + &cfg_type_qstring +}; static cfg_clausedef_t cfg_http_description_clauses[] = { - { "endpoints", &cfg_type_bracketed_qstring_list, 0 }, { NULL, NULL, 0 } + { "endpoints", &cfg_type_bracketed_http_endpoint_list, 0 }, }; static cfg_clausedef_t *http_description_clausesets[] = { diff --git a/lib/isccfg/parser.c b/lib/isccfg/parser.c index 7d552a68a4b..37027733dd9 100644 --- a/lib/isccfg/parser.c +++ b/lib/isccfg/parser.c @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include diff --git a/lib/ns/interfacemgr.c b/lib/ns/interfacemgr.c index adebd6e4a16..2d0d60ca6da 100644 --- a/lib/ns/interfacemgr.c +++ b/lib/ns/interfacemgr.c @@ -549,6 +549,7 @@ ns_interface_listenhttp(ns_interface_t *ifp, isc_tlsctx_t *sslctx, char **eps, if (result == ISC_R_SUCCESS) { for (size_t i = 0; i < neps; i++) { + INSIST(isc_nm_http_path_isvalid(eps[i])); result = isc_nm_http_endpoint(sock, eps[i], ns__client_request, ifp, sizeof(ns_client_t));