Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add --overlay and --ro-overlay command line options #167

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 150 additions & 0 deletions bubblewrap.c
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ typedef enum {
SETUP_BIND_MOUNT,
SETUP_RO_BIND_MOUNT,
SETUP_DEV_BIND_MOUNT,
SETUP_OVERLAY_MOUNT,
SETUP_RO_OVERLAY_MOUNT,
SETUP_MOUNT_PROC,
SETUP_MOUNT_DEV,
SETUP_MOUNT_TMPFS,
Expand All @@ -101,6 +103,12 @@ struct _SetupOp
SetupOpType type;
const char *source;
const char *dest;

/* for overlayfs: */
const char *layers;
const char *workdir;
const char *options;

int fd;
SetupOpFlag flags;
SetupOp *next;
Expand All @@ -122,6 +130,7 @@ static LockFile *last_lock_file = NULL;
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,
Expand Down Expand Up @@ -202,6 +211,11 @@ usage (int ecode, FILE *out)
" --dev-bind SRC DEST Bind mount the host path SRC on DEST, allowing device access\n"
" --ro-bind SRC DEST Bind mount the host path SRC readonly on DEST\n"
" --remount-ro DEST Remount DEST as readonly, it doesn't recursively remount\n"
" --overlay LAYERS DEST WORKDIR Mount overlayfs on DEST. LAYERS is a colon seperated list of\n"
" directories. WORKDIR must be an empty directory on the same\n"
" filesystem as the last layer.\n"
" --overlay-ro LAYERS DEST Mount overlayfs read-only on DEST. LAYERS is a colon seperated list\n"
" of directories\n"
" --exec-label LABEL Exec Label for the sandbox\n"
" --file-label LABEL File label for temporary sandbox content\n"
" --proc DEST Mount procfs on DEST\n"
Expand Down Expand Up @@ -778,6 +792,12 @@ privileged_op (int privileged_op_socket,
die_with_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. */
Expand All @@ -792,6 +812,93 @@ privileged_op (int privileged_op_socket,
}
}

struct _StringBuilder
{
char * str;
size_t size;
size_t offset;
};

static void
strappend(struct _StringBuilder *dest, const char *src)
{
size_t len = strlen(src);
if (dest->offset + len >= dest->size) {
dest->size = (dest->size + len) * 2;
dest->str = realloc(dest->str, dest->size);
if (dest->str == NULL)
die ("Out of memory");
}

strcpy(dest->str + dest->offset, src);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not comfortable with strcpy being used in a codebase that is sometimes setuid root. However careful you've been to allocate enough memory, and to avoid entering this code while privileged, it's just too easy to get wrong. As a minimum, this should use strncpy - but if it can be made to use things like asprintf then that would likely be safer.

dest->offset += len;
}

/*
* "/hello:/goodbye" -> "lowerdir=/oldroot/hello:/oldroot/goodbye"
*/
static char *
ro_overlay_options(const char* layers)
{
struct _StringBuilder sb = {0};
cleanup_free char *layers_mut = strdup(layers);
char buf[PATH_MAX];
char * token;
int first = 1;

strappend(&sb, "lowerdir=");

token = strtok(layers_mut, ":");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really happy about the amount of strtok() here: C string manipulation is notoriously easy to get wrong in security-sensitive ways, and bubblewrap is sometimes setuid root.

I'd prefer it if the layers can be defined in a way that doesn't need to be parsed, perhaps something like

--overlay-upper /foo/upper --overlay-lower /lower1 --overlay-lower /lower2 [--overlay-rw /foo/work] --overlay-onto /foo/merged

with these options only allowed in exactly that order (similar to how the --perms and --size modifiers work).

while (token != NULL) {
if (!first)
strappend(&sb, ":");
strappend(&sb, "/oldroot");
/* Resolve absolute symlinks before we remount under /oldroot: */
strappend(&sb, realpath (token, buf));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to handle errors from realpath here.


token = strtok(NULL, ":");
first = 0;
}
return sb.str;
}

/*
* "/hello:/goodbye", "/moo" -> "lowerdir=/oldroot/hello,upperdir=/oldroot/goodbye,workdir=/oldroot/moo"
* "/hello:/3:/goodbye", "/moo" -> "lowerdir=/oldroot/hello:/oldroot/3,upperdir=/oldroot/goodbye,workdir=/oldroot/moo"
*/
static char *
rw_overlay_options(const char* layers, const char* workdir)
{
struct _StringBuilder sb = {0};
cleanup_free char *layers_mut = strdup(layers);
char buf[PATH_MAX];
char *path, *next;
int first = 1;

strappend(&sb, "lowerdir=");

next = strtok(layers_mut, ":");
while (1) {
path = next;
next = strtok(NULL, ":");
if (next == NULL)
break;

if (!first)
strappend(&sb, ":");
strappend(&sb, "/oldroot");
/* Resolve absolute symlinks before we remount under /oldroot: */
strappend(&sb, realpath (path, buf));

first = 0;
}
strappend(&sb, ",upperdir=/oldroot");
strappend(&sb, realpath (path, buf));
strappend(&sb, ",workdir=/oldroot");
strappend(&sb, realpath (workdir, buf));
return sb.str;
}

/* This is run unprivileged in the child namespace but can request
* some privileged operations (also in the child namespace) via the
* privileged_op_socket.
Expand Down Expand Up @@ -846,6 +953,18 @@ setup_newroot (bool unshare_pid,
source, dest);
break;

case SETUP_OVERLAY_MOUNT:
case SETUP_RO_OVERLAY_MOUNT:
{
cleanup_free char *options = NULL;
if (mkdir (dest, 0755) != 0 && errno != EEXIST)
die_with_error ("Can't mkdir %s", op->dest);

privileged_op (privileged_op_socket,
PRIV_SEP_OP_OVERLAY_MOUNT, 0, op->options, dest);
}
break;

case SETUP_REMOUNT_RO_NO_RECURSIVE:
privileged_op (privileged_op_socket,
PRIV_SEP_OP_REMOUNT_RO_NO_RECURSIVE, BIND_READONLY, NULL, dest);
Expand Down Expand Up @@ -1063,6 +1182,12 @@ resolve_symlinks_in_ops (void)
if (op->source == NULL)
die_with_error ("Can't find source path %s", old_source);
break;
case SETUP_RO_OVERLAY_MOUNT:
op->options = ro_overlay_options(op->layers);
break;
case SETUP_OVERLAY_MOUNT:
op->options = rw_overlay_options(op->layers, op->workdir);
break;
default:
break;
}
Expand Down Expand Up @@ -1315,6 +1440,31 @@ parse_args_recurse (int *argcp,
op->source = argv[1];
op->dest = argv[2];

argv += 2;
argc -= 2;
}
else if (strcmp (arg, "--overlay") == 0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This option should be rejected when privileged, to avoid handing out abilities that the kernel thinks should only be given to root.

{
if (argc < 4)
die ("--overlay takes three arguments");

op = setup_op_new (SETUP_OVERLAY_MOUNT);
op->layers = argv[1];
op->dest = argv[2];
op->workdir = argv[3];

argv += 3;
argc -= 3;
}
else if (strcmp (arg, "--ro-overlay") == 0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise for this

{
if (argc < 3)
die ("--ro-overlay takes two arguments");

op = setup_op_new (SETUP_RO_OVERLAY_MOUNT);
op->layers = argv[1];
op->dest = argv[2];

argv += 2;
argc -= 2;
}
Expand Down
35 changes: 35 additions & 0 deletions bwrap.xml
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,41 @@
<term><option>--remount-ro <arg choice="plain">DEST</arg></option></term>
<listitem><para>Remount the path <arg choice="plain">DEST</arg> as readonly. It works only on the specified mount point, without changing any other mount point under the specified path</para></listitem>
</varlistentry>
<varlistentry>
<term><option>--overlay <arg choice="plain">LAYERS</arg> <arg choice="plain">DEST</arg> <arg choice="plain">WORKDIR</arg></option></term>
</varlistentry>
<varlistentry>
<term><option>--ro-overlay <arg choice="plain">SRC</arg> <arg choice="plain">DEST</arg></option></term>
<listitem>
<para>Use overlayfs to bind mount the host paths
<arg choice="plain">LAYERS</arg> on <arg choice="plain">DEST</arg>.
<arg choice="plain">LAYERS</arg> is a colon seperated list of paths.
<arg choice="plain">DEST</arg> will contain the union of all the files
in all the <arg choice="plain">LAYERS</arg>. The paths listed in
LAYERS may not contain a comma (,) or a colon (:).
Comment on lines +208 to +209
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are syntactic restrictions like this, the code should enforce them.

</para>
<para>
With <arg choice="plain">--overlay</arg> all writes will go to the
top layer which is the last layer in the list.
<arg choice="plain">WORKDIR</arg> must be an empty directory on the
same filesystem as the top layer.
</para>
<para>
With <arg choice="plain">--ro-overlay</arg> the filesystem will be
mounted read-only so a <arg choice="plain">WORKDIR</arg> is not needed
and shouldn't be provided.
</para>
<para>
Using <arg choice="plain">--ro-overlay</arg> or providing more than
one layer requires a Linux kernel version of 4.0 or later.
</para>
<para>
For more information see the Overlay Filesystem documentation in the
Linux kernel at
https://www.kernel.org/doc/Documentation/filesystems/overlayfs.txt
</para>
</listitem>
</varlistentry>
<varlistentry>
<term><option>--proc <arg choice="plain">DEST</arg></option></term>
<listitem><para>Mount procfs on <arg choice="plain">DEST</arg></para></listitem>
Expand Down
2 changes: 2 additions & 0 deletions completions/bash/bwrap
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ _bwrap() {
--args
--bind
--bind-data
--overlay
--ro-overlay
--block-fd
--chdir
--dev
Expand Down