/snap-confine

Add support for bind profiles #43

Closed
wants to merge 10 commits into
from
+106 −6

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@mvo5 @zyga @tyhicks
@mvo5
Contributor

mvo5 commented Jun 19, 2016

This adds support for the new "bind" security backend of snapd. It is used for content sharing between snaps and the format that snapd writes is very simple: /src /dst (ro) or /src /dst (rw).
src/mount-support.c
+ die("snprintf returned unexpected value");
+ }
+
+ f = fopen(profile_path, "r");
@zyga

zyga Jun 20, 2016

Collaborator

Instead of parsing this manually I'd recommend using a glibc function like getmntent. This way we can have a very familiar syntax and no parser to write.

@mvo5

mvo5 Jun 21, 2016

Contributor

Thanks, this is done now.

spread-tests/basic/task.yaml
+ snap install hello-world
+ hello-world.echo | grep Hello
+ hello-world.env | grep SNAP_NAME=hello-world
+ hello-world.evil && exit 1 || true
@zyga

zyga Jun 21, 2016

Collaborator

What is the || true part for?

spread.yaml
@@ -0,0 +1,38 @@
+project: snap-confine
@zyga

zyga Jun 21, 2016

Collaborator

Thank you for doing this :)

spread.yaml
+
+prepare: |
+ [ "$REUSE_PROJECT" != 1 ] || exit 0
+
@zyga

zyga Jun 21, 2016

Collaborator

How can we differentiate prepare tasks for different distributions?

@mvo5

mvo5 Jun 21, 2016

Contributor

I don't know, sorry, I think it will have to be something like if grep ubuntu /etc/os-release and similar.

src/mount-support.c
@@ -293,3 +294,58 @@ void setup_slave_mount_namespace()
die("can not make make / rslave");
}
}
+
+#define BIND_MAX_LINE_LENGTH 512 // arbitrary
@zyga

zyga Jun 21, 2016

Collaborator

I'd say it's PATH_MAX * 2 :)

@mvo5

mvo5 Jun 21, 2016

Contributor

Thanks, this is actually a leftover from the previous iteration of the code, removed now.

