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

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

[Eric Paris]

[-- Attachment #1: Type: text/plain, Size: 1739 bytes --]

-----Original Message-----
From: Jamie Lokier [jamie@shareable.org]
Received: Saturday, 14 Jan 2012, 8:56am
To: Andy Lutomirski [luto@amacapital.net]
CC: Linus Torvalds [torvalds@linux-foundation.org]; Alan Cox [alan@lxorguk.ukuu.org.uk]; Oleg Nesterov [oleg@redhat.com]; Will Drewry [wad@chromium.org]; linux-kernel@vger.kernel.org, keescook@chromium.org, john.johansen@canonical.com, serge.hallyn@canonical.com, coreyb@linux.vnet.ibm.com, pmoore@redhat.com, eparis@redhat.com, djm@mindrot.org, segoon@openwall.com, rostedt@goodmis.org, jmorris@namei.org, scarybeasts@gmail.com, avi@redhat.com, penberg@cs.helsinki.fi, viro@zeniv.linux.org.uk, luto@mit.edu, mingo@elte.hu, akpm@linux-foundation.org, khilman@ti.com, borislav.petkov@amd.com, amwang@redhat.com, ak@linux.intel.com, eric.dumazet@gmail.com, gregkh@suse.de, dhowells@redhat.com, daniel.lezcano@free.fr, linux-fsdevel@vger.kernel.org, linux-security-module@vger.kernel.org, olofj@chromium.org, mhalcrow@google.com, dlaor@redhat.com, corbet@lwn.net
Subject: Re: [PATCH] Add PR_{GET,SET}_NO_NEW_PRIVS to prevent execve from granting privs

Andy Lutomirski wrote:

> Is the current exec_no_trans check enough for you?  With my patch,
> selinux can already block the execve if it wants.  (The policy is the
> same as it would be if a program explicitly asked to run the new
> executable with an unchanged security context.)  I'd be happy to fail
> the exec in AppArmor, and then maybe AppArmor will change its mind
> if/when users get annoyed :)

Does SELinux block if userspace does exec entirely in userspace using
mmap() and not execve()?  If not, why is execve() allowed to be different?

Yes, we do (or can, and usually do in policy)

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

[Andy Lutomirski]

On Sat, Jan 14, 2012 at 7:17 AM, Eric Paris <eparis@redhat.com> wrote:
> -----Original Message-----
> From: Jamie Lokier [jamie@shareable.org]

> Andy Lutomirski wrote:
>
>> Is the current exec_no_trans check enough for you?  With my patch,
>> selinux can already block the execve if it wants.  (The policy is the
>> same as it would be if a program explicitly asked to run the new
>> executable with an unchanged security context.)  I'd be happy to fail
>> the exec in AppArmor, and then maybe AppArmor will change its mind
>> if/when users get annoyed :)
>
> Does SELinux block if userspace does exec entirely in userspace using
> mmap() and not execve()?  If not, why is execve() allowed to be different?
>
> Yes, we do (or can, and usually do in policy)
>

By blocking open, read, mmap, or mprotect?  And, more to the point, why?

I've always found it weird/annoying that selinux blocks things that
can neither be used to gain privilege nor to DoS the system.
Certainly a fully confined selinux program can emulate the execution
of anything it can read -- it just might be slow.

--Andy

[PATCH] 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>
---

For the git-inclined, this patch will show up here shortly:
https://git.kernel.org/?p=linux/kernel/git/luto/linux.git;a=shortlog;h=refs/heads/security/no_new_privs/patch_v1

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

 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 |    3 +++
 security/commoncap.c       |    7 +++++--
 security/selinux/hooks.c   |   10 +++++++++-
 9 files changed, 56 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..7f480b7 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -360,6 +360,9 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
 	if (bprm->cred_prepared)
 		return 0;
 
+	/* XXX: someone who understands apparmor needs to fix this. */
+	BUG_ON(bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS);
+
 	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] Add PR_{GET,SET}_NO_NEW_PRIVS to prevent execve from granting privs

