The different fixed limits for absolute and relative path lengths of
Xenstore nodes make it possible to create per-domain nodes via
absolute paths which are not accessible using relative paths, as the
two limits differ by 1024 characters.

Instead of this weird limits use only one limit, which applies to the
relative path length of per-domain nodes and to the absolute path
length of all other nodes. This means, the path length check is
applied to the path after removing a possible start of
"/local/domain/<n>/" with <n> being a domain id.

There has been the request to be able to limit the path lengths even
more, so an additional quota is added which can be applied to path
lengths. It is XENSTORE_REL_PATH_MAX (2048) per default, but can be
set to lower values. This is done via the new "-M" or "--path-max"
option when invoking xenstored.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Paul Durrant <paul@xen.org>
Acked-by: Julien Grall <jgrall@amazon.com>
---
This patch was originally thought to be part of XSA-323, but later it
was decided not to include it, as in C Xenstored this is no security
issue.
---
 tools/xenstore/xenstored_core.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 746a1247b3..3082a36d3a 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -102,6 +102,7 @@ int quota_nb_watch_per_domain = 128;
 int quota_max_entry_size = 2048; /* 2K */
 int quota_max_transaction = 10;
 int quota_nb_perms_per_node = 5;
+int quota_max_path_len = XENSTORE_REL_PATH_MAX;
 
 void trace(const char *fmt, ...)
 {
@@ -734,6 +735,9 @@ static bool valid_chars(const char *node)
 
 bool is_valid_nodename(const char *node)
 {
+	int local_off = 0;
+	unsigned int domid;
+
 	/* Must start in /. */
 	if (!strstarts(node, "/"))
 		return false;
@@ -746,7 +750,10 @@ bool is_valid_nodename(const char *node)
 	if (strstr(node, "//"))
 		return false;
 
-	if (strlen(node) > XENSTORE_ABS_PATH_MAX)
+	if (sscanf(node, "/local/domain/%5u/%n", &domid, &local_off) != 1)
+		local_off = 0;
+
+	if (strlen(node) > local_off + quota_max_path_len)
 		return false;
 
 	return valid_chars(node);
@@ -806,6 +813,8 @@ static struct node *get_node_canonicalized(struct connection *conn,
 	if (!canonical_name)
 		canonical_name = &tmp_name;
 	*canonical_name = canonicalize(conn, ctx, name);
+	if (!*canonical_name)
+		return NULL;
 	return get_node(conn, ctx, *canonical_name, perm);
 }
 
@@ -1926,6 +1935,7 @@ static void usage(void)
 "  -W, --watch-nb <nb>     limit the number of watches per domain,\n"
 "  -t, --transaction <nb>  limit the number of transaction allowed per domain,\n"
 "  -A, --perm-nb <nb>      limit the number of permissions per node,\n"
+"  -M, --path-max <chars>  limit the allowed Xenstore node path length,\n"
 "  -R, --no-recovery       to request that no recovery should be attempted when\n"
 "                          the store is corrupted (debug only),\n"
 "  -I, --internal-db       store database in memory, not on disk\n"
@@ -1947,6 +1957,7 @@ static struct option options[] = {
 	{ "trace-file", 1, NULL, 'T' },
 	{ "transaction", 1, NULL, 't' },
 	{ "perm-nb", 1, NULL, 'A' },
+	{ "path-max", 1, NULL, 'M' },
 	{ "no-recovery", 0, NULL, 'R' },
 	{ "internal-db", 0, NULL, 'I' },
 	{ "verbose", 0, NULL, 'V' },
@@ -1969,7 +1980,7 @@ int main(int argc, char *argv[])
 	int timeout;
 
 
-	while ((opt = getopt_long(argc, argv, "DE:F:HNPS:t:A:T:RVW:", options,
+	while ((opt = getopt_long(argc, argv, "DE:F:HNPS:t:A:M:T:RVW:", options,
 				  NULL)) != -1) {
 		switch (opt) {
 		case 'D':
@@ -2014,6 +2025,10 @@ int main(int argc, char *argv[])
 		case 'A':
 			quota_nb_perms_per_node = strtol(optarg, NULL, 10);
 			break;
+			quota_max_path_len = strtol(optarg, NULL, 10);
+			quota_max_path_len = min(XENSTORE_REL_PATH_MAX,
+						 quota_max_path_len);
+			break;
 		case 'e':
 			dom0_event = strtol(optarg, NULL, 10);
 			break;
-- 
2.26.2

On 15/12/2020 15:04, Juergen Gross wrote:
> The different fixed limits for absolute and relative path lengths of
> Xenstore nodes make it possible to create per-domain nodes via
> absolute paths which are not accessible using relative paths, as the
> two limits differ by 1024 characters.
>
> Instead of this weird limits use only one limit, which applies to the
> relative path length of per-domain nodes and to the absolute path
> length of all other nodes. This means, the path length check is
> applied to the path after removing a possible start of
> "/local/domain/<n>/" with <n> being a domain id.
>
> There has been the request to be able to limit the path lengths even
> more, so an additional quota is added which can be applied to path
> lengths. It is XENSTORE_REL_PATH_MAX (2048) per default, but can be
> set to lower values. This is done via the new "-M" or "--path-max"
> option when invoking xenstored.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> Reviewed-by: Paul Durrant <paul@xen.org>
> Acked-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

On Tue, Dec 15, 2020 at 04:04:11PM +0100, Juergen Gross wrote:
> The different fixed limits for absolute and relative path lengths of
> Xenstore nodes make it possible to create per-domain nodes via
> absolute paths which are not accessible using relative paths, as the
> two limits differ by 1024 characters.
> 
> Instead of this weird limits use only one limit, which applies to the
> relative path length of per-domain nodes and to the absolute path
> length of all other nodes. This means, the path length check is
> applied to the path after removing a possible start of
> "/local/domain/<n>/" with <n> being a domain id.
> 
> There has been the request to be able to limit the path lengths even
> more, so an additional quota is added which can be applied to path
> lengths. It is XENSTORE_REL_PATH_MAX (2048) per default, but can be
> set to lower values. This is done via the new "-M" or "--path-max"
> option when invoking xenstored.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> Reviewed-by: Paul Durrant <paul@xen.org>
> Acked-by: Julien Grall <jgrall@amazon.com>

Acked-by: Wei Liu <wl@xen.org>