src/mount-support.c
+
+void setup_bind_mounts(const char *appname)
+{
+ debug("setup_bind_mounts %s", appname);
@zyga

zyga Jun 21, 2016

Collaborator

debug("%s: %s", __FUNCTION__, appname);

@mvo5

mvo5 Jun 21, 2016

Contributor

Thanks

src/mount-support.c
+ FILE *f = NULL;
+ const char *bind_profile_dir = "/var/lib/snapd/bind/profiles/";
+
+ char profile_path[512]; // arbitrary path name limit
@zyga

zyga Jun 21, 2016 •
Edited 1 time

Collaborator

Did you mean to use the macro above? (BIND_MOUNT_MAX_LINE_LENGTH)

@mvo5

mvo5 Jun 21, 2016

Contributor

I use PATH_MAX there now, I think that makes most sense.

src/mount-support.c
+ return;
+ // however any other error is a real error
+ if (f == NULL) {
+ fprintf(stderr, "Can not open %s (%s)\n", profile_path,
@zyga

zyga Jun 21, 2016

Collaborator

I guess you can just die("cannot open %s", profile_path) and let it do the rest

@mvo5

mvo5 Jun 21, 2016

Contributor

Updated, thanks.

src/mount-support.c
+ if (strcmp(m->mnt_type, "") != 0) {
+ die("only bind mounts are supported");
+ }
+ if (strstr(m->mnt_opts, "bind") == NULL) {
@zyga

zyga Jun 21, 2016

Collaborator

I'd use hasmntopt instead

@mvo5

mvo5 Jun 21, 2016

Contributor

Nice!

src/mount-support.c
+ if (strstr(m->mnt_opts, "bind") == NULL) {
+ die("need bind mount flag");
+ }
+ if (strstr(m->mnt_opts, "ro") != NULL) {
@zyga

zyga Jun 21, 2016

Collaborator

Ditto

src/mount-support.c
+ flags |= MS_RDONLY;
+ }
+
+ if (mount(m->mnt_fsname, m->mnt_dir, NULL, flags, NULL) != 0) {
@zyga

zyga Jun 21, 2016

Collaborator

I'd pass all the data from m here, including m->mnt_type. While we don't use it yet we should validate all the values (and we do) so we should also just pass them here naturally. Not a strong opinion but please consider it.

@mvo5

mvo5 Jun 21, 2016

Contributor

I'm sitting a bit on the fence. It does make sense, OTOH given that this is suid and we don't need it seems ok to leave out.

src/mount-support.c
+ die("unable to bind private /tmp");
+ }
+ }
+
@zyga

zyga Jun 21, 2016

Collaborator

I'd just fclose(...) it as it should be harmless

@mvo5

mvo5 Jun 21, 2016

Contributor

I took that from the seccomp code from jamie, I agree that it should be fine to just fclose()

tests/test_setup_bind_mounts
+EOF
+
+printf "Test that bind mounts work "
+"$L" snap.name.app /bin/true
@zyga

zyga Jun 21, 2016

Collaborator

It would be better to test that they really work by creating /src/foo and checking it exists in /dst/foo

@mvo5

mvo5 Jun 21, 2016

Contributor

Thanks, this test is actually bogus :/ I think we need a proper spread test here, its a bit tricky because snapd does not have the full support for writing the fstab file yet, so I will ponder a bit if I can fabricate stuff or if I will just wait for snapd.

src/mount-support.c
@@ -318,8 +317,7 @@ void setup_bind_mounts(const char *appname)
return;
// however any other error is a real error
if (f == NULL) {
- fprintf(stderr, "Can not open %s (%s)\n", profile_path,
- strerror(errno));
+ fprintf(stderr, "cannot open %s\n", profile_path);
@zyga

zyga Jun 21, 2016

Collaborator

I meat that die() does fprintf anyway so you can just do the die thing I listed and it will do the rest

@mvo5

mvo5 Jun 22, 2016

Contributor

Oh, indeed, thank you.

src/mount-support.c
@@ -317,8 +317,7 @@ void setup_bind_mounts(const char *appname)
return;
// however any other error is a real error
if (f == NULL) {
- fprintf(stderr, "cannot open %s\n", profile_path);
- die("aborting");
+ die("cannot open %s\n", profile_path);
@zyga

zyga Jun 22, 2016

Collaborator

die does the \n by itself AFAIR

@zyga
Collaborator

zyga commented Jun 22, 2016

I think this looks good now.

I'd like to see @tyhicks or @jdstrand ack it before it lands
src/mount-support.c
+ char profile_path[PATH_MAX];
+ int snprintf_rc =
+ snprintf(profile_path, sizeof(profile_path), "%s/%s.bind",
+ bind_profile_dir, appname);
@tyhicks

tyhicks Jun 22, 2016

Collaborator

Please switch to the helper function must_snprintf() so that you don't have to do your own error handling here.

@zyga

zyga Jun 27, 2016

Collaborator

This is done in my fork of this branch

src/mount-support.c
+ while ((m = getmntent(f)) != NULL) {
+ int flags = MS_BIND;
+
+ if (strcmp(m->mnt_type, "") != 0) {
@tyhicks

tyhicks Jun 22, 2016

Collaborator

Is "none" for the fs_vfstype in the fstab file converted to an empty string? I think I remember that being the case but it isn't documented in the man page.

src/mount-support.c
+ if (hasmntopt(m, "ro") != NULL) {
+ flags |= MS_RDONLY;
+ }
+
@tyhicks

tyhicks Jun 22, 2016

Collaborator

What about the other mount options that need to be converted from string options to flags before calling mount()? I think that we at least need to support the other security related ones. MS_NODEV, MS_NOEXEC, and MS_NOSUID.

@tyhicks

tyhicks Jun 22, 2016

Collaborator

Thinking about this some more, we should probably default to MS_NODEV, MS_NOEXEC, and MS_NOSUID and then require "dev", "exec", and/or "suid" to clear those bits.

@mvo5

mvo5 Jun 22, 2016

Contributor

I'm fine adding this, I'm not sure about MS_NOEXEC - that seems to be very little risk, no? Given that everything will run inside an apparmor confinement anyway?

@tyhicks

tyhicks Jun 22, 2016

Collaborator

You're correct that AppArmor will be confining everything that is bind mounted in so any binaries made available via the bind mount will still be confined. My suggestion was simply based on the idea to have the most secure defaults and then have the interface author have to specify "exec" if they expected binaries to be executed from the bind mount.

I'm ok either way but defaulting to nosuid,nodev while also defaulting to exec may be a bit confusing to interface authors.

src/mount-support.c
+ die("need bind mount flag");
+ }
+ if (hasmntopt(m, "ro") != NULL) {
+ flags |= MS_RDONLY;
@tyhicks

tyhicks Jun 22, 2016

Collaborator

Won't the majority of bind mounts be ro? If so, I'd prefer that we initialize flags with (MS_BIND | MS_RDONLY) at the top of this function and then require "rw" to be present in the options in order to clear the MS_RDONLY bit and make the mount writable.

@zyga

zyga Jun 27, 2016

Collaborator

This is already done (by Michael) in my fork of this branch.

src/mount-support.c
+ flags |= MS_RDONLY;
+ }
+
+ if (mount(m->mnt_fsname, m->mnt_dir, NULL, flags, NULL) != 0) {
@tyhicks

tyhicks Jun 22, 2016

Collaborator

This is where real vulnerabilities could creep in. I need a little more time to think about the source and destination mount points. There are two possible attacks that come to mind.

  1. An unprivileged user tricks the running-as-root setup_bind_mounts() to gain privileges on the system by setting up malicious symlinks in the source mount point path.
  2. A malicious snap modifies a rw bind mount and drops in a malicious symlink that is then followed by setup_bind_mounts() the next time the snap's bind mounts are set up.

We protect against the 2nd attack in LXC by very carefully chdir()'ing to the destination mount point, verifying inode ownership and not following symlinks along the way.

The 1st attack is a tough one. Hopefully we wouldn't need bind mount a directory owned by a non-root user and could simply enforce that in the code. What are your thoughts on that?

@mvo5

mvo5 Jun 22, 2016

Contributor

Thanks, this is interessting. We control the writing of the fstab file via snapd. The most common case is that we will have both src and dst in the a snap directory, which means that the directories will be both read-only. However there are other cases like when we allow bind mounting of fonts from the OS or when we allow bind mounting a shared data space. I think we can enforce the root requirement here, i.e. both src and dst must be root owned. Not sure if that is enough in a world where we give snaps root though. Suggestions for more defensiveness certainly welcome.

@tyhicks

tyhicks Jun 23, 2016

Collaborator

You say that, in most cases, they'll be read only but will the snap author have control of any components in the src or dst paths?

Hmm... good point about snaps having root. That adds a new twist that I hadn't thought about.

I think if we can prevent from following symlinks in the src and dst paths, we should be ok. As I mentioned previously, we had to solve this problem for LXC and we could reuse a lot of that code. The patch can be found at lxc/lxc@592fd47.

Are src and dst both expected to be absolute paths or can they be relative paths, also?

@zyga

zyga Jun 27, 2016

Collaborator

Both src and dst are what snapd is written to do. I can expect us to use absolute paths here.

I'll add a mount wrapper that does what you describe and ask for a re-review.

@tyhicks

tyhicks Jun 27, 2016

Collaborator

The question of whether or not the snap author have control of any components in the src or dst paths is still open. I think that question must be answered before we can decide on what the bind mount code needs to protect against.

Also, will the unprivileged user running the snap have control of any components in the src or dst paths?

@zyga zyga changed the title from add setup_bind_mounts() to Add support for bind profiles Jun 24, 2016

debian/usr.bin.snap-confine
@@ -69,6 +69,9 @@
# reading seccomp filters
/{tmp/snap.rootfs_*/,}var/lib/snapd/seccomp/profiles/* r,
+
+ # reading mount profiles
+ /{tmp/snap.rootfs_*/,}var/lib/snapd/mount/profiles/* r,
@zyga

zyga Jun 24, 2016

Collaborator

The profiles/ component was removed from the snapd side. Please update this here.

src/mount-support.c
+ debug("%s: %s", __FUNCTION__, appname);
+
+ FILE *f = NULL;
+ const char *bind_profile_dir = "/var/lib/snapd/bind/profiles/";
@zyga

zyga Jun 24, 2016

Collaborator

This is stale, the correct value is /var/lib/snapd/mount/

src/mount-support.c
+ const char *bind_profile_dir = "/var/lib/snapd/bind/profiles/";
+
+ char profile_path[PATH_MAX];
+ must_snprintf(profile_path, sizeof(profile_path), "%s/%s.bind",
@zyga

zyga Jun 24, 2016

Collaborator

The correct extension is .fstab

src/mount-support.c
+ must_snprintf(profile_path, sizeof(profile_path), "%s/%s.bind",
+ bind_profile_dir, appname);
+
+ f = fopen(profile_path, "r");
@zyga

zyga Jun 24, 2016

Collaborator

Can you please use __attribute__((cleanup(sc_cleanup_file))) and add sc_close_file to cleanup-funcs.[ch] with something like this inside:

void sc_cleanup_file(f **FILE) {
    if (*f != NULL) {
        fclose(*f);
    }
}

This should simplify the logic below.

src/mount-support.h
@@ -23,4 +23,6 @@ void setup_private_pts();
void setup_snappy_os_mounts();
void setup_slave_mount_namespace();
+void setup_bind_mounts(const char *appname);
@zyga

zyga Jun 24, 2016

Collaborator

I'd prefer this to be the security tag, the app name is not so true here AFAIK

@zyga
Collaborator

zyga commented Jun 24, 2016

As discussed with @mvo5 I'll continue this branch to address the issues.
@zyga
Collaborator

zyga commented Jun 27, 2016

Closing, reopened as #51

@zyga zyga closed this Jun 27, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment