[PATCH v2 0/4] PR_SET_NO_NEW_PRIVS, unshare, and chroot

[PATCH v2 0/4] PR_SET_NO_NEW_PRIVS, unshare, and chroot

[Andy Lutomirski]

To make the no_new_privs discussion more concrete, here is an updated
series that is actually useful.  It adds PR_SET_NO_NEW_PRIVS with the
same semantics as before (plus John Johansen's AppArmor fix and with
improved bisectability).  It then allows some unshare flags and chroot
(sometimes) when no_new_privs is set.

The unprivileged chroot could be quite useful, even though it's rather
constrained for now.

I think that blocking setresuid, setuid, and capset in no_new_privs mode
will make this a little less useful.  Comments are welcome.

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_v2

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 35
#define PR_GET_NO_NEW_PRIVS 36

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                  |   16 ++++++++++++++--
 include/linux/prctl.h      |   15 +++++++++++++++
 include/linux/sched.h      |    2 ++
 include/linux/security.h   |    1 +
 kernel/fork.c              |    2 ++
 kernel/nsproxy.c           |    8 +++++++-
 kernel/sys.c               |   10 ++++++++++
 security/apparmor/domain.c |   35 +++++++++++++++++++++++++++++++++++
 security/commoncap.c       |    7 +++++--
 security/selinux/hooks.c   |   10 +++++++++-
 11 files changed, 109 insertions(+), 7 deletions(-)

-- 
1.7.7.5

[PATCH v2 1/4] Add PR_{GET,SET}_NO_NEW_PRIVS to prevent execve from granting privs

