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.

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