linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/4] PR_SET_NO_NEW_PRIVS, unshare, and chroot
@ 2012-01-30 16:17 Andy Lutomirski
  2012-01-30 16:17 ` [PATCH v3 1/4] Add PR_{GET,SET}_NO_NEW_PRIVS to prevent execve from granting privs Andy Lutomirski
                   ` (3 more replies)
  0 siblings, 4 replies; 22+ messages in thread
From: Andy Lutomirski @ 2012-01-30 16:17 UTC (permalink / raw)
  To: Will Drewry, linux-kernel
  Cc: Casey Schaufler, Linus Torvalds, Jamie Lokier, keescook,
	john.johansen, serge.hallyn, coreyb, pmoore, eparis, djm, segoon,
	rostedt, jmorris, scarybeasts, avi, penberg, viro, mingo, akpm,
	khilman, borislav.petkov, amwang, oleg, ak, eric.dumazet, gregkh,
	dhowells, daniel.lezcano, linux-fsdevel, linux-security-module,
	olofj, mhalcrow, dlaor, corbet, alan, Al Viro, Andy Lutomirski

This adds PR_{GET,SET}_NO_NEW_PRIVS.  As an example of its use, it
allows some unshare operations and (sometimes) chroot when no_new_privs
is set.  Another example is the experimental pam module here:

http://web.mit.edu/luto/www/linux/

After some impressively long mailing list threads, I still think that
blocking setresuid, setuid, and capset in no_new_privs mode is
unnecessary and overcomplicated.  Additionally, blocking those calls
will make my pam module either fail or become a giant security hole
(depending on how carefully the core pam stuff is written -- I haven't
checked).

Changes from v2:
 - Rebased onto a very recent -linus tree.
 - Changed prctl numbering.  (Needed because prctl 35 is now taken.)
 - Fixed a typo or two.
 - Removed explicit propagation of no_new_privs.  dup_task_struct is enough.
 - Reworked the chroot patch.  It now uses hopefully much more sane logic
   to decide whether the user is chrooted.  It also checks that fs is not
   shared (which was a big security hole in the earlier version).

For the git-inclined, this series is here:
https://git.kernel.org/?p=linux/kernel/git/luto/linux.git;a=shortlog;h=refs/heads/security/no_new_privs/patch_v3

Test it like this:

---- begin test case

#include <sys/prctl.h>
#include <stdio.h>
#include <unistd.h>
#include <errno.h>

#define PR_SET_NO_NEW_PRIVS 36
#define PR_GET_NO_NEW_PRIVS 37

int main()
{
  int nnp = prctl(PR_GET_NO_NEW_PRIVS, 0, 0, 0, 0);
  if (nnp == -EINVAL) {
    printf("Failed!\n");
    return 1;
  }

  printf("nnp was %d\n", nnp);

  if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) != 0) {
    printf("Failed!\n");
    return 1;
  }

  nnp = prctl(PR_GET_NO_NEW_PRIVS, 0, 0, 0, 0);
  if (nnp == -EINVAL) {
    printf("Failed!\n");
    return 1;
  }

  printf("nnp is %d\n", nnp);

  printf("here goes...\n");
  execlp("bash", "bash", NULL);
  printf("Failed to exec bash\n");
  return 1;
}

---- end test case

Andy Lutomirski (3):
  Add PR_{GET,SET}_NO_NEW_PRIVS to prevent execve from granting privs
  Allow unprivileged CLONE_NEWUTS and CLONE_NEWIPC with no_new_privs
  Allow unprivileged chroot when safe

John Johansen (1):
  Fix apparmor for PR_{GET,SET}_NO_NEW_PRIVS

 fs/exec.c                  |   10 ++++++++-
 fs/open.c                  |   46 ++++++++++++++++++++++++++++++++++++++++++-
 include/linux/prctl.h      |   15 ++++++++++++++
 include/linux/sched.h      |    2 +
 include/linux/security.h   |    1 +
 kernel/nsproxy.c           |    8 ++++++-
 kernel/sys.c               |   10 +++++++++
 security/apparmor/domain.c |   35 +++++++++++++++++++++++++++++++++
 security/commoncap.c       |    7 ++++-
 security/selinux/hooks.c   |   10 ++++++++-
 10 files changed, 137 insertions(+), 7 deletions(-)

-- 
1.7.7.6


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 1/4] Add PR_{GET,SET}_NO_NEW_PRIVS to prevent execve from granting privs
  2012-01-30 16:17 [PATCH v3 0/4] PR_SET_NO_NEW_PRIVS, unshare, and chroot Andy Lutomirski