[Kees Cook]

On Thu, Jan 12, 2012 at 6:12 PM, 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>
> [....]
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index c1e18ba..7f480b7 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -360,6 +360,9 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>        if (bprm->cred_prepared)
>                return 0;
>
> +       /* XXX: someone who understands apparmor needs to fix this. */
> +       BUG_ON(bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS);
> +
>        cxt = bprm->cred->security;
>        BUG_ON(!cxt);
>

Since apparmor_bprm_set_creds() calls cap_bprm_set_creds() already[1],
I think AppArmor needs no changes at all, but John will know better.
:)

-Kees

[1] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=security/apparmor/domain.c;h=c1e18ba5bdc09c65d259ad4bd9f374ef04dffd2f;hb=HEAD#l356

-- 
Kees Cook
ChromeOS Security

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

[Andy Lutomirski]

On Thu, Jan 12, 2012 at 9:58 PM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Jan 12, 2012 at 6:12 PM, 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>
>> [....]
>> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
>> index c1e18ba..7f480b7 100644
>> --- a/security/apparmor/domain.c
>> +++ b/security/apparmor/domain.c
>> @@ -360,6 +360,9 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>>        if (bprm->cred_prepared)
>>                return 0;
>>
>> +       /* XXX: someone who understands apparmor needs to fix this. */
>> +       BUG_ON(bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS);
>> +
>>        cxt = bprm->cred->security;
>>        BUG_ON(!cxt);
>>
>
> Since apparmor_bprm_set_creds() calls cap_bprm_set_creds() already[1],
> I think AppArmor needs no changes at all, but John will know better.
> :)

I think that AppArmor determines what a program is allowed to do by
looking at the path of the executable.  We don't want newly-executed
programs to gain permissions because they're a different executable
when we're in no_new_privs mode, so (if I'm right) something different
needs to happen.

--Andy

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

[Kees Cook]

On Thu, Jan 12, 2012 at 10:02 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Thu, Jan 12, 2012 at 9:58 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Thu, Jan 12, 2012 at 6:12 PM, 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>
>>> [....]
>>> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
>>> index c1e18ba..7f480b7 100644
>>> --- a/security/apparmor/domain.c
>>> +++ b/security/apparmor/domain.c
>>> @@ -360,6 +360,9 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>>>        if (bprm->cred_prepared)
>>>                return 0;
>>>
>>> +       /* XXX: someone who understands apparmor needs to fix this. */
>>> +       BUG_ON(bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS);
>>> +
>>>        cxt = bprm->cred->security;
>>>        BUG_ON(!cxt);
>>>
>>
>> Since apparmor_bprm_set_creds() calls cap_bprm_set_creds() already[1],
>> I think AppArmor needs no changes at all, but John will know better.
>> :)
>
> I think that AppArmor determines what a program is allowed to do by
> looking at the path of the executable.  We don't want newly-executed
> programs to gain permissions because they're a different executable
> when we're in no_new_privs mode, so (if I'm right) something different
> needs to happen.

I'll have to go look more closely. I thought cap_bprm_set_creds() was
already evaluating the new privs and blocking any gained privs with
the changes you were making.

-Kees

-- 
Kees Cook
ChromeOS Security

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

[John Johansen]

