diff --git a/CHANGES b/CHANGES index 238d5993d1..4c3dc38ef7 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +5683. [func] The configuration checking code now verifies + HTTP paths. [GL !5231] + 5682. [bug] Not all changes to zone-statistics settings were properly processed. [GL #2820] 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 0000000000..ef1839aa7d --- /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 0000000000..6432ac1939 --- /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 0000000000..57c179b19d --- /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 0f14eeb17e..299718e2a7 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 c27de55476..e97983cce6 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 23605e2928..b494426e25 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 a51dbf233e..a7de43aeaf 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 2aec55fda5..7b6ccb1cc3 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 555c7d4a65..7d283b0e4a 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 7d552a68a4..37027733dd 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 adebd6e4a1..2d0d60ca6d 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));