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.

Sign up for GitHub

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

I propose adding Landlock support to Bubblewrap #519

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

I propose adding Landlock support to Bubblewrap #519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e4465cc
Experimental changes for Landlock support
ChrysoliteAzalea Jul 8, 2022
dacb654
Added a warning to README
ChrysoliteAzalea Jul 14, 2022
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
7 changes: 6 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
# WARNING! THIS IS AN EXPERIMENTAL BRANCH CONTAINING CHANGES THAT HAVE NOT BEEN TESTED ENOUGH! PLEASE, USE WITH CAUTION!

Bubblewrap
==========

Expand Down Expand Up @@ -72,6 +74,8 @@ If you need to build bubblewrap from source, you can do this with meson or autot

meson:

**Warning: Meson build hasn't been tested with experimental changes. Not recommeneded for experimental branch**

```
meson _builddir
meson compile -C _builddir
Expand All @@ -81,10 +85,11 @@ meson install -C _builddir
autotools:

```
./autogen.sh
./autogen.sh $path
make
sudo make install
```
where $path -- a full path to the directory containing [helper functions built as shared libraries](https://github.com/ChrysoliteAzalea/landlock-functions/).

Usage
-----
Expand Down
2 changes: 1 addition & 1 deletion autogen.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,4 @@ mkdir -p m4
autoreconf --force --install --verbose

cd "$olddir"
test -n "$NOCONFIGURE" || "$srcdir/configure" "$@"
test -n "$NOCONFIGURE" || LIBS="-L$1 -lllwrapper -laddrule" "$srcdir/configure" "$@"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because bubblewrap is sometimes setuid root, we have to be very careful about external dependencies: every external dependency has to be written in a way that makes it safe to call into with elevated privileges, and maintained in a way that treats misplaced trust in the execution environment as a security vulnerability. I'm reasonably confident that glibc, libcap and libselinux are like that, but you'll notice we don't depend on anything larger, like GLib.

Is your Landlock library intended to be setuid-safe?

Is this a personal project, or does it have a bus factor of more than 1?

bubblewrap is already short of maintainer bandwidth (as you can see from the time it took to get this reviewed), so it is not really viable for us to end up in a situation where the bubblewrap maintainers also become responsible for maintaining and securing a Landlock helper library.

If this is going to be an external dependency, then it would have to be integrated into the build system properly, and build-time optional, like the libselinux code already is. --landlock would fail with an error message if the external dependency was not enabled. If we had a new option like --landlock-try, it could carry on regardless.

If you can guarantee that a Landlock library will be safe for setuid processes to link against (no constructors/initializers that e.g. trust environment variables), then another possibility would be to link to the library, but refuse to do anything with it (make --landlock fail with an error) if we find that we're setuid, similar to what's done for --userns.

Alternatively, if what you're doing with Landlock is sufficiently simple, then bwrap could open-code the syscalls in its own code. We'd have to look at a proposed implementation to make a judgement on whether it's small enough that we can take responsibility for its security, and we'd also have to think about cost vs. benefit in terms of what new hardening or new features Landlock gives us.

76 changes: 73 additions & 3 deletions bubblewrap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,13 @@
#include <linux/sched.h>
#include <linux/seccomp.h>
#include <linux/filter.h>
#include <linux/landlock.h>

#include "utils.h"
#include "network.h"
#include "bind-mount.h"
#include "ll_wrapper.h"
#include "add_rule.h"

#ifndef CLONE_NEWCGROUP
#define CLONE_NEWCGROUP 0x02000000 /* New cgroup namespace */
Expand Down Expand Up @@ -85,6 +88,7 @@ int opt_userns_block_fd = -1;
int opt_info_fd = -1;
int opt_json_status_fd = -1;
int opt_seccomp_fd = -1;
int opt_landlock_ruleset_fd = -1;
const char *opt_sandbox_hostname = NULL;
char *opt_args_data = NULL; /* owned */
int opt_userns_fd = -1;
Expand Down Expand Up @@ -331,6 +335,7 @@ usage (int ecode, FILE *out)
" --symlink SRC DEST Create symlink at DEST with target SRC\n"
" --seccomp FD Load and use seccomp rules from FD (not repeatable)\n"
" --add-seccomp-fd FD Load and use seccomp rules from FD (repeatable)\n"
" --landlock Enable Landlock self-restriction\n"
" --block-fd FD Block on FD until some data to read is available\n"
" --userns-block-fd FD Block on FD until the user namespace is ready\n"
" --info-fd FD Write information about the running container to FD\n"
Expand Down Expand Up @@ -1070,6 +1075,13 @@ privileged_op (int privileged_op_socket,
break;

case PRIV_SEP_OP_REMOUNT_RO_NO_RECURSIVE:
if (opt_landlock_ruleset_fd>-1)
{
int exposed_fd = open(arg2,O_PATH | O_CLOEXEC);
add_read_access_rule(opt_landlock_ruleset_fd,exposed_fd);
add_execute_rule(opt_landlock_ruleset_fd,exposed_fd);
close(exposed_fd);
}
bind_result = bind_mount (proc_fd, NULL, arg2, BIND_READONLY);

if (bind_result != BIND_MOUNT_SUCCESS)
Expand All @@ -1081,6 +1093,15 @@ privileged_op (int privileged_op_socket,
case PRIV_SEP_OP_BIND_MOUNT:
/* We always bind directories recursively, otherwise this would let us
access files that are otherwise covered on the host */

if (opt_landlock_ruleset_fd>-1)
{
int exposed_fd = open(arg1,O_PATH | O_CLOEXEC);
Copy link
Collaborator

Choose a reason for hiding this comment

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

bubblewrap is in a GNUish coding style, the same as GLib and Flatpak (I'm not going to repeat this everywhere, but the same principles apply everywhere):

Suggested change
int exposed_fd = open(arg1,O_PATH | O_CLOEXEC);
int exposed_fd = open (arg1, O_PATH | O_CLOEXEC);

add_read_access_rule(opt_landlock_ruleset_fd,exposed_fd);
add_write_access_rule(opt_landlock_ruleset_fd,exposed_fd,1);
add_execute_rule(opt_landlock_ruleset_fd,exposed_fd);
close(exposed_fd);
Comment on lines +1099 to +1103
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like something that would benefit from being factored out into a helper function.

}
bind_result = bind_mount (proc_fd, arg1, arg2, BIND_RECURSIVE | flags);

if (bind_result != BIND_MOUNT_SUCCESS)
Expand All @@ -1090,28 +1111,62 @@ privileged_op (int privileged_op_socket,
break;

case PRIV_SEP_OP_PROC_MOUNT:
if (mount ("proc", arg1, "proc", MS_NOSUID | MS_NOEXEC | MS_NODEV, NULL) != 0)
die_with_error ("Can't mount proc on %s", arg1);
break;
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't need re-indenting into a block, and it would be more obviously correct without the re-indent.

if (mount ("proc", arg1, "proc", MS_NOSUID | MS_NOEXEC | MS_NODEV, NULL) != 0)
die_with_error ("Can't mount proc on %s", arg1);
if (opt_landlock_ruleset_fd>-1)
{
int exposed_fd = open(arg1,O_PATH | O_CLOEXEC);
add_read_access_rule(opt_landlock_ruleset_fd,exposed_fd);
add_write_access_rule(opt_landlock_ruleset_fd,exposed_fd,1);
add_execute_rule(opt_landlock_ruleset_fd,exposed_fd);
close(exposed_fd);
}
break;
}

case PRIV_SEP_OP_TMPFS_MOUNT:
{
cleanup_free char *mode = xasprintf ("mode=%#o", perms);
cleanup_free char *opt = label_mount (mode, opt_file_label);
if (mount ("tmpfs", arg1, "tmpfs", MS_NOSUID | MS_NODEV, opt) != 0)
die_with_error ("Can't mount tmpfs on %s", arg1);
if (opt_landlock_ruleset_fd>-1)
{
int exposed_fd = open(arg1,O_PATH | O_CLOEXEC);
add_read_access_rule(opt_landlock_ruleset_fd,exposed_fd);
add_write_access_rule(opt_landlock_ruleset_fd,exposed_fd,1);
add_execute_rule(opt_landlock_ruleset_fd,exposed_fd);
close(exposed_fd);
}
break;
}

case PRIV_SEP_OP_DEVPTS_MOUNT:
if (mount ("devpts", arg1, "devpts", MS_NOSUID | MS_NOEXEC,
"newinstance,ptmxmode=0666,mode=620") != 0)
die_with_error ("Can't mount devpts on %s", arg1);
if (opt_landlock_ruleset_fd>-1)
{
int exposed_fd = open(arg1,O_PATH | O_CLOEXEC);
add_read_access_rule(opt_landlock_ruleset_fd,exposed_fd);
add_write_access_rule(opt_landlock_ruleset_fd,exposed_fd,0);
add_execute_rule(opt_landlock_ruleset_fd,exposed_fd);
close(exposed_fd);
}
break;

case PRIV_SEP_OP_MQUEUE_MOUNT:
if (mount ("mqueue", arg1, "mqueue", 0, NULL) != 0)
die_with_error ("Can't mount mqueue on %s", arg1);
if (opt_landlock_ruleset_fd>-1)
{
int exposed_fd = open(arg1,O_PATH | O_CLOEXEC);
add_read_access_rule(opt_landlock_ruleset_fd,exposed_fd);
add_write_access_rule(opt_landlock_ruleset_fd,exposed_fd,1);
add_execute_rule(opt_landlock_ruleset_fd,exposed_fd);
close(exposed_fd);
}
break;

case PRIV_SEP_OP_SET_HOSTNAME:
Expand Down Expand Up @@ -1705,6 +1760,16 @@ parse_args_recurse (int *argcp,
argv += 1;
argc -= 1;
}
else if (strcmp (arg, "--landlock") == 0)
{
struct landlock_ruleset_attr llattrs;
llattrs.handled_access_fs = LANDLOCK_ACCESS_FS_EXECUTE | LANDLOCK_ACCESS_FS_READ_FILE | LANDLOCK_ACCESS_FS_READ_DIR | LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_REMOVE_FILE | LANDLOCK_ACCESS_FS_REMOVE_DIR | LANDLOCK_ACCESS_FS_MAKE_CHAR | LANDLOCK_ACCESS_FS_MAKE_DIR | LANDLOCK_ACCESS_FS_MAKE_REG | LANDLOCK_ACCESS_FS_MAKE_SYM;
opt_landlock_ruleset_fd = landlock_create_ruleset(&llattrs,sizeof(llattrs),0);
if (opt_landlock_ruleset_fd<0) {
warn("An error has occured while creating Landlock ruleset. The sandbox will start without Landlock protection.\n");
opt_landlock_ruleset_fd = -1;
}
}
else if (strcmp (arg, "--unshare-all") == 0)
{
/* Keep this in order with the older (legacy) --unshare arguments,
Expand Down Expand Up @@ -3214,6 +3279,11 @@ main (int argc,

/* Should be the last thing before execve() so that filters don't
* need to handle anything above */
Comment on lines 3280 to 3281
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment should stay attached to seccomp_programs_apply: one of the things this comment means is that we don't want to require users' seccomp programs to allow landlock_restrict_self().

if (opt_landlock_ruleset_fd>-1)
{
if (landlock_restrict_self(opt_landlock_ruleset_fd,0)!=0)
warn("An error has occured while enabling Landlock restrictions. The sandbox will start without Landlock protection.\n");
Comment on lines +3284 to +3285
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we want Landlock to be a security restriction, then it should fail closed. We can have a separate --landlock-try option for "try to do Landlock, but fall back to not", if you want to also support it as an optional hardening thing - but the default should be fail-closed.

}
seccomp_programs_apply ();

if (setup_finished_pipe[1] != -1)
Expand Down