@ 2012-01-30 16:17 ` Andy Lutomirski
  2012-02-01 18:14   ` Kees Cook
  2012-01-30 16:17 ` [PATCH v3 2/4] Fix apparmor for PR_{GET,SET}_NO_NEW_PRIVS Andy Lutomirski
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 22+ messages in thread
From: Andy Lutomirski @ 2012-01-30 16:17 UTC (permalink / raw)
  To: Will Drewry, linux-kernel
  Cc: Casey Schaufler, Linus Torvalds, Jamie Lokier, keescook,
	john.johansen, serge.hallyn, coreyb, pmoore, eparis, djm, segoon,
	rostedt, jmorris, scarybeasts, avi, penberg, viro, mingo, akpm,
	khilman, borislav.petkov, amwang, oleg, ak, eric.dumazet, gregkh,
	dhowells, daniel.lezcano, linux-fsdevel, linux-security-module,
	olofj, mhalcrow, dlaor, corbet, alan, Al Viro, Andy Lutomirski

With this set, a lot of dangerous operations (chroot, unshare, etc)
become a lot less dangerous because there is no possibility of
subverting privileged binaries.

This patch completely breaks apparmor.  Someone who understands (and
uses) apparmor should fix it or at least give me a hint.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 fs/exec.c                  |   10 +++++++++-
 include/linux/prctl.h      |   15 +++++++++++++++
 include/linux/sched.h      |    2 ++
 include/linux/security.h   |    1 +
 kernel/sys.c               |   10 ++++++++++
 security/apparmor/domain.c |    4 ++++
 security/commoncap.c       |    7 +++++--
 security/selinux/hooks.c   |   10 +++++++++-
 8 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index aeb135c..378e1bb 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1242,6 +1242,13 @@ static int check_unsafe_exec(struct linux_binprm *bprm)
 			bprm->unsafe |= LSM_UNSAFE_PTRACE;
 	}
 
+	/*
+	 * This isn't strictly necessary, but it makes it harder for LSMs to
+	 * mess up.
+	 */
+	if (current->no_new_privs)
+		bprm->unsafe |= LSM_UNSAFE_NO_NEW_PRIVS;
+
 	n_fs = 1;
 	spin_lock(&p->fs->lock);
 	rcu_read_lock();
@@ -1285,7 +1292,8 @@ int prepare_binprm(struct linux_binprm *bprm)
 	bprm->cred->euid = current_euid();
 	bprm->cred->egid = current_egid();
 
-	if (!(bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID)) {
+	if (!(bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) &&
+	    !current->no_new_privs) {
 		/* Set-uid? */
 		if (mode & S_ISUID) {
 			bprm->per_clear |= PER_CLEAR_ON_SETID;
diff --git a/include/linux/prctl.h b/include/linux/prctl.h
index 7ddc7f1..a6b5ac9 100644
--- a/include/linux/prctl.h
+++ b/include/linux/prctl.h
@@ -114,4 +114,19 @@
 # define PR_SET_MM_START_BRK		6
 # define PR_SET_MM_BRK			7
 
+/*
+ * If no_new_privs is set, then operations that grant new privileges (i.e.
+ * execve) will either fail or not grant them.  This affects suid/sgid,
+ * file capabilities, and LSMs.
+ *
+ * Operations that merely manipulate or drop existing privileges (setresuid,
+ * capset, etc.) will still work.  Drop those privileges if you want them gone.
+ *
+ * Changing LSM security domain is considered a new privilege.  So, for example,
+ * asking selinux for a specific new context (e.g. with runcon) will result
+ * in execve returning -EPERM.
+ */
+#define PR_SET_NO_NEW_PRIVS 36
+#define PR_GET_NO_NEW_PRIVS 37
+
 #endif /* _LINUX_PRCTL_H */
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 2234985..01d93b3 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1314,6 +1314,8 @@ struct task_struct {
 				 * execve */
 	unsigned in_iowait:1;
 
+	/* task may not gain privileges */
+	unsigned no_new_privs:1;
 
 	/* Revert to default priority/policy when forking */
 	unsigned sched_reset_on_fork:1;
diff --git a/include/linux/security.h b/include/linux/security.h
index 83c18e8..8dce830 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -129,6 +129,7 @@ struct request_sock;
 #define LSM_UNSAFE_SHARE	1
 #define LSM_UNSAFE_PTRACE	2
 #define LSM_UNSAFE_PTRACE_CAP	4
+#define LSM_UNSAFE_NO_NEW_PRIVS	8
 
 #ifdef CONFIG_MMU
 /*
diff --git a/kernel/sys.c b/kernel/sys.c
index 4070153..12e862a 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1962,6 +1962,16 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 		case PR_SET_MM:
 			error = prctl_set_mm(arg2, arg3, arg4, arg5);
 			break;
+		case PR_SET_NO_NEW_PRIVS:
+			if (arg2 != 1 || arg3 || arg4 || arg5)
+				return -EINVAL;
+
+			current->no_new_privs = 1;
+			break;
+		case PR_GET_NO_NEW_PRIVS:
+			if (arg2 || arg3 || arg4 || arg5)
+				return -EINVAL;
+			return current->no_new_privs ? 1 : 0;
 		default:
 			error = -EINVAL;
 			break;
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index c1e18ba..fcfb610 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -360,6 +360,10 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
 	if (bprm->cred_prepared)
 		return 0;
 
+	/* XXX: no_new_privs is not usable with AppArmor yet */
+	if (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS)
+		return -EPERM;
+
 	cxt = bprm->cred->security;
 	BUG_ON(!cxt);
 
diff --git a/security/commoncap.c b/security/commoncap.c
index 7ce191e..d700b38 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -505,14 +505,17 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 skip:
 
 	/* Don't let someone trace a set[ug]id/setpcap binary with the revised
-	 * credentials unless they have the appropriate permit
+	 * credentials unless they have the appropriate permit.
+	 *
+	 * In addition, if NO_NEW_PRIVS, then ensure we get no new privs.
 	 */
 	if ((new->euid != old->uid ||
 	     new->egid != old->gid ||
 	     !cap_issubset(new->cap_permitted, old->cap_permitted)) &&
 	    bprm->unsafe & ~LSM_UNSAFE_PTRACE_CAP) {
 		/* downgrade; they get no more than they had, and maybe less */
-		if (!capable(CAP_SETUID)) {
+		if (!capable(CAP_SETUID) ||
+		    (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS)) {
 			new->euid = new->uid;
 			new->egid = new->gid;
 		}
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 6a3683e..18e57cc 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1998,6 +1998,13 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
 		new_tsec->sid = old_tsec->exec_sid;
 		/* Reset exec SID on execve. */
 		new_tsec->exec_sid = 0;
+
+		/*
+		 * Minimize confusion: if no_new_privs and a transition is
+		 * explicitly requested, then fail the exec.
+		 */
+		if (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS)
+			return -EPERM;
 	} else {
 		/* Check for a default transition on this program. */
 		rc = security_transition_sid(old_tsec->sid, isec->sid,
@@ -2010,7 +2017,8 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
 	COMMON_AUDIT_DATA_INIT(&ad, PATH);
 	ad.u.path = bprm->file->f_path;
 
-	if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID)
+	if ((bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) ||
+	    (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS))
 		new_tsec->sid = old_tsec->sid;
 
 	if (new_tsec->sid == old_tsec->sid) {
-- 
1.7.7.6


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v3 2/4] Fix apparmor for PR_{GET,SET}_NO_NEW_PRIVS
  2012-01-30 16:17 [PATCH v3 0/4] PR_SET_NO_NEW_PRIVS, unshare, and chroot Andy Lutomirski
  2012-01-30 16:17 ` [PATCH v3 1/4] Add PR_{GET,SET}_NO_NEW_PRIVS to prevent execve from granting privs Andy Lutomirski
@ 2012-01-30 16:17 ` Andy Lutomirski
  2012-01-30 16:17 ` [PATCH v3 3/4] Allow unprivileged CLONE_NEWUTS and CLONE_NEWIPC with no_new_privs Andy Lutomirski
  2012-01-30 16:17 ` [PATCH v3 4/4] Allow unprivileged chroot when safe Andy Lutomirski
  3 siblings, 0 replies; 22+ messages in thread
From: Andy Lutomirski @ 2012-01-30 16:17 UTC (permalink / raw)
  To: Will Drewry, linux-kernel
  Cc: Casey Schaufler, Linus Torvalds, Jamie Lokier, keescook,
	john.johansen, serge.hallyn, coreyb, pmoore, eparis, djm, segoon,
	rostedt, jmorris, scarybeasts, avi, penberg, viro, mingo, akpm,
	khilman, borislav.petkov, amwang, oleg, ak, eric.dumazet, gregkh,
	dhowells, daniel.lezcano, linux-fsdevel, linux-security-module,
	olofj, mhalcrow, dlaor, corbet, alan, Al Viro, Andy Lutomirski

From: John Johansen <john.johansen@canonical.com>

Signed-off-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 security/apparmor/domain.c |   39 +++++++++++++++++++++++++++++++++++----
 1 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index fcfb610..7316d77 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -360,10 +360,6 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
 	if (bprm->cred_prepared)
 		return 0;
 
-	/* XXX: no_new_privs is not usable with AppArmor yet */
-	if (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS)
-		return -EPERM;
-
 	cxt = bprm->cred->security;
 	BUG_ON(!cxt);
 
@@ -399,6 +395,11 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
 			new_profile = find_attach(ns, &ns->base.profiles, name);
 		if (!new_profile)
 			goto cleanup;
+		/*
+		 * NOTE: Domain transitions from unconfined are allowed
+		 * even when no_new_privs is set because this aways results
+		 * in a further reduction of permissions.
+		 */
 		goto apply;
 	}
 
@@ -459,6 +460,16 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
 		/* fail exec */
 		error = -EACCES;
 
+	/*
+	 * Policy has specified a domain transition, if no_new_privs then
+	 * fail the exec.
+	 */
+	if (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS) {
+		aa_put_profile(new_profile);
+		error = -EPERM;
+		goto cleanup;
+	}
+
 	if (!new_profile)
 		goto audit;
 
@@ -613,6 +624,14 @@ int aa_change_hat(const char *hats[], int count, u64 token, bool permtest)
 	const char *target = NULL, *info = NULL;
 	int error = 0;
 
+	/*
+	 * Fail explicitly requested domain transitions if no_new_privs.
+	 * There is no exception for unconfined as change_hat is not
+	 * available.
+	 */
+	if (current->no_new_privs)
+		return -EPERM;
+
 	/* released below */
 	cred = get_current_cred();
 	cxt = cred->security;
@@ -754,6 +773,18 @@ int aa_change_profile(const char *ns_name, const char *hname, bool onexec,
 	cxt = cred->security;
 	profile = aa_cred_profile(cred);
 
+	/*
+	 * Fail explicitly requested domain transitions if no_new_privs
+	 * and not unconfined.
+	 * Domain transitions from unconfined are allowed even when
+	 * no_new_privs is set because this aways results in a reduction
+	 * of permissions.
+	 */
+	if (current->no_new_privs && !unconfined(profile)) {
+		put_cred(cred);
+		return -EPERM;
+	}
+
 	if (ns_name) {
 		/* released below */
 		ns = aa_find_namespace(profile->ns, ns_name);
-- 
1.7.7.6


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v3 3/4] Allow unprivileged CLONE_NEWUTS and CLONE_NEWIPC with no_new_privs
  2012-01-30 16:17 [PATCH v3 0/4] PR_SET_NO_NEW_PRIVS, unshare, and chroot Andy Lutomirski
  2012-01-30 16:17 ` [PATCH v3 1/4] Add PR_{GET,SET}_NO_NEW_PRIVS to prevent execve from granting privs Andy Lutomirski
  2012-01-30 16:17 ` [PATCH v3 2/4] Fix apparmor for PR_{GET,SET}_NO_NEW_PRIVS Andy Lutomirski
@ 2012-01-30 16:17 ` Andy Lutomirski
  2012-02-01 19:02   ` Kees Cook
  2012-01-30 16:17 ` [PATCH v3 4/4] Allow unprivileged chroot when safe Andy Lutomirski
  3 siblings, 1 reply; 22+ messages in thread