On 01/13/2012 07:09 AM, Kees Cook wrote:
> On Thu, Jan 12, 2012 at 10:02 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Thu, Jan 12, 2012 at 9:58 PM, Kees Cook <keescook@chromium.org> wrote:
>>> On Thu, Jan 12, 2012 at 6:12 PM, 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>
>>>> [....]
>>>> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
>>>> index c1e18ba..7f480b7 100644
>>>> --- a/security/apparmor/domain.c
>>>> +++ b/security/apparmor/domain.c
>>>> @@ -360,6 +360,9 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>>>>        if (bprm->cred_prepared)
>>>>                return 0;
>>>>
>>>> +       /* XXX: someone who understands apparmor needs to fix this. */
>>>> +       BUG_ON(bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS);
>>>> +
>>>>        cxt = bprm->cred->security;
>>>>        BUG_ON(!cxt);
>>>>
>>>
>>> Since apparmor_bprm_set_creds() calls cap_bprm_set_creds() already[1],
>>> I think AppArmor needs no changes at all, but John will know better.
>>> :)
>>
>> I think that AppArmor determines what a program is allowed to do by
>> looking at the path of the executable.  We don't want newly-executed
>> programs to gain permissions because they're a different executable
>> when we're in no_new_privs mode, so (if I'm right) something different
>> needs to happen.
> 
> I'll have to go look more closely. I thought cap_bprm_set_creds() was
> already evaluating the new privs and blocking any gained privs with
> the changes you were making.
> 
We do want to do something more.  A first pass at it would be to allow execs
that inherit the current context, and we will also want to reject apparmor's
equiv of setcon, and setexeccon, at the interface.

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

[John Johansen]

On 01/13/2012 03:12 AM, Andy Lutomirski 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.
> 

This should fix apparmor

---

>From b4d142f1a81d252ac2a72587841e7b430190a889 Mon Sep 17 00:00:00 2001
From: John Johansen <john.johansen@canonical.com>
Date: Fri, 13 Jan 2012 14:20:47 +0100
Subject: [PATCH] Fix apparmor for PR_{GET,SET}_NO_NEW_PRIV

Signed-off-by: John Johansen <john.johansen@canonical.com>
---
 security/apparmor/domain.c |   38 +++++++++++++++++++++++++++++++++++---
 1 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 7f480b7..7316d77 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -360,9 +360,6 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
 	if (bprm->cred_prepared)
 		return 0;
 
-	/* XXX: someone who understands apparmor needs to fix this. */
-	BUG_ON(bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS);
-
 	cxt = bprm->cred->security;
 	BUG_ON(!cxt);
 
@@ -398,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;
 	}
 
@@ -458,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;
 
@@ -612,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;
@@ -753,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.3

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

[Oleg Nesterov]

On 01/12, 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;

unnecessary, this is copied by dup_task_struct().

Oleg.

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

[Alan Cox]

This still appears to be a bit broken

There are three problems here

1. I can stop an app changing privs which in some SELinux or APParmour
cases might mean I prevent it being dropped into a less privileged
position. That's something only the security policy knows.

So for SELinux and Apparmour and the like in some situations you are
potentially adding a security hole. That one seems hard to fix unless you
fail the exec if it causes a security transition, as opposed to just
keeping the old one. For non change cases we can however still pass the
filter on, which is the usual sane case.

2. ptrace

You neeed to also stop ptrace otherwise the locked down process can use
ptrace to proxy its activity via another task with the same uid. That's
easy enough to add fortunately.

3. file access

You have the same attacks via patching files of running apps etc. In the
intended circumstances I'm not sure this matters or is cleanly fixable.
It's the point at which you need a real system wide policy and SELinux
etc anyway.

Alan

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

[Eric Paris]

On Fri, 2012-01-13 at 18:24 +0000, Alan Cox wrote:
> This still appears to be a bit broken
> 
> There are three problems here
> 
> 1. I can stop an app changing privs which in some SELinux or APParmour
> cases might mean I prevent it being dropped into a less privileged
> position. That's something only the security policy knows.
> 
> So for SELinux and Apparmour and the like in some situations you are
> potentially adding a security hole. That one seems hard to fix unless you
> fail the exec if it causes a security transition, as opposed to just
> keeping the old one. For non change cases we can however still pass the
> filter on, which is the usual sane case.

I can't speak about AppArmour at all, but not transitioning in SELinux
(the same as MNT_NOSUID) is safe since policy will still make a security
decision if you are allowed to launch the binary without transitioning.
I have thoughts on how to make the SELinux approach more flexible and
policy controlled, but I'd be fine with this flag just applying no
transition for now and adding that as a new feature down the road.

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

