Merge branch 'artem/doh-http-path-validation' into 'main'

Verify HTTP paths both in incoming requests and in config file

See merge request isc-projects/bind9!5231

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]
 

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; };
+};

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; };
+};

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; };
+};

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:
 
 ::

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 = "<unknown 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) {

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);
 /*%<

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));
+}

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,

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[] = {

lib/isccfg/parser.c

@@ -28,6 +28,7 @@
 #include <isc/mem.h>
 #include <isc/net.h>
 #include <isc/netaddr.h>
+#include <isc/netmgr.h>
 #include <isc/netscope.h>
 #include <isc/print.h>
 #include <isc/sockaddr.h>

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));