From: Andy Lutomirski @ 2012-01-30 16:17 UTC (permalink / raw)
  To: Will Drewry, linux-kernel
  Cc: Casey Schaufler, Linus Torvalds, Jamie Lokier, keescook,
	john.johansen, serge.hallyn, coreyb, pmoore, eparis, djm, segoon,
	rostedt, jmorris, scarybeasts, avi, penberg, viro, mingo, akpm,
	khilman, borislav.petkov, amwang, oleg, ak, eric.dumazet, gregkh,
	dhowells, daniel.lezcano, linux-fsdevel, linux-security-module,
	olofj, mhalcrow, dlaor, corbet, alan, Al Viro, Andy Lutomirski

They are normally disallowed because they could be used to subvert
setuid programs.  But if setuid is disabled, then they are safe.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 kernel/nsproxy.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
index b576f7f..47cf873 100644
--- a/kernel/nsproxy.c
+++ b/kernel/nsproxy.c
@@ -191,7 +191,13 @@ int unshare_nsproxy_namespaces(unsigned long unshare_flags,
 			       CLONE_NEWNET)))
 		return 0;
 
-	if (!capable(CAP_SYS_ADMIN))
+	/* We require either no_new_privs or CAP_SYS_ADMIN for all modes */
+	if (!current->no_new_privs && !capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	/* NEWNS and NEWNET always require CAP_SYS_ADMIN. */
+	if ((unshare_flags & (CLONE_NEWNS | CLONE_NEWNET)) &&
+	    !capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
 	*new_nsp = create_new_namespaces(unshare_flags, current,
-- 
1.7.7.6


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v3 4/4] Allow unprivileged chroot when safe
  2012-01-30 16:17 [PATCH v3 0/4] PR_SET_NO_NEW_PRIVS, unshare, and chroot Andy Lutomirski
                   ` (2 preceding siblings ...)
  2012-01-30 16:17 ` [PATCH v3 3/4] Allow unprivileged CLONE_NEWUTS and CLONE_NEWIPC with no_new_privs Andy Lutomirski
@ 2012-01-30 16:17 ` Andy Lutomirski
  2012-01-30 21:58   ` Colin Walters
  3 siblings, 1 reply; 22+ messages in thread
From: Andy Lutomirski @ 2012-01-30 16:17 UTC (permalink / raw)
  To: Will Drewry, linux-kernel
  Cc: Casey Schaufler, Linus Torvalds, Jamie Lokier, keescook,
	john.johansen, serge.hallyn, coreyb, pmoore, eparis, djm, segoon,
	rostedt, jmorris, scarybeasts, avi, penberg, viro, mingo, akpm,
	khilman, borislav.petkov, amwang, oleg, ak, eric.dumazet, gregkh,
	dhowells, daniel.lezcano, linux-fsdevel, linux-security-module,
	olofj, mhalcrow, dlaor, corbet, alan, Al Viro, Andy Lutomirski

Chroot can easily be used to subvert setuid programs.  If no_new_privs,
then setuid programs don't gain any privilege, so allow chroot.

Because chroot is an easy way to break out of chroot jail, CAP_SYS_ADMIN
is still required if the caller is already chrooted.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 fs/open.c |   46 ++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/fs/open.c b/fs/open.c
index 77becc0..2e2887a 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -418,10 +418,39 @@ out:
 	return error;
 }
 
+static bool is_chrooted(struct fs_struct *fs)
+{
+	bool ret;
+
+	/*
+	 * This is equivalent to checking whether "/.." is the same
+	 * directory as "/", where the ".." part ignores the current
+	 * root.  This logic is the same as follow_dotdot except that we
+	 * ignore fs->root and we don't need to follow the final
+	 * mountpoint we end up on.
+	 */
+	struct path path = fs->root;
+	path_get(&path);
+	while (true) {
+		if (path.dentry != path.mnt->mnt_root) {
+			ret = true;  /* .. moves up within a vfsmount. */
+			break;
+		}
+
+		if (!follow_up(&path)) {
+			ret = false;  /* We've hit the real root. */
+			break;
+		}
+	}
+	path_put(&path);
+	return ret;
+}
+
 SYSCALL_DEFINE1(chroot, const char __user *, filename)
 {
 	struct path path;
 	int error;
+	struct fs_struct *fs = current->fs;
 
 	error = user_path_dir(filename, &path);
 	if (error)
@@ -432,13 +461,26 @@ SYSCALL_DEFINE1(chroot, const char __user *, filename)
 		goto dput_and_out;
 
 	error = -EPERM;
-	if (!capable(CAP_SYS_CHROOT))
+	/*
+	 * Chroot is dangerous unless no_new_privs is set, and we also
+	 * don't want to allow unprivileged users to break out of chroot
+	 * jail with another chroot call.
+	 *
+	 * We therefore allow chroot under one of two circumstances:
+	 *  a) no_new_privs (so setuid and similar programs can't be
+	 *     exploited), fs not shared (to avoid bypassing no_new_privs),
+	 *     and not already chrooted (so there's no chroot jail to break
+	 *     out of)
+	 *  b) CAP_SYS_CHROOT
+	 */
+	if (!(current->no_new_privs && fs->users == 1 && !is_chrooted(fs)) &&
+	    !capable(CAP_SYS_CHROOT))
 		goto dput_and_out;
 	error = security_path_chroot(&path);
 	if (error)
 		goto dput_and_out;
 
-	set_fs_root(current->fs, &path);
+	set_fs_root(fs, &path);
 	error = 0;
 dput_and_out:
 	path_put(&path);
-- 
1.7.7.6


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 4/4] Allow unprivileged chroot when safe
  2012-01-30 16:17 ` [PATCH v3 4/4] Allow unprivileged chroot when safe Andy Lutomirski
@ 2012-01-30 21:58   ` Colin Walters
  2012-01-30 22:10     ` Andy Lutomirski
  2012-01-30 22:18     ` Steven Rostedt
  0 siblings, 2 replies; 22+ messages in thread
From: Colin Walters @ 2012-01-30 21:58 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Will Drewry, linux-kernel, Casey Schaufler, Linus Torvalds,
	Jamie Lokier, keescook, john.johansen, serge.hallyn, coreyb,
	pmoore, eparis, djm, segoon, rostedt, jmorris, scarybeasts, avi,
	penberg, viro, mingo, akpm, khilman, borislav.petkov, amwang,
	oleg, ak, eric.dumazet, gregkh, dhowells, daniel.lezcano,
	linux-fsdevel, linux-security-module, olofj, mhalcrow, dlaor,
	corbet, alan

On Mon, 2012-01-30 at 08:17 -0800, Andy Lutomirski wrote:
> Chroot can easily be used to subvert setuid programs.  If no_new_privs,
> then setuid programs don't gain any privilege, so allow chroot.

Is this needed/desired by anyone now, or are you just using it to "demo"
NO_NEW_PRIVS?  I don't see it as very useful on its own, since in any
"container"-type chroot you really want /proc and /dev, and your patch
doesn't help with that.

System daemons that do chroot for a modicum of security already start
privileged, so this doesn't help them either.




^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 4/4] Allow unprivileged chroot when safe
  2012-01-30 21:58   ` Colin Walters
@ 2012-01-30 22:10     ` Andy Lutomirski
  2012-01-30 22:41       ` Colin Walters
  2012-01-30 22:18     ` Steven Rostedt
  1 sibling, 1 reply; 22+ messages in thread