[Andy Lutomirski]

On Fri, Jan 13, 2012 at 10:24 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> This still appears to be a bit broken
>
> There are three problems here
>
> 1. I can stop an app changing privs which in some SELinux or APParmour
> cases might mean I prevent it being dropped into a less privileged
> position. That's something only the security policy knows.
>
> So for SELinux and Apparmour and the like in some situations you are
> potentially adding a security hole. That one seems hard to fix unless you
> fail the exec if it causes a security transition, as opposed to just
> keeping the old one. For non change cases we can however still pass the
> filter on, which is the usual sane case.

SELinux can already control this via exec_no_trans.

Changing it to fail the exec when a transition would occur will make
seccomp considerably less useful to selinux users -- the presence of
MAC policy on a program (regardless of what that policy is) will make
it unusable inside a sandbox.

<rant>This is exactly why I think that changing security context on
execve() is an awful idea.  If administrators and distros want to
define fancy contexts, fine.  But programs should *ask* to enter the
contexts.  (This would be easy enough with some glibc / libselinux
magic.)  And the use of MAC should not prevent the use of IMO
considerably more secure user-controlled sandbox technologies.
execve_nosecurity was my first attempt to fix it without hitting this
issue.</rant>

>
> 2. ptrace
>
> You neeed to also stop ptrace otherwise the locked down process can use
> ptrace to proxy its activity via another task with the same uid. That's
> easy enough to add fortunately.
>
> 3. file access
>
> You have the same attacks via patching files of running apps etc. In the
> intended circumstances I'm not sure this matters or is cleanly fixable.
> It's the point at which you need a real system wide policy and SELinux
> etc anyway.

I disagree, but maybe this is a sign that no_new_privs is a bad name.
no_new_privs is not intended to be a sandbox at all -- it's a way to
make it safe for a task to manipulate itself in a way that would allow
it to subvert its own children (or itself after execve).  So ptrace
isn't a problem at all -- PR_SET_NO_NEW_PRIVS + chroot + ptrace is
exactly as unsafe as ptrace without PR_SET_NO_NEW_PRIVS.  Neither one
allows privilege escalation beyond what you started with.

If you want a sandbox, call PR_SET_NO_NEW_PRIVS, then enable seccomp
(or whatever) to disable ptrace, evil file access, connections on unix
sockets that authenticate via uid, etc.  (IMO MAC has no place here --
maybe we need a new buzzword like "Voluntary Access Control".)

--Andy

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

[Linus Torvalds]

On Fri, Jan 13, 2012 at 11:00 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> Changing it to fail the exec when a transition would occur will make
> seccomp considerably less useful to selinux users -- the presence of
> MAC policy on a program (regardless of what that policy is) will make
> it unusable inside a sandbox.

I don't agree. I think that's exactly what we want sandboxing for, to
avoid any kind of subtle security issues.

And in 99.99% of all sandboxes, you would never ever want to execute
something with a different MAC policy on it. In fact, most of the
whole point of the sandboxing tends to be to make sure that the user
stays inside the exact *small* environment that was provided just for
that thing.

So I bet the google chrome people are not at all interested in
"running random binaries", and might want execve() very much for
"running some specific binaries that we ship with or install from the
browser".

So I really think that the *only* valid model is the "fail the execve
on any changes", not the "mnt-nosuid" approach of trying to run things
with the wrong permissions and get perhaps odd results. And I think it
works even - and perhaps *especially* - in models like selinux or
apparmor that do have a lot of "implicit MAC knowledge" on specific
binaries.

                    Linus

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

[Andy Lutomirski]