[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/fork.c              |    2 ++
 kernel/sys.c               |   10 ++++++++++
 security/apparmor/domain.c |    4 ++++
 security/commoncap.c       |    7 +++++--
 security/selinux/hooks.c   |   10 +++++++++-
 9 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 25dcbe5..ca6a966 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1242,6 +1242,13 @@ 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 a3baeb2..7bafc98 100644
--- a/include/linux/prctl.h
+++ b/include/linux/prctl.h
@@ -102,4 +102,19 @@
 
 #define PR_MCE_KILL_GET 34
 
+/*
+ * 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 35
+#define PR_GET_NO_NEW_PRIVS 36
+
 #endif /* _LINUX_PRCTL_H */
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 41d0237..02b342c 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1300,6 +1300,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 ebd2a53..48294e0 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/fork.c b/kernel/fork.c
index 8e6b6f4..97cd34a 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1117,6 +1117,8 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	if (retval < 0)
 		goto bad_fork_free;
 
+	p->no_new_privs = current->no_new_privs;
+
 	/*
 	 * If multiple threads are within copy_process(), then this check
 	 * triggers too late. This doesn't hurt, the check is only there
diff --git a/kernel/sys.c b/kernel/sys.c
index 1dbbe69..219b3dc 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1837,6 +1837,16 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 			else
 				error = PR_MCE_KILL_DEFAULT;
 			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..a24ee44 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 useable 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 a93b3b7..b0e0f7b 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -511,14 +511,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 266a229..43eea9b 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2003,6 +2003,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,
@@ -2015,7 +2022,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.5

Re: [PATCH v2 1/4] Add PR_{GET,SET}_NO_NEW_PRIVS to prevent execve from granting privs

[Oleg Nesterov]

On 01/15, Andy Lutomirski wrote:
>
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1117,6 +1117,8 @@ static struct task_struct *copy_process(unsigned long clone_flags,
>  	if (retval < 0)
>  		goto bad_fork_free;
>
> +	p->no_new_privs = current->no_new_privs;

Again, this is unneeded, ->no_new_privs was copied by dup_task_struct().

Oleg.

Re: [PATCH v2 1/4] Add PR_{GET,SET}_NO_NEW_PRIVS to prevent execve from granting privs

[Andy Lutomirski]

On Mon, Jan 16, 2012 at 9:33 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 01/15, Andy Lutomirski wrote:
>>
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
>> @@ -1117,6 +1117,8 @@ static struct task_struct *copy_process(unsigned long clone_flags,
>>       if (retval < 0)
>>               goto bad_fork_free;
>>
>> +     p->no_new_privs = current->no_new_privs;
>
> Again, this is unneeded, ->no_new_privs was copied by dup_task_struct().

Yeah, sorry.  Will fix in v3.

--Andy

[PATCH v2 2/4] Fix apparmor for PR_{GET,SET}_NO_NEW_PRIVS

[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 a24ee44..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 useable 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.5

[PATCH v2 3/4] Allow unprivileged CLONE_NEWUTS and CLONE_NEWIPC with no_new_privs

[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 9aeab4b..471f4a3 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.5

[PATCH 4/4] Allow unprivileged chroot when safe

[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 |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/fs/open.c b/fs/open.c
index f711921..80ca7e2 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -422,6 +422,8 @@ SYSCALL_DEFINE1(chroot, const char __user *, filename)
 {
 	struct path path;
 	int error;
+	struct fs_struct *fs = current->fs;
+	bool is_chrooted;
 
 	error = user_path_dir(filename, &path);
 	if (error)
@@ -432,13 +434,23 @@ 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.  But we also
+	 * don't want to allow unprivileged users to break out of chroot
+	 * jail with another chroot call, so we require either CAP_SYS_CHROOT
+	 * unless we're not chrooted already and we have no_new_privs.
+	 */
+	is_chrooted = (fs->root.mnt->mnt_mountpoint !=
+		       fs->root.mnt->mnt_parent->mnt_root ||
+		       fs->root.dentry != fs->root.mnt->mnt_root);
+	if (!(current->no_new_privs && !is_chrooted) &&
+	    !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.5

Re: [PATCH 4/4] Allow unprivileged chroot when safe

[Linus Torvalds]

On Sun, Jan 15, 2012 at 4:37 PM, Andy Lutomirski <luto@amacapital.net> 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.
>
> Because chroot is an easy way to break out of chroot jail, CAP_SYS_ADMIN
> is still required if the caller is already chrooted.

So I think this whole chroot thing needs more people looking at it. I
brought up chroot as an example, but there may be other reasons why
you don't want user chrooting things than just the setuid confusion.

There's also the whole issue with doing things like local non-root
bind mounts, which are arguably more useful than chroot, and which are
disallowed for similar reasons. So I don't think chroot is all that
special.

                         Linus

Re: [PATCH 4/4] Allow unprivileged chroot when safe

[Andy Lutomirski]

On Sun, Jan 15, 2012 at 4:45 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sun, Jan 15, 2012 at 4:37 PM, Andy Lutomirski <luto@amacapital.net> 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.
>>
>> Because chroot is an easy way to break out of chroot jail, CAP_SYS_ADMIN
>> is still required if the caller is already chrooted.
>
> So I think this whole chroot thing needs more people looking at it. I
> brought up chroot as an example, but there may be other reasons why
> you don't want user chrooting things than just the setuid confusion.

Agreed.  There are plenty of security people cc'd.  Thoughts (and
attacks) are welcome!

>
> There's also the whole issue with doing things like local non-root
> bind mounts, which are arguably more useful than chroot, and which are
> disallowed for similar reasons. So I don't think chroot is all that
> special.

They're almost certainly more useful.  Binding the tree of your choice
on top of / is a nice (and more secure) way to emulate chroot.  The
only downside I've thought of in five minutes is that it would prevent
the administrator from blocking access to a directory by bind-mounting
something on to of it -- an unprivileged non-recursive bind mount of
the containing filesystem would get the hidden directory back.  I'm
not sure this is a real problem.

--Andy

Re: [PATCH 4/4] Allow unprivileged chroot when safe

[Colin Walters]

On Sun, 2012-01-15 at 16:37 -0800, Andy Lutomirski wrote:

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

This part is pretty gross.  It means it won't work for stuff like
containers (systemd-nspawn etc.) and furthermore I have plans that
involve running OS trees inside a chroot, and this would obviously not
work for that.

Incidentally I ended up putting my setuid program here:
http://git.gnome.org/browse/linux-user-chroot/

Now unfortunately, even if we say that a new setuid program is the way
to gain these privileges, you still can't nest it, because all of these
things are predicated on disabling setuid programs.  But it would at
least not fail initially if your process was inside a chroot.

Re: [PATCH 4/4] Allow unprivileged chroot when safe

[Andy Lutomirski]

On Mon, Jan 16, 2012 at 11:26 AM, Colin Walters <walters@verbum.org> wrote:
> On Sun, 2012-01-15 at 16:37 -0800, Andy Lutomirski wrote:
>
>> Because chroot is an easy way to break out of chroot jail, CAP_SYS_ADMIN
>> is still required if the caller is already chrooted.
>
> This part is pretty gross.  It means it won't work for stuff like
> containers (systemd-nspawn etc.) and furthermore I have plans that
> involve running OS trees inside a chroot, and this would obviously not
> work for that.

Agreed.

Unprivileged bind mounts would be a much better approach, but that
would need some concept of an unprivileged user owning a namespace.
Maybe the namespace id work would make this work.

--Andy

Re: [PATCH 4/4] Allow unprivileged chroot when safe

[Jamie Lokier]

Colin Walters wrote:
> On Sun, 2012-01-15 at 16:37 -0800, Andy Lutomirski wrote:
> 
> > Because chroot is an easy way to break out of chroot jail, CAP_SYS_ADMIN
> > is still required if the caller is already chrooted.
> 
> This part is pretty gross.  It means it won't work for stuff like
> containers (systemd-nspawn etc.) and furthermore

> I have plans that involve running OS trees inside a chroot, and this
> would obviously not work for that.

Indeed, I do run many of my machines inside a chroot.
The real filesystem has:

    /distro/that
    /distro/newer
    /distro/this

instead of partitions.  I'm chroot'd and fully booted up in
/distro/this, although I can see files in the others and I might run a
few things from them as well.

This isn't "userland chroot", it's the top level of the process tree:
/distro/this/sbin/init.  It would be a shame if it behaved differently
just because "it's a chroot".

-- Jamie

Re: [PATCH 4/4] Allow unprivileged chroot when safe

[Al Viro]

On Sun, Jan 15, 2012 at 04:37:21PM -0800, Andy Lutomirski wrote:

> +	is_chrooted = (fs->root.mnt->mnt_mountpoint !=
> +		       fs->root.mnt->mnt_parent->mnt_root ||
> +		       fs->root.dentry != fs->root.mnt->mnt_root);

Folks, is it _that_ hard to at least try to compile your patches?  Hint:
this one will *not*.  That sad detail aside, this test would have been
a pile of garbage even on the kernels that used to have mnt_mountpoint
in struct vfsmount.  What *are* you trying to test here?  The last part
at least does make some sense - it's "process root happens to be the
root of some vfsmount".  The first part, though, makes no sense whatsoever;
it's "... and that vfsmount is mounted on top of root of some other
vfsmount".

Grr...  NAKed, with extreme prejudice.

Re: [PATCH 4/4] Allow unprivileged chroot when safe

[Andy Lutomirski]

On Mon, Jan 16, 2012 at 12:06 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sun, Jan 15, 2012 at 04:37:21PM -0800, Andy Lutomirski wrote:
>
>> +     is_chrooted = (fs->root.mnt->mnt_mountpoint !=
>> +                    fs->root.mnt->mnt_parent->mnt_root ||
>> +                    fs->root.dentry != fs->root.mnt->mnt_root);
>
> Folks, is it _that_ hard to at least try to compile your patches?  Hint:
> this one will *not*.  That sad detail aside, this test would have been
> a pile of garbage even on the kernels that used to have mnt_mountpoint
> in struct vfsmount.  What *are* you trying to test here?  The last part
> at least does make some sense - it's "process root happens to be the
> root of some vfsmount".  The first part, though, makes no sense whatsoever;
> it's "... and that vfsmount is mounted on top of root of some other
> vfsmount".

I compiled it, booted it, and tested it.  I based it off an oldish
kernel, though, so I can rebase.

The first approach I tried was (from memory -- may not compile at all
on any version) fs->root.mnt != fs->root.mnt->mnt_parent.  That didn't
work.  The issue is that on dracut-based distros, AFAICT, the root (in
the sense of the root of the tree of struct vfsmounts) is rootfs.  The
apparent root (the filesystem containing /, /usr, etc) is mounted on
top of (rootfs)/.  Dracut then does something with the effect of
chroot("/").  So you end up with the vfsmount that contains "/" not
being the actual root vfsmount.  But there's nothing hidden by the
chroot -- even if fs->root.mnt pointed at rootfs, "/" would still
follow the mountpoint into the actual filesystem.

An different approach would be to have fs_struct keep track of a hard
and a soft root.  chroot would stay CAP_SYS_ADMIN only and change both
roots.  A new unprivileged_chroot would change only the soft root.
follow_dotdot would check both, so unprivileged_chroot wouldn't be
useful for breaking chroot.  The big downside would be an extra branch
on every follow_dotdot.

--Andy

Re: [PATCH 4/4] Allow unprivileged chroot when safe

[Al Viro]

On Mon, Jan 16, 2012 at 12:15:09PM -0800, Andy Lutomirski wrote:

> The first approach I tried was (from memory -- may not compile at all
> on any version) fs->root.mnt != fs->root.mnt->mnt_parent.  That didn't
> work.  The issue is that on dracut-based distros, AFAICT, the root (in
> the sense of the root of the tree of struct vfsmounts) is rootfs.  The
> apparent root (the filesystem containing /, /usr, etc) is mounted on
> top of (rootfs)/.  Dracut then does something with the effect of
> chroot("/").  So you end up with the vfsmount that contains "/" not
> being the actual root vfsmount.  But there's nothing hidden by the
> chroot -- even if fs->root.mnt pointed at rootfs, "/" would still
> follow the mountpoint into the actual filesystem.

That has nothing whatsoever to do with dracut.  _Everything_ ends up
that way; IOW, everything including init(8) runs chrooted into the
final userland root.  On any normal distro.  Your test is complete BS - e.g.
mount /dev/crap /mnt/blah
mount /dev/garbage /mnt/blah
chroot /mnt/blah
will *NOT* be chrooted per your definition.

Re: [PATCH 4/4] Allow unprivileged chroot when safe

[Oleg Nesterov]

On 01/15, 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.
>
> ...
>
> +	if (!(current->no_new_privs && !is_chrooted) &&
> +	    !capable(CAP_SYS_CHROOT))

I must have missed something. How no_new_privs can help if fs->users != 1 ?

Oleg.

Re: [PATCH 4/4] Allow unprivileged chroot when safe

[Andy Lutomirski]

On Tue, Jan 17, 2012 at 8:23 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 01/15, 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.
>>
>> ...
>>
>> +     if (!(current->no_new_privs && !is_chrooted) &&
>> +         !capable(CAP_SYS_CHROOT))
>
> I must have missed something. How no_new_privs can help if fs->users != 1 ?

Whoops.  That needs fixing.

--Andy

Re: [PATCH v2 0/4] PR_SET_NO_NEW_PRIVS, unshare, and chroot

[Andy Lutomirski]

On Sun, Jan 15, 2012 at 4:37 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> To make the no_new_privs discussion more concrete, here is an updated
> series that is actually useful.  It adds PR_SET_NO_NEW_PRIVS with the
> same semantics as before (plus John Johansen's AppArmor fix and with
> improved bisectability).  It then allows some unshare flags and chroot
> (sometimes) when no_new_privs is set.

Here's another reason to allow setuid/setresuid when no_new_privs is set:

I'd like a pam_no_new_privs module that lets me set no_new_privs for
certain highly untrusted users.  The way to do it is:

PR_SET_NO_NEW_PRIVS
setuid(uid)
execve(the user's shell)

If setuid were disallowed, it would have to be:

setuid(uid)
PR_SET_NO_NEW_PRIVS
execve(the user's shell)

The latter is racy: the user can log in once, then log in a second
time and ptrace the login or sshd process between setuid and
PR_SET_NO_NEW_PRIVS.

--Andy

Re: [PATCH v2 0/4] PR_SET_NO_NEW_PRIVS, unshare, and chroot

[Colin Walters]

On Sun, 2012-01-15 at 16:37 -0800, Andy Lutomirski wrote:
> To make the no_new_privs discussion more concrete, here is an updated
> series that is actually useful.  It adds PR_SET_NO_NEW_PRIVS

I think it'd be clearer to call it PR_SET_NOSUID - basically it should
match the semantics for MS_NOSUID mounts, as if on every exec()
thereafter the target binary was on a nosuid filesystem.

You might then change this flag to only take effect on a later exec(),
which would solve your race condition for the hypothetical PAM module.

>  with the
> same semantics as before (plus John Johansen's AppArmor fix and with
> improved bisectability).  It then allows some unshare flags 

What's the rationale behind the unshare subset?  Did you actually
analyze any setuid binaries found on Debian/Fedora etc. and determined
that e.g. CLONE_NEWNET was problematic for some reason?

I actually want CLONE_NEWNET for my build tool, so I can be sure the
arbitrary code I'm executing as part of the build at least isn't
downloading more new code.

Re: [PATCH v2 0/4] PR_SET_NO_NEW_PRIVS, unshare, and chroot

[Andy Lutomirski]

On Mon, Jan 16, 2012 at 12:49 PM, Colin Walters <walters@verbum.org> wrote:
> On Sun, 2012-01-15 at 16:37 -0800, Andy Lutomirski wrote:
>> To make the no_new_privs discussion more concrete, here is an updated
>> series that is actually useful.  It adds PR_SET_NO_NEW_PRIVS
>
> I think it'd be clearer to call it PR_SET_NOSUID - basically it should
> match the semantics for MS_NOSUID mounts, as if on every exec()
> thereafter the target binary was on a nosuid filesystem.

The MS_NOSUID semantics are somewhat ridiculous for selinux, and I'd
rather not make them match for no_new_privs.  AppArmor completely
ignores MS_NOSUID, so I think the rename would just cause more
confusion.

>
> You might then change this flag to only take effect on a later exec(),
> which would solve your race condition for the hypothetical PAM module.

That would just make it more complicated.  The race is already solved
in the current patch, anyway.

>
>>  with the
>> same semantics as before (plus John Johansen's AppArmor fix and with
>> improved bisectability).  It then allows some unshare flags
>
> What's the rationale behind the unshare subset?  Did you actually
> analyze any setuid binaries found on Debian/Fedora etc. and determined
> that e.g. CLONE_NEWNET was problematic for some reason?

CLONE_NEWNET seems more likely to consume significant kernel resources
than the others.  I didn't have a great reason, though.  Unsharing the
filesystem namespace is possibly dangerous because it could prevent an
unmount in the original namespace from taking effect everywhere.

>
> I actually want CLONE_NEWNET for my build tool, so I can be sure the
> arbitrary code I'm executing as part of the build at least isn't
> downloading more new code.
>
>

Fair enough.  I may add this in v3.  seccomp is an even better
solution, though :)

--Andy

Re: [PATCH v2 0/4] PR_SET_NO_NEW_PRIVS, unshare, and chroot

[Colin Walters]

On Mon, 2012-01-16 at 13:25 -0800, Andy Lutomirski wrote:

> The MS_NOSUID semantics are somewhat ridiculous for selinux, 

I don't see how they're ridiculous.

> and I'd
> rather not make them match for no_new_privs. 

Note your patch for selinux does exactly the same thing in the NOSUID
case and your NO_NEW_PRIVS flag.  Right?

-       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;


>  AppArmor completely
> ignores MS_NOSUID,

Ugh...well, I guess if it doesn't store any security data associated
with files, only with file names, then there's nothing for it to do.
Like I said before though, I think SELinux is the only sane LSM.

> CLONE_NEWNET seems more likely to consume significant kernel resources
> than the others. 

This actually brings up something we need to think about - if we're
heading towards being able to do bind mounts as non-root (which is
necessary for me) we'd need limits on e.g. the number of mounts that can
be made for a given uid/cgroup.

I have a picked-from-thin-air hardcoded limit of 50 in my setuid binary,
but I just realized that that's 50*RLIMIT_NPROC which is kind of
large...

>  I didn't have a great reason, though.  Unsharing the
> filesystem namespace is possibly dangerous because it could prevent an
> unmount in the original namespace from taking effect everywhere.

Hmmm...hadn't considered that either.  So the issue here is if a server
admin has e.g. a NFS mount and my build tool makes a new copy of the
mount namespace, a process may still have it busy when she goes to
unmount it?

> Fair enough.  I may add this in v3.  seccomp is an even better
> solution, though :)

Yeah, definitely more flexible, though realistic use of seccomp depends
on someone making a nice userspace tool to compile sets of syscalls like
"no networking".

Re: [PATCH v2 0/4] PR_SET_NO_NEW_PRIVS, unshare, and chroot

[Andy Lutomirski]

On Mon, Jan 16, 2012 at 1:47 PM, Colin Walters <walters@verbum.org> wrote:
> On Mon, 2012-01-16 at 13:25 -0800, Andy Lutomirski wrote:
>
>> The MS_NOSUID semantics are somewhat ridiculous for selinux,
>
> I don't see how they're ridiculous.

exec_sid is silently ignored.  So runcon will not switch context but
will still appear to succeed.

>
>> and I'd
>> rather not make them match for no_new_privs.
>
> Note your patch for selinux does exactly the same thing in the NOSUID
> case and your NO_NEW_PRIVS flag.  Right?
>
> -       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;
>

See several lines up.

>
>>  AppArmor completely
>> ignores MS_NOSUID,
>
> Ugh...well, I guess if it doesn't store any security data associated
> with files, only with file names, then there's nothing for it to do.

Nope.  It looks it up by file name or path, I think.

> Like I said before though, I think SELinux is the only sane LSM.

I think the fact that there is a bprm_set_creds hook at all is insane,
but maybe that's just me.  I think this is one of the things that
Windows does far better than POSIX.  On Windows, CreateProcess (the
moral equivalent of execve) never gains privileges.

--Andy