From: Andy Lutomirski @ 2012-01-30 22:10 UTC (permalink / raw)
  To: Colin Walters
  Cc: Will Drewry, linux-kernel, Casey Schaufler, Linus Torvalds,
	Jamie Lokier, keescook, john.johansen, serge.hallyn, coreyb,
	pmoore, eparis, djm, segoon, rostedt, jmorris, scarybeasts, avi,
	penberg, viro, mingo, akpm, khilman, borislav.petkov, amwang,
	oleg, ak, eric.dumazet, gregkh, dhowells, daniel.lezcano,
	linux-fsdevel, linux-security-module, olofj, mhalcrow, dlaor,
	corbet, alan

On Mon, Jan 30, 2012 at 1:58 PM, Colin Walters <walters@verbum.org> wrote:
> On Mon, 2012-01-30 at 08:17 -0800, Andy Lutomirski wrote:
>> Chroot can easily be used to subvert setuid programs.  If no_new_privs,
>> then setuid programs don't gain any privilege, so allow chroot.
>
> Is this needed/desired by anyone now, or are you just using it to "demo"
> NO_NEW_PRIVS?  I don't see it as very useful on its own, since in any
> "container"-type chroot you really want /proc and /dev, and your patch
> doesn't help with that.

It's a demo, but it could still be useful for container-ish things.
If something privileged sets up /proc, /sys, and /dev, then
unprivileged code can chroot into the container.  This would allow
much simpler implementations of tools like schroot.

>
> System daemons that do chroot for a modicum of security already start
> privileged, so this doesn't help them either.

With this change, they wouldn't need to start privileged.
(Admittedly, this isn't a great argument for this patch.)

It would be really nice to have unprivileged filesystem namespace
features, but that would be more complicated to do safely.

--Andy

>
>
>



-- 
Andy Lutomirski
AMA Capital Management, LLC
Office: (310) 553-5322
Mobile: (650) 906-0647

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 4/4] Allow unprivileged chroot when safe
  2012-01-30 21:58   ` Colin Walters
  2012-01-30 22:10     ` Andy Lutomirski
@ 2012-01-30 22:18     ` Steven Rostedt
  2012-01-30 22:28       ` Andy Lutomirski
  2012-01-30 22:38       ` Will Drewry
  1 sibling, 2 replies; 22+ messages in thread
From: Steven Rostedt @ 2012-01-30 22:18 UTC (permalink / raw)
  To: Colin Walters
  Cc: Andy Lutomirski, Will Drewry, linux-kernel, Casey Schaufler,
	Linus Torvalds, Jamie Lokier, keescook, john.johansen,
	serge.hallyn, coreyb, pmoore, eparis, djm, segoon, jmorris,
	scarybeasts, avi, penberg, viro, mingo, akpm, khilman,
	borislav.petkov, amwang, oleg, ak, eric.dumazet, gregkh,
	dhowells, daniel.lezcano, linux-fsdevel, linux-security-module,
	olofj, mhalcrow, dlaor, corbet, alan

On Mon, 2012-01-30 at 16:58 -0500, Colin Walters wrote:
> On Mon, 2012-01-30 at 08:17 -0800, Andy Lutomirski wrote:
> > Chroot can easily be used to subvert setuid programs.  If no_new_privs,
> > then setuid programs don't gain any privilege, so allow chroot.
> 
> Is this needed/desired by anyone now, or are you just using it to "demo"
> NO_NEW_PRIVS?  I don't see it as very useful on its own, since in any
> "container"-type chroot you really want /proc and /dev, and your patch
> doesn't help with that.
> 
> System daemons that do chroot for a modicum of security already start
> privileged, so this doesn't help them either.

I thought this was all for sandboxing? If a browers (or user) wants to
run some untrusted code, perhaps a chroot is the best way to do so. It
just will break if it needs to access /proc or /dev. And perhaps we
don't want untrusted code accessing /proc and /dev.

-- Steve




^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 4/4] Allow unprivileged chroot when safe
  2012-01-30 22:18     ` Steven Rostedt
@ 2012-01-30 22:28       ` Andy Lutomirski
  2012-01-30 22:38       ` Will Drewry
  1 sibling, 0 replies; 22+ messages in thread
From: Andy Lutomirski @ 2012-01-30 22:28 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Colin Walters, Will Drewry, linux-kernel, Casey Schaufler,
	Linus Torvalds, Jamie Lokier, keescook, john.johansen,
	serge.hallyn, coreyb, pmoore, eparis, djm, segoon, jmorris,
	scarybeasts, avi, penberg, viro, mingo, akpm, khilman,
	borislav.petkov, amwang, oleg, ak, eric.dumazet, gregkh,
	dhowells, daniel.lezcano, linux-fsdevel, linux-security-module,
	olofj, mhalcrow, dlaor, corbet, alan

On Mon, Jan 30, 2012 at 2:18 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Mon, 2012-01-30 at 16:58 -0500, Colin Walters wrote:
>> On Mon, 2012-01-30 at 08:17 -0800, Andy Lutomirski wrote:
>> > Chroot can easily be used to subvert setuid programs.  If no_new_privs,
>> > then setuid programs don't gain any privilege, so allow chroot.
>>
>> Is this needed/desired by anyone now, or are you just using it to "demo"
>> NO_NEW_PRIVS?  I don't see it as very useful on its own, since in any
>> "container"-type chroot you really want /proc and /dev, and your patch
>> doesn't help with that.
>>
>> System daemons that do chroot for a modicum of security already start
>> privileged, so this doesn't help them either.
>
> I thought this was all for sandboxing? If a browers (or user) wants to
> run some untrusted code, perhaps a chroot is the best way to do so. It
> just will break if it needs to access /proc or /dev. And perhaps we
> don't want untrusted code accessing /proc and /dev.

True.  A BPF seccomp filter that disables open, bind, connect, rename,
unlink, etc may be better, though.

(I like this patch, although I don't think it's at all essential.  It
could certainly be made more flexible and more useful, but it would
get considerably more complicated in the process.)

--Andy

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 4/4] Allow unprivileged chroot when safe
  2012-01-30 22:18     ` Steven Rostedt
  2012-01-30 22:28       ` Andy Lutomirski