On Fri, Jan 13, 2012 at 11:12 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Jan 13, 2012 at 11:00 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>>
>> Changing it to fail the exec when a transition would occur will make
>> seccomp considerably less useful to selinux users -- the presence of
>> MAC policy on a program (regardless of what that policy is) will make
>> it unusable inside a sandbox.
>
> I don't agree. I think that's exactly what we want sandboxing for, to
> avoid any kind of subtle security issues.
>
> And in 99.99% of all sandboxes, you would never ever want to execute
> something with a different MAC policy on it. In fact, most of the
> whole point of the sandboxing tends to be to make sure that the user
> stays inside the exact *small* environment that was provided just for
> that thing.

Some day I want to run entire fancy desktop apps inside
non-root-controlled sandboxes.  We're not there yet, but we're not
*that* far.  Also, running things like MATLAB inside a chroot or mount
namespace is very useful (I've done it -- there are MATLAB programs
with remarkable dependencies on very particular versions of system
libraries), and being able to do it without custom setuid wrappers to
set everything up would be nice.

>
> So I bet the google chrome people are not at all interested in
> "running random binaries", and might want execve() very much for
> "running some specific binaries that we ship with or install from the
> browser".

I want to be able to run "magic_seccomp_sandbox gv somefile.ps".  And
I want that to still work even if my distro gets fancy and uses
selinux policy to prevent gv from accessing the network (which would
not be an unreasonable thing to do).  The chromium project already has
a little seccomp wrapper that can (sort of) do this.

>
> So I really think that the *only* valid model is the "fail the execve
> on any changes", not the "mnt-nosuid" approach of trying to run things
> with the wrong permissions and get perhaps odd results. And I think it
> works even - and perhaps *especially* - in models like selinux or
> apparmor that do have a lot of "implicit MAC knowledge" on specific
> binaries.

Is the current exec_no_trans check enough for you?  With my patch,
selinux can already block the execve if it wants.  (The policy is the
same as it would be if a program explicitly asked to run the new
executable with an unchanged security context.)  I'd be happy to fail
the exec in AppArmor, and then maybe AppArmor will change its mind
if/when users get annoyed :)

--Andy

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

[Linus Torvalds]

On Fri, Jan 13, 2012 at 11:39 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> Is the current exec_no_trans check enough for you?  With my patch,
> selinux can already block the execve if it wants.

If this feature has "selinux can do xyz if it wants", it is broken.

The *whole* point is to get the f*^%ing crazy "security managers can
do xyz" things away from it.

The flag - when set - should give a 100% guarantee that security
context doesn't change, and an operation that would change it would
error out.

Not a "selinux can block it if it wants". None of that "wants" crap.
None of the "you can configure security rules to do xyz" crap.

One simple rule: no security changes from the context that set the flag.

Any other rule will inevitably cause random gray areas where some
random security manager does something stupid. We have enough of those
already. No more.

                     Linus

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

[Andy Lutomirski]

On Fri, Jan 13, 2012 at 11:45 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Jan 13, 2012 at 11:39 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>>
>> Is the current exec_no_trans check enough for you?  With my patch,
>> selinux can already block the execve if it wants.
>
> If this feature has "selinux can do xyz if it wants", it is broken.
>
> The *whole* point is to get the f*^%ing crazy "security managers can
> do xyz" things away from it.
>
> The flag - when set - should give a 100% guarantee that security
> context doesn't change, and an operation that would change it would
> error out.
>
> Not a "selinux can block it if it wants". None of that "wants" crap.
> None of the "you can configure security rules to do xyz" crap.
>
> One simple rule: no security changes from the context that set the flag.
>
> Any other rule will inevitably cause random gray areas where some
> random security manager does something stupid. We have enough of those
> already. No more.

I'm confused.  The patch does "no security context changes on execve".

My interpretation of the flag is "LSMs, file caps, and suid/sgid, stay
away".  It does that.  Users are welcome to shoot themselves in the
foot with this behavior (e.g. by setting the flag, running something
that selinux would otherwise have restricted, and getting it without
the restrictions).

