From 785b1c3b309dd50b93ae03a9da125e031a9041d9 Mon Sep 17 00:00:00 2001 From: Mike Frysinger Date: Fri, 23 Feb 2018 15:47:24 -0500 Subject: extend -K to accept a mount propagation type By default, minijail will mark all mounts as private. The -K flag allows them to skip that step which will retain all existing mount settings. We now have scenarios where we want to share some mount points, so lets extend -K to accept the propagation mode. This lets people use -Kslave and mark all the mount points as slaves. Bug: chromium:813131 Test: `make check` and using -Kslave allows changes in the parent namespace Change-Id: I571e402a383ecf60a6104f87ef97b76710a34d38 --- libminijail.c | 29 +++++++++++++++++------------ libminijail.h | 1 + minijail0.1 | 20 +++++++++++++++++++- minijail0_cli.c | 29 ++++++++++++++++++++++++++--- minijail0_cli_unittest.cc | 31 +++++++++++++++++++++++++++++++ 5 files changed, 94 insertions(+), 16 deletions(-) diff --git a/libminijail.c b/libminijail.c index 900acd5..f9fb0e9 100644 --- a/libminijail.c +++ b/libminijail.c @@ -126,7 +126,6 @@ struct minijail { int set_ambient_caps : 1; int vfs : 1; int enter_vfs : 1; - int skip_remount_private : 1; int pids : 1; int ipc : 1; int uts : 1; @@ -177,6 +176,7 @@ struct minijail { struct mountpoint *mounts_head; struct mountpoint *mounts_tail; size_t mounts_count; + unsigned long remount_mode; size_t tmpfs_size; char *cgroups[MAX_CGROUPS]; size_t cgroup_count; @@ -216,7 +216,6 @@ void minijail_preenter(struct minijail *j) { j->flags.vfs = 0; j->flags.enter_vfs = 0; - j->flags.skip_remount_private = 0; j->flags.remount_proc_ro = 0; j->flags.pids = 0; j->flags.do_init = 0; @@ -224,6 +223,7 @@ void minijail_preenter(struct minijail *j) j->flags.pid_file = 0; j->flags.cgroups = 0; j->flags.forward_signals = 0; + j->remount_mode = 0; } /* @@ -234,7 +234,6 @@ void minijail_preexec(struct minijail *j) { int vfs = j->flags.vfs; int enter_vfs = j->flags.enter_vfs; - int skip_remount_private = j->flags.skip_remount_private; int remount_proc_ro = j->flags.remount_proc_ro; int userns = j->flags.userns; if (j->user) @@ -248,7 +247,6 @@ void minijail_preexec(struct minijail *j) /* Now restore anything we meant to keep. */ j->flags.vfs = vfs; j->flags.enter_vfs = enter_vfs; - j->flags.skip_remount_private = skip_remount_private; j->flags.remount_proc_ro = remount_proc_ro; j->flags.userns = userns; /* Note, |pids| will already have been used before this call. */ @@ -258,7 +256,9 @@ void minijail_preexec(struct minijail *j) struct minijail API *minijail_new(void) { - return calloc(1, sizeof(struct minijail)); + struct minijail *j = calloc(1, sizeof(struct minijail)); + j->remount_mode = MS_PRIVATE; + return j; } void API minijail_change_uid(struct minijail *j, uid_t uid) @@ -441,9 +441,14 @@ void API minijail_skip_setting_securebits(struct minijail *j, j->securebits_skip_mask = securebits_skip_mask; } +void API minijail_remount_mode(struct minijail *j, unsigned long mode) +{ + j->remount_mode = mode; +} + void API minijail_skip_remount_private(struct minijail *j) { - j->flags.skip_remount_private = 1; + j->remount_mode = 0; } void API minijail_namespace_pids(struct minijail *j) @@ -1486,7 +1491,7 @@ static int enter_pivot_root(const struct minijail *j) pdie("failed to fchdir to old /"); /* - * If j->flags.skip_remount_private was enabled for minijail_enter(), + * If skip_remount_private was enabled for minijail_enter(), * there could be a shared mount point under |oldroot|. In that case, * mounts under this shared mount point will be unmounted below, and * this unmounting will propagate to the original mount namespace @@ -1945,13 +1950,13 @@ void API minijail_enter(const struct minijail *j) if (unshare(CLONE_NEWNS)) pdie("unshare(CLONE_NEWNS) failed"); /* - * Unless asked not to, remount all filesystems as private. - * If they are shared, new bind mounts will creep out of our - * namespace. + * By default, remount all filesystems as private, unless + * - Passed a specific remount mode, in which case remount with that, + * - Asked not to remount at all, in which case skip the mount(2) call. * https://www.kernel.org/doc/Documentation/filesystems/sharedsubtree.txt */ - if (!j->flags.skip_remount_private) { - if (mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL)) + if (j->remount_mode) { + if (mount(NULL, "/", NULL, MS_REC | j->remount_mode, NULL)) pdie("mount(NULL, /, NULL, MS_REC | MS_PRIVATE," " NULL) failed"); } diff --git a/libminijail.h b/libminijail.h index 04e4153..2200422 100644 --- a/libminijail.h +++ b/libminijail.h @@ -101,6 +101,7 @@ void minijail_skip_setting_securebits(struct minijail *j, * minijail_namespace_vfs(). You very likely don't need this. */ void minijail_skip_remount_private(struct minijail *j); +void minijail_remount_mode(struct minijail *j, unsigned long mode); void minijail_namespace_ipc(struct minijail *j); void minijail_namespace_uts(struct minijail *j); int minijail_namespace_set_hostname(struct minijail *j, const char *name); diff --git a/minijail0.1 b/minijail0.1 index 7c535e0..ce8e67c 100644 --- a/minijail0.1 +++ b/minijail0.1 @@ -84,10 +84,28 @@ must be an absolute path (e.g. \fI/dev/sda1\fR and not \fIsda1\fR). If the destination does not exist, it will be created as a directory (including missing parent directories). .TP -\fB-K\fR +\fB-K[mode]\fR Don't mark all existing mounts as MS_PRIVATE. This option is \fBdangerous\fR as it negates most of the functionality of \fB-v\fR. You very likely don't need this. + +You may specify a mount propagation mode in which case, that will be used +instead of the default MS_PRIVATE. See the \fBmount\fR(2) man page and the +kernel docs \fIDocumentation/filesystems/sharedsubtree.txt\fR for more +technical details, but a brief guide: + +.IP +\[bu] \fBslave\fR Changes in the parent mount namespace will propagate in, but +changes in this mount namespace will not propagate back out. This is usually +what people want to use. +.IP +\[bu] \fBprivate\fR No changes in either mount namespace will propagate. +This is the default behavior if you don't specify \fB-K\fR. +.IP +\[bu] \fBshared\fR Changes in the parent and this mount namespace will freely +propagate back and forth. This is not recommended. +.IP +\[bu] \fBunbindable\fR Mark all mounts as unbindable. .TP \fB-l\fR Run inside a new IPC namespace. This option makes the program's System V IPC diff --git a/minijail0_cli.c b/minijail0_cli.c index 8d3240e..b6aa593 100644 --- a/minijail0_cli.c +++ b/minijail0_cli.c @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -335,6 +336,24 @@ static void use_profile(struct minijail *j, const char *profile, } } +static void set_remount_mode(struct minijail *j, const char *mode) +{ + unsigned long msmode; + if (!strcmp(mode, "shared")) + msmode = MS_SHARED; + else if (!strcmp(mode, "private")) + msmode = MS_PRIVATE; + else if (!strcmp(mode, "slave")) + msmode = MS_SLAVE; + else if (!strcmp(mode, "unbindable")) + msmode = MS_UNBINDABLE; + else { + fprintf(stderr, "Unknown remount mode: '%s'\n", mode); + exit(1); + } + minijail_remount_mode(j, msmode); +} + static void usage(const char *progn) { size_t i; @@ -373,7 +392,8 @@ static void usage(const char *progn) " -H: Seccomp filter help message.\n" " -i: Exit immediately after fork (do not act as init).\n" " -I: Run as init (pid 1) inside a new pid namespace (implies -p).\n" - " -K: Don't mark all existing mounts as MS_PRIVATE.\n" + " -K: Do not change share mode of any existing mounts.\n" + " -K: Mark all existing mounts as instead of MS_PRIVATE.\n" " -l: Enter new IPC namespace.\n" " -L: Report blocked syscalls to syslog when using seccomp filter.\n" " Forces the following syscalls to be allowed:\n" @@ -457,7 +477,7 @@ int parse_args(struct minijail *j, int argc, char * const argv[], int log_to_stderr = 0; const char *optstring = - "+u:g:sS:c:C:P:b:B:V:f:m::M::k:a:e::R:T:vrGhHinNplLt::IUKwyYzd"; + "+u:g:sS:c:C:P:b:B:V:f:m::M::k:a:e::R:T:vrGhHinNplLt::IUK::wyYzd"; /* clang-format off */ const struct option long_options[] = { {"help", no_argument, 0, 'h'}, @@ -535,7 +555,10 @@ int parse_args(struct minijail *j, int argc, char * const argv[], add_mount(j, optarg); break; case 'K': - minijail_skip_remount_private(j); + if (optarg) + set_remount_mode(j, optarg); + else + minijail_skip_remount_private(j); skip_remount = 1; break; case 'P': diff --git a/minijail0_cli_unittest.cc b/minijail0_cli_unittest.cc index 2427a01..f74bb3d 100644 --- a/minijail0_cli_unittest.cc +++ b/minijail0_cli_unittest.cc @@ -415,3 +415,34 @@ TEST_F(CliTest, invalid_mount) { argv[2] = "none,/"; ASSERT_EXIT(parse_args_(argv), testing::ExitedWithCode(1), ""); } + +// Valid calls to the remount mode option. +TEST_F(CliTest, valid_remount_mode) { + std::vector argv = {"-v", "", "/bin/sh"}; + + // Mode is optional. + argv[1] = "-K"; + ASSERT_TRUE(parse_args_(argv)); + + // This should list all valid modes. + const std::vector modes = { + "shared", + "private", + "slave", + "unbindable", + }; + + for (const auto& mode : modes) { + argv[1] = "-K" + mode; + ASSERT_TRUE(parse_args_(argv)); + } +} + +// Invalid calls to the remount mode option. +TEST_F(CliTest, invalid_remount_mode) { + std::vector argv = {"-v", "", "/bin/sh"}; + + // Unknown mode. + argv[1] = "-Kfoo"; + ASSERT_EXIT(parse_args_(argv), testing::ExitedWithCode(1), ""); +} -- cgit v1.2.3