@ 2012-01-30 22:38       ` Will Drewry
  2012-01-30 22:48         ` Colin Walters
  2012-01-30 22:51         ` Andy Lutomirski
  1 sibling, 2 replies; 22+ messages in thread
From: Will Drewry @ 2012-01-30 22:38 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Colin Walters, Andy Lutomirski, linux-kernel, Casey Schaufler,
	Linus Torvalds, Jamie Lokier, keescook, john.johansen,
	serge.hallyn, coreyb, pmoore, eparis, djm, segoon, jmorris,
	scarybeasts, avi, penberg, viro, mingo, akpm, khilman,
	borislav.petkov, amwang, oleg, ak, eric.dumazet, gregkh,
	dhowells, daniel.lezcano, linux-fsdevel, linux-security-module,
	olofj, mhalcrow, dlaor, corbet, alan

On Mon, Jan 30, 2012 at 4:18 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Mon, 2012-01-30 at 16:58 -0500, Colin Walters wrote:
>> On Mon, 2012-01-30 at 08:17 -0800, Andy Lutomirski wrote:
>> > Chroot can easily be used to subvert setuid programs.  If no_new_privs,
>> > then setuid programs don't gain any privilege, so allow chroot.
>>
>> Is this needed/desired by anyone now, or are you just using it to "demo"
>> NO_NEW_PRIVS?  I don't see it as very useful on its own, since in any
>> "container"-type chroot you really want /proc and /dev, and your patch
>> doesn't help with that.
>>
>> System daemons that do chroot for a modicum of security already start
>> privileged, so this doesn't help them either.
>
> I thought this was all for sandboxing? If a browers (or user) wants to
> run some untrusted code, perhaps a chroot is the best way to do so. It
> just will break if it needs to access /proc or /dev. And perhaps we
> don't want untrusted code accessing /proc and /dev.

Interestingly, I believe this change would work for the Chromium
setuid sandbox[1]. It uses a fancy clone trick (CLONE_FS) to start the
process then chroot once all its dependencies are loaded. It then
chroot()s to /proc/self/fd_info (or another empty process-specific
directory).  Of course, pid namespacing still wouldn't be there, but
it'd be nice to have a fallback if someone doesn't want the sandboxing
setup code to have privileges (or can only install unpriv'd code).

cheers!
will
1 - http://code.google.com/p/setuid-sandbox/source/browse/privdrop.c

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 4/4] Allow unprivileged chroot when safe
  2012-01-30 22:10     ` Andy Lutomirski
@ 2012-01-30 22:41       ` Colin Walters
  2012-01-30 22:43         ` Andy Lutomirski
  0 siblings, 1 reply; 22+ messages in thread
From: Colin Walters @ 2012-01-30 22:41 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Will Drewry, linux-kernel, Casey Schaufler, Linus Torvalds,
	Jamie Lokier, keescook, john.johansen, serge.hallyn, coreyb,
	pmoore, eparis, djm, segoon, rostedt, jmorris, scarybeasts, avi,
	penberg, viro, mingo, akpm, khilman, borislav.petkov, amwang,
	oleg, ak, eric.dumazet, gregkh, dhowells, daniel.lezcano,
	linux-fsdevel, linux-security-module, olofj, mhalcrow, dlaor,
	corbet, alan

On Mon, 2012-01-30 at 14:10 -0800, Andy Lutomirski wrote:
> On Mon, Jan 30, 2012 at 1:58 PM, Colin Walters <walters@verbum.org> wrote:
> > On Mon, 2012-01-30 at 08:17 -0800, Andy Lutomirski wrote:
> >> Chroot can easily be used to subvert setuid programs.  If no_new_privs,
> >> then setuid programs don't gain any privilege, so allow chroot.
> >
> > Is this needed/desired by anyone now, or are you just using it to "demo"
> > NO_NEW_PRIVS?  I don't see it as very useful on its own, since in any
> > "container"-type chroot you really want /proc and /dev, and your patch
> > doesn't help with that.
> 
> It's a demo, but it could still be useful for container-ish things.
> If something privileged sets up /proc, /sys, and /dev, then
> unprivileged code can chroot into the container.  This would allow
> much simpler implementations of tools like schroot.

What's the win if you still need a setuid binary?  schroot (and my
linux-user chroot binary) can just as easily call chroot as they can
create bind mounts; I'm not buying a code complexity argument.

> With this change, they wouldn't need to start privileged.
> (Admittedly, this isn't a great argument for this patch.)

Right...I'm not aware of anyone who would find that useful.



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 4/4] Allow unprivileged chroot when safe
  2012-01-30 22:41       ` Colin Walters
@ 2012-01-30 22:43         ` Andy Lutomirski
  2012-01-30 23:10           ` Colin Walters
  0 siblings, 1 reply; 22+ messages in thread
From: Andy Lutomirski @ 2012-01-30 22:43 UTC (permalink / raw)
  To: Colin Walters
  Cc: Will Drewry, linux-kernel, Casey Schaufler, Linus Torvalds,
	Jamie Lokier, keescook, john.johansen, serge.hallyn, coreyb,
	pmoore, eparis, djm, segoon, rostedt, jmorris, scarybeasts, avi,
	penberg, viro, mingo, akpm, khilman, borislav.petkov, amwang,
	oleg, ak, eric.dumazet, gregkh, dhowells, daniel.lezcano,
	linux-fsdevel, linux-security-module, olofj, mhalcrow, dlaor,
	corbet, alan

On Mon, Jan 30, 2012 at 2:41 PM, Colin Walters <walters@verbum.org> wrote:
> On Mon, 2012-01-30 at 14:10 -0800, Andy Lutomirski wrote:
>> On Mon, Jan 30, 2012 at 1:58 PM, Colin Walters <walters@verbum.org> wrote:
>> > On Mon, 2012-01-30 at 08:17 -0800, Andy Lutomirski wrote:
>> >> Chroot can easily be used to subvert setuid programs.  If no_new_privs,
>> >> then setuid programs don't gain any privilege, so allow chroot.
>> >
>> > Is this needed/desired by anyone now, or are you just using it to "demo"
>> > NO_NEW_PRIVS?  I don't see it as very useful on its own, since in any
>> > "container"-type chroot you really want /proc and /dev, and your patch
>> > doesn't help with that.
>>
>> It's a demo, but it could still be useful for container-ish things.
>> If something privileged sets up /proc, /sys, and /dev, then
>> unprivileged code can chroot into the container.  This would allow
>> much simpler implementations of tools like schroot.
>
> What's the win if you still need a setuid binary?  schroot (and my
> linux-user chroot binary) can just as easily call chroot as they can
> create bind mounts; I'm not buying a code complexity argument.

You don't need a setuid binary.  Just have an initscript set up the bind mounts.

--Andy

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 4/4] Allow unprivileged chroot when safe
  2012-01-30 22:38       ` Will Drewry
@ 2012-01-30 22:48         ` Colin Walters
  2012-01-30 22:51         ` Andy Lutomirski
  1 sibling, 0 replies; 22+ messages in thread
From: Colin Walters @ 2012-01-30 22:48 UTC (permalink / raw)
  To: Will Drewry
  Cc: Steven Rostedt, Andy Lutomirski, linux-kernel, Casey Schaufler,
	Linus Torvalds, Jamie Lokier, keescook, john.johansen,
	serge.hallyn, coreyb, pmoore, eparis, djm, segoon, jmorris,
	scarybeasts, avi, penberg, viro, mingo, akpm, khilman,
	borislav.petkov, amwang, oleg, ak, eric.dumazet, gregkh,
	dhowells, daniel.lezcano, linux-fsdevel, linux-security-module,
	olofj, mhalcrow, dlaor, corbet, alan

On Mon, 2012-01-30 at 16:38 -0600, Will Drewry wrote:
> On Mon, Jan 30, 2012 at 4:18 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Mon, 2012-01-30 at 16:58 -0500, Colin Walters wrote:
> >> On Mon, 2012-01-30 at 08:17 -0800, Andy Lutomirski wrote:
> >> > Chroot can easily be used to subvert setuid programs.  If no_new_privs,
> >> > then setuid programs don't gain any privilege, so allow chroot.
> >>
> >> Is this needed/desired by anyone now, or are you just using it to "demo"
> >> NO_NEW_PRIVS?  I don't see it as very useful on its own, since in any
> >> "container"-type chroot you really want /proc and /dev, and your patch
> >> doesn't help with that.
> >>
> >> System daemons that do chroot for a modicum of security already start
> >> privileged, so this doesn't help them either.
> >
> > I thought this was all for sandboxing? If a browers (or user) wants to
> > run some untrusted code, perhaps a chroot is the best way to do so. It
> > just will break if it needs to access /proc or /dev.

I think you'll find your definition of "code" becomes very limited
without /dev/null, /dev/zero and /proc/cpuinfo for example, as used by
glibc.

