From d51f210cf4c82462730f2877ece14513717aa42e Mon Sep 17 00:00:00 2001 From: Ryan Hendrickson Date: Sat, 13 May 2023 23:32:00 -0400 Subject: [PATCH 1/5] Add --overlay and related options This commit adds --overlay, --tmp-overlay, --ro-overlay, and --overlay-src options to enable bubblewrap to create overlay mounts. These options are only permitted when bubblewrap is not installed setuid. Resolves: https://github.com/containers/bubblewrap/issues/412 Co-authored-by: William Manley Signed-off-by: Ryan Hendrickson --- bubblewrap.c | 180 +++++++++++++++++++++++++++++++++++++++++ bwrap.xml | 65 +++++++++++++++ completions/bash/bwrap | 4 + tests/test-run.sh | 108 +++++++++++++++++++++++++ tests/test-utils.c | 20 +++++ utils.c | 80 ++++++++++++++++++ utils.h | 17 ++++ 7 files changed, 474 insertions(+) diff --git a/bubblewrap.c b/bubblewrap.c index 9b78a9ae..2cbc8508 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -100,8 +100,10 @@ static char *opt_args_data = NULL; /* owned */ static int opt_userns_fd = -1; static int opt_userns2_fd = -1; static int opt_pidns_fd = -1; +static int opt_tmp_overlay_count = 0; static int next_perms = -1; static size_t next_size_arg = 0; +static int next_overlay_src_count = 0; #define CAP_TO_MASK_0(x) (1L << ((x) & 31)) #define CAP_TO_MASK_1(x) CAP_TO_MASK_0(x - 32) @@ -131,6 +133,10 @@ typedef enum { SETUP_BIND_MOUNT, SETUP_RO_BIND_MOUNT, SETUP_DEV_BIND_MOUNT, + SETUP_OVERLAY_MOUNT, + SETUP_TMP_OVERLAY_MOUNT, + SETUP_RO_OVERLAY_MOUNT, + SETUP_OVERLAY_SRC, SETUP_MOUNT_PROC, SETUP_MOUNT_DEV, SETUP_MOUNT_TMPFS, @@ -176,6 +182,7 @@ struct _LockFile enum { PRIV_SEP_OP_DONE, PRIV_SEP_OP_BIND_MOUNT, + PRIV_SEP_OP_OVERLAY_MOUNT, PRIV_SEP_OP_PROC_MOUNT, PRIV_SEP_OP_TMPFS_MOUNT, PRIV_SEP_OP_DEVPTS_MOUNT, @@ -342,6 +349,11 @@ usage (int ecode, FILE *out) " --ro-bind SRC DEST Bind mount the host path SRC readonly on DEST\n" " --ro-bind-try SRC DEST Equal to --ro-bind but ignores non-existent SRC\n" " --remount-ro DEST Remount DEST as readonly; does not recursively remount\n" + " --overlay-src SRC Read files from SRC in the following overlay\n" + " --overlay RWSRC WORKDIR DEST Mount overlayfs on DEST, with RWSRC as the host path for writes and\n" + " WORKDIR an empty directory on the same filesystem as RWSRC\n" + " --tmp-overlay DEST Mount overlayfs on DEST, with writes going to an invisible tmpfs\n" + " --ro-overlay DEST Mount overlayfs read-only on DEST\n" " --exec-label LABEL Exec label for the sandbox\n" " --file-label LABEL File label for temporary sandbox content\n" " --proc DEST Mount new procfs on DEST\n" @@ -1147,6 +1159,12 @@ privileged_op (int privileged_op_socket, die_with_mount_error ("Can't mount mqueue on %s", arg1); break; + case PRIV_SEP_OP_OVERLAY_MOUNT: + if (mount ("overlay", arg2, "overlay", MS_MGC_VAL, arg1) != 0) + die_with_error ("Can't make overlay mount on %s with options %s", + arg2, arg1); + break; + case PRIV_SEP_OP_SET_HOSTNAME: /* This is checked at the start, but lets verify it here in case something manages to send hacked priv-sep operation requests. */ @@ -1170,6 +1188,7 @@ setup_newroot (bool unshare_pid, int privileged_op_socket) { SetupOp *op; + int tmp_overlay_idx = 0; for (op = ops; op != NULL; op = op->next) { @@ -1233,6 +1252,45 @@ setup_newroot (bool unshare_pid, 0, 0, source, dest); break; + case SETUP_OVERLAY_MOUNT: + case SETUP_RO_OVERLAY_MOUNT: + case SETUP_TMP_OVERLAY_MOUNT: + { + StringBuilder sb = {0}; + bool multi_src = FALSE; + + if (mkdir (dest, 0755) != 0 && errno != EEXIST) + die_with_error ("Can't mkdir %s", op->dest); + + if (op->source != NULL) + { + strappend (&sb, "upperdir=/oldroot"); + strappend_escape_for_mount_options (&sb, op->source); + strappend (&sb, ",workdir=/oldroot"); + op = op->next; + strappend_escape_for_mount_options (&sb, op->source); + strappend (&sb, ","); + } + else if (op->type == SETUP_TMP_OVERLAY_MOUNT) + strappendf (&sb, "upperdir=/tmp-overlay-upper-%1$d,workdir=/tmp-overlay-work-%1$d,", + tmp_overlay_idx++); + + strappend (&sb, "lowerdir=/oldroot"); + while (op->next != NULL && op->next->type == SETUP_OVERLAY_SRC) + { + op = op->next; + if (multi_src) + strappend (&sb, ":/oldroot"); + strappend_escape_for_mount_options (&sb, op->source); + multi_src = TRUE; + } + + privileged_op (privileged_op_socket, + PRIV_SEP_OP_OVERLAY_MOUNT, 0, 0, 0, sb.str, dest); + free (sb.str); + } + break; + case SETUP_REMOUNT_RO_NO_RECURSIVE: privileged_op (privileged_op_socket, PRIV_SEP_OP_REMOUNT_RO_NO_RECURSIVE, 0, 0, 0, NULL, dest); @@ -1496,6 +1554,7 @@ setup_newroot (bool unshare_pid, op->dest, NULL); break; + case SETUP_OVERLAY_SRC: /* handled by SETUP_OVERLAY_MOUNT */ default: die ("Unexpected type %d", op->type); } @@ -1538,6 +1597,8 @@ resolve_symlinks_in_ops (void) case SETUP_RO_BIND_MOUNT: case SETUP_DEV_BIND_MOUNT: case SETUP_BIND_MOUNT: + case SETUP_OVERLAY_SRC: + case SETUP_OVERLAY_MOUNT: old_source = op->source; op->source = realpath (old_source, NULL); if (op->source == NULL) @@ -1549,6 +1610,8 @@ resolve_symlinks_in_ops (void) } break; + case SETUP_RO_OVERLAY_MOUNT: + case SETUP_TMP_OVERLAY_MOUNT: case SETUP_MOUNT_PROC: case SETUP_MOUNT_DEV: case SETUP_MOUNT_TMPFS: @@ -1640,6 +1703,32 @@ warn_only_last_option (const char *name) warn ("Only the last %s option will take effect", name); } +static void +make_setup_overlay_src_ops (const char *const *const argv) +{ + /* SETUP_OVERLAY_SRC is unlike other SETUP_* ops in that it exists to hold + * data for SETUP_{,TMP_,RO_}OVERLAY_MOUNT ops, not to be its own operation. + * This lets us reuse existing code paths to handle resolving the realpaths + * of each source, as no other operations involve multiple sources the way + * the *_OVERLAY_MOUNT ops do. + * + * While the --overlay-src arguments are expected to precede the + * --overlay argument, in bottom-to-top order, the SETUP_OVERLAY_SRC ops + * follow their corresponding *_OVERLAY_MOUNT op, in top-to-bottom order + * (the order in which overlayfs will want them). They are handled specially + * in setup_new_root () during the processing of *_OVERLAY_MOUNT. + */ + int i; + SetupOp *op; + + for (i = 1; i <= next_overlay_src_count; i++) + { + op = setup_op_new (SETUP_OVERLAY_SRC); + op->source = argv[1 - 2 * i]; + } + next_overlay_src_count = 0; +} + static void parse_args_recurse (int *argcp, const char ***argvp, @@ -1877,6 +1966,76 @@ parse_args_recurse (int *argcp, argv += 2; argc -= 2; } + else if (strcmp (arg, "--overlay-src") == 0) + { + if (is_privileged) + die ("The --overlay-src option is not permitted in setuid mode"); + + next_overlay_src_count++; + + argv += 1; + argc -= 1; + } + else if (strcmp (arg, "--overlay") == 0) + { + SetupOp *workdir_op; + + if (is_privileged) + die ("The --overlay option is not permitted in setuid mode"); + + if (argc < 4) + die ("--overlay takes three arguments"); + + if (next_overlay_src_count < 1) + die ("--overlay requires at least one --overlay-src"); + + op = setup_op_new (SETUP_OVERLAY_MOUNT); + op->source = argv[1]; + workdir_op = setup_op_new (SETUP_OVERLAY_SRC); + workdir_op->source = argv[2]; + op->dest = argv[3]; + make_setup_overlay_src_ops (argv); + + argv += 3; + argc -= 3; + } + else if (strcmp (arg, "--tmp-overlay") == 0) + { + if (is_privileged) + die ("The --tmp-overlay option is not permitted in setuid mode"); + + if (argc < 2) + die ("--tmp-overlay takes an argument"); + + if (next_overlay_src_count < 1) + die ("--tmp-overlay requires at least one --overlay-src"); + + op = setup_op_new (SETUP_TMP_OVERLAY_MOUNT); + op->dest = argv[1]; + make_setup_overlay_src_ops (argv); + opt_tmp_overlay_count++; + + argv += 1; + argc -= 1; + } + else if (strcmp (arg, "--ro-overlay") == 0) + { + if (is_privileged) + die ("The --ro-overlay option is not permitted in setuid mode"); + + if (argc < 2) + die ("--ro-overlay takes an argument"); + + if (next_overlay_src_count < 2) + die ("--ro-overlay requires at least two --overlay-src"); + + op = setup_op_new (SETUP_RO_OVERLAY_MOUNT); + op->dest = argv[1]; + make_setup_overlay_src_ops (argv); + + argv += 1; + argc -= 1; + } else if (strcmp (arg, "--proc") == 0) { if (argc < 2) @@ -2546,6 +2705,10 @@ parse_args_recurse (int *argcp, if (!is_modifier_option(arg) && next_size_arg != 0) die ("--size must be followed by --tmpfs"); + /* Similarly for --overlay-src. */ + if (strcmp (arg, "--overlay-src") != 0 && next_overlay_src_count > 0) + die ("--overlay-src must be followed by another --overlay-src or one of --overlay, --tmp-overlay, or --ro-overlay"); + argv++; argc--; } @@ -2561,6 +2724,9 @@ parse_args (int *argcp, int total_parsed_argc = *argcp; parse_args_recurse (argcp, argvp, FALSE, &total_parsed_argc); + + if (next_overlay_src_count > 0) + die ("--overlay-src must be followed by another --overlay-src or one of --overlay, --tmp-overlay, or --ro-overlay"); } static void @@ -2666,6 +2832,7 @@ main (int argc, cleanup_free char *args_data UNUSED = NULL; int intermediate_pids_sockets[2] = {-1, -1}; const char *exec_path = NULL; + int i; /* Handle --version early on before we try to acquire/drop * any capabilities so it works in a build environment; @@ -3106,6 +3273,19 @@ main (int argc, if (mkdir ("oldroot", 0755)) die_with_error ("Creating oldroot failed"); + for (i = 0; i < opt_tmp_overlay_count; i++) + { + char *dirname; + dirname = xasprintf ("tmp-overlay-upper-%d", i); + if (mkdir (dirname, 0755)) + die_with_error ("Creating --tmp-overlay upperdir failed"); + free (dirname); + dirname = xasprintf ("tmp-overlay-work-%d", i); + if (mkdir (dirname, 0755)) + die_with_error ("Creating --tmp-overlay workdir failed"); + free (dirname); + } + if (pivot_root (base_path, "oldroot")) die_with_error ("pivot_root"); diff --git a/bwrap.xml b/bwrap.xml index 3bb50820..51b4f433 100644 --- a/bwrap.xml +++ b/bwrap.xml @@ -297,6 +297,71 @@ Remount the path DEST as readonly. It works only on the specified mount point, without changing any other mount point under the specified path + + + + + This option does nothing on its own, and must be followed by one of + the other overlay options. It specifies a host + path from which files should be read if they aren't present in a + higher layer. + + + This option can be used multiple times to provide multiple sources. + The sources are overlaid from bottom to top: if a given path to be + read exists in more than one source, the file is read from the last + such source specified. + + + + + + + + + + + + + + Use overlayfs to mount the host paths specified by + RWSRC and all immediately preceding + on DEST. + DEST will contain the union of all the files + in all the layers. + + + With --overlay all writes will go to + RWSRC. Reads will come preferentially from + RWSRC, and then from any + paths. + WORKDIR must be an empty directory on the + same filesystem as RWSRC. + + + With --tmp-overlay all writes will go to + the tmpfs that hosts the sandbox root, in a location not accessible + from either the host or the child process. Writes will therefore not + be persisted across multiple runs. + + + With --ro-overlay the filesystem will be + mounted read-only. This option requires at least two + to precede it. + + + None of these options are available in the setuid version of + bubblewrap. Using --ro-overlay or providing + more than one requires a Linux kernel + version of 4.0 or later. + + + For more information see the Overlay Filesystem documentation in the + Linux kernel at + https://www.kernel.org/doc/Documentation/filesystems/overlayfs.txt + + + Mount procfs on DEST diff --git a/completions/bash/bwrap b/completions/bash/bwrap index e6d28462..e7a523c2 100644 --- a/completions/bash/bwrap +++ b/completions/bash/bwrap @@ -51,15 +51,19 @@ _bwrap() { --hostname --info-fd --lock-file + --overlay + --overlay-src --perms --proc --remount-ro --ro-bind + --ro-overlay --seccomp --setenv --size --symlink --sync-fd + --tmp-overlay --uid --unsetenv --userns-block-fd diff --git a/tests/test-run.sh b/tests/test-run.sh index 6151f1a8..4eb8a7a8 100755 --- a/tests/test-run.sh +++ b/tests/test-run.sh @@ -565,4 +565,112 @@ $RUN --argv0 right sh -c 'echo $0' > stdout assert_file_has_content stdout right ok "argv0 manipulation" +if test -n "${bwrap_is_suid:-}"; then + ok_skip "no --overlay support" + ok_skip "no --overlay support" + ok_skip "no --tmp-overlay support" + ok_skip "no --ro-overlay support" + ok_skip "no --overlay-src support" +else + mkdir lower1 lower2 upper work + printf 1 > lower1/a + printf 2 > lower1/b + printf 3 > lower2/b + printf 4 > upper/a + + # Check if unprivileged overlayfs is available + if ! unshare -rm mount -t overlay -o lowerdir=lower1,upperdir=upper,workdir=work overlay lower2; then + ok_skip "no kernel support for unprivileged overlayfs" + ok_skip "no kernel support for unprivileged overlayfs" + ok_skip "no kernel support for unprivileged overlayfs" + ok_skip "no kernel support for unprivileged overlayfs" + ok_skip "no kernel support for unprivileged overlayfs" + else + + # Test --overlay + if $RUN --overlay upper work /tmp true 2>err.txt; then + assert_not_reached At least one --overlay-src not required + fi + assert_file_has_content err.txt "^bwrap: --overlay requires at least one --overlay-src" + $RUN --overlay-src lower1 --overlay upper work /tmp/x/y/z cat /tmp/x/y/z/a > stdout + assert_file_has_content stdout '^4$' + $RUN --overlay-src lower1 --overlay upper work /tmp/x/y/z cat /tmp/x/y/z/b > stdout + assert_file_has_content stdout '^2$' + $RUN --overlay-src lower1 --overlay-src lower2 --overlay upper work /tmp/x/y/z cat /tmp/x/y/z/a > stdout + assert_file_has_content stdout '^4$' + $RUN --overlay-src lower1 --overlay-src lower2 --overlay upper work /tmp/x/y/z cat /tmp/x/y/z/b > stdout + assert_file_has_content stdout '^3$' + $RUN --overlay-src lower1 --overlay-src lower2 --overlay upper work /tmp/x/y/z sh -c 'printf 5 > /tmp/x/y/z/b; cat /tmp/x/y/z/b' > stdout + assert_file_has_content stdout '^5$' + assert_file_has_content upper/b '^5$' + ok "--overlay" + + # Test --overlay path escaping + # Coincidentally, :,\ is the face I make contemplating anyone who might + # need this functionality, not that that's going to stop me from supporting + # it. + mkdir 'lower :,\' 'upper :,\' 'work :,\' + printf 1 > 'lower :,\'/a + $RUN --overlay-src 'lower :,\' --overlay 'upper :,\' 'work :,\' /tmp/x sh -c 'cat /tmp/x/a; printf 2 > /tmp/x/a; cat /tmp/x/a' > stdout + assert_file_has_content stdout '^12$' + assert_file_has_content 'lower :,\'/a '^1$' + assert_file_has_content 'upper :,\'/a '^2$' + ok "--overlay path escaping" + + # Test --tmp-overlay + printf 1 > lower1/a + printf 2 > lower1/b + printf 3 > lower2/b + if $RUN --tmp-overlay /tmp true 2>err.txt; then + assert_not_reached At least one --overlay-src not required + fi + assert_file_has_content err.txt "^bwrap: --tmp-overlay requires at least one --overlay-src" + $RUN --overlay-src lower1 --tmp-overlay /tmp/x/y/z cat /tmp/x/y/z/a > stdout + assert_file_has_content stdout '^1$' + $RUN --overlay-src lower1 --tmp-overlay /tmp/x/y/z cat /tmp/x/y/z/b > stdout + assert_file_has_content stdout '^2$' + $RUN --overlay-src lower1 --overlay-src lower2 --tmp-overlay /tmp/x/y/z cat /tmp/x/y/z/a > stdout + assert_file_has_content stdout '^1$' + $RUN --overlay-src lower1 --overlay-src lower2 --tmp-overlay /tmp/x/y/z cat /tmp/x/y/z/b > stdout + assert_file_has_content stdout '^3$' + $RUN --overlay-src lower1 --overlay-src lower2 --tmp-overlay /tmp/x/y/z sh -c 'printf 4 > /tmp/x/y/z/b; cat /tmp/x/y/z/b' > stdout + assert_file_has_content stdout '^4$' + $RUN --overlay-src lower1 --tmp-overlay /tmp/x --overlay-src lower2 --tmp-overlay /tmp/y sh -c 'cat /tmp/x/b; printf 4 > /tmp/x/b; cat /tmp/x/b; cat /tmp/y/b' > stdout + assert_file_has_content stdout '^243$' + ok "--tmp-overlay" + + # Test --ro-overlay + printf 1 > lower1/a + printf 2 > lower1/b + printf 3 > lower2/b + if $RUN --ro-overlay /tmp true 2>err.txt; then + assert_not_reached At least two --overlay-src not required + fi + assert_file_has_content err.txt "^bwrap: --ro-overlay requires at least two --overlay-src" + if $RUN --overlay-src lower1 --ro-overlay /tmp true 2>err.txt; then + assert_not_reached At least two --overlay-src not required + fi + assert_file_has_content err.txt "^bwrap: --ro-overlay requires at least two --overlay-src" + $RUN --overlay-src lower1 --overlay-src lower2 --ro-overlay /tmp/x/y/z cat /tmp/x/y/z/a > stdout + assert_file_has_content stdout '^1$' + $RUN --overlay-src lower1 --overlay-src lower2 --ro-overlay /tmp/x/y/z cat /tmp/x/y/z/b > stdout + assert_file_has_content stdout '^3$' + $RUN --overlay-src lower1 --overlay-src lower2 --ro-overlay /tmp/x/y/z sh -c 'printf 4 > /tmp/x/y/z/b; cat /tmp/x/y/z/b' > stdout + assert_file_has_content stdout '^3$' + ok "--ro-overlay" + + # Test --overlay-src restrictions + if $RUN --overlay-src /tmp true 2>err.txt; then + assert_not_reached Trailing --overlay-src allowed + fi + assert_file_has_content err.txt "^bwrap: --overlay-src must be followed by another --overlay-src or one of --overlay, --tmp-overlay, or --ro-overlay" + if $RUN --overlay-src /tmp --chdir / true 2>err.txt; then + assert_not_reached --overlay-src allowed to precede non-overlay options + fi + assert_file_has_content err.txt "^bwrap: --overlay-src must be followed by another --overlay-src or one of --overlay, --tmp-overlay, or --ro-overlay" + ok "--overlay-src restrictions" + + fi +fi + done_testing diff --git a/tests/test-utils.c b/tests/test-utils.c index 41874a15..cdbd491c 100644 --- a/tests/test-utils.c +++ b/tests/test-utils.c @@ -200,6 +200,25 @@ test_has_path_prefix (void) } } +static void +test_string_builder (void) +{ + StringBuilder sb = {0}; + + strappend (&sb, "aaa"); + g_assert_cmpstr (sb.str, ==, "aaa"); + strappend (&sb, "bbb"); + g_assert_cmpstr (sb.str, ==, "aaabbb"); + strappendf (&sb, "c%dc%s", 9, "x"); + g_assert_cmpstr (sb.str, ==, "aaabbbc9cx"); + strappend_escape_for_mount_options (&sb, "/path :,\\"); + g_assert_cmpstr (sb.str, ==, "aaabbbc9cx/path \\:\\,\\\\"); + strappend (&sb, "zzz"); + g_assert_cmpstr (sb.str, ==, "aaabbbc9cx/path \\:\\,\\\\zzz"); + + free (sb.str); +} + int main (int argc UNUSED, char **argv UNUSED) @@ -210,6 +229,7 @@ main (int argc UNUSED, test_strconcat3 (); test_has_prefix (); test_has_path_prefix (); + test_string_builder (); printf ("1..%u\n", test_number); return 0; } diff --git a/utils.c b/utils.c index 43c8d798..cbd53ddb 100644 --- a/utils.c +++ b/utils.c @@ -23,6 +23,7 @@ #include #include #include +#include #ifdef HAVE_SELINUX #include #endif @@ -943,3 +944,82 @@ mount_strerror (int errsv) return strerror (errsv); } } + +void +strappend (StringBuilder *dest, const char *src) +{ + size_t len = strlen (src); + + if (dest->offset + len >= dest->size) + { + dest->size = (dest->size + len + 1) * 2; + dest->str = xrealloc (dest->str, dest->size); + } + + strncpy (dest->str + dest->offset, src, len + 1); + dest->offset += len; +} + +__attribute__((format (printf, 2, 3))) +void +strappendf (StringBuilder *dest, const char *fmt, ...) +{ + va_list args; + int len; + + va_start (args, fmt); + len = vsnprintf (dest->str + dest->offset, dest->size - dest->offset, fmt, args); + va_end (args); + if (len < 0) + die_with_error ("vsnprintf"); + if (dest->offset + len >= dest->size) + { + dest->size = (dest->size + len + 1) * 2; + dest->str = xrealloc (dest->str, dest->size); + va_start (args, fmt); + len = vsnprintf (dest->str + dest->offset, dest->size - dest->offset, fmt, args); + va_end (args); + if (len < 0) + die_with_error ("vsnprintf"); + } + + dest->offset += len; +} + +void +strappend_escape_for_mount_options (StringBuilder *dest, const char *src) +{ + bool unescaped = TRUE; + + for (;;) + { + if (dest->offset == dest->size) + { + dest->size = MAX (64, dest->size * 2); + dest->str = xrealloc (dest->str, dest->size); + } + switch (*src) + { + case '\0': + dest->str[dest->offset] = '\0'; + return; + + case '\\': + case ',': + case ':': + if (unescaped) + { + dest->str[dest->offset++] = '\\'; + unescaped = FALSE; + continue; + } + /* else fall through */ + + default: + dest->str[dest->offset++] = *src; + unescaped = TRUE; + break; + } + src++; + } +} diff --git a/utils.h b/utils.h index 9f17297d..444307a8 100644 --- a/utils.h +++ b/utils.h @@ -186,3 +186,20 @@ steal_pointer (void *pp) /* type safety */ #define steal_pointer(pp) \ (0 ? (*(pp)) : (steal_pointer) (pp)) + +typedef struct _StringBuilder StringBuilder; + +struct _StringBuilder +{ + char * str; + size_t size; + size_t offset; +}; + +void strappend (StringBuilder *dest, + const char *src); +void strappendf (StringBuilder *dest, + const char *fmt, + ...); +void strappend_escape_for_mount_options (StringBuilder *dest, + const char *src); From 5a86db6b5aad5baf870525ab6c14b35e1668f8e5 Mon Sep 17 00:00:00 2001 From: Ryan Hendrickson Date: Fri, 17 Nov 2023 20:47:01 -0500 Subject: [PATCH 2/5] Touch up comments, tests Signed-off-by: Ryan Hendrickson --- bubblewrap.c | 2 +- tests/test-run.sh | 12 ++++++------ tests/test-utils.c | 12 ++++++++++++ utils.c | 3 +++ 4 files changed, 22 insertions(+), 7 deletions(-) diff --git a/bubblewrap.c b/bubblewrap.c index 2cbc8508..e751ce7c 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -1712,7 +1712,7 @@ make_setup_overlay_src_ops (const char *const *const argv) * of each source, as no other operations involve multiple sources the way * the *_OVERLAY_MOUNT ops do. * - * While the --overlay-src arguments are expected to precede the + * While the --overlay-src arguments are expected to (directly) precede the * --overlay argument, in bottom-to-top order, the SETUP_OVERLAY_SRC ops * follow their corresponding *_OVERLAY_MOUNT op, in top-to-bottom order * (the order in which overlayfs will want them). They are handled specially diff --git a/tests/test-run.sh b/tests/test-run.sh index 4eb8a7a8..8386e361 100755 --- a/tests/test-run.sh +++ b/tests/test-run.sh @@ -606,15 +606,15 @@ else ok "--overlay" # Test --overlay path escaping - # Coincidentally, :,\ is the face I make contemplating anyone who might + # Coincidentally, ":,\ is the face I make contemplating anyone who might # need this functionality, not that that's going to stop me from supporting # it. - mkdir 'lower :,\' 'upper :,\' 'work :,\' - printf 1 > 'lower :,\'/a - $RUN --overlay-src 'lower :,\' --overlay 'upper :,\' 'work :,\' /tmp/x sh -c 'cat /tmp/x/a; printf 2 > /tmp/x/a; cat /tmp/x/a' > stdout + mkdir 'lower ":,\' 'upper ":,\' 'work ":,\' + printf 1 > 'lower ":,\'/a + $RUN --overlay-src 'lower ":,\' --overlay 'upper ":,\' 'work ":,\' /tmp/x sh -c 'cat /tmp/x/a; printf 2 > /tmp/x/a; cat /tmp/x/a' > stdout assert_file_has_content stdout '^12$' - assert_file_has_content 'lower :,\'/a '^1$' - assert_file_has_content 'upper :,\'/a '^2$' + assert_file_has_content 'lower ":,\'/a '^1$' + assert_file_has_content 'upper ":,\'/a '^2$' ok "--overlay path escaping" # Test --tmp-overlay diff --git a/tests/test-utils.c b/tests/test-utils.c index cdbd491c..c3fe32c3 100644 --- a/tests/test-utils.c +++ b/tests/test-utils.c @@ -216,6 +216,18 @@ test_string_builder (void) strappend (&sb, "zzz"); g_assert_cmpstr (sb.str, ==, "aaabbbc9cx/path \\:\\,\\\\zzz"); + free (sb.str); + sb = (StringBuilder){0}; + + strappend_escape_for_mount_options (&sb, "aaa"); + g_assert_cmpstr (sb.str, ==, "aaa"); + + free (sb.str); + sb = (StringBuilder){0}; + + strappend_escape_for_mount_options (&sb, "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); + g_assert_cmpstr (sb.str, ==, "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); + free (sb.str); } diff --git a/utils.c b/utils.c index cbd53ddb..89cd5024 100644 --- a/utils.c +++ b/utils.c @@ -956,6 +956,9 @@ strappend (StringBuilder *dest, const char *src) dest->str = xrealloc (dest->str, dest->size); } + /* Preserves the invariant that dest->str is always null-terminated, even + * though the offset is positioned at the null byte for the next write. + */ strncpy (dest->str + dest->offset, src, len + 1); dest->offset += len; } From d1fb5a4fcebf20af61c6b22b454d42a6dd78be23 Mon Sep 17 00:00:00 2001 From: Ryan Hendrickson Date: Fri, 17 Nov 2023 20:54:31 -0500 Subject: [PATCH 3/5] Mention overlapping directories gotcha Signed-off-by: Ryan Hendrickson --- bubblewrap.c | 12 ++++++++++-- bwrap.xml | 8 ++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/bubblewrap.c b/bubblewrap.c index e751ce7c..c55877ac 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -1161,8 +1161,16 @@ privileged_op (int privileged_op_socket, case PRIV_SEP_OP_OVERLAY_MOUNT: if (mount ("overlay", arg2, "overlay", MS_MGC_VAL, arg1) != 0) - die_with_error ("Can't make overlay mount on %s with options %s", - arg2, arg1); + { + /* The standard message for ELOOP, "Too many levels of symbolic + * links", is not helpful here. */ + if (errno == ELOOP) + die ("Can't make overlay mount on %s with options %s: " + "Overlay directories may not overlap", + arg2, arg1); + die_with_error ("Can't make overlay mount on %s with options %s", + arg2, arg1); + } break; case PRIV_SEP_OP_SET_HOSTNAME: diff --git a/bwrap.xml b/bwrap.xml index 51b4f433..58d7bcd9 100644 --- a/bwrap.xml +++ b/bwrap.xml @@ -355,6 +355,14 @@ more than one requires a Linux kernel version of 4.0 or later. + + Due to limitations of overlayfs, no host directory given via + --overlay-src or + --overlay may be an ancestor of another, + after resolving symlinks. Depending on version, the Linux kernel may + or may not enforce this, but if not then overlayfs's behavior is + undefined. + For more information see the Overlay Filesystem documentation in the Linux kernel at From 06bf46061cdff2bf6eab98beabd525793d9148cc Mon Sep 17 00:00:00 2001 From: Ryan Hendrickson Date: Sun, 21 Apr 2024 23:48:04 -0400 Subject: [PATCH 4/5] Use userxattr mount flag Signed-off-by: Ryan Hendrickson --- bubblewrap.c | 2 ++ tests/test-run.sh | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/bubblewrap.c b/bubblewrap.c index c55877ac..175013a4 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -1293,6 +1293,8 @@ setup_newroot (bool unshare_pid, multi_src = TRUE; } + strappend (&sb, ",userxattr"); + privileged_op (privileged_op_socket, PRIV_SEP_OP_OVERLAY_MOUNT, 0, 0, 0, sb.str, dest); free (sb.str); diff --git a/tests/test-run.sh b/tests/test-run.sh index 8386e361..3c3e71d4 100755 --- a/tests/test-run.sh +++ b/tests/test-run.sh @@ -579,7 +579,7 @@ else printf 4 > upper/a # Check if unprivileged overlayfs is available - if ! unshare -rm mount -t overlay -o lowerdir=lower1,upperdir=upper,workdir=work overlay lower2; then + if ! unshare -rm mount -t overlay -o lowerdir=lower1,upperdir=upper,workdir=work,userxattr overlay lower2; then ok_skip "no kernel support for unprivileged overlayfs" ok_skip "no kernel support for unprivileged overlayfs" ok_skip "no kernel support for unprivileged overlayfs" From 15eb0b5077a8d37de3c3946f077331087b643a24 Mon Sep 17 00:00:00 2001 From: Ryan Hendrickson Date: Mon, 22 Apr 2024 20:32:56 -0400 Subject: [PATCH 5/5] fixup! Add --overlay and related options Signed-off-by: Ryan Hendrickson --- bubblewrap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bubblewrap.c b/bubblewrap.c index 175013a4..6cf69cf5 100644 --- a/bubblewrap.c +++ b/bubblewrap.c @@ -1168,8 +1168,8 @@ privileged_op (int privileged_op_socket, die ("Can't make overlay mount on %s with options %s: " "Overlay directories may not overlap", arg2, arg1); - die_with_error ("Can't make overlay mount on %s with options %s", - arg2, arg1); + die_with_mount_error ("Can't make overlay mount on %s with options %s", + arg2, arg1); } break;