Skip to content

Commit

Permalink
Drop setting the dumpable flag entirely in favor of setfsuid()
Browse files Browse the repository at this point in the history
The root cause of
containers#107
aka CVE-2016-8659 is that we were explictly turning on the dumpable
flag, which allows the caller to `ptrace()` us.

In fact, Linux already introduced `setfsuid()` for the NFS server for
a very similar reason; see `man setfsuid`:

```
       At the time when this system call was introduced, one process
       could send a signal to another process with the same effective
       user ID.  This meant that if a privileged process changed its
       effective user ID for the purpose of file permission checking,
       then it could become vulnerable to receiving signals sent by
       another (unprivileged) process with the same user ID.
```

Let's make use of this, which makes us the same as other setuid
binaries, without introducing any additional risk from being
potentially `ptrace()able`.
  • Loading branch information
cgwalters committed Oct 13, 2016
1 parent 133dcb7 commit 9c2182c
Showing 1 changed file with 8 additions and 4 deletions.
12 changes: 8 additions & 4 deletions bubblewrap.c
Expand Up @@ -27,6 +27,7 @@
#include <sys/wait.h>
#include <sys/eventfd.h>
#include <sys/signalfd.h>
#include <sys/fsuid.h>
#include <sys/capability.h>
#include <sys/prctl.h>
#include <linux/sched.h>
Expand Down Expand Up @@ -436,10 +437,6 @@ acquire_caps (void)
die_with_error ("capset failed");
}
/* Else, we try unprivileged user namespaces */

/* We need the process to be dumpable, or we can't access /proc/self/uid_map */
if (prctl (PR_SET_DUMPABLE, 1, 0, 0, 0) < 0)
die_with_error ("prctl(PR_SET_DUMPABLE) failed");
}

static void
Expand Down Expand Up @@ -484,6 +481,10 @@ write_uid_gid_map (uid_t sandbox_uid,
cleanup_free char *gid_map = NULL;
cleanup_free char *dir = NULL;
cleanup_fd int dir_fd = -1;
uid_t prev_uid = getuid ();

if (setfsuid (0) != prev_uid)
die_with_error ("setfsuid() failed, previous uid was %u", prev_uid);

if (pid == -1)
dir = xstrdup ("self");
Expand Down Expand Up @@ -523,6 +524,9 @@ write_uid_gid_map (uid_t sandbox_uid,

if (write_file_at (dir_fd, "gid_map", gid_map) != 0)
die_with_error ("setting up gid map");

if (setfsuid (uid) != 0)
die_with_error ("setfsuid() failed, previous uid was not 0");
}

static void
Expand Down

0 comments on commit 9c2182c

Please sign in to comment.