Personally I find it amazing we're even debating putting new
security-relevant API in the kernel with no known userspace consumer.
It can always go in later if someone actually wants it.

> Interestingly, I believe this change would work for the Chromium
> setuid sandbox[1]. It uses a fancy clone trick (CLONE_FS) to start the
> process then chroot once all its dependencies are loaded. It then
> chroot()s to /proc/self/fd_info (or another empty process-specific
> directory). 

But...it's setuid, so it can call chroot already?  I'm not following how
this change would benefit the helper.



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 4/4] Allow unprivileged chroot when safe
  2012-01-30 22:38       ` Will Drewry
  2012-01-30 22:48         ` Colin Walters
@ 2012-01-30 22:51         ` Andy Lutomirski
  2012-02-09  9:35           ` Vasiliy Kulikov
  1 sibling, 1 reply; 22+ messages in thread
From: Andy Lutomirski @ 2012-01-30 22:51 UTC (permalink / raw)
  To: Will Drewry
  Cc: Steven Rostedt, Colin Walters, linux-kernel, Casey Schaufler,
	Linus Torvalds, Jamie Lokier, keescook, john.johansen,
	serge.hallyn, coreyb, pmoore, eparis, djm, segoon, jmorris,
	scarybeasts, avi, penberg, viro, mingo, akpm, khilman,
	borislav.petkov, amwang, oleg, ak, eric.dumazet, gregkh,
	dhowells, daniel.lezcano, linux-fsdevel, linux-security-module,
	olofj, mhalcrow, dlaor, corbet, alan

On Mon, Jan 30, 2012 at 2:38 PM, Will Drewry <wad@chromium.org> wrote:
> On Mon, Jan 30, 2012 at 4:18 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> On Mon, 2012-01-30 at 16:58 -0500, Colin Walters wrote:
>>> On Mon, 2012-01-30 at 08:17 -0800, Andy Lutomirski wrote:
>>> > Chroot can easily be used to subvert setuid programs.  If no_new_privs,
>>> > then setuid programs don't gain any privilege, so allow chroot.
>>>
>>> Is this needed/desired by anyone now, or are you just using it to "demo"
>>> NO_NEW_PRIVS?  I don't see it as very useful on its own, since in any
>>> "container"-type chroot you really want /proc and /dev, and your patch
>>> doesn't help with that.
>>>
>>> System daemons that do chroot for a modicum of security already start
>>> privileged, so this doesn't help them either.
>>
>> I thought this was all for sandboxing? If a browers (or user) wants to
>> run some untrusted code, perhaps a chroot is the best way to do so. It
>> just will break if it needs to access /proc or /dev. And perhaps we
>> don't want untrusted code accessing /proc and /dev.
>
> Interestingly, I believe this change would work for the Chromium
> setuid sandbox[1]. It uses a fancy clone trick (CLONE_FS) to start the
> process then chroot once all its dependencies are loaded. It then
> chroot()s to /proc/self/fd_info (or another empty process-specific
> directory).  Of course, pid namespacing still wouldn't be there, but
> it'd be nice to have a fallback if someone doesn't want the sandboxing
> setup code to have privileges (or can only install unpriv'd code).
>
> cheers!
> will
> 1 - http://code.google.com/p/setuid-sandbox/source/browse/privdrop.c

That's neat!  CLONE_NEWPID might be safe with no_new_privs, too.
Unprivileged CLONE_NEWPID would also be a nice, straightforward way to
start up a process hierarchy and then reliably kill the whole thing
when you're done with it.

I'll add this to v5 as yet another optional patch.  All we need is
someone to pick up the patches for 3.4 :)

(Incidentally, I think privdrop.c wants CLONE_NEWIPC as well.)

--Andy

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 4/4] Allow unprivileged chroot when safe
  2012-01-30 22:43         ` Andy Lutomirski
@ 2012-01-30 23:10           ` Colin Walters
  2012-01-30 23:15             ` Andy Lutomirski
  0 siblings, 1 reply; 22+ messages in thread
From: Colin Walters @ 2012-01-30 23:10 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Will Drewry, linux-kernel, Casey Schaufler, Linus Torvalds,
	Jamie Lokier, keescook, john.johansen, serge.hallyn, coreyb,
	pmoore, eparis, djm, segoon, rostedt, jmorris, scarybeasts, avi,
	penberg, viro, mingo, akpm, khilman, borislav.petkov, amwang,
	oleg, ak, eric.dumazet, gregkh, dhowells, daniel.lezcano,
	linux-fsdevel, linux-security-module, olofj, mhalcrow, dlaor,
	corbet, alan

On Mon, 2012-01-30 at 14:43 -0800, Andy Lutomirski wrote:

> You don't need a setuid binary.  Just have an initscript set up the bind mounts.

The point is that dchroot is already setuid root, and calls chroot, so
it gains nothing from the ability to do it unprivileged.

(And wow, I just looked at the source, it's a setuid C++ binary!  Using
boost.  Ugh...)




^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 4/4] Allow unprivileged chroot when safe
  2012-01-30 23:10           ` Colin Walters
@ 2012-01-30 23:15             ` Andy Lutomirski
  2012-01-30 23:55               ` Colin Walters
  0 siblings, 1 reply; 22+ messages in thread
From: Andy Lutomirski @ 2012-01-30 23:15 UTC (permalink / raw)
  To: Colin Walters
  Cc: Will Drewry, linux-kernel, Casey Schaufler, Linus Torvalds,
	Jamie Lokier, keescook, john.johansen, serge.hallyn, coreyb,
	pmoore, eparis, djm, segoon, rostedt, jmorris, scarybeasts, avi,
	penberg, viro, mingo, akpm, khilman, borislav.petkov, amwang,
	oleg, ak, eric.dumazet, gregkh, dhowells, daniel.lezcano,
	linux-fsdevel, linux-security-module, olofj, mhalcrow, dlaor,
	corbet, alan

On Mon, Jan 30, 2012 at 3:10 PM, Colin Walters <walters@verbum.org> wrote:
> On Mon, 2012-01-30 at 14:43 -0800, Andy Lutomirski wrote:
>
>> You don't need a setuid binary.  Just have an initscript set up the bind mounts.
>
> The point is that dchroot is already setuid root, and calls chroot, so
> it gains nothing from the ability to do it unprivileged.
>
> (And wow, I just looked at the source, it's a setuid C++ binary!  Using
> boost.  Ugh...)

Exactly!

You can accomplish the same thing *without a scary setuid binary*.
The use case doesn't even need a new complicated userspace tool.  You
would set up an initscript or some /etc/fstab entries and then:

no_new_privs chroot /var/chroot/ubuntu_oneiric/ /bin/bash

et voila.  (Where no_new_privs would be a really simple tool that does
PR_SET_NO_NEW_PRIVS and then execs its argument.)

Maybe it's just me, but I think this is useful and I would, in fact,
use it in my regular workflow.