If you are *already* in a selinux context that is forbidden from
executing something without changing security context, then you can't
execute the program.  If no_new_privs is set or if you ask (via
selinuxfs) for no transition, then execve fails when selinux refuses
exec_no_trans.  The exec_no_trans callback cannot *change* context --
it just asks whether *not* changing context is okay.

I think this is exactly the desired behavior.  And I misunderstanding
you?  (I can't speak for the AppArmor part of the patch -- I haven't
looked closely enough.  But I think we want the equivalent behavior on
AppArmor.)

If this behavior is *not* okay, then I would rather just have the flag
disable execve entirely and consider execve_nosecurity instead.  I
don't want a situation where fancy seccomp/chroot/whatever apps don't
even run on Fedora because of the mere existence of selinux policy.
If we go that route, I'd rather they didn't run anywhere so the
sandbox authors work around the problem appropriately.

--Andy

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

[Linus Torvalds]

On Fri, Jan 13, 2012 at 12:05 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> I'm confused.  The patch does "no security context changes on execve".

So that's what I wanted and thought you did, but your comment:

  "With my patch, selinux can already block the execve if it wants"

is what I reacted to. The "selinux *can*" and the "if it wants" part
was what made my hackles rise.

If it is not about what selinux can and what selinux wants, I'm happy.
The security manager shouldn't have any choices in the matter. No
'can', no 'want'.

Your choice of words made me think your patch had left that door open.

                 Linus

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

[Andy Lutomirski]

On Fri, Jan 13, 2012 at 12:13 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Jan 13, 2012 at 12:05 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>
>> I'm confused.  The patch does "no security context changes on execve".
>
> So that's what I wanted and thought you did, but your comment:
>
>  "With my patch, selinux can already block the execve if it wants"
>
> is what I reacted to. The "selinux *can*" and the "if it wants" part
> was what made my hackles rise.
>
> If it is not about what selinux can and what selinux wants, I'm happy.
> The security manager shouldn't have any choices in the matter. No
> 'can', no 'want'.
>
> Your choice of words made me think your patch had left that door open.

Fair enough.

It's unavoidable that selinux can block the exec, though -- it could
prevent you from reading the file, in which case good luck execing it
:)

I'll respin this so that it doesn't oops if bisected with AppArmor
running.  Any maintainers want to pick it up?

--Andy

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

[Eric Paris]

On Fri, 2012-01-13 at 11:45 -0800, Linus Torvalds wrote:
> On Fri, Jan 13, 2012 at 11:39 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> >
> > Is the current exec_no_trans check enough for you?  With my patch,
> > selinux can already block the execve if it wants.
> 
> If this feature has "selinux can do xyz if it wants", it is broken.
> 
> The *whole* point is to get the f*^%ing crazy "security managers can
> do xyz" things away from it.
> 
> The flag - when set - should give a 100% guarantee that security
> context doesn't change, and an operation that would change it would
> error out.

That's what you would get today following the MNT_NOSUID example.
SELinux just has the additional property that the policy can either
error (and fail the exec) or allow no selinux transition to happen.  

> Not a "selinux can block it if it wants". None of that "wants" crap.
> None of the "you can configure security rules to do xyz" crap.
> 
> One simple rule: no security changes from the context that set the flag.
> 
> Any other rule will inevitably cause random gray areas where some
> random security manager does something stupid. We have enough of those
> already. No more.

So you can't drop capabilities(7)?  If you come in with permission you
can't get rid of it?  Ouch.

My thought on expanding the SELinux support beyond 'no
transition' (which I suggest we do today) would be that we might allow
SELinux transitions if we can show the the 'child' domain is a subset of
the 'parent' domain. Much the same as I imagine you can still drop
capabilities after setting this flag you might be able to drop SELinux
permissions, but that's something that would need a lot of thought and
that we don't have a good way to do today...

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

[Linus Torvalds]

On Fri, Jan 13, 2012 at 12:13 PM, Eric Paris <eparis@redhat.com> wrote:
>
> So you can't drop capabilities(7)?  If you come in with permission you
> can't get rid of it?  Ouch.

