Skip to content

Commit

Permalink
Security: Avoid permission check bypass when bind-mounting directories
Browse files Browse the repository at this point in the history
As the mount() syscall requires administrator privileges, we called
it as root. However, this could have been used to access paths whose
last component was accessible to the calling user, but some of the
previous components were not.

Now we call mount() with effective identity of the calling user with
the CAP_SYS_ADMIN capability added. This way, mount() is still possible,
but the file permissions are checked with respect to the calling user.

Please note that Isolate now requires the libcap library.
  • Loading branch information
gollux committed Aug 1, 2017
1 parent 5965056 commit 6c1ade3
Show file tree
Hide file tree
Showing 5 changed files with 56 additions and 9 deletions.
5 changes: 3 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
# Makefile for Isolate
# (c) 2015--2016 Martin Mares <[email protected]>
# (c) 2015--2017 Martin Mares <[email protected]>

all: isolate isolate.1 isolate.1.html

CC=gcc
CFLAGS=-std=gnu99 -Wall -Wextra -Wno-parentheses -Wno-unused-result -Wno-missing-field-initializers -Wstrict-prototypes -Wmissing-prototypes -D_GNU_SOURCE
LIBS=-lcap

VERSION=1.3
YEAR=2016
Expand All @@ -23,7 +24,7 @@ MAN1DIR = $(MANDIR)/man1
BOXDIR = $(VARPREFIX)/lib/isolate

isolate: isolate.o util.o rules.o cg.o config.o
$(CC) $(LDFLAGS) -o $@ $^
$(CC) $(LDFLAGS) -o $@ $^ $(LIBS)

%.o: %.c isolate.h config.h
$(CC) $(CFLAGS) -c -o $@ $<
Expand Down
3 changes: 3 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,6 @@ If you are interested in more details, please read Martin's
and Bernard's [paper](http://mj.ucw.cz/papers/isolate.pdf) presented
at the IOI Conference. Also, Isolate's [manual page](http://www.ucw.cz/moe/isolate.1.html)
is available online.

To compile Isolate, you need the headers for the libcap library
(usually available in a libcap-dev package).
6 changes: 4 additions & 2 deletions isolate.c
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ static pid_t box_pid;

uid_t box_uid;
gid_t box_gid;
static uid_t orig_uid;
static gid_t orig_gid;
uid_t orig_uid;
gid_t orig_gid;

static int partial_line;
static int cleanup_ownership;
Expand Down Expand Up @@ -901,6 +901,8 @@ main(int argc, char **argv)

if (geteuid())
die("Must be started as root");
if (getegid() && setegid(0) < 0)
die("Cannot switch to root group: %m");
orig_uid = getuid();
orig_gid = getgid();

Expand Down
6 changes: 3 additions & 3 deletions isolate.h
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* Process Isolator
*
* (c) 2012-2016 Martin Mares <[email protected]>
* (c) 2012-2017 Martin Mares <[email protected]>
* (c) 2012-2014 Bernard Blackham <[email protected]>
*/

Expand All @@ -28,8 +28,8 @@ extern int cg_memory_limit;
extern int cg_timing;

extern int box_id;
extern uid_t box_uid;
extern gid_t box_gid;
extern uid_t box_uid, orig_uid;
extern gid_t box_gid, orig_gid;

/* util.c */

Expand Down
45 changes: 43 additions & 2 deletions rules.c
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* Process Isolator -- Rules
*
* (c) 2012-2016 Martin Mares <[email protected]>
* (c) 2012-2017 Martin Mares <[email protected]>
* (c) 2012-2014 Bernard Blackham <[email protected]>
*/

Expand All @@ -12,6 +12,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/capability.h>
#include <sys/mount.h>
#include <sys/quota.h>
#include <sys/stat.h>
Expand Down Expand Up @@ -264,7 +265,25 @@ void init_dir_rules(void)
set_dir_action("usr");
}

void apply_dir_rules(void)
static void
set_cap_sys_admin(void)
{
cap_t caps;
if (!(caps = cap_get_proc()))
die("Cannot get capabilities: %m");

cap_value_t cap_list[] = { CAP_SYS_ADMIN };
if (cap_set_flag(caps, CAP_EFFECTIVE, 1, cap_list, CAP_SET) < 0)
die("Cannot modify capabilities");

if (cap_set_proc(caps) < 0)
die("Cannot set capabilities: %m");

cap_free(caps);
}

void
apply_dir_rules(void)
{
for (struct dir_rule *r = first_dir_rule; r; r=r->next)
{
Expand Down Expand Up @@ -304,10 +323,32 @@ void apply_dir_rules(void)
{
mount_flags |= MS_BIND | MS_NOSUID;
msg("Binding %s on %s (flags %lx)\n", out, in, mount_flags);

/*
* This is tricky. We cannot run mount() with root privileges, since
* it could be used to bypass access control if the mounted path
* contains elements inaccessible to the user running isolate.
*
* We switch effective UID and GID back to the calling user (which clears
* all capabilities, but keeps them in the permitted set) and then
* enable CAP_SYS_ADMIN. So we have CAP_SYS_ADMIN (needed for mount),
* but not CAP_DAC_OVERRIDE (which allows to bypass permission checks).
*/

if (setresuid(orig_uid, orig_uid, 0) < 0 ||
setresgid(orig_gid, orig_gid, 0) < 0)
die("Cannot switch UID and GID: %m");

set_cap_sys_admin();

// Most mount flags need remount to work
if (mount(out, root_in, "none", mount_flags, "") < 0 ||
mount(out, root_in, "none", MS_REMOUNT | mount_flags, "") < 0)
die("Cannot mount %s on %s: %m", out, in);

if (setresuid(orig_uid, 0, orig_uid) < 0 ||
setresgid(orig_gid, 0, orig_gid) < 0)
die("Cannot switch UID and GID: %m");
}
}
}
Expand Down

0 comments on commit 6c1ade3

Please sign in to comment.