--Andy

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 4/4] Allow unprivileged chroot when safe
  2012-01-30 23:15             ` Andy Lutomirski
@ 2012-01-30 23:55               ` Colin Walters
  2012-01-31  0:13                 ` Andy Lutomirski
  0 siblings, 1 reply; 22+ messages in thread
From: Colin Walters @ 2012-01-30 23:55 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Will Drewry, linux-kernel, Casey Schaufler, Linus Torvalds,
	Jamie Lokier, keescook, john.johansen, serge.hallyn, coreyb,
	pmoore, eparis, djm, segoon, rostedt, jmorris, scarybeasts, avi,
	penberg, viro, mingo, akpm, khilman, borislav.petkov, amwang,
	oleg, ak, eric.dumazet, gregkh, dhowells, daniel.lezcano,
	linux-fsdevel, linux-security-module, olofj, mhalcrow, dlaor,
	corbet, alan

On Mon, 2012-01-30 at 15:15 -0800, Andy Lutomirski wrote:

> You can accomplish the same thing *without a scary setuid binary*.
> The use case doesn't even need a new complicated userspace tool.  You
> would set up an initscript or some /etc/fstab entries and then:

That requires administrative access to the system and custom
configuration; if you have that, you could just as easily set up a
wrapper script to run sudo + shell script to do whatever you want for
example.

That's the role schroot fills now - basically pre-canned scripts, but
you don't get out of custom configuration or needing root access to set
it up.   And as I mentioned in https://lkml.org/lkml/2011/12/9/213, it's
not as interesting as you might think even in the model of
"pre-configure, give out access to regular users", because if you allow
uploading .debs, it's just an elaborate root shell.

The most interesting thing to me is an entire setup that doesn't require
administrative access, so you can do it on any server or workstation,
and I have that with linux-user-chroot.

> no_new_privs chroot /var/chroot/ubuntu_oneiric/ /bin/bash
> 
> et voila.  (Where no_new_privs would be a really simple tool that does
> PR_SET_NO_NEW_PRIVS and then execs its argument.)
> 
> Maybe it's just me, but I think this is useful and I would, in fact,
> use it in my regular workflow.

workflow for what?  Building software?  Let's try to narrow down the
problem we're solving here.

It sounds like you're saying "schroot could be implemented with no
setuid binary", which I'm not sure is true.  The program has a big pile
of shell script hooks that are presently run as root - the bind mounts
you mention are part of it, but other stuff like synchronizing the NSS
database is there too.

I guess I'd find this patch a lot more convincing if you had actually
written code to use it and in practice found it useful, not just *in
theory*.



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 4/4] Allow unprivileged chroot when safe
  2012-01-30 23:55               ` Colin Walters
@ 2012-01-31  0:13                 ` Andy Lutomirski
  0 siblings, 0 replies; 22+ messages in thread
From: Andy Lutomirski @ 2012-01-31  0:13 UTC (permalink / raw)
  To: Colin Walters
  Cc: Will Drewry, linux-kernel, Casey Schaufler, Linus Torvalds,
	Jamie Lokier, keescook, john.johansen, serge.hallyn, coreyb,
	pmoore, eparis, djm, segoon, rostedt, jmorris, scarybeasts, avi,
	penberg, viro, mingo, akpm, khilman, borislav.petkov, amwang,
	oleg, ak, eric.dumazet, gregkh, dhowells, daniel.lezcano,
	linux-fsdevel, linux-security-module, olofj, mhalcrow, dlaor,
	corbet, alan

On Mon, Jan 30, 2012 at 3:55 PM, Colin Walters <walters@verbum.org> wrote:
> On Mon, 2012-01-30 at 15:15 -0800, Andy Lutomirski wrote:
>
>> You can accomplish the same thing *without a scary setuid binary*.
>> The use case doesn't even need a new complicated userspace tool.  You
>> would set up an initscript or some /etc/fstab entries and then:
>
> That requires administrative access to the system and custom
> configuration; if you have that, you could just as easily set up a
> wrapper script to run sudo + shell script to do whatever you want for
> example.
>
> That's the role schroot fills now - basically pre-canned scripts, but
> you don't get out of custom configuration or needing root access to set
> it up.   And as I mentioned in https://lkml.org/lkml/2011/12/9/213, it's
> not as interesting as you might think even in the model of
> "pre-configure, give out access to regular users", because if you allow
> uploading .debs, it's just an elaborate root shell.
>
> The most interesting thing to me is an entire setup that doesn't require
> administrative access, so you can do it on any server or workstation,
> and I have that with linux-user-chroot.
>
>> no_new_privs chroot /var/chroot/ubuntu_oneiric/ /bin/bash
>>
>> et voila.  (Where no_new_privs would be a really simple tool that does
>> PR_SET_NO_NEW_PRIVS and then execs its argument.)
>>
>> Maybe it's just me, but I think this is useful and I would, in fact,
>> use it in my regular workflow.
>
> workflow for what?  Building software?  Let's try to narrow down the
> problem we're solving here.

Building software.  I run Fedora and I write software and generate
binaries that need to work on Ubuntu.  So I keep an Ubuntu chroot
around.  On the occasions when I update the chroot, I have no problem
sudoing from the Fedora side (although this is suboptimal).  I
certainly don't need the NSS databases kept in sync, and I'd rather
minimize the complexity of things that run setuid root.

no_new_privs chroot /var/chroot/ubuntu /bin/bash is sufficient for my needs.

If we could have a full fakeroot-like setup supported, that would be
even better.  But that isn't likely to happen with a 44-line patch.


Like I said, the chroot patch is an example.  I think it has enough
valid usecases to more than justify its minimal complexity.  I also
think it's far less important than the core no_new_privs patch, which
enables lots of things beyond just chroot.

--Andy

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 1/4] Add PR_{GET,SET}_NO_NEW_PRIVS to prevent execve from granting privs
  2012-01-30 16:17 ` [PATCH v3 1/4] Add PR_{GET,SET}_NO_NEW_PRIVS to prevent execve from granting privs Andy Lutomirski
@ 2012-02-01 18:14   ` Kees Cook
  0 siblings, 0 replies; 22+ messages in thread
From: Kees Cook @ 2012-02-01 18:14 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Will Drewry, linux-kernel, Casey Schaufler, Linus Torvalds,
	Jamie Lokier, john.johansen, serge.hallyn, coreyb, pmoore,
	eparis, djm, segoon, rostedt, jmorris, scarybeasts, avi, penberg,
	viro, mingo, akpm, khilman, borislav.petkov, amwang, oleg, ak,
	eric.dumazet, gregkh, dhowells, daniel.lezcano, linux-fsdevel,
	linux-security-module, olofj, mhalcrow, dlaor, corbet, alan

On Mon, Jan 30, 2012 at 8:17 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> With this set, a lot of dangerous operations (chroot, unshare, etc)
> become a lot less dangerous because there is no possibility of
> subverting privileged binaries.
>
> This patch completely breaks apparmor.  Someone who understands (and
> uses) apparmor should fix it or at least give me a hint.
>
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>

Looking forward to this -- it'll give us a lot more flexibility.

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook
ChromeOS Security

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 3/4] Allow unprivileged CLONE_NEWUTS and CLONE_NEWIPC with no_new_privs
  2012-01-30 16:17 ` [PATCH v3 3/4] Allow unprivileged CLONE_NEWUTS and CLONE_NEWIPC with no_new_privs Andy Lutomirski
@ 2012-02-01 19:02   ` Kees Cook
  2012-02-01 20:35     ` Andy Lutomirski
  0 siblings, 1 reply; 22+ messages in thread
From: Kees Cook @ 2012-02-01 19:02 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Will Drewry, linux-kernel, Casey Schaufler, Linus Torvalds,
	Jamie Lokier, john.johansen, serge.hallyn, coreyb, pmoore,
	eparis, djm, segoon, rostedt, jmorris, scarybeasts, avi, penberg,
	viro, mingo, akpm, khilman, borislav.petkov, amwang, oleg, ak,
	eric.dumazet, gregkh, dhowells, daniel.lezcano, linux-fsdevel,
	linux-security-module, olofj, mhalcrow, dlaor, corbet, alan

