From 9fb7980ec745d5200f0d88c01d735dbff1f90aad Mon Sep 17 00:00:00 2001 From: abhinavmir Date: Mon, 24 Aug 2026 15:37:34 -0700 Subject: [PATCH] Reject empty path arguments Path arguments become real filenames by concatenation onto "/oldroot/" or "/newroot/", so an empty path turns into the root directory. `--bind-try "" DEST` therefore gave the sandbox read/write access to the whole filesystem, and an empty destination replaced the sandbox root in the same way. The variants without `-try` failed earlier, because realpath() rejects an empty source, but the other options that take a path had no check at all. Check every path argument during option parsing, and report an empty one as a usage error. Signed-off-by: abhinavmir --- bubblewrap.c | 68 ++++++++++++++++++++++++++++------------------- bwrap.xml | 4 +++ tests/test-run.sh | 28 +++++++++++++++++++ 3 files changed, 73 insertions(+), 27 deletions(-) diff --git a/bubblewrap.c b/bubblewrap.c index 3b9719c4..bccd8d8a 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -1449,6 +1449,20 @@ warn_only_last_option (const char *name) warn ("Only the last %s option will take effect", name); } +/* Path arguments are turned into real filenames by concatenating them onto + "/oldroot/" or "/newroot/", so an empty path would silently be taken to + mean the root directory. That is never what the caller meant, so treat it + as a usage error. */ +static const char * +path_argument (const char *option, + const char *path) +{ + if (path[0] == '\0') + die ("%s does not take an empty path argument", option); + + return path; +} + static void make_setup_overlay_src_ops (const char *const *const argv) { @@ -1470,7 +1484,7 @@ make_setup_overlay_src_ops (const char *const *const argv) for (i = 1; i <= next_overlay_src_count; i++) { op = setup_op_new (SETUP_OVERLAY_SRC); - op->source = argv[1 - 2 * i]; + op->source = path_argument ("--overlay-src", argv[1 - 2 * i]); } next_overlay_src_count = 0; } @@ -1648,7 +1662,7 @@ parse_args_recurse (int *argcp, if (opt_chdir_path != NULL) warn_only_last_option ("--chdir"); - opt_chdir_path = argv[1]; + opt_chdir_path = path_argument (arg, argv[1]); argv++; argc--; } @@ -1666,7 +1680,7 @@ parse_args_recurse (int *argcp, die ("--remount-ro takes one argument"); op = setup_op_new (SETUP_REMOUNT_RO_NO_RECURSIVE); - op->dest = argv[1]; + op->dest = path_argument (arg, argv[1]); argv++; argc--; @@ -1678,8 +1692,8 @@ parse_args_recurse (int *argcp, die ("%s takes two arguments", arg); op = setup_op_new (SETUP_BIND_MOUNT); - op->source = argv[1]; - op->dest = argv[2]; + op->source = path_argument (arg, argv[1]); + op->dest = path_argument (arg, argv[2]); if (strcmp(arg, "--bind-try") == 0) op->flags = ALLOW_NOTEXIST; @@ -1693,8 +1707,8 @@ parse_args_recurse (int *argcp, die ("%s takes two arguments", arg); op = setup_op_new (SETUP_RO_BIND_MOUNT); - op->source = argv[1]; - op->dest = argv[2]; + op->source = path_argument (arg, argv[1]); + op->dest = path_argument (arg, argv[2]); if (strcmp(arg, "--ro-bind-try") == 0) op->flags = ALLOW_NOTEXIST; @@ -1708,8 +1722,8 @@ parse_args_recurse (int *argcp, die ("%s takes two arguments", arg); op = setup_op_new (SETUP_DEV_BIND_MOUNT); - op->source = argv[1]; - op->dest = argv[2]; + op->source = path_argument (arg, argv[1]); + op->dest = path_argument (arg, argv[2]); if (strcmp(arg, "--dev-bind-try") == 0) op->flags = ALLOW_NOTEXIST; @@ -1735,7 +1749,7 @@ parse_args_recurse (int *argcp, op = setup_op_new (SETUP_BIND_MOUNT); op->source = xasprintf ("/proc/self/fd/%d", src_fd); op->fd = src_fd; - op->dest = argv[2]; + op->dest = path_argument (arg, argv[2]); argv += 2; argc -= 2; @@ -1758,10 +1772,10 @@ parse_args_recurse (int *argcp, die ("--overlay requires at least one --overlay-src"); op = setup_op_new (SETUP_OVERLAY_MOUNT); - op->source = argv[1]; + op->source = path_argument (arg, argv[1]); workdir_op = setup_op_new (SETUP_OVERLAY_SRC); - workdir_op->source = argv[2]; - op->dest = argv[3]; + workdir_op->source = path_argument (arg, argv[2]); + op->dest = path_argument (arg, argv[3]); make_setup_overlay_src_ops (argv); argv += 3; @@ -1776,7 +1790,7 @@ parse_args_recurse (int *argcp, die ("--tmp-overlay requires at least one --overlay-src"); op = setup_op_new (SETUP_TMP_OVERLAY_MOUNT); - op->dest = argv[1]; + op->dest = path_argument (arg, argv[1]); make_setup_overlay_src_ops (argv); opt_tmp_overlay_count++; @@ -1792,7 +1806,7 @@ parse_args_recurse (int *argcp, die ("--ro-overlay requires at least two --overlay-src"); op = setup_op_new (SETUP_RO_OVERLAY_MOUNT); - op->dest = argv[1]; + op->dest = path_argument (arg, argv[1]); make_setup_overlay_src_ops (argv); argv += 1; @@ -1804,7 +1818,7 @@ parse_args_recurse (int *argcp, die ("--proc takes an argument"); op = setup_op_new (SETUP_MOUNT_PROC); - op->dest = argv[1]; + op->dest = path_argument (arg, argv[1]); argv += 1; argc -= 1; @@ -1845,7 +1859,7 @@ parse_args_recurse (int *argcp, die ("--dev takes an argument"); op = setup_op_new (SETUP_MOUNT_DEV); - op->dest = argv[1]; + op->dest = path_argument (arg, argv[1]); opt_needs_devpts = true; argv += 1; @@ -1857,7 +1871,7 @@ parse_args_recurse (int *argcp, die ("--tmpfs takes an argument"); op = setup_op_new (SETUP_MOUNT_TMPFS); - op->dest = argv[1]; + op->dest = path_argument (arg, argv[1]); /* We historically hard-coded the mode of a tmpfs as 0755. */ if (next_perms >= 0) @@ -1882,7 +1896,7 @@ parse_args_recurse (int *argcp, die ("--mqueue takes an argument"); op = setup_op_new (SETUP_MOUNT_MQUEUE); - op->dest = argv[1]; + op->dest = path_argument (arg, argv[1]); argv += 1; argc -= 1; @@ -1893,7 +1907,7 @@ parse_args_recurse (int *argcp, die ("--dir takes an argument"); op = setup_op_new (SETUP_MAKE_DIR); - op->dest = argv[1]; + op->dest = path_argument (arg, argv[1]); /* We historically hard-coded the mode of a --dir as 0755. */ if (next_perms >= 0) @@ -1919,7 +1933,7 @@ parse_args_recurse (int *argcp, op = setup_op_new (SETUP_MAKE_FILE); op->fd = file_fd; - op->dest = argv[2]; + op->dest = path_argument (arg, argv[2]); /* We historically hard-coded the mode of a --file as 0666. */ if (next_perms >= 0) @@ -1945,7 +1959,7 @@ parse_args_recurse (int *argcp, op = setup_op_new (SETUP_MAKE_BIND_FILE); op->fd = file_fd; - op->dest = argv[2]; + op->dest = path_argument (arg, argv[2]); /* This is consistent with previous bubblewrap behaviour: * before implementing --perms, we took the permissions @@ -1973,7 +1987,7 @@ parse_args_recurse (int *argcp, op = setup_op_new (SETUP_MAKE_RO_BIND_FILE); op->fd = file_fd; - op->dest = argv[2]; + op->dest = path_argument (arg, argv[2]); /* This is consistent with previous bubblewrap behaviour: * before implementing --perms, we took the permissions @@ -1993,8 +2007,8 @@ parse_args_recurse (int *argcp, die ("--symlink takes two arguments"); op = setup_op_new (SETUP_MAKE_SYMLINK); - op->source = argv[1]; - op->dest = argv[2]; + op->source = path_argument (arg, argv[1]); + op->dest = path_argument (arg, argv[2]); argv += 2; argc -= 2; @@ -2004,7 +2018,7 @@ parse_args_recurse (int *argcp, if (argc < 2) die ("--lock-file takes an argument"); - (void) lock_file_new (argv[1]); + (void) lock_file_new (path_argument (arg, argv[1])); argv += 1; argc -= 1; @@ -2434,7 +2448,7 @@ parse_args_recurse (int *argcp, op = setup_op_new (SETUP_CHMOD); op->flags = NO_CREATE_DEST; op->perms = (int) perms; - op->dest = argv[2]; + op->dest = path_argument (arg, argv[2]); argv += 2; argc -= 2; diff --git a/bwrap.xml b/bwrap.xml index ca717abd..b48cf5b4 100644 --- a/bwrap.xml +++ b/bwrap.xml @@ -73,6 +73,10 @@ When options are used multiple times, the last option wins, unless otherwise specified. + + An empty string is not a valid path, so options that take a path + reject it with an error. + General options: diff --git a/tests/test-run.sh b/tests/test-run.sh index 2d0d307d..a78e2b56 100755 --- a/tests/test-run.sh +++ b/tests/test-run.sh @@ -176,6 +176,34 @@ fi assert_file_has_content err.txt "^bwrap: Can't find source path.*source-enoent" ok "error prefixing" +# Test that an empty path argument is rejected. Empty paths used to be +# interpreted as the root directory, so for example --bind-try "" DEST +# silently gave the sandbox access to the whole filesystem. +for opt in --bind --bind-try --dev-bind --dev-bind-try --ro-bind --ro-bind-try; do + if $RUN "$opt" "" /mnt true 2>err.txt; then + assert_not_reached "$opt accepted an empty source" + fi + assert_file_has_content err.txt "^bwrap: $opt does not take an empty path argument" + + if $RUN "$opt" / "" true 2>err.txt; then + assert_not_reached "$opt accepted an empty destination" + fi + assert_file_has_content err.txt "^bwrap: $opt does not take an empty path argument" +done + +for opt in --chdir --dev --dir --mqueue --proc --remount-ro --tmpfs; do + if $RUN "$opt" "" true 2>err.txt; then + assert_not_reached "$opt accepted an empty path" + fi + assert_file_has_content err.txt "^bwrap: $opt does not take an empty path argument" +done + +if $RUN --symlink /usr "" true 2>err.txt; then + assert_not_reached "--symlink accepted an empty destination" +fi +assert_file_has_content err.txt "^bwrap: --symlink does not take an empty path argument" +ok "empty path arguments are rejected" + if ! ${is_uidzero}; then # When invoked as non-root, check that by default we have no caps left for OPT in "" "--unshare-user-try --as-pid-1" "--unshare-user-try" "--as-pid-1"; do