I really don't understand why people get confused about this.

No, you can't drop capabilities. You're in a sandbox, the whole point
is that you're running untrusted code, you had better not *have* any
capabilities that you worry about dropping.

The whole "drop capabilities" is a red herring. If you think you need
it, you're doing something wrong.

                        Linus

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

[Jamie Lokier]

Andy Lutomirski wrote:
> > So I bet the google chrome people are not at all interested in
> > "running random binaries", and might want execve() very much for
> > "running some specific binaries that we ship with or install from the
> > browser".
> 
> I want to be able to run "magic_seccomp_sandbox gv somefile.ps".  And
> I want that to still work even if my distro gets fancy and uses
> selinux policy to prevent gv from accessing the network (which would
> not be an unreasonable thing to do).  The chromium project already has
> a little seccomp wrapper that can (sort of) do this.

Chrome also comes with NaCL.  They *are* "running random binaries",
but they use code analysis to check the code first.  Maybe because
seccomp wasn't good enough ;-)

> > So I really think that the *only* valid model is the "fail the execve
> > on any changes", not the "mnt-nosuid" approach of trying to run things
> > with the wrong permissions and get perhaps odd results. And I think it
> > works even - and perhaps *especially* - in models like selinux or
> > apparmor that do have a lot of "implicit MAC knowledge" on specific
> > binaries.
> 
> Is the current exec_no_trans check enough for you?  With my patch,
> selinux can already block the execve if it wants.  (The policy is the
> same as it would be if a program explicitly asked to run the new
> executable with an unchanged security context.)  I'd be happy to fail
> the exec in AppArmor, and then maybe AppArmor will change its mind
> if/when users get annoyed :)

Does SELinux block if userspace does exec entirely in userspace using
mmap() and not execve()?  If not, why is execve() allowed to be different?

-- Jamie

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

[Eric W. Biederman]

Andy Lutomirski <luto@amacapital.net> writes:

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

Foolish implementation question.

Is there a reason why we are putting this in a new field in the task
struct instead of adding a new flag in securebits in struct cred?

It seems to me like putting this information on the task_struct instead
of struct cred will tend to make this frozen state transition less
discoverable for future developers.

Also I would expect if you were applying this policy that you would
to ensure that task->read_cred == task->cred.

Do we want to disable setuid() and it's friends as well?  Certainly
freezing everything at exec is good I'm just wondering if handling
the handful of other cases where we explicitly change the permissions
on a process might be interesting.

My gut says you are trying to implement SECURE_NO_CHANGEPRIVS.

Eric

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

[Andy Lutomirski]

On Tue, Jan 17, 2012 at 3:57 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Andy Lutomirski <luto@amacapital.net> writes:
>
>> 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.
>
> Foolish implementation question.
>
> Is there a reason why we are putting this in a new field in the task
> struct instead of adding a new flag in securebits in struct cred?

Linus said:

I think it almost has to be task state, since we very much want to
make sure it's trivial to see that nothing ever clears that bit, and
that it always gets copied right over a fork/exec/whatever.

Putting it in some cred or capability bit or somethin would make that
kind of transparency pretty much totally impossible.


I personally don't care that much.

>
> It seems to me like putting this information on the task_struct instead
> of struct cred will tend to make this frozen state transition less
> discoverable for future developers.
>
> Also I would expect if you were applying this policy that you would
> to ensure that task->read_cred == task->cred.

What sets cred to something other than real_cred.

>
> Do we want to disable setuid() and it's friends as well?  Certainly
> freezing everything at exec is good I'm just wondering if handling
> the handful of other cases where we explicitly change the permissions
> on a process might be interesting.

I still think we shouldn't.  If you want to disable setuid, just turn
off CAP_SETUID.  The ability to setuid (the syscall, not the mode bit)
doesn't make things like seccomp and chroot more dangerous.  And I
need setuid if I want to implement my hypothetical pam_no_new_privs.

--Andy