On Mon, Jan 30, 2012 at 8:17 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> They are normally disallowed because they could be used to subvert
> setuid programs.  But if setuid is disabled, then they are safe.
>
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
>  kernel/nsproxy.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> index b576f7f..47cf873 100644
> --- a/kernel/nsproxy.c
> +++ b/kernel/nsproxy.c
> @@ -191,7 +191,13 @@ int unshare_nsproxy_namespaces(unsigned long unshare_flags,
>                               CLONE_NEWNET)))
>                return 0;
>
> -       if (!capable(CAP_SYS_ADMIN))
> +       /* We require either no_new_privs or CAP_SYS_ADMIN for all modes */
> +       if (!current->no_new_privs && !capable(CAP_SYS_ADMIN))
> +               return -EPERM;
> +
> +       /* NEWNS and NEWNET always require CAP_SYS_ADMIN. */
> +       if ((unshare_flags & (CLONE_NEWNS | CLONE_NEWNET)) &&
> +           !capable(CAP_SYS_ADMIN))
>                return -EPERM;
>
>        *new_nsp = create_new_namespaces(unshare_flags, current,

While I think it's unlikely that the list handled by
unshare_nsproxy_namespaces() is going to change, I'd still prefer that
the logic of this test be reversed so that the nnp-allowed flags are
listed instead of the CAP_SYS_ADMIN-required ones so that it will
default to disallowing new flags. It's a little less readable, but
maybe something like this (untested):

unsigned long handled_mask = (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
                                 CLONE_NEWNET);
unsigned long npp_mask = (CLONE_NEWUTS | CLONE_NEWIPC);

if (!(unshare_flags & handled_mask))
        return 0;

if (  !(current->no_new_privs &&
        !(unshare_flags & (handled_mask ^ npp_mask))) &&
      !capable(CAP_SYS_ADMIN))
        return -EPERM;
...

This also has the side-effect of removing the double-check of
capable() in some cases.

-Kees

-- 
Kees Cook
ChromeOS Security

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 3/4] Allow unprivileged CLONE_NEWUTS and CLONE_NEWIPC with no_new_privs
  2012-02-01 19:02   ` Kees Cook
@ 2012-02-01 20:35     ` Andy Lutomirski
  0 siblings, 0 replies; 22+ messages in thread
From: Andy Lutomirski @ 2012-02-01 20:35 UTC (permalink / raw)
  To: Kees Cook
  Cc: Will Drewry, linux-kernel, Casey Schaufler, Linus Torvalds,
	Jamie Lokier, john.johansen, serge.hallyn, coreyb, pmoore,
	eparis, djm, segoon, rostedt, jmorris, scarybeasts, avi, penberg,
	viro, mingo, akpm, khilman, borislav.petkov, amwang, oleg, ak,
	eric.dumazet, gregkh, dhowells, daniel.lezcano, linux-fsdevel,
	linux-security-module, olofj, mhalcrow, dlaor, corbet, alan

On Wed, Feb 1, 2012 at 11:02 AM, Kees Cook <keescook@chromium.org> wrote:
> On Mon, Jan 30, 2012 at 8:17 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>> They are normally disallowed because they could be used to subvert
>> setuid programs.  But if setuid is disabled, then they are safe.
>>
>> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>> ---
>>  kernel/nsproxy.c |    8 +++++++-
>>  1 files changed, 7 insertions(+), 1 deletions(-)
>>
>> diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
>> index b576f7f..47cf873 100644
>> --- a/kernel/nsproxy.c
>> +++ b/kernel/nsproxy.c
>> @@ -191,7 +191,13 @@ int unshare_nsproxy_namespaces(unsigned long unshare_flags,
>>                               CLONE_NEWNET)))
>>                return 0;
>>
>> -       if (!capable(CAP_SYS_ADMIN))
>> +       /* We require either no_new_privs or CAP_SYS_ADMIN for all modes */
>> +       if (!current->no_new_privs && !capable(CAP_SYS_ADMIN))
>> +               return -EPERM;
>> +
>> +       /* NEWNS and NEWNET always require CAP_SYS_ADMIN. */
>> +       if ((unshare_flags & (CLONE_NEWNS | CLONE_NEWNET)) &&
>> +           !capable(CAP_SYS_ADMIN))
>>                return -EPERM;
>>
>>        *new_nsp = create_new_namespaces(unshare_flags, current,
>
> While I think it's unlikely that the list handled by
> unshare_nsproxy_namespaces() is going to change, I'd still prefer that
> the logic of this test be reversed so that the nnp-allowed flags are
> listed instead of the CAP_SYS_ADMIN-required ones so that it will
> default to disallowing new flags. It's a little less readable, but
> maybe something like this (untested):
>
> unsigned long handled_mask = (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
>                                 CLONE_NEWNET);
> unsigned long npp_mask = (CLONE_NEWUTS | CLONE_NEWIPC);
>
> if (!(unshare_flags & handled_mask))
>        return 0;
>
> if (  !(current->no_new_privs &&
>        !(unshare_flags & (handled_mask ^ npp_mask))) &&
>      !capable(CAP_SYS_ADMIN))
>        return -EPERM;
> ...
>
> This also has the side-effect of removing the double-check of
> capable() in some cases.

Agreed -- will fix.

This patch is also missing the corresponding change for clone.  I'll add that.

I'm tempted to add CLONE_NEWPID as well (it's _useful_), but there's
an unresolved issue with SCM_CREDENTIALS.

--Andy

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 4/4] Allow unprivileged chroot when safe
  2012-01-30 22:51         ` Andy Lutomirski
@ 2012-02-09  9:35           ` Vasiliy Kulikov
  0 siblings, 0 replies; 22+ messages in thread
From: Vasiliy Kulikov @ 2012-02-09  9:35 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Will Drewry, Steven Rostedt, Colin Walters, linux-kernel,
	Casey Schaufler, Linus Torvalds, Jamie Lokier, keescook,
	john.johansen, serge.hallyn, coreyb, pmoore, eparis, djm,
	jmorris, scarybeasts, avi, penberg, viro, mingo, akpm, khilman,
	borislav.petkov, amwang, oleg, ak, eric.dumazet, gregkh,
	dhowells, daniel.lezcano, linux-fsdevel, linux-security-module,
	olofj, mhalcrow, dlaor, corbet, alan

On Mon, Jan 30, 2012 at 14:51 -0800, Andy Lutomirski wrote:
> That's neat!  CLONE_NEWPID might be safe with no_new_privs, too.
> Unprivileged CLONE_NEWPID would also be a nice, straightforward way to
> start up a process hierarchy and then reliably kill the whole thing
> when you're done with it.

It worth checking whether creating HUGE number or pid namespaces is
able to lock down the system for a significant period of time.  E.g.
triggering thousands of pid_ns enumeration under a spinlock.

The same with every "enable this privileged feature to unprivileged
users under certain circumstances" step.

Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2012-02-09  9:39 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-30 16:17 [PATCH v3 0/4] PR_SET_NO_NEW_PRIVS, unshare, and chroot Andy Lutomirski
2012-01-30 16:17 ` [PATCH v3 1/4] Add PR_{GET,SET}_NO_NEW_PRIVS to prevent execve from granting privs Andy Lutomirski
2012-02-01 18:14   ` Kees Cook
2012-01-30 16:17 ` [PATCH v3 2/4] Fix apparmor for PR_{GET,SET}_NO_NEW_PRIVS Andy Lutomirski
2012-01-30 16:17 ` [PATCH v3 3/4] Allow unprivileged CLONE_NEWUTS and CLONE_NEWIPC with no_new_privs Andy Lutomirski
2012-02-01 19:02   ` Kees Cook
2012-02-01 20:35     ` Andy Lutomirski
2012-01-30 16:17 ` [PATCH v3 4/4] Allow unprivileged chroot when safe Andy Lutomirski
2012-01-30 21:58   ` Colin Walters
2012-01-30 22:10     ` Andy Lutomirski
2012-01-30 22:41       ` Colin Walters
2012-01-30 22:43         ` Andy Lutomirski
2012-01-30 23:10           ` Colin Walters
2012-01-30 23:15             ` Andy Lutomirski
2012-01-30 23:55               ` Colin Walters
2012-01-31  0:13                 ` Andy Lutomirski
2012-01-30 22:18     ` Steven Rostedt
2012-01-30 22:28       ` Andy Lutomirski
2012-01-30 22:38       ` Will Drewry
2012-01-30 22:48         ` Colin Walters
2012-01-30 22:51         ` Andy Lutomirski
2012-02-09  9:35           ` Vasiliy Kulikov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).