[PATCH 1/2] groups: Factor out a function to set a pre-sorted group list

[PATCH 1/2] groups: Factor out a function to set a pre-sorted group list

[Josh Triplett]

This way, functions that already need to sort the group list need not do
so twice.

The new set_groups_sorted is intentionally not exported.

Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
 kernel/groups.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/kernel/groups.c b/kernel/groups.c
index 451698f..f0667e7 100644
--- a/kernel/groups.c
+++ b/kernel/groups.c
@@ -154,16 +154,26 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
 }
 
 /**
+ * set_groups_sorted - Change a group subscription in a set of credentials
+ * @new: The newly prepared set of credentials to alter
+ * @group_info: The group list to install; must be sorted
+ */
+static void set_groups_sorted(struct cred *new, struct group_info *group_info)
+{
+	put_group_info(new->group_info);
+	get_group_info(group_info);
+	new->group_info = group_info;
+}
+
+/**
  * set_groups - Change a group subscription in a set of credentials
  * @new: The newly prepared set of credentials to alter
  * @group_info: The group list to install
  */
 void set_groups(struct cred *new, struct group_info *group_info)
 {
-	put_group_info(new->group_info);
 	groups_sort(group_info);
-	get_group_info(group_info);
-	new->group_info = group_info;
+	set_groups_sorted(new, group_info);
 }
 
 EXPORT_SYMBOL(set_groups);
-- 
2.1.3

[PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Josh Triplett]

Currently, unprivileged processes (without CAP_SETGID) cannot call
setgroups at all.  In particular, processes with a set of supplementary
groups cannot further drop permissions without obtaining elevated
permissions first.

Allow unprivileged processes to call setgroups with a subset of their
current groups; only require CAP_SETGID to add a group the process does
not currently have.

The kernel already maintains the list of supplementary group IDs in
sorted order, and setgroups already needs to sort the new list, so this
just requires a linear comparison of the two sorted lists.

This moves the CAP_SETGID test from setgroups into set_current_groups.

Tested via the following test program:

#include <err.h>
#include <grp.h>
#include <stdio.h>
#include <sys/types.h>
#include <unistd.h>

void run_id(void)
{
    pid_t p = fork();
    switch (p) {
        case -1:
            err(1, "fork");
        case 0:
            execl("/usr/bin/id", "id", NULL);
            err(1, "exec");
        default:
            if (waitpid(p, NULL, 0) < 0)
                err(1, "waitpid");
    }
}

int main(void)
{
    gid_t list1[] = { 1, 2, 3, 4, 5 };
    gid_t list2[] = { 2, 3, 4 };
    run_id();
    if (setgroups(5, list1) < 0)
        err(1, "setgroups 1");
    run_id();
    if (setresgid(1, 1, 1) < 0)
        err(1, "setresgid");
    if (setresuid(1, 1, 1) < 0)
        err(1, "setresuid");
    run_id();
    if (setgroups(3, list2) < 0)
        err(1, "setgroups 2");
    run_id();
    if (setgroups(5, list1) < 0)
        err(1, "setgroups 3");
    run_id();

    return 0;
}

Without this patch, the test program gets EPERM from the second
setgroups call, after dropping root privileges.  With this patch, the
test program successfully drops groups 1 and 5, but then gets EPERM from
the third setgroups call, since that call attempts to add groups the
process does not currently have.

Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
 kernel/groups.c | 33 ++++++++++++++++++++++++++++++---
 kernel/uid16.c  |  2 --
 2 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/kernel/groups.c b/kernel/groups.c
index f0667e7..fe7367d 100644
--- a/kernel/groups.c
+++ b/kernel/groups.c
@@ -153,6 +153,29 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
 	return 0;
 }
 
+/* Compare two sorted group lists; return true if the first is a subset of the
+ * second.
+ */
+static bool is_subset(const struct group_info *g1, const struct group_info *g2)
+{
+	unsigned int i, j;
+
+	for (i = 0, j = 0; i < g1->ngroups; i++) {
+		kgid_t gid1 = GROUP_AT(g1, i);
+		kgid_t gid2;
+		for (; j < g2->ngroups; j++) {
+			gid2 = GROUP_AT(g2, j);
+			if (gid_lte(gid1, gid2))
+				break;
+		}
+		if (j >= g2->ngroups || !gid_eq(gid1, gid2))
+			return false;
+		j++;
+	}
+
+	return true;
+}
+
 /**
  * set_groups_sorted - Change a group subscription in a set of credentials
  * @new: The newly prepared set of credentials to alter
@@ -189,11 +212,17 @@ int set_current_groups(struct group_info *group_info)
 {
 	struct cred *new;
 
+	groups_sort(group_info);
 	new = prepare_creds();
 	if (!new)
 		return -ENOMEM;
+	if (!ns_capable(current_user_ns(), CAP_SETGID)
+	    && !is_subset(group_info, new->group_info)) {
+		abort_creds(new);
+		return -EPERM;
+	}
 
-	set_groups(new, group_info);
+	set_groups_sorted(new, group_info);
 	return commit_creds(new);
 }
 
@@ -233,8 +262,6 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)
 	struct group_info *group_info;
 	int retval;
 
-	if (!ns_capable(current_user_ns(), CAP_SETGID))
-		return -EPERM;
 	if ((unsigned)gidsetsize > NGROUPS_MAX)
 		return -EINVAL;
 
diff --git a/kernel/uid16.c b/kernel/uid16.c
index 602e5bb..b27e167 100644
--- a/kernel/uid16.c
+++ b/kernel/uid16.c
@@ -176,8 +176,6 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist)
 	struct group_info *group_info;
 	int retval;
 
-	if (!ns_capable(current_user_ns(), CAP_SETGID))
-		return -EPERM;
 	if ((unsigned)gidsetsize > NGROUPS_MAX)
 		return -EINVAL;
 
-- 
2.1.3

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Eric W. Biederman]

Josh Triplett <josh@joshtriplett.org> writes:

> Currently, unprivileged processes (without CAP_SETGID) cannot call
> setgroups at all.  In particular, processes with a set of supplementary
> groups cannot further drop permissions without obtaining elevated
> permissions first.
>
> Allow unprivileged processes to call setgroups with a subset of their
> current groups; only require CAP_SETGID to add a group the process does
> not currently have.

A couple of questions.
- Is there precedence in other unix flavors for this?
- What motiviates this change?
- Have you looked to see if anything might for bug compatibilty
  require applications not to be able to drop supplementary groups?

> The kernel already maintains the list of supplementary group IDs in
> sorted order, and setgroups already needs to sort the new list, so this
> just requires a linear comparison of the two sorted lists.
>
> This moves the CAP_SETGID test from setgroups into set_current_groups.
>
> Tested via the following test program:
>
> #include <err.h>
> #include <grp.h>
> #include <stdio.h>
> #include <sys/types.h>
> #include <unistd.h>
>
> void run_id(void)
> {
>     pid_t p = fork();
>     switch (p) {
>         case -1:
>             err(1, "fork");
>         case 0:
>             execl("/usr/bin/id", "id", NULL);
>             err(1, "exec");
>         default:
>             if (waitpid(p, NULL, 0) < 0)
>                 err(1, "waitpid");
>     }
> }
>
> int main(void)
> {
>     gid_t list1[] = { 1, 2, 3, 4, 5 };
>     gid_t list2[] = { 2, 3, 4 };
>     run_id();
>     if (setgroups(5, list1) < 0)
>         err(1, "setgroups 1");
>     run_id();
>     if (setresgid(1, 1, 1) < 0)
>         err(1, "setresgid");
>     if (setresuid(1, 1, 1) < 0)
>         err(1, "setresuid");
>     run_id();
>     if (setgroups(3, list2) < 0)
>         err(1, "setgroups 2");
>     run_id();
>     if (setgroups(5, list1) < 0)
>         err(1, "setgroups 3");
>     run_id();
>
>     return 0;
> }
>
> Without this patch, the test program gets EPERM from the second
> setgroups call, after dropping root privileges.  With this patch, the
> test program successfully drops groups 1 and 5, but then gets EPERM from
> the third setgroups call, since that call attempts to add groups the
> process does not currently have.
>
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> ---
>  kernel/groups.c | 33 ++++++++++++++++++++++++++++++---
>  kernel/uid16.c  |  2 --
>  2 files changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/groups.c b/kernel/groups.c
> index f0667e7..fe7367d 100644
> --- a/kernel/groups.c
> +++ b/kernel/groups.c
> @@ -153,6 +153,29 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
>  	return 0;
>  }
>  
> +/* Compare two sorted group lists; return true if the first is a subset of the
> + * second.
> + */
> +static bool is_subset(const struct group_info *g1, const struct group_info *g2)
> +{
> +	unsigned int i, j;
> +
> +	for (i = 0, j = 0; i < g1->ngroups; i++) {
> +		kgid_t gid1 = GROUP_AT(g1, i);
> +		kgid_t gid2;
> +		for (; j < g2->ngroups; j++) {
> +			gid2 = GROUP_AT(g2, j);
> +			if (gid_lte(gid1, gid2))
> +				break;
> +		}
> +		if (j >= g2->ngroups || !gid_eq(gid1, gid2))
> +			return false;
> +		j++;
> +	}
> +
> +	return true;
> +}
> +
>  /**
>   * set_groups_sorted - Change a group subscription in a set of credentials
>   * @new: The newly prepared set of credentials to alter
> @@ -189,11 +212,17 @@ int set_current_groups(struct group_info *group_info)
>  {
>  	struct cred *new;
>  
> +	groups_sort(group_info);
>  	new = prepare_creds();
>  	if (!new)
>  		return -ENOMEM;
> +	if (!ns_capable(current_user_ns(), CAP_SETGID)
> +	    && !is_subset(group_info, new->group_info)) {
> +		abort_creds(new);
> +		return -EPERM;
> +	}
>  
> -	set_groups(new, group_info);
> +	set_groups_sorted(new, group_info);
>  	return commit_creds(new);
>  }
>  
> @@ -233,8 +262,6 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)
>  	struct group_info *group_info;
>  	int retval;
>  
> -	if (!ns_capable(current_user_ns(), CAP_SETGID))
> -		return -EPERM;
>  	if ((unsigned)gidsetsize > NGROUPS_MAX)
>  		return -EINVAL;
>  
> diff --git a/kernel/uid16.c b/kernel/uid16.c
> index 602e5bb..b27e167 100644
> --- a/kernel/uid16.c
> +++ b/kernel/uid16.c
> @@ -176,8 +176,6 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist)
>  	struct group_info *group_info;
>  	int retval;
>  
> -	if (!ns_capable(current_user_ns(), CAP_SETGID))
> -		return -EPERM;
>  	if ((unsigned)gidsetsize > NGROUPS_MAX)
>  		return -EINVAL;

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Josh Triplett]

On Sat, Nov 15, 2014 at 09:37:27AM -0600, Eric W. Biederman wrote:
> Josh Triplett <josh@joshtriplett.org> writes:
> 
> > Currently, unprivileged processes (without CAP_SETGID) cannot call
> > setgroups at all.  In particular, processes with a set of supplementary
> > groups cannot further drop permissions without obtaining elevated
> > permissions first.
> >
> > Allow unprivileged processes to call setgroups with a subset of their
> > current groups; only require CAP_SETGID to add a group the process does
> > not currently have.
> 
> A couple of questions.
> - Is there precedence in other unix flavors for this?

I found a few references to now-nonexistent pages at MIT about a system
with this property, but other than that no.

I've also found more than a few references to people wanting this
functionality.

> - What motiviates this change?

I have a series of patches planned to add more ways to drop elevated
privileges without requiring a transition through root to do so.  That
would improve the ability for unprivileged users to run programs
sandboxed with even *less* privileges.  (Among other things, that would
also allow programs running with no_new_privs to further *reduce* their
privileges, which they can't currently do in this case.)

> - Have you looked to see if anything might for bug compatibilty
>   require applications not to be able to drop supplementary groups?

I haven't found any such case; that doesn't mean such a case does not
exist.  Feedback welcome.

The only case I can think of (and I don't know of any examples of such a
system): some kind of quota system that limits the members of a group to
a certain amount of storage, but places no such limit on non-members.

However, the idea of *holding* a credential (a supplementary group ID)
giving *less* privileges, and *dropping* a credential giving *more*
privileges, would completely invert normal security models.  (The sane
way to design such a system would be to have a privileged group for
"users who can exceed the quota".)

If it turns out that a real case exists that people care about, I could
easily make this configurable, either at compile time or via a sysctl.

- Josh Triplett

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Andy Lutomirski]

On Sat, Nov 15, 2014 at 11:29 AM, Josh Triplett <josh@joshtriplett.org> wrote:
> On Sat, Nov 15, 2014 at 09:37:27AM -0600, Eric W. Biederman wrote:
>> Josh Triplett <josh@joshtriplett.org> writes:
>>
>> > Currently, unprivileged processes (without CAP_SETGID) cannot call
>> > setgroups at all.  In particular, processes with a set of supplementary
>> > groups cannot further drop permissions without obtaining elevated
>> > permissions first.
>> >
>> > Allow unprivileged processes to call setgroups with a subset of their
>> > current groups; only require CAP_SETGID to add a group the process does
>> > not currently have.
>>
>> A couple of questions.
>> - Is there precedence in other unix flavors for this?
>
> I found a few references to now-nonexistent pages at MIT about a system
> with this property, but other than that no.
>
> I've also found more than a few references to people wanting this
> functionality.
>
>> - What motiviates this change?
>
> I have a series of patches planned to add more ways to drop elevated
> privileges without requiring a transition through root to do so.  That
> would improve the ability for unprivileged users to run programs
> sandboxed with even *less* privileges.  (Among other things, that would
> also allow programs running with no_new_privs to further *reduce* their
> privileges, which they can't currently do in this case.)
>
>> - Have you looked to see if anything might for bug compatibilty
>>   require applications not to be able to drop supplementary groups?
>
> I haven't found any such case; that doesn't mean such a case does not
> exist.  Feedback welcome.
>
> The only case I can think of (and I don't know of any examples of such a
> system): some kind of quota system that limits the members of a group to
> a certain amount of storage, but places no such limit on non-members.
>
> However, the idea of *holding* a credential (a supplementary group ID)
> giving *less* privileges, and *dropping* a credential giving *more*
> privileges, would completely invert normal security models.  (The sane
> way to design such a system would be to have a privileged group for
> "users who can exceed the quota".)

Agreed.  And, if you want to bypass quotas by dropping a supplementary
group, you already can by unsharing your user namespace.

However, sudoers seems to allow negative group matches.  So maybe
allowing this only with no_new_privs already set would make sense.

--Andy

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Josh Triplett]

On Sat, Nov 15, 2014 at 12:06:20PM -0800, Andy Lutomirski wrote:
> On Sat, Nov 15, 2014 at 11:29 AM, Josh Triplett <josh@joshtriplett.org> wrote:
> > On Sat, Nov 15, 2014 at 09:37:27AM -0600, Eric W. Biederman wrote:
> >> Josh Triplett <josh@joshtriplett.org> writes:
> >>
> >> > Currently, unprivileged processes (without CAP_SETGID) cannot call
> >> > setgroups at all.  In particular, processes with a set of supplementary
> >> > groups cannot further drop permissions without obtaining elevated
> >> > permissions first.
> >> >
> >> > Allow unprivileged processes to call setgroups with a subset of their
> >> > current groups; only require CAP_SETGID to add a group the process does
> >> > not currently have.
> >>
> >> A couple of questions.
> >> - Is there precedence in other unix flavors for this?
> >
> > I found a few references to now-nonexistent pages at MIT about a system
> > with this property, but other than that no.
> >
> > I've also found more than a few references to people wanting this
> > functionality.
> >
> >> - What motiviates this change?
> >
> > I have a series of patches planned to add more ways to drop elevated
> > privileges without requiring a transition through root to do so.  That
> > would improve the ability for unprivileged users to run programs
> > sandboxed with even *less* privileges.  (Among other things, that would
> > also allow programs running with no_new_privs to further *reduce* their
> > privileges, which they can't currently do in this case.)
> >
> >> - Have you looked to see if anything might for bug compatibilty
> >>   require applications not to be able to drop supplementary groups?
> >
> > I haven't found any such case; that doesn't mean such a case does not
> > exist.  Feedback welcome.
> >
> > The only case I can think of (and I don't know of any examples of such a
> > system): some kind of quota system that limits the members of a group to
> > a certain amount of storage, but places no such limit on non-members.
> >
> > However, the idea of *holding* a credential (a supplementary group ID)
> > giving *less* privileges, and *dropping* a credential giving *more*
> > privileges, would completely invert normal security models.  (The sane
> > way to design such a system would be to have a privileged group for
> > "users who can exceed the quota".)
> 
> Agreed.  And, if you want to bypass quotas by dropping a supplementary
> group, you already can by unsharing your user namespace.

Good point!  Given that a process can run with a new user namespace and
no other namespaces, and then drop all its other privileges that way,
the ability to drop privileges without using a user namespace seems
completely harmless, with one exception that you noted:

> However, sudoers seems to allow negative group matches.  So maybe
> allowing this only with no_new_privs already set would make sense.

Sigh, bad sudo.  Sure, restricting this to no_new_privs only seems fine.
I'll do that in v2, and document that in the manpage.

- Josh Triplett

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Theodore Ts'o]

On Sat, Nov 15, 2014 at 12:20:42PM -0800, Josh Triplett wrote:
> > However, sudoers seems to allow negative group matches.  So maybe
> > allowing this only with no_new_privs already set would make sense.
> 
> Sigh, bad sudo.  Sure, restricting this to no_new_privs only seems fine.
> I'll do that in v2, and document that in the manpage.

I've also seen use cases (generally back in the bad old days of big
timesharing VAX 750's :-) where the system admin might assign someone
to the "games-abusers" group, and then set /usr/games to mode 705
root:games-abusers --- presumably because it's easier to add a few
people to the deny list rather than having to add all of the EECS
department to the games group minus the abusers.

So arbitrarily anyone to drop groups from their supplemental group
list will result in a change from both existing practice and legacy
Unix systems, and it could potentially lead to a security exposure.

Cheers,

						- Ted

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Josh Triplett]

On November 15, 2014 6:05:11 PM PST, Theodore Ts'o <tytso@mit.edu> wrote:
>On Sat, Nov 15, 2014 at 12:20:42PM -0800, Josh Triplett wrote:
>> > However, sudoers seems to allow negative group matches.  So maybe
>> > allowing this only with no_new_privs already set would make sense.
>> 
>> Sigh, bad sudo.  Sure, restricting this to no_new_privs only seems
>fine.
>> I'll do that in v2, and document that in the manpage.
>
>I've also seen use cases (generally back in the bad old days of big
>timesharing VAX 750's :-) where the system admin might assign someone
>to the "games-abusers" group, and then set /usr/games to mode 705
>root:games-abusers --- presumably because it's easier to add a few
>people to the deny list rather than having to add all of the EECS
>department to the games group minus the abusers.
>
>So arbitrarily anyone to drop groups from their supplemental group
>list will result in a change from both existing practice and legacy
>Unix systems, and it could potentially lead to a security exposure.

As Andy pointed out, you can already do that with a user namespace, for any case not involving a setuid or setgid (or otherwise privilege-gaining) program.  And requiring no_new_privs handles that.

Given the combination of those two things, do you still see any problematic cases?

- Josh Triplett

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Eric W. Biederman]

Josh Triplett <josh@joshtriplett.org> writes:

> On November 15, 2014 6:05:11 PM PST, Theodore Ts'o <tytso@mit.edu> wrote:
>>On Sat, Nov 15, 2014 at 12:20:42PM -0800, Josh Triplett wrote:
>>> > However, sudoers seems to allow negative group matches.  So maybe
>>> > allowing this only with no_new_privs already set would make sense.
>>> 
>>> Sigh, bad sudo.  Sure, restricting this to no_new_privs only seems
>>fine.
>>> I'll do that in v2, and document that in the manpage.
>>
>>I've also seen use cases (generally back in the bad old days of big
>>timesharing VAX 750's :-) where the system admin might assign someone
>>to the "games-abusers" group, and then set /usr/games to mode 705
>>root:games-abusers --- presumably because it's easier to add a few
>>people to the deny list rather than having to add all of the EECS
>>department to the games group minus the abusers.
>>
>>So arbitrarily anyone to drop groups from their supplemental group
>>list will result in a change from both existing practice and legacy
>>Unix systems, and it could potentially lead to a security exposure.
>
> As Andy pointed out, you can already do that with a user namespace,

That may be a bug with the user namespace permission check.  Perhaps we
shouldn't allow dropping groups that aren't mapped in the user
namespace.

To add to the discussion there are also sg and newgroup, though I don't
think they touch supplementary groups.

> for any case not involving a setuid or setgid (or otherwise
> privilege-gaining) program.  And requiring no_new_privs handles that.

Frankly adding a no_new_privs check to allow something/anything scares
me.  That converts no_new_privs from something simple and easy to
understand to something much more complicated.

> Given the combination of those two things, do you still see any
> problematic cases?

Josh I think it is time to stop and make certain you understand how
unix groups work in the large.

It seems to me that either supplementary groups can be dropped or
supplementary groups can not be dropped.  If they can be dropped we
shouldn't need complicated checks to allow them to be dropped in some
circumstances but not others.

Unix groups are an old interface and they have been used in a lot of
ways over the years.  We need to be careful any change we make is good
and truly backwards compatible and that we do our darndest not to
introduce security holes.

Eric

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Josh Triplett]

On Sat, Nov 15, 2014 at 09:08:07PM -0600, Eric W. Biederman wrote:
> Josh Triplett <josh@joshtriplett.org> writes:
> > On November 15, 2014 6:05:11 PM PST, Theodore Ts'o <tytso@mit.edu> wrote:
> >>On Sat, Nov 15, 2014 at 12:20:42PM -0800, Josh Triplett wrote:
> >>> > However, sudoers seems to allow negative group matches.  So maybe
> >>> > allowing this only with no_new_privs already set would make sense.
> >>> 
> >>> Sigh, bad sudo.  Sure, restricting this to no_new_privs only seems
> >>fine.
> >>> I'll do that in v2, and document that in the manpage.
> >>
> >>I've also seen use cases (generally back in the bad old days of big
> >>timesharing VAX 750's :-) where the system admin might assign someone
> >>to the "games-abusers" group, and then set /usr/games to mode 705
> >>root:games-abusers --- presumably because it's easier to add a few
> >>people to the deny list rather than having to add all of the EECS
> >>department to the games group minus the abusers.
> >>
> >>So arbitrarily anyone to drop groups from their supplemental group
> >>list will result in a change from both existing practice and legacy
> >>Unix systems, and it could potentially lead to a security exposure.
> >
> > As Andy pointed out, you can already do that with a user namespace,
> 
> That may be a bug with the user namespace permission check.  Perhaps we
> shouldn't allow dropping groups that aren't mapped in the user
> namespace.

Changing that would break containers for users on many common Linux
distributions, which put users in various supplementary groups by
default.

I do actually have another patch planned, to allow users to set up gid
maps for groups they currently have permission for, not just for their
primary group.  But even with that change, breaking the ability for
container-root to use setgroups to drop all its supplementary groups
seems very wrong, and seems highly likely to break real-world software
running in containers.

> To add to the discussion there are also sg and newgroup, though I don't
> think they touch supplementary groups.

Both sg and newgrp do prove a different point, though: an unprivileged
user should be able to change their gid to one of their supplementary
groups.  That's another patch I planned to submit after this one:
allow setgid/setresgid/etc to a supplementary group ID.

> > for any case not involving a setuid or setgid (or otherwise
> > privilege-gaining) program.  And requiring no_new_privs handles that.
> 
> Frankly adding a no_new_privs check to allow something/anything scares
> me.  That converts no_new_privs from something simple and easy to
> understand to something much more complicated.

no_new_privs already exists in large part to support such use cases,
such as setting seccomp filters, which would be dangerous for
privilege-escalating programs to inherit.

> > Given the combination of those two things, do you still see any
> > problematic cases?
> 
> Josh I think it is time to stop and make certain you understand how
> unix groups work in the large.
>
> It seems to me that either supplementary groups can be dropped or
> supplementary groups can not be dropped.  If they can be dropped we
> shouldn't need complicated checks to allow them to be dropped in some
> circumstances but not others.
> 
> Unix groups are an old interface and they have been used in a lot of
> ways over the years.  We need to be careful any change we make is good
> and truly backwards compatible and that we do our darndest not to
> introduce security holes.

Seeking the decades-old precedents and corner cases that might
contradict the most obvious semantics is a large part of what I'm
looking for out of this thread.

However, I don't necessarily think that we must permit unprivileged
setgroups in *all* cases if we permit it in *any*; it may well make
sense to reduce the surface area of the change by limiting the
circumstances in which you can do this.  Whether it does end up making
sense to do so depends on the outcome of this discussion.

I added the no_new_privs check at Andy's suggestion, since the use case
I had in mind will work just fine with that restriction.  I'm also open
to just allowing setgroups in all cases, if that semantic seems
preferable, or to adding some separate check specific to this case if
that really seems necessary.

- Josh Triplett

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Theodore Ts'o]

On Sat, Nov 15, 2014 at 09:08:07PM -0600, Eric W. Biederman wrote:
> 
> That may be a bug with the user namespace permission check.  Perhaps we
> shouldn't allow dropping groups that aren't mapped in the user
> namespace.

I'm not saying that we can't change the behavior of whether or not a
user can drop a group permission.  I'm just saying that we need to do
so consciously.  The setgroups()/getgroups() ABI isn't part of
POSIX/SuSv3 so we wouldn't be breaking POSIX compatibility, for those
people who care about that.

The bigger deal is that it's very different from how BSD 4.x has
handled things, which means there is two decades of history that we're
looking at here.  And there are times when taking away permissions in
an expected fashion can cause security problems.  (As a silly example;
some architect at Digital wrote a spec that said that setuid must
return EINVAL for values greater than 32k --- back in the days when
uid's were a signed short.  The junior programmer who implemented this
for Ultrix made the check for 32,000 decimal.  Guess what happened
when /bin/login got a failure with setuid when it wasn't expecting one
--- since root could never get an error with that system call, right?
And MIT Project Athena started ran out of lower numbered uid's and
froshlings started getting assigned uid's > 32,000....)

In this particular case, the change is probably a little less likely
to cause serious problems, although the fact that sudo does allow
negative group assignments is an example of another potential
breakage.

OTOH, I'm aware of how this could cause major problems to the concept
of allowing an untrusted user to set up their own containers to
constrain what program with a possibly untrusted provinance might be
able to do.  I can see times when I might want to run in a container
where the user didn't have access to groups that I have access to by
default --- including groups such as disk, sudo, lpadmin, etc.

If we do want to make such a change, my suggestion is to keep things
*very* simple.  Let it be a boot-time option whether or not users are
allowed to drop group permissions, and let it affect all possible ways
that users can drop groups.  And we can create a shell script that
will search for the obvious ways that a user could get screwed by
enabling this, which we can encourage distributions to package up for
their end users.  And then we document the heck out of the fact that
this option exists, and when/if we want to make it the default, so
it's perfectly clear and transparent to all what is happening.

One of the things that scare me about the addition of the forced
capability "setuid" binary was that the feature was just silently slid
in, and we didn't do a good job reaching out to the tripwire and
rootkit and other such programs out there, such that several years
after we enabled capability support, most sysadmins and security
scanning programs are still apparently obivious to the this very
convenient feature that we gave to rootkit and malware authors.  Now
we can say that we're just adding new features, and we owe no debt
other parts of the ecosystem --- this is the attitude used by upower
and other freedesktop.org components when they made incompatible
changes made available by new features provided by systemd[1].

[1] http://m.memegen.com/u7o1tk.jpg

But as kernel developers, who pride ourselves on not breaking
userspace, I think we should try for a higher standard than this.  And
perhaps this change with allowing groups to be dropped --- which is
admittedly a useful thing to be able to allow --- is a good place to
start trying to model a better way of doing things.  We should try to
be responsible about how we add new features, and think of all of the
potential downstream consequences to the _all_ of the ecosystem.

	  	     		     	 - Ted

P.S.  And we really should try reaching out to the security scanners
about capabilities, too....

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Andy Lutomirski]

On Sun, Nov 16, 2014 at 5:32 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Sat, Nov 15, 2014 at 09:08:07PM -0600, Eric W. Biederman wrote:
>>
>> That may be a bug with the user namespace permission check.  Perhaps we
>> shouldn't allow dropping groups that aren't mapped in the user
>> namespace.
>
> I'm not saying that we can't change the behavior of whether or not a
> user can drop a group permission.  I'm just saying that we need to do
> so consciously.  The setgroups()/getgroups() ABI isn't part of
> POSIX/SuSv3 so we wouldn't be breaking POSIX compatibility, for those
> people who care about that.

It may make sense to reach out to some place like oss-security.

FWIW, I think we should ask, at the same time, about:

 - Dropping supplementary groups.
 - Switching gid/egid/sgid to a supplementary group.
 - Denying ptrace of a process with supplementary groups that the
tracer doesn't have.

Also, I much prefer a sysctl to a boot option.  Boot options are nasty
to configure in many distributions.

--Andy

>
> The bigger deal is that it's very different from how BSD 4.x has
> handled things, which means there is two decades of history that we're
> looking at here.  And there are times when taking away permissions in
> an expected fashion can cause security problems.  (As a silly example;
> some architect at Digital wrote a spec that said that setuid must
> return EINVAL for values greater than 32k --- back in the days when
> uid's were a signed short.  The junior programmer who implemented this
> for Ultrix made the check for 32,000 decimal.  Guess what happened
> when /bin/login got a failure with setuid when it wasn't expecting one
> --- since root could never get an error with that system call, right?
> And MIT Project Athena started ran out of lower numbered uid's and
> froshlings started getting assigned uid's > 32,000....)
>
> In this particular case, the change is probably a little less likely
> to cause serious problems, although the fact that sudo does allow
> negative group assignments is an example of another potential
> breakage.
>
> OTOH, I'm aware of how this could cause major problems to the concept
> of allowing an untrusted user to set up their own containers to
> constrain what program with a possibly untrusted provinance might be
> able to do.  I can see times when I might want to run in a container
> where the user didn't have access to groups that I have access to by
> default --- including groups such as disk, sudo, lpadmin, etc.
>
> If we do want to make such a change, my suggestion is to keep things
> *very* simple.  Let it be a boot-time option whether or not users are
> allowed to drop group permissions, and let it affect all possible ways
> that users can drop groups.  And we can create a shell script that
> will search for the obvious ways that a user could get screwed by
> enabling this, which we can encourage distributions to package up for
> their end users.  And then we document the heck out of the fact that
> this option exists, and when/if we want to make it the default, so
> it's perfectly clear and transparent to all what is happening.
>
> One of the things that scare me about the addition of the forced
> capability "setuid" binary was that the feature was just silently slid
> in, and we didn't do a good job reaching out to the tripwire and
> rootkit and other such programs out there, such that several years
> after we enabled capability support, most sysadmins and security
> scanning programs are still apparently obivious to the this very
> convenient feature that we gave to rootkit and malware authors.  Now
> we can say that we're just adding new features, and we owe no debt
> other parts of the ecosystem --- this is the attitude used by upower
> and other freedesktop.org components when they made incompatible
> changes made available by new features provided by systemd[1].
>
> [1] http://m.memegen.com/u7o1tk.jpg
>
> But as kernel developers, who pride ourselves on not breaking
> userspace, I think we should try for a higher standard than this.  And
> perhaps this change with allowing groups to be dropped --- which is
> admittedly a useful thing to be able to allow --- is a good place to
> start trying to model a better way of doing things.  We should try to
> be responsible about how we add new features, and think of all of the
> potential downstream consequences to the _all_ of the ecosystem.
>
>                                          - Ted
>
> P.S.  And we really should try reaching out to the security scanners
> about capabilities, too....



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Josh Triplett]

On Sun, Nov 16, 2014 at 07:42:30AM -0800, Andy Lutomirski wrote:
> On Sun, Nov 16, 2014 at 5:32 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> > On Sat, Nov 15, 2014 at 09:08:07PM -0600, Eric W. Biederman wrote:
> >>
> >> That may be a bug with the user namespace permission check.  Perhaps we
> >> shouldn't allow dropping groups that aren't mapped in the user
> >> namespace.
> >
> > I'm not saying that we can't change the behavior of whether or not a
> > user can drop a group permission.  I'm just saying that we need to do
> > so consciously.  The setgroups()/getgroups() ABI isn't part of
> > POSIX/SuSv3 so we wouldn't be breaking POSIX compatibility, for those
> > people who care about that.
> 
> It may make sense to reach out to some place like oss-security.
> 
> FWIW, I think we should ask, at the same time, about:
> 
>  - Dropping supplementary groups.
>  - Switching gid/egid/sgid to a supplementary group.
>  - Denying ptrace of a process with supplementary groups that the
> tracer doesn't have.

I wonder how crazy it would be to just require either CAP_SYS_PTRACE or
cred1 == cred2 (as in, you have *exactly* the same credentials as the
target)?

> Also, I much prefer a sysctl to a boot option.  Boot options are nasty
> to configure in many distributions.

Agreed.

- Josh Triplett

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Josh Triplett]

On Sun, Nov 16, 2014 at 08:32:30AM -0500, Theodore Ts'o wrote:
> On Sat, Nov 15, 2014 at 09:08:07PM -0600, Eric W. Biederman wrote:
> > That may be a bug with the user namespace permission check.  Perhaps we
> > shouldn't allow dropping groups that aren't mapped in the user
> > namespace.
> 
> I'm not saying that we can't change the behavior of whether or not a
> user can drop a group permission.  I'm just saying that we need to do
> so consciously.

Agreed.

> The setgroups()/getgroups() ABI isn't part of
> POSIX/SuSv3 so we wouldn't be breaking POSIX compatibility, for those
> people who care about that.

POSIX.1-2001 actually specifies getgroups, but not setgroups.  In any
case, yes, POSIX doesn't say anything about this behavior.

> The bigger deal is that it's very different from how BSD 4.x has
> handled things, which means there is two decades of history that we're
> looking at here.  And there are times when taking away permissions in
> an expected fashion can cause security problems.  (As a silly example;
> some architect at Digital wrote a spec that said that setuid must
> return EINVAL for values greater than 32k --- back in the days when
> uid's were a signed short.  The junior programmer who implemented this
> for Ultrix made the check for 32,000 decimal.  Guess what happened
> when /bin/login got a failure with setuid when it wasn't expecting one
> --- since root could never get an error with that system call, right?

Ignored it and kept going, starting the user's shell as root?

I'd guess that a similar story motivated the note in the Linux manpages
for setuid, setresuid, and similar, saying "Note: there are cases where
setuid() can fail even when the caller is UID 0; it is a grave security
error to omit checking for a failure return from setuid().".

(Also, these days, glibc marks setuid and similar with the
warn_unused_result attribute.)

> And MIT Project Athena started ran out of lower numbered uid's and
> froshlings started getting assigned uid's > 32,000....)
> 
> In this particular case, the change is probably a little less likely
> to cause serious problems, although the fact that sudo does allow
> negative group assignments is an example of another potential
> breakage.
> 
> OTOH, I'm aware of how this could cause major problems to the concept
> of allowing an untrusted user to set up their own containers to
> constrain what program with a possibly untrusted provinance might be
> able to do.  I can see times when I might want to run in a container
> where the user didn't have access to groups that I have access to by
> default --- including groups such as disk, sudo, lpadmin, etc.
> 
> If we do want to make such a change, my suggestion is to keep things
> *very* simple.  Let it be a boot-time option whether or not users are
> allowed to drop group permissions, and let it affect all possible ways
> that users can drop groups.  And we can create a shell script that
> will search for the obvious ways that a user could get screwed by
> enabling this, which we can encourage distributions to package up for
> their end users.  And then we document the heck out of the fact that
> this option exists, and when/if we want to make it the default, so
> it's perfectly clear and transparent to all what is happening.

An option sounds sensible to me.  I think a sysctl makes more sense,
though.  I'll add one in v4.

What did you have in mind about the shell script? Something like:
grep -r !% /etc/sudoers /etc/sudoers.d
?

- Josh Triplett

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Theodore Ts'o]

On Sat, Nov 15, 2014 at 06:35:05PM -0800, Josh Triplett wrote:
> >So arbitrarily anyone to drop groups from their supplemental group
> >list will result in a change from both existing practice and legacy
> >Unix systems, and it could potentially lead to a security exposure.
> 
> As Andy pointed out, you can already do that with a user namespace,
> for any case not involving a setuid or setgid (or otherwise
> privilege-gaining) program.  And requiring no_new_privs handles
> that.

Well, it's no worse than what we can do already with the user
namespace, yes.  I'm still worried it's going to come as a surprise
for some configurations because it's a change from what was allowed
historically.  Then again, pretty much all of the tripwire and rootkit
scanners won't notice a "setuid" program that uses capabilities
instead of the traditional setuid bit, and most sysadmins won't think
to check for an executable with a forced capability mask, so this
isn't exactly a new problem....

							- Ted
							

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Josh Triplett]

On Sat, Nov 15, 2014 at 10:40:06PM -0500, Theodore Ts'o wrote:
> On Sat, Nov 15, 2014 at 06:35:05PM -0800, Josh Triplett wrote:
> > >So arbitrarily anyone to drop groups from their supplemental group
> > >list will result in a change from both existing practice and legacy
> > >Unix systems, and it could potentially lead to a security exposure.
> > 
> > As Andy pointed out, you can already do that with a user namespace,
> > for any case not involving a setuid or setgid (or otherwise
> > privilege-gaining) program.  And requiring no_new_privs handles
> > that.
> 
> Well, it's no worse than what we can do already with the user
> namespace, yes.  I'm still worried it's going to come as a surprise
> for some configurations because it's a change from what was allowed
> historically.  Then again, pretty much all of the tripwire and rootkit
> scanners won't notice a "setuid" program that uses capabilities
> instead of the traditional setuid bit, and most sysadmins won't think
> to check for an executable with a forced capability mask, so this
> isn't exactly a new problem....

We certainly have introduced orthogonal APIs in various areas before,
such that applications written prior to those APIs may interact
interestingly with them; we don't allow *breaking* those applications,
or introducing security holes, but the existence of applications
designed to block one particular way to do something doesn't
*automatically* rule out the possibility of adding another way to do it.
It does require some careful thought, though.

When we introduced seccomp filters for syscalls, we tied them to
no_new_privs to prevent any possible security holes caused by selective
syscall denial/filtration.

In this case, I'm indifferent about whether unprivileged setgroups works
without no_new_privs; if people are comfortable with that, fine, and if
people would prefer no_new_privs (or for that matter a sysctl, a
compile-time option, or any other means of making the behavior
optional), I can do that too.  The security model of "having a group
gives you less privilege than not having it" seems crazy, but
nonetheless I can see a couple of easy ways that we can avoid breaking
that pattern, no_new_privs being one of them.  I'd like to make sure
that nobody sees any other real-world corner case that unprivileged
setgroups would break.

- Josh Triplett

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[One Thousand Gnomes]

> optional), I can do that too.  The security model of "having a group
> gives you less privilege than not having it" seems crazy, but
> nonetheless I can see a couple of easy ways that we can avoid breaking

It's an old pattern of use that makes complete sense in a traditional
Unix permission world because it's the only way to do "exclude {list}"
nicely. Our default IMHO shouldn't break this.

> that pattern, no_new_privs being one of them.  I'd like to make sure
> that nobody sees any other real-world corner case that unprivileged
> setgroups would break.

Barring the usual risk of people doing improper error checking I don't
see one immediately.

For containers I think it actually makes sense that the sysctl can be
applied per container anyway.

Alan

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Andy Lutomirski]

On Nov 17, 2014 3:37 AM, "One Thousand Gnomes"
<gnomes@lxorguk.ukuu.org.uk> wrote:
>
> > optional), I can do that too.  The security model of "having a group
> > gives you less privilege than not having it" seems crazy, but
> > nonetheless I can see a couple of easy ways that we can avoid breaking
>
> It's an old pattern of use that makes complete sense in a traditional
> Unix permission world because it's the only way to do "exclude {list}"
> nicely. Our default IMHO shouldn't break this.
>
> > that pattern, no_new_privs being one of them.  I'd like to make sure
> > that nobody sees any other real-world corner case that unprivileged
> > setgroups would break.
>
> Barring the usual risk of people doing improper error checking I don't
> see one immediately.
>
> For containers I think it actually makes sense that the sysctl can be
> applied per container anyway.

We'll probably need per container sysctls some day.

>
> Alan

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Eric W.Biederman]

On November 17, 2014 1:07:30 PM EST, Andy Lutomirski <luto@amacapital.net> wrote:
>On Nov 17, 2014 3:37 AM, "One Thousand Gnomes"
><gnomes@lxorguk.ukuu.org.uk> wrote:
>>
>> > optional), I can do that too.  The security model of "having a
>group
>> > gives you less privilege than not having it" seems crazy, but
>> > nonetheless I can see a couple of easy ways that we can avoid
>breaking
>>
>> It's an old pattern of use that makes complete sense in a traditional
>> Unix permission world because it's the only way to do "exclude
>{list}"
>> nicely. Our default IMHO shouldn't break this.
>>
>> > that pattern, no_new_privs being one of them.  I'd like to make
>sure
>> > that nobody sees any other real-world corner case that unprivileged
>> > setgroups would break.
>>
>> Barring the usual risk of people doing improper error checking I
>don't
>> see one immediately.
>>
>> For containers I think it actually makes sense that the sysctl can be
>> applied per container anyway.
>
>We'll probably need per container sysctls some day.

We already have a mess of per network namespace sysctls,
as well as few for other namespaces.

We have the infrastructure it is just a matter of using it for whatever purpose we need.

Eric

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Andy Lutomirski]

On Mon, Nov 17, 2014 at 2:11 PM, Eric W.Biederman <ebiederm@xmission.com> wrote:
>
>
> On November 17, 2014 1:07:30 PM EST, Andy Lutomirski <luto@amacapital.net> wrote:
>>On Nov 17, 2014 3:37 AM, "One Thousand Gnomes"
>><gnomes@lxorguk.ukuu.org.uk> wrote:
>>>
>>> > optional), I can do that too.  The security model of "having a
>>group
>>> > gives you less privilege than not having it" seems crazy, but
>>> > nonetheless I can see a couple of easy ways that we can avoid
>>breaking
>>>
>>> It's an old pattern of use that makes complete sense in a traditional
>>> Unix permission world because it's the only way to do "exclude
>>{list}"
>>> nicely. Our default IMHO shouldn't break this.
>>>
>>> > that pattern, no_new_privs being one of them.  I'd like to make
>>sure
>>> > that nobody sees any other real-world corner case that unprivileged
>>> > setgroups would break.
>>>
>>> Barring the usual risk of people doing improper error checking I
>>don't
>>> see one immediately.
>>>
>>> For containers I think it actually makes sense that the sysctl can be
>>> applied per container anyway.
>>
>>We'll probably need per container sysctls some day.
>
> We already have a mess of per network namespace sysctls,
> as well as few for other namespaces.
>
> We have the infrastructure it is just a matter of using it for whatever purpose we need.
>

A list of group id ranges that it's permissible to drop would do the
trick, both for setgroups and for unshare.  The downside would be that
users in those groups (i.e. everyone by default) would not be able to
unshare their user ns.

Better ideas welcome.

--Andy

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[josh]

On Mon, Nov 17, 2014 at 02:22:59PM -0800, Andy Lutomirski wrote:
> On Mon, Nov 17, 2014 at 2:11 PM, Eric W.Biederman <ebiederm@xmission.com> wrote:
> >
> >
> > On November 17, 2014 1:07:30 PM EST, Andy Lutomirski <luto@amacapital.net> wrote:
> >>On Nov 17, 2014 3:37 AM, "One Thousand Gnomes"
> >><gnomes@lxorguk.ukuu.org.uk> wrote:
> >>>
> >>> > optional), I can do that too.  The security model of "having a
> >>group
> >>> > gives you less privilege than not having it" seems crazy, but
> >>> > nonetheless I can see a couple of easy ways that we can avoid
> >>breaking
> >>>
> >>> It's an old pattern of use that makes complete sense in a traditional
> >>> Unix permission world because it's the only way to do "exclude
> >>{list}"
> >>> nicely. Our default IMHO shouldn't break this.
> >>>
> >>> > that pattern, no_new_privs being one of them.  I'd like to make
> >>sure
> >>> > that nobody sees any other real-world corner case that unprivileged
> >>> > setgroups would break.
> >>>
> >>> Barring the usual risk of people doing improper error checking I
> >>don't
> >>> see one immediately.
> >>>
> >>> For containers I think it actually makes sense that the sysctl can be
> >>> applied per container anyway.
> >>
> >>We'll probably need per container sysctls some day.
> >
> > We already have a mess of per network namespace sysctls,
> > as well as few for other namespaces.
> >
> > We have the infrastructure it is just a matter of using it for whatever purpose we need.
> >
> 
> A list of group id ranges that it's permissible to drop would do the
> trick, both for setgroups and for unshare.  The downside would be that
> users in those groups (i.e. everyone by default) would not be able to
> unshare their user ns.
> 
> Better ideas welcome.

Personally, I think that seems like more flexibility than necessary to
achieve the goal.  I think a sysctl turning group-dropping on and off
would suffice; systems that know they don't use groups to exclude
specific users can enable that sysctl.

- Josh Triplett

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Casey Schaufler]

On 11/17/2014 2:37 PM, josh@joshtriplett.org wrote:
> On Mon, Nov 17, 2014 at 02:22:59PM -0800, Andy Lutomirski wrote:
>> On Mon, Nov 17, 2014 at 2:11 PM, Eric W.Biederman <ebiederm@xmission.com> wrote:
>>>
>>> On November 17, 2014 1:07:30 PM EST, Andy Lutomirski <luto@amacapital.net> wrote:
>>>> On Nov 17, 2014 3:37 AM, "One Thousand Gnomes"
>>>> <gnomes@lxorguk.ukuu.org.uk> wrote:
>>>>>> optional), I can do that too.  The security model of "having a
>>>> group
>>>>>> gives you less privilege than not having it" seems crazy, but
>>>>>> nonetheless I can see a couple of easy ways that we can avoid
>>>> breaking
>>>>> It's an old pattern of use that makes complete sense in a traditional
>>>>> Unix permission world because it's the only way to do "exclude
>>>> {list}"
>>>>> nicely. Our default IMHO shouldn't break this.
>>>>>
>>>>>> that pattern, no_new_privs being one of them.  I'd like to make
>>>> sure
>>>>>> that nobody sees any other real-world corner case that unprivileged
>>>>>> setgroups would break.
>>>>> Barring the usual risk of people doing improper error checking I
>>>> don't
>>>>> see one immediately.
>>>>>
>>>>> For containers I think it actually makes sense that the sysctl can be
>>>>> applied per container anyway.
>>>> We'll probably need per container sysctls some day.
>>> We already have a mess of per network namespace sysctls,
>>> as well as few for other namespaces.
>>>
>>> We have the infrastructure it is just a matter of using it for whatever purpose we need.
>>>
>> A list of group id ranges that it's permissible to drop would do the
>> trick, both for setgroups and for unshare.  The downside would be that
>> users in those groups (i.e. everyone by default) would not be able to
>> unshare their user ns.
>>
>> Better ideas welcome.
> Personally, I think that seems like more flexibility than necessary to
> achieve the goal.  I think a sysctl turning group-dropping on and off
> would suffice; systems that know they don't use groups to exclude
> specific users can enable that sysctl.

Right. Until someone comes along and installs a package that
uses groups in this particular way. You can't count on the fact
that someone isn't using it in that particular way today as an
indicator that they won't tomorrow. Are you thinking about providing
a tool that will tell sysadmins whether or not their system is safe
to use this option? Certainly you are going to suggest that most
sysadmins would know how to figure out if it is safe to use this
option.

The developers of user namespaces didn't notice it might be a problem.
You can't count on sysadmins or distro developers to do better.

>
> - Josh Triplett
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Casey Schaufler]

On 11/15/2014 1:01 AM, Josh Triplett wrote:
> Currently, unprivileged processes (without CAP_SETGID) cannot call
> setgroups at all.  In particular, processes with a set of supplementary
> groups cannot further drop permissions without obtaining elevated
> permissions first.

Has anyone put any thought into how this will interact with
POSIX ACLs? I don't see that anywhere in the discussion.

Tizen takes advantage of the fact you can't drop groups. If
a process can drop itself out of groups without privilege
it can circumvent the system security policy.

Back when the LSM was introduced a choice was made between
authoritative hooks (which would have allowed this sort of thing)
and restrictive hooks (which would not). Authoritative hooks were
rejected because they would have "broken Linux". I hope that the
people who spoke up then will speak up now.

This is going to introduce a whole class of vulnerabilities.
Don't even think of arguing that the reduction in use of privilege
will make up for that. Developers have enough trouble with the
difference between setuid() and seteuid() to expect them to use
group dropping properly.

>
> Allow unprivileged processes to call setgroups with a subset of their
> current groups; only require CAP_SETGID to add a group the process does
> not currently have.
>
> The kernel already maintains the list of supplementary group IDs in
> sorted order, and setgroups already needs to sort the new list, so this
> just requires a linear comparison of the two sorted lists.
>
> This moves the CAP_SETGID test from setgroups into set_current_groups.
>
> Tested via the following test program:
>
> #include <err.h>
> #include <grp.h>
> #include <stdio.h>
> #include <sys/types.h>
> #include <unistd.h>
>
> void run_id(void)
> {
>     pid_t p = fork();
>     switch (p) {
>         case -1:
>             err(1, "fork");
>         case 0:
>             execl("/usr/bin/id", "id", NULL);
>             err(1, "exec");
>         default:
>             if (waitpid(p, NULL, 0) < 0)
>                 err(1, "waitpid");
>     }
> }
>
> int main(void)
> {
>     gid_t list1[] = { 1, 2, 3, 4, 5 };
>     gid_t list2[] = { 2, 3, 4 };
>     run_id();
>     if (setgroups(5, list1) < 0)
>         err(1, "setgroups 1");
>     run_id();
>     if (setresgid(1, 1, 1) < 0)
>         err(1, "setresgid");
>     if (setresuid(1, 1, 1) < 0)
>         err(1, "setresuid");
>     run_id();
>     if (setgroups(3, list2) < 0)
>         err(1, "setgroups 2");
>     run_id();
>     if (setgroups(5, list1) < 0)
>         err(1, "setgroups 3");
>     run_id();
>
>     return 0;
> }
>
> Without this patch, the test program gets EPERM from the second
> setgroups call, after dropping root privileges.  With this patch, the
> test program successfully drops groups 1 and 5, but then gets EPERM from
> the third setgroups call, since that call attempts to add groups the
> process does not currently have.
>
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> ---
>  kernel/groups.c | 33 ++++++++++++++++++++++++++++++---
>  kernel/uid16.c  |  2 --
>  2 files changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/groups.c b/kernel/groups.c
> index f0667e7..fe7367d 100644
> --- a/kernel/groups.c
> +++ b/kernel/groups.c
> @@ -153,6 +153,29 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
>  	return 0;
>  }
>  
> +/* Compare two sorted group lists; return true if the first is a subset of the
> + * second.
> + */
> +static bool is_subset(const struct group_info *g1, const struct group_info *g2)
> +{
> +	unsigned int i, j;
> +
> +	for (i = 0, j = 0; i < g1->ngroups; i++) {
> +		kgid_t gid1 = GROUP_AT(g1, i);
> +		kgid_t gid2;
> +		for (; j < g2->ngroups; j++) {
> +			gid2 = GROUP_AT(g2, j);
> +			if (gid_lte(gid1, gid2))
> +				break;
> +		}
> +		if (j >= g2->ngroups || !gid_eq(gid1, gid2))
> +			return false;
> +		j++;
> +	}
> +
> +	return true;
> +}
> +
>  /**
>   * set_groups_sorted - Change a group subscription in a set of credentials
>   * @new: The newly prepared set of credentials to alter
> @@ -189,11 +212,17 @@ int set_current_groups(struct group_info *group_info)
>  {
>  	struct cred *new;
>  
> +	groups_sort(group_info);
>  	new = prepare_creds();
>  	if (!new)
>  		return -ENOMEM;
> +	if (!ns_capable(current_user_ns(), CAP_SETGID)
> +	    && !is_subset(group_info, new->group_info)) {
> +		abort_creds(new);
> +		return -EPERM;
> +	}
>  
> -	set_groups(new, group_info);
> +	set_groups_sorted(new, group_info);
>  	return commit_creds(new);
>  }
>  
> @@ -233,8 +262,6 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)
>  	struct group_info *group_info;
>  	int retval;
>  
> -	if (!ns_capable(current_user_ns(), CAP_SETGID))
> -		return -EPERM;
>  	if ((unsigned)gidsetsize > NGROUPS_MAX)
>  		return -EINVAL;
>  
> diff --git a/kernel/uid16.c b/kernel/uid16.c
> index 602e5bb..b27e167 100644
> --- a/kernel/uid16.c
> +++ b/kernel/uid16.c
> @@ -176,8 +176,6 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist)
>  	struct group_info *group_info;
>  	int retval;
>  
> -	if (!ns_capable(current_user_ns(), CAP_SETGID))
> -		return -EPERM;
>  	if ((unsigned)gidsetsize > NGROUPS_MAX)
>  		return -EINVAL;
>  

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Andy Lutomirski]

On Mon, Nov 17, 2014 at 10:06 AM, Casey Schaufler
<casey@schaufler-ca.com> wrote:
> On 11/15/2014 1:01 AM, Josh Triplett wrote:
>> Currently, unprivileged processes (without CAP_SETGID) cannot call
>> setgroups at all.  In particular, processes with a set of supplementary
>> groups cannot further drop permissions without obtaining elevated
>> permissions first.
>
> Has anyone put any thought into how this will interact with
> POSIX ACLs? I don't see that anywhere in the discussion.

That means that user namespaces are a problem, too, and we need to fix
it.  Or we should add some control to turn unprivileged user namespace
creation on and off and document that turning it on defeats POSIX ACLs
with a group entry that is more restrictive than the other entry.

--Andy

>
> Tizen takes advantage of the fact you can't drop groups. If
> a process can drop itself out of groups without privilege
> it can circumvent the system security policy.
>
> Back when the LSM was introduced a choice was made between
> authoritative hooks (which would have allowed this sort of thing)
> and restrictive hooks (which would not). Authoritative hooks were
> rejected because they would have "broken Linux". I hope that the
> people who spoke up then will speak up now.
>
> This is going to introduce a whole class of vulnerabilities.
> Don't even think of arguing that the reduction in use of privilege
> will make up for that. Developers have enough trouble with the
> difference between setuid() and seteuid() to expect them to use
> group dropping properly.
>
>>
>> Allow unprivileged processes to call setgroups with a subset of their
>> current groups; only require CAP_SETGID to add a group the process does
>> not currently have.
>>
>> The kernel already maintains the list of supplementary group IDs in
>> sorted order, and setgroups already needs to sort the new list, so this
>> just requires a linear comparison of the two sorted lists.
>>
>> This moves the CAP_SETGID test from setgroups into set_current_groups.
>>
>> Tested via the following test program:
>>
>> #include <err.h>
>> #include <grp.h>
>> #include <stdio.h>
>> #include <sys/types.h>
>> #include <unistd.h>
>>
>> void run_id(void)
>> {
>>     pid_t p = fork();
>>     switch (p) {
>>         case -1:
>>             err(1, "fork");
>>         case 0:
>>             execl("/usr/bin/id", "id", NULL);
>>             err(1, "exec");
>>         default:
>>             if (waitpid(p, NULL, 0) < 0)
>>                 err(1, "waitpid");
>>     }
>> }
>>
>> int main(void)
>> {
>>     gid_t list1[] = { 1, 2, 3, 4, 5 };
>>     gid_t list2[] = { 2, 3, 4 };
>>     run_id();
>>     if (setgroups(5, list1) < 0)
>>         err(1, "setgroups 1");
>>     run_id();
>>     if (setresgid(1, 1, 1) < 0)
>>         err(1, "setresgid");
>>     if (setresuid(1, 1, 1) < 0)
>>         err(1, "setresuid");
>>     run_id();
>>     if (setgroups(3, list2) < 0)
>>         err(1, "setgroups 2");
>>     run_id();
>>     if (setgroups(5, list1) < 0)
>>         err(1, "setgroups 3");
>>     run_id();
>>
>>     return 0;
>> }
>>
>> Without this patch, the test program gets EPERM from the second
>> setgroups call, after dropping root privileges.  With this patch, the
>> test program successfully drops groups 1 and 5, but then gets EPERM from
>> the third setgroups call, since that call attempts to add groups the
>> process does not currently have.
>>
>> Signed-off-by: Josh Triplett <josh@joshtriplett.org>
>> ---
>>  kernel/groups.c | 33 ++++++++++++++++++++++++++++++---
>>  kernel/uid16.c  |  2 --
>>  2 files changed, 30 insertions(+), 5 deletions(-)
>>
>> diff --git a/kernel/groups.c b/kernel/groups.c
>> index f0667e7..fe7367d 100644
>> --- a/kernel/groups.c
>> +++ b/kernel/groups.c
>> @@ -153,6 +153,29 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
>>       return 0;
>>  }
>>
>> +/* Compare two sorted group lists; return true if the first is a subset of the
>> + * second.
>> + */
>> +static bool is_subset(const struct group_info *g1, const struct group_info *g2)
>> +{
>> +     unsigned int i, j;
>> +
>> +     for (i = 0, j = 0; i < g1->ngroups; i++) {
>> +             kgid_t gid1 = GROUP_AT(g1, i);
>> +             kgid_t gid2;
>> +             for (; j < g2->ngroups; j++) {
>> +                     gid2 = GROUP_AT(g2, j);
>> +                     if (gid_lte(gid1, gid2))
>> +                             break;
>> +             }
>> +             if (j >= g2->ngroups || !gid_eq(gid1, gid2))
>> +                     return false;
>> +             j++;
>> +     }
>> +
>> +     return true;
>> +}
>> +
>>  /**
>>   * set_groups_sorted - Change a group subscription in a set of credentials
>>   * @new: The newly prepared set of credentials to alter
>> @@ -189,11 +212,17 @@ int set_current_groups(struct group_info *group_info)
>>  {
>>       struct cred *new;
>>
>> +     groups_sort(group_info);
>>       new = prepare_creds();
>>       if (!new)
>>               return -ENOMEM;
>> +     if (!ns_capable(current_user_ns(), CAP_SETGID)
>> +         && !is_subset(group_info, new->group_info)) {
>> +             abort_creds(new);
>> +             return -EPERM;
>> +     }
>>
>> -     set_groups(new, group_info);
>> +     set_groups_sorted(new, group_info);
>>       return commit_creds(new);
>>  }
>>
>> @@ -233,8 +262,6 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)
>>       struct group_info *group_info;
>>       int retval;
>>
>> -     if (!ns_capable(current_user_ns(), CAP_SETGID))
>> -             return -EPERM;
>>       if ((unsigned)gidsetsize > NGROUPS_MAX)
>>               return -EINVAL;
>>
>> diff --git a/kernel/uid16.c b/kernel/uid16.c
>> index 602e5bb..b27e167 100644
>> --- a/kernel/uid16.c
>> +++ b/kernel/uid16.c
>> @@ -176,8 +176,6 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist)
>>       struct group_info *group_info;
>>       int retval;
>>
>> -     if (!ns_capable(current_user_ns(), CAP_SETGID))
>> -             return -EPERM;
>>       if ((unsigned)gidsetsize > NGROUPS_MAX)
>>               return -EINVAL;
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-api" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Andy Lutomirski]

On Mon, Nov 17, 2014 at 10:31 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Mon, Nov 17, 2014 at 10:06 AM, Casey Schaufler
> <casey@schaufler-ca.com> wrote:
>> On 11/15/2014 1:01 AM, Josh Triplett wrote:
>>> Currently, unprivileged processes (without CAP_SETGID) cannot call
>>> setgroups at all.  In particular, processes with a set of supplementary
>>> groups cannot further drop permissions without obtaining elevated
>>> permissions first.
>>
>> Has anyone put any thought into how this will interact with
>> POSIX ACLs? I don't see that anywhere in the discussion.
>
> That means that user namespaces are a problem, too, and we need to fix
> it.  Or we should add some control to turn unprivileged user namespace
> creation on and off and document that turning it on defeats POSIX ACLs
> with a group entry that is more restrictive than the other entry.
>

This is a significant enough issue that I posted it to oss-security:

http://www.openwall.com/lists/oss-security/2014/11/17/19

It's not at all obvious to me how to fix it.  We could disallow userns
creation of any supplementary groups don't match fsuid, or we could
keep negative-only groups around in the userns.

It may be worth adding a sysctl to change the behavior, too.  IMO it's
absurd to use groups to deny permissions that are otherwise available.

--Andy

> --Andy
>
>>
>> Tizen takes advantage of the fact you can't drop groups. If
>> a process can drop itself out of groups without privilege
>> it can circumvent the system security policy.
>>
>> Back when the LSM was introduced a choice was made between
>> authoritative hooks (which would have allowed this sort of thing)
>> and restrictive hooks (which would not). Authoritative hooks were
>> rejected because they would have "broken Linux". I hope that the
>> people who spoke up then will speak up now.
>>
>> This is going to introduce a whole class of vulnerabilities.
>> Don't even think of arguing that the reduction in use of privilege
>> will make up for that. Developers have enough trouble with the
>> difference between setuid() and seteuid() to expect them to use
>> group dropping properly.
>>
>>>
>>> Allow unprivileged processes to call setgroups with a subset of their
>>> current groups; only require CAP_SETGID to add a group the process does
>>> not currently have.
>>>
>>> The kernel already maintains the list of supplementary group IDs in
>>> sorted order, and setgroups already needs to sort the new list, so this
>>> just requires a linear comparison of the two sorted lists.
>>>
>>> This moves the CAP_SETGID test from setgroups into set_current_groups.
>>>
>>> Tested via the following test program:
>>>
>>> #include <err.h>
>>> #include <grp.h>
>>> #include <stdio.h>
>>> #include <sys/types.h>
>>> #include <unistd.h>
>>>
>>> void run_id(void)
>>> {
>>>     pid_t p = fork();
>>>     switch (p) {
>>>         case -1:
>>>             err(1, "fork");
>>>         case 0:
>>>             execl("/usr/bin/id", "id", NULL);
>>>             err(1, "exec");
>>>         default:
>>>             if (waitpid(p, NULL, 0) < 0)
>>>                 err(1, "waitpid");
>>>     }
>>> }
>>>
>>> int main(void)
>>> {
>>>     gid_t list1[] = { 1, 2, 3, 4, 5 };
>>>     gid_t list2[] = { 2, 3, 4 };
>>>     run_id();
>>>     if (setgroups(5, list1) < 0)
>>>         err(1, "setgroups 1");
>>>     run_id();
>>>     if (setresgid(1, 1, 1) < 0)
>>>         err(1, "setresgid");
>>>     if (setresuid(1, 1, 1) < 0)
>>>         err(1, "setresuid");
>>>     run_id();
>>>     if (setgroups(3, list2) < 0)
>>>         err(1, "setgroups 2");
>>>     run_id();
>>>     if (setgroups(5, list1) < 0)
>>>         err(1, "setgroups 3");
>>>     run_id();
>>>
>>>     return 0;
>>> }
>>>
>>> Without this patch, the test program gets EPERM from the second
>>> setgroups call, after dropping root privileges.  With this patch, the
>>> test program successfully drops groups 1 and 5, but then gets EPERM from
>>> the third setgroups call, since that call attempts to add groups the
>>> process does not currently have.
>>>
>>> Signed-off-by: Josh Triplett <josh@joshtriplett.org>
>>> ---
>>>  kernel/groups.c | 33 ++++++++++++++++++++++++++++++---
>>>  kernel/uid16.c  |  2 --
>>>  2 files changed, 30 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/kernel/groups.c b/kernel/groups.c
>>> index f0667e7..fe7367d 100644
>>> --- a/kernel/groups.c
>>> +++ b/kernel/groups.c
>>> @@ -153,6 +153,29 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
>>>       return 0;
>>>  }
>>>
>>> +/* Compare two sorted group lists; return true if the first is a subset of the
>>> + * second.
>>> + */
>>> +static bool is_subset(const struct group_info *g1, const struct group_info *g2)
>>> +{
>>> +     unsigned int i, j;
>>> +
>>> +     for (i = 0, j = 0; i < g1->ngroups; i++) {
>>> +             kgid_t gid1 = GROUP_AT(g1, i);
>>> +             kgid_t gid2;
>>> +             for (; j < g2->ngroups; j++) {
>>> +                     gid2 = GROUP_AT(g2, j);
>>> +                     if (gid_lte(gid1, gid2))
>>> +                             break;
>>> +             }
>>> +             if (j >= g2->ngroups || !gid_eq(gid1, gid2))
>>> +                     return false;
>>> +             j++;
>>> +     }
>>> +
>>> +     return true;
>>> +}
>>> +
>>>  /**
>>>   * set_groups_sorted - Change a group subscription in a set of credentials
>>>   * @new: The newly prepared set of credentials to alter
>>> @@ -189,11 +212,17 @@ int set_current_groups(struct group_info *group_info)
>>>  {
>>>       struct cred *new;
>>>
>>> +     groups_sort(group_info);
>>>       new = prepare_creds();
>>>       if (!new)
>>>               return -ENOMEM;
>>> +     if (!ns_capable(current_user_ns(), CAP_SETGID)
>>> +         && !is_subset(group_info, new->group_info)) {
>>> +             abort_creds(new);
>>> +             return -EPERM;
>>> +     }
>>>
>>> -     set_groups(new, group_info);
>>> +     set_groups_sorted(new, group_info);
>>>       return commit_creds(new);
>>>  }
>>>
>>> @@ -233,8 +262,6 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)
>>>       struct group_info *group_info;
>>>       int retval;
>>>
>>> -     if (!ns_capable(current_user_ns(), CAP_SETGID))
>>> -             return -EPERM;
>>>       if ((unsigned)gidsetsize > NGROUPS_MAX)
>>>               return -EINVAL;
>>>
>>> diff --git a/kernel/uid16.c b/kernel/uid16.c
>>> index 602e5bb..b27e167 100644
>>> --- a/kernel/uid16.c
>>> +++ b/kernel/uid16.c
>>> @@ -176,8 +176,6 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist)
>>>       struct group_info *group_info;
>>>       int retval;
>>>
>>> -     if (!ns_capable(current_user_ns(), CAP_SETGID))
>>> -             return -EPERM;
>>>       if ((unsigned)gidsetsize > NGROUPS_MAX)
>>>               return -EINVAL;
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-api" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Andy Lutomirski
> AMA Capital Management, LLC



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Casey Schaufler]

On 11/17/2014 10:46 AM, Andy Lutomirski wrote:
> On Mon, Nov 17, 2014 at 10:31 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Mon, Nov 17, 2014 at 10:06 AM, Casey Schaufler
>> <casey@schaufler-ca.com> wrote:
>>> On 11/15/2014 1:01 AM, Josh Triplett wrote:
>>>> Currently, unprivileged processes (without CAP_SETGID) cannot call
>>>> setgroups at all.  In particular, processes with a set of supplementary
>>>> groups cannot further drop permissions without obtaining elevated
>>>> permissions first.
>>> Has anyone put any thought into how this will interact with
>>> POSIX ACLs? I don't see that anywhere in the discussion.
>> That means that user namespaces are a problem, too, and we need to fix
>> it.  Or we should add some control to turn unprivileged user namespace
>> creation on and off and document that turning it on defeats POSIX ACLs
>> with a group entry that is more restrictive than the other entry.
>>
> This is a significant enough issue that I posted it to oss-security:
>
> http://www.openwall.com/lists/oss-security/2014/11/17/19
>
> It's not at all obvious to me how to fix it.  We could disallow userns
> creation of any supplementary groups don't match fsuid, or we could
> keep negative-only groups around in the userns.
>
> It may be worth adding a sysctl to change the behavior, too.  IMO it's
> absurd to use groups to deny permissions that are otherwise available.

Absurd or not, it's traditional behavior, and if you don't have ACLs it
is the best way to accomplish the security goal.


>
> --Andy
>
>> --Andy
>>
>>> Tizen takes advantage of the fact you can't drop groups. If
>>> a process can drop itself out of groups without privilege
>>> it can circumvent the system security policy.
>>>
>>> Back when the LSM was introduced a choice was made between
>>> authoritative hooks (which would have allowed this sort of thing)
>>> and restrictive hooks (which would not). Authoritative hooks were
>>> rejected because they would have "broken Linux". I hope that the
>>> people who spoke up then will speak up now.
>>>
>>> This is going to introduce a whole class of vulnerabilities.
>>> Don't even think of arguing that the reduction in use of privilege
>>> will make up for that. Developers have enough trouble with the
>>> difference between setuid() and seteuid() to expect them to use
>>> group dropping properly.
>>>
>>>> Allow unprivileged processes to call setgroups with a subset of their
>>>> current groups; only require CAP_SETGID to add a group the process does
>>>> not currently have.
>>>>
>>>> The kernel already maintains the list of supplementary group IDs in
>>>> sorted order, and setgroups already needs to sort the new list, so this
>>>> just requires a linear comparison of the two sorted lists.
>>>>
>>>> This moves the CAP_SETGID test from setgroups into set_current_groups.
>>>>
>>>> Tested via the following test program:
>>>>
>>>> #include <err.h>
>>>> #include <grp.h>
>>>> #include <stdio.h>
>>>> #include <sys/types.h>
>>>> #include <unistd.h>
>>>>
>>>> void run_id(void)
>>>> {
>>>>     pid_t p = fork();
>>>>     switch (p) {
>>>>         case -1:
>>>>             err(1, "fork");
>>>>         case 0:
>>>>             execl("/usr/bin/id", "id", NULL);
>>>>             err(1, "exec");
>>>>         default:
>>>>             if (waitpid(p, NULL, 0) < 0)
>>>>                 err(1, "waitpid");
>>>>     }
>>>> }
>>>>
>>>> int main(void)
>>>> {
>>>>     gid_t list1[] = { 1, 2, 3, 4, 5 };
>>>>     gid_t list2[] = { 2, 3, 4 };
>>>>     run_id();
>>>>     if (setgroups(5, list1) < 0)
>>>>         err(1, "setgroups 1");
>>>>     run_id();
>>>>     if (setresgid(1, 1, 1) < 0)
>>>>         err(1, "setresgid");
>>>>     if (setresuid(1, 1, 1) < 0)
>>>>         err(1, "setresuid");
>>>>     run_id();
>>>>     if (setgroups(3, list2) < 0)
>>>>         err(1, "setgroups 2");
>>>>     run_id();
>>>>     if (setgroups(5, list1) < 0)
>>>>         err(1, "setgroups 3");
>>>>     run_id();
>>>>
>>>>     return 0;
>>>> }
>>>>
>>>> Without this patch, the test program gets EPERM from the second
>>>> setgroups call, after dropping root privileges.  With this patch, the
>>>> test program successfully drops groups 1 and 5, but then gets EPERM from
>>>> the third setgroups call, since that call attempts to add groups the
>>>> process does not currently have.
>>>>
>>>> Signed-off-by: Josh Triplett <josh@joshtriplett.org>
>>>> ---
>>>>  kernel/groups.c | 33 ++++++++++++++++++++++++++++++---
>>>>  kernel/uid16.c  |  2 --
>>>>  2 files changed, 30 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/kernel/groups.c b/kernel/groups.c
>>>> index f0667e7..fe7367d 100644
>>>> --- a/kernel/groups.c
>>>> +++ b/kernel/groups.c
>>>> @@ -153,6 +153,29 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
>>>>       return 0;
>>>>  }
>>>>
>>>> +/* Compare two sorted group lists; return true if the first is a subset of the
>>>> + * second.
>>>> + */
>>>> +static bool is_subset(const struct group_info *g1, const struct group_info *g2)
>>>> +{
>>>> +     unsigned int i, j;
>>>> +
>>>> +     for (i = 0, j = 0; i < g1->ngroups; i++) {
>>>> +             kgid_t gid1 = GROUP_AT(g1, i);
>>>> +             kgid_t gid2;
>>>> +             for (; j < g2->ngroups; j++) {
>>>> +                     gid2 = GROUP_AT(g2, j);
>>>> +                     if (gid_lte(gid1, gid2))
>>>> +                             break;
>>>> +             }
>>>> +             if (j >= g2->ngroups || !gid_eq(gid1, gid2))
>>>> +                     return false;
>>>> +             j++;
>>>> +     }
>>>> +
>>>> +     return true;
>>>> +}
>>>> +
>>>>  /**
>>>>   * set_groups_sorted - Change a group subscription in a set of credentials
>>>>   * @new: The newly prepared set of credentials to alter
>>>> @@ -189,11 +212,17 @@ int set_current_groups(struct group_info *group_info)
>>>>  {
>>>>       struct cred *new;
>>>>
>>>> +     groups_sort(group_info);
>>>>       new = prepare_creds();
>>>>       if (!new)
>>>>               return -ENOMEM;
>>>> +     if (!ns_capable(current_user_ns(), CAP_SETGID)
>>>> +         && !is_subset(group_info, new->group_info)) {
>>>> +             abort_creds(new);
>>>> +             return -EPERM;
>>>> +     }
>>>>
>>>> -     set_groups(new, group_info);
>>>> +     set_groups_sorted(new, group_info);
>>>>       return commit_creds(new);
>>>>  }
>>>>
>>>> @@ -233,8 +262,6 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)
>>>>       struct group_info *group_info;
>>>>       int retval;
>>>>
>>>> -     if (!ns_capable(current_user_ns(), CAP_SETGID))
>>>> -             return -EPERM;
>>>>       if ((unsigned)gidsetsize > NGROUPS_MAX)
>>>>               return -EINVAL;
>>>>
>>>> diff --git a/kernel/uid16.c b/kernel/uid16.c
>>>> index 602e5bb..b27e167 100644
>>>> --- a/kernel/uid16.c
>>>> +++ b/kernel/uid16.c
>>>> @@ -176,8 +176,6 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist)
>>>>       struct group_info *group_info;
>>>>       int retval;
>>>>
>>>> -     if (!ns_capable(current_user_ns(), CAP_SETGID))
>>>> -             return -EPERM;
>>>>       if ((unsigned)gidsetsize > NGROUPS_MAX)
>>>>               return -EINVAL;
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-api" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>> --
>> Andy Lutomirski
>> AMA Capital Management, LLC
>
>

[CFT][PATCH] userns: Avoid problems with negative groups

[Eric W. Biederman]

Classic unix permission checks have an interesting feature.  The group
permissions for a file can be set to less than the other permissions
on a file.  Occassionally this is used deliberately to give a certain
group of users fewer permissions than the default.

Overlooking negative groups has resulted in the permission checks for
setting up a group mapping in a user namespace to be too lax.  Tighten
the permission checks in new_idmap_permitted to ensure that mapping
uids and gids into user namespaces without privilege will not result
in new combinations of credentials being available to the users.

When setting mappings without privilege only the creator of the user
namespace is interesting as all other users that have CAP_SETUID over
the user namespace will also have CAP_SETUID over the user namespaces
parent.  So the scope of the unprivileged check is reduced to just
the case where cred->euid is the namespace creator.

For setting a uid mapping without privilege only euid is considered as
setresuid can set uid, suid and fsuid from euid without privielege
making any combination of uids possible with user namespaces already
possible without them.

For setting a gid mapping without privilege only egid on a credential
without supplementary groups is condsidered, as setresgid can set gid,
sgid and fsgid from egid without privilege.  The requirement for no
supplementary groups is because CAP_SETUID in a user namespace allows
supplementary groups to be cleared, which unfortunately means allowing
a credential with supplementary groups would allow new combinations
of credentials to exist, and thus would allow defeating negative
groups without permission.
 
This change should break userspace by the minimal amount needed
to fix this issue.

This should fix CVE-2014-8989.

Cc: stable@vger.kernel.org
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---

If people can test and review this change and verify  this and verify it
doesn't break anything I can't help breaking I will appreciate it.

 kernel/user_namespace.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index aa312b0dc3ec..b338c42d04fd 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -812,16 +812,24 @@ static bool new_idmap_permitted(const struct file *file,
 				struct user_namespace *ns, int cap_setid,
 				struct uid_gid_map *new_map)
 {
-	/* Allow mapping to your own filesystem ids */
-	if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1)) {
+	const struct cred *cred = file->f_cred;
+
+	/* Allow mapping an id without CAP_SETUID and CAP_SETGID when
+	 * allowing the root of the user namespace CAP_SETUID or
+	 * CAP_SETGID restricted to just that id will not change the
+	 * set of credentials available that user.
+	 */
+	if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1) &&
+	    uid_eq(ns->owner, cred->euid)) {
 		u32 id = new_map->extent[0].lower_first;
 		if (cap_setid == CAP_SETUID) {
 			kuid_t uid = make_kuid(ns->parent, id);
-			if (uid_eq(uid, file->f_cred->fsuid))
+			if (uid_eq(uid, cred->euid))
 				return true;
 		} else if (cap_setid == CAP_SETGID) {
 			kgid_t gid = make_kgid(ns->parent, id);
-			if (gid_eq(gid, file->f_cred->fsgid))
+			if (gid_eq(gid, cred->egid) &&
+			    (cred->group_info->ngroups == 0))
 				return true;
 		}
 	}
-- 
1.9.1

Re: [CFT][PATCH] userns: Avoid problems with negative groups

[Andy Lutomirski]

On Thu, Nov 27, 2014 at 8:59 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> Classic unix permission checks have an interesting feature.  The group
> permissions for a file can be set to less than the other permissions
> on a file.  Occassionally this is used deliberately to give a certain
> group of users fewer permissions than the default.
>
> Overlooking negative groups has resulted in the permission checks for
> setting up a group mapping in a user namespace to be too lax.  Tighten
> the permission checks in new_idmap_permitted to ensure that mapping
> uids and gids into user namespaces without privilege will not result
> in new combinations of credentials being available to the users.
>
> When setting mappings without privilege only the creator of the user
> namespace is interesting as all other users that have CAP_SETUID over
> the user namespace will also have CAP_SETUID over the user namespaces
> parent.  So the scope of the unprivileged check is reduced to just
> the case where cred->euid is the namespace creator.
>
> For setting a uid mapping without privilege only euid is considered as
> setresuid can set uid, suid and fsuid from euid without privielege
> making any combination of uids possible with user namespaces already
> possible without them.
>
> For setting a gid mapping without privilege only egid on a credential
> without supplementary groups is condsidered, as setresgid can set gid,
> sgid and fsgid from egid without privilege.  The requirement for no
> supplementary groups is because CAP_SETUID in a user namespace allows
> supplementary groups to be cleared, which unfortunately means allowing
> a credential with supplementary groups would allow new combinations
> of credentials to exist, and thus would allow defeating negative
> groups without permission.
>
> This change should break userspace by the minimal amount needed
> to fix this issue.
>
> This should fix CVE-2014-8989.

I think this is both unnecessarily restrictive and that it doesn't fix
the bug.  For example, I can exploit CVE-2014-8989 without ever
writing a uid map or a gid map.

IIUC, the only real issue is that user namespaces allow groups to be
dropped using setgroups that wouldn't otherwise be dropped.  Can we
get away with adding a per-user-ns flag that determines whether
setgroups can be used?

setgroups would be unusable until the gid_map has been written and
then it would become usable if and only if the parent userns could use
setgroups and the opener of gid_map was privileged.

If we wanted to allow finer-grained control, we could allow writing
control lines like:

options +setgroups

or

options -setgroups

in gid_map, or we could add user_ns_flags that can only be written
once and only before either uid_map or gid_map is written.

--Andy

>
> Cc: stable@vger.kernel.org
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>
> If people can test and review this change and verify  this and verify it
> doesn't break anything I can't help breaking I will appreciate it.
>
>  kernel/user_namespace.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index aa312b0dc3ec..b338c42d04fd 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -812,16 +812,24 @@ static bool new_idmap_permitted(const struct file *file,
>                                 struct user_namespace *ns, int cap_setid,
>                                 struct uid_gid_map *new_map)
>  {
> -       /* Allow mapping to your own filesystem ids */
> -       if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1)) {
> +       const struct cred *cred = file->f_cred;
> +
> +       /* Allow mapping an id without CAP_SETUID and CAP_SETGID when
> +        * allowing the root of the user namespace CAP_SETUID or
> +        * CAP_SETGID restricted to just that id will not change the
> +        * set of credentials available that user.
> +        */
> +       if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1) &&
> +           uid_eq(ns->owner, cred->euid)) {
>                 u32 id = new_map->extent[0].lower_first;
>                 if (cap_setid == CAP_SETUID) {
>                         kuid_t uid = make_kuid(ns->parent, id);
> -                       if (uid_eq(uid, file->f_cred->fsuid))
> +                       if (uid_eq(uid, cred->euid))
>                                 return true;
>                 } else if (cap_setid == CAP_SETGID) {
>                         kgid_t gid = make_kgid(ns->parent, id);
> -                       if (gid_eq(gid, file->f_cred->fsgid))
> +                       if (gid_eq(gid, cred->egid) &&
> +                           (cred->group_info->ngroups == 0))
>                                 return true;
>                 }
>         }
> --
> 1.9.1
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [CFT][PATCH] userns: Avoid problems with negative groups

[Eric W. Biederman]

Andy Lutomirski <luto@amacapital.net> writes:

>> This change should break userspace by the minimal amount needed
>> to fix this issue.
>>
>> This should fix CVE-2014-8989.
>
> I think this is both unnecessarily restrictive and that it doesn't fix
> the bug.  

You are going to have to work very hard to convince me this is
unnecessarily restrictive.  

>For example, I can exploit CVE-2014-8989 without ever
> writing a uid map or a gid map.

Yes.  I realized just after I sent the patch that setgroups(0, NULL)
would still work without a mapping set.  That is a first glass grade a
oversight that resulted in a bug.  None of the other uid or gid changing
syscalls without a mapping set, and setgroups was just overlooked
because it was different.  Oops.

I will send an updated patch that stops setgroups from working without
a mapping set shortly.

> IIUC, the only real issue is that user namespaces allow groups to be
> dropped using setgroups that wouldn't otherwise be dropped.  Can we
> get away with adding a per-user-ns flag that determines whether
> setgroups can be used?

Being able to call setgroups is fundamental to login programs, and login
programs are one of the things user namespaces need to support.  So
adding an extra flag and an extra place where privilege is required
is just noise, and will wind up breaking every user of user namespaces.

Further being able to setup uid and gid mappings without privilege is
primarily a nice to have.  The original design did not have unprivileged
setting of uid and gid maps and if it proves insecure I goofed and the
feature isn't safe so it needs to be removed.

This does mean that running a system with negative groups and users
delegated subordinate gids in /etc/subuid is a bad idea and system
administrators shouldn't do that as those negative groups won't prove
effective in stopping their users.  But this is all under system
administrator control so shrug.  There isn't a way to avoid that
fundamental conflict.

> setgroups would be unusable until the gid_map has been written and
> then it would become usable if and only if the parent userns could use
> setgroups and the opener of gid_map was privileged.

That proposal sounds a lot more restrictive and a lot more of a pain
to use than what I have implemented in my patch.

> If we wanted to allow finer-grained control, we could allow writing
> control lines like:
>
> options +setgroups
>
> or
>
> options -setgroups
>
> in gid_map, or we could add user_ns_flags that can only be written
> once and only before either uid_map or gid_map is written.

Definitely more complicated and I can't imagine a case where I need
a gid map without needing to call setgroups.

Eric

[CFT][PATCH v2] userns: Avoid problems with negative groups

[Eric W. Biederman]

Classic unix permission checks have an interesting feature.  The group
permissions for a file can be set to less than the other permissions
on a file.  Occassionally this is used deliberately to give a certain
group of users fewer permissions than the default.

Overlooking negative groups has resulted in the permission checks for
setting up a group mapping in a user namespace to be too lax.  Tighten
the permission checks in new_idmap_permitted to ensure that mapping
uids and gids into user namespaces without privilege will not result
in new combinations of credentials being available to the users.

When setting mappings without privilege only the creator of the user
namespace is interesting as all other users that have CAP_SETUID over
the user namespace will also have CAP_SETUID over the user namespaces
parent.  So the scope of the unprivileged check is reduced to just
the case where cred->euid is the namespace creator.

For setting a uid mapping without privilege only euid is considered as
setresuid can set uid, suid and fsuid from euid without privielege
making any combination of uids possible with user namespaces already
possible without them.

For setting a gid mapping without privilege only egid on a credential
without supplementary groups is condsidered, as setresgid can set gid,
sgid and fsgid from egid without privilege.  The requirement for no
supplementary groups is because CAP_SETUID in a user namespace allows
supplementary groups to be cleared, which unfortunately means allowing
a credential with supplementary groups would allow new combinations
of credentials to exist, and thus would allow defeating negative
groups without permission.

setgroups is modified to fail not only when the group ids do not
map but also when there are no gid mappings at all, preventing
setgroups(0, NULL) from succeeding when gid mappings have not been
established.

This change should break userspace by the minimal amount needed
to fix this issue.

This should fix CVE-2014-8989.

Cc: stable@vger.kernel.org
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 include/linux/user_namespace.h | 10 ++++++++++
 kernel/groups.c                |  7 ++++++-
 kernel/uid16.c                 |  5 ++++-
 kernel/user_namespace.c        | 16 ++++++++++++----
 4 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index e95372654f09..26d5e8f5db97 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -46,6 +46,11 @@ static inline struct user_namespace *get_user_ns(struct user_namespace *ns)
 	return ns;
 }
 
+static inline bool gid_mapping_possible(const struct user_namespace *ns)
+{
+	return ns->gid_map.nr_extents != 0;
+}
+
 extern int create_user_ns(struct cred *new);
 extern int unshare_userns(unsigned long unshare_flags, struct cred **new_cred);
 extern void free_user_ns(struct user_namespace *ns);
@@ -70,6 +75,11 @@ static inline struct user_namespace *get_user_ns(struct user_namespace *ns)
 	return &init_user_ns;
 }
 
+static inline bool gid_mapping_possible(const struct user_namespace *ns)
+{
+	return true;
+}
+
 static inline int create_user_ns(struct cred *new)
 {
 	return -EINVAL;
diff --git a/kernel/groups.c b/kernel/groups.c
index 451698f86cfa..302aa415158f 100644
--- a/kernel/groups.c
+++ b/kernel/groups.c
@@ -6,6 +6,7 @@
 #include <linux/slab.h>
 #include <linux/security.h>
 #include <linux/syscalls.h>
+#include <linux/user_namespace.h>
 #include <asm/uaccess.h>
 
 /* init to 2 - one for init_task, one to ensure it is never freed */
@@ -220,14 +221,18 @@ out:
 
 SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)
 {
+	struct user_namespace *user_ns = current_user_ns();
 	struct group_info *group_info;
 	int retval;
 
-	if (!ns_capable(current_user_ns(), CAP_SETGID))
+	if (!gid_mapping_possible(user_ns) ||
+	    !ns_capable(user_ns, CAP_SETGID))
 		return -EPERM;
 	if ((unsigned)gidsetsize > NGROUPS_MAX)
 		return -EINVAL;
 
+	/* How do I alloc a negative gidsetsize??? */
+
 	group_info = groups_alloc(gidsetsize);
 	if (!group_info)
 		return -ENOMEM;
diff --git a/kernel/uid16.c b/kernel/uid16.c
index 602e5bbbceff..602c7de2aa11 100644
--- a/kernel/uid16.c
+++ b/kernel/uid16.c
@@ -13,6 +13,7 @@
 #include <linux/highuid.h>
 #include <linux/security.h>
 #include <linux/syscalls.h>
+#include <linux/user_namespace.h>
 
 #include <asm/uaccess.h>
 
@@ -173,10 +174,12 @@ out:
 
 SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist)
 {
+	struct user_namespace *user_ns = current_user_ns();
 	struct group_info *group_info;
 	int retval;
 
-	if (!ns_capable(current_user_ns(), CAP_SETGID))
+	if (!gid_mapping_possible(user_ns) ||
+	    !ns_capable(user_ns, CAP_SETGID))
 		return -EPERM;
 	if ((unsigned)gidsetsize > NGROUPS_MAX)
 		return -EINVAL;
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index aa312b0dc3ec..b338c42d04fd 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -812,16 +812,24 @@ static bool new_idmap_permitted(const struct file *file,
 				struct user_namespace *ns, int cap_setid,
 				struct uid_gid_map *new_map)
 {
-	/* Allow mapping to your own filesystem ids */
-	if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1)) {
+	const struct cred *cred = file->f_cred;
+
+	/* Allow mapping an id without CAP_SETUID and CAP_SETGID when
+	 * allowing the root of the user namespace CAP_SETUID or
+	 * CAP_SETGID restricted to just that id will not change the
+	 * set of credentials available that user.
+	 */
+	if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1) &&
+	    uid_eq(ns->owner, cred->euid)) {
 		u32 id = new_map->extent[0].lower_first;
 		if (cap_setid == CAP_SETUID) {
 			kuid_t uid = make_kuid(ns->parent, id);
-			if (uid_eq(uid, file->f_cred->fsuid))
+			if (uid_eq(uid, cred->euid))
 				return true;
 		} else if (cap_setid == CAP_SETGID) {
 			kgid_t gid = make_kgid(ns->parent, id);
-			if (gid_eq(gid, file->f_cred->fsgid))
+			if (gid_eq(gid, cred->egid) &&
+			    (cred->group_info->ngroups == 0))
 				return true;
 		}
 	}
-- 
1.9.1

Re: [CFT][PATCH] userns: Avoid problems with negative groups

[Andy Lutomirski]

On Thu, Nov 27, 2014 at 9:21 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Andy Lutomirski <luto@amacapital.net> writes:
>
>>> This change should break userspace by the minimal amount needed
>>> to fix this issue.
>>>
>>> This should fix CVE-2014-8989.
>>
>> I think this is both unnecessarily restrictive and that it doesn't fix
>> the bug.
>
> You are going to have to work very hard to convince me this is
> unnecessarily restrictive.
>
>>For example, I can exploit CVE-2014-8989 without ever
>> writing a uid map or a gid map.
>
> Yes.  I realized just after I sent the patch that setgroups(0, NULL)
> would still work without a mapping set.  That is a first glass grade a
> oversight that resulted in a bug.  None of the other uid or gid changing
> syscalls without a mapping set, and setgroups was just overlooked
> because it was different.  Oops.
>
> I will send an updated patch that stops setgroups from working without
> a mapping set shortly.
>
>> IIUC, the only real issue is that user namespaces allow groups to be
>> dropped using setgroups that wouldn't otherwise be dropped.  Can we
>> get away with adding a per-user-ns flag that determines whether
>> setgroups can be used?
>
> Being able to call setgroups is fundamental to login programs, and login
> programs are one of the things user namespaces need to support.  So
> adding an extra flag and an extra place where privilege is required
> is just noise, and will wind up breaking every user of user namespaces.
>
> Further being able to setup uid and gid mappings without privilege is
> primarily a nice to have.  The original design did not have unprivileged
> setting of uid and gid maps and if it proves insecure I goofed and the
> feature isn't safe so it needs to be removed.

Being able to set a single-user uid map and gid map is very useful for
sandboxing.  This lets unprivileged users drop filesystem and network
access and still run most normal programs.  A surprising number of
normal unprivileged programs fail if run without a mapping.

>
> This does mean that running a system with negative groups and users
> delegated subordinate gids in /etc/subuid is a bad idea and system
> administrators shouldn't do that as those negative groups won't prove
> effective in stopping their users.  But this is all under system
> administrator control so shrug.  There isn't a way to avoid that
> fundamental conflict.
>
>> setgroups would be unusable until the gid_map has been written and
>> then it would become usable if and only if the parent userns could use
>> setgroups and the opener of gid_map was privileged.
>
> That proposal sounds a lot more restrictive and a lot more of a pain
> to use than what I have implemented in my patch.
>
>> If we wanted to allow finer-grained control, we could allow writing
>> control lines like:
>>
>> options +setgroups
>>
>> or
>>
>> options -setgroups
>>
>> in gid_map, or we could add user_ns_flags that can only be written
>> once and only before either uid_map or gid_map is written.
>
> Definitely more complicated and I can't imagine a case where I need
> a gid map without needing to call setgroups.

I do it all the time.  Unshare, set mappings (with no inner uid 0 at
all), set no_new_privs, drop caps, and go.

Can we try the intermediate approach?  If you set gid_map without
privilege and you have supplementary groups, then let the write to
gid_map succeed but prevent setgroups from ever working?  That should
only be a couple of lines of code longer than your patch, and it will
avoid breaking sandbox use cases.

If we want to get really fancy in the future, we could have a concept
of pinned groups.  That is, if you're in a userns and you're a member
of an unmapped group, then you can't drop that group.  (Actually, that
all by itself might be enough to fix this issue.)

--Andy

>
> Eric



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [CFT][PATCH] userns: Avoid problems with negative groups

[Eric W. Biederman]

Andy Lutomirski <luto@amacapital.net> writes:

> On Thu, Nov 27, 2014 at 9:21 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>> Andy Lutomirski <luto@amacapital.net> writes:
>>
>>>> This change should break userspace by the minimal amount needed
>>>> to fix this issue.
>>>>
>>>> This should fix CVE-2014-8989.
>>>
>>> I think this is both unnecessarily restrictive and that it doesn't fix
>>> the bug.
>>
>> You are going to have to work very hard to convince me this is
>> unnecessarily restrictive.
>>
>>>For example, I can exploit CVE-2014-8989 without ever
>>> writing a uid map or a gid map.
>>
>> Yes.  I realized just after I sent the patch that setgroups(0, NULL)
>> would still work without a mapping set.  That is a first glass grade a
>> oversight that resulted in a bug.  None of the other uid or gid changing
>> syscalls without a mapping set, and setgroups was just overlooked
>> because it was different.  Oops.
>>
>> I will send an updated patch that stops setgroups from working without
>> a mapping set shortly.
>>
>>> IIUC, the only real issue is that user namespaces allow groups to be
>>> dropped using setgroups that wouldn't otherwise be dropped.  Can we
>>> get away with adding a per-user-ns flag that determines whether
>>> setgroups can be used?
>>
>> Being able to call setgroups is fundamental to login programs, and login
>> programs are one of the things user namespaces need to support.  So
>> adding an extra flag and an extra place where privilege is required
>> is just noise, and will wind up breaking every user of user namespaces.
>>
>> Further being able to setup uid and gid mappings without privilege is
>> primarily a nice to have.  The original design did not have unprivileged
>> setting of uid and gid maps and if it proves insecure I goofed and the
>> feature isn't safe so it needs to be removed.
>
> Being able to set a single-user uid map and gid map is very useful for
> sandboxing.  This lets unprivileged users drop filesystem and network
> access and still run most normal programs.  A surprising number of
> normal unprivileged programs fail if run without a mapping.

You can still set a single uid map unprivileged.  That should be enough
to keep your capabilities inside the namespace as long as you need them.

I am sad that in practice you can't set a single gid map, as everyone
calls setgroups.

Although I sort of think it might be worth scouring userspace for
something that will call setgroups and drop all of your groups.  If we
can find something preexisting that will solve this entire mess in a
much more elegant way.

>> This does mean that running a system with negative groups and users
>> delegated subordinate gids in /etc/subuid is a bad idea and system
>> administrators shouldn't do that as those negative groups won't prove
>> effective in stopping their users.  But this is all under system
>> administrator control so shrug.  There isn't a way to avoid that
>> fundamental conflict.
>>
>>> setgroups would be unusable until the gid_map has been written and
>>> then it would become usable if and only if the parent userns could use
>>> setgroups and the opener of gid_map was privileged.
>>
>> That proposal sounds a lot more restrictive and a lot more of a pain
>> to use than what I have implemented in my patch.
>>
>>> If we wanted to allow finer-grained control, we could allow writing
>>> control lines like:
>>>
>>> options +setgroups
>>>
>>> or
>>>
>>> options -setgroups
>>>
>>> in gid_map, or we could add user_ns_flags that can only be written
>>> once and only before either uid_map or gid_map is written.
>>
>> Definitely more complicated and I can't imagine a case where I need
>> a gid map without needing to call setgroups.
>
> I do it all the time.  Unshare, set mappings (with no inner uid 0 at
> all), set no_new_privs, drop caps, and go.
>
> Can we try the intermediate approach?  If you set gid_map without
> privilege and you have supplementary groups, then let the write to
> gid_map succeed but prevent setgroups from ever working?  That should
> only be a couple of lines of code longer than your patch, and it will
> avoid breaking sandbox use cases.

I am torn.  Send me an incremental patch and I will be happy to evaluate
it and if all is good fold the change in.  I hate breaking userspace but
if I break it I would rather it be a clean break that is easy to spot
and work around rather than something that almost works, and causes
people a lot of difficulty debugging.

My expectation is that systems that are serious will have /etc/subuid
and /etc/subgid and newuidmap and newgidmap setup.  Which is the other
way to allow you to map your own gid.

> If we want to get really fancy in the future, we could have a concept
> of pinned groups.  That is, if you're in a userns and you're a member
> of an unmapped group, then you can't drop that group.  (Actually, that
> all by itself might be enough to fix this issue.)

Not allowing you to drop groups really isn't enough.  One of the first
things applications like ssh do is call setgroups(0, NULL) to drop the
privileges granted by supplementary groups.  Further login program
somewhere call setgroups(N, ....) and give you every group mapped
by /etc/group.

I don't think I want to run containers with every supplementary group I
might want to drop mapped, and more than that, it would require changing
a lot more userspace than just the userspace that just does unpriv
containers with a single uid, and a single gid mapped.

But please test and see if you really need to map your group, and send
me an incremental patch if you see a way to do better.  Breaking
userspace sucks.

Eric

Re: [CFT][PATCH] userns: Avoid problems with negative groups

[Andy Lutomirski]

cc: Serge and Stephane, since this may end up affecting LXC.

On Fri, Nov 28, 2014 at 8:34 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Andy Lutomirski <luto@amacapital.net> writes:
>
>> On Thu, Nov 27, 2014 at 9:21 PM, Eric W. Biederman
>> <ebiederm@xmission.com> wrote:
>>> Andy Lutomirski <luto@amacapital.net> writes:
>>>
>>>>> This change should break userspace by the minimal amount needed
>>>>> to fix this issue.
>>>>>
>>>>> This should fix CVE-2014-8989.
>>>>
>>>> I think this is both unnecessarily restrictive and that it doesn't fix
>>>> the bug.
>>>
>>> You are going to have to work very hard to convince me this is
>>> unnecessarily restrictive.
>>>
>>>>For example, I can exploit CVE-2014-8989 without ever
>>>> writing a uid map or a gid map.
>>>
>>> Yes.  I realized just after I sent the patch that setgroups(0, NULL)
>>> would still work without a mapping set.  That is a first glass grade a
>>> oversight that resulted in a bug.  None of the other uid or gid changing
>>> syscalls without a mapping set, and setgroups was just overlooked
>>> because it was different.  Oops.
>>>
>>> I will send an updated patch that stops setgroups from working without
>>> a mapping set shortly.
>>>
>>>> IIUC, the only real issue is that user namespaces allow groups to be
>>>> dropped using setgroups that wouldn't otherwise be dropped.  Can we
>>>> get away with adding a per-user-ns flag that determines whether
>>>> setgroups can be used?
>>>
>>> Being able to call setgroups is fundamental to login programs, and login
>>> programs are one of the things user namespaces need to support.  So
>>> adding an extra flag and an extra place where privilege is required
>>> is just noise, and will wind up breaking every user of user namespaces.
>>>
>>> Further being able to setup uid and gid mappings without privilege is
>>> primarily a nice to have.  The original design did not have unprivileged
>>> setting of uid and gid maps and if it proves insecure I goofed and the
>>> feature isn't safe so it needs to be removed.
>>
>> Being able to set a single-user uid map and gid map is very useful for
>> sandboxing.  This lets unprivileged users drop filesystem and network
>> access and still run most normal programs.  A surprising number of
>> normal unprivileged programs fail if run without a mapping.
>
> You can still set a single uid map unprivileged.  That should be enough
> to keep your capabilities inside the namespace as long as you need them.
>
> I am sad that in practice you can't set a single gid map, as everyone
> calls setgroups.
>

That's not the problem.  The problem is that a surprising number of
libraries expect getegid(), etc to return sensible values.

> Although I sort of think it might be worth scouring userspace for
> something that will call setgroups and drop all of your groups.  If we
> can find something preexisting that will solve this entire mess in a
> much more elegant way.
>
>>> This does mean that running a system with negative groups and users
>>> delegated subordinate gids in /etc/subuid is a bad idea and system
>>> administrators shouldn't do that as those negative groups won't prove
>>> effective in stopping their users.  But this is all under system
>>> administrator control so shrug.  There isn't a way to avoid that
>>> fundamental conflict.
>>>
>>>> setgroups would be unusable until the gid_map has been written and
>>>> then it would become usable if and only if the parent userns could use
>>>> setgroups and the opener of gid_map was privileged.
>>>
>>> That proposal sounds a lot more restrictive and a lot more of a pain
>>> to use than what I have implemented in my patch.
>>>
>>>> If we wanted to allow finer-grained control, we could allow writing
>>>> control lines like:
>>>>
>>>> options +setgroups
>>>>
>>>> or
>>>>
>>>> options -setgroups
>>>>
>>>> in gid_map, or we could add user_ns_flags that can only be written
>>>> once and only before either uid_map or gid_map is written.
>>>
>>> Definitely more complicated and I can't imagine a case where I need
>>> a gid map without needing to call setgroups.
>>
>> I do it all the time.  Unshare, set mappings (with no inner uid 0 at
>> all), set no_new_privs, drop caps, and go.
>>
>> Can we try the intermediate approach?  If you set gid_map without
>> privilege and you have supplementary groups, then let the write to
>> gid_map succeed but prevent setgroups from ever working?  That should
>> only be a couple of lines of code longer than your patch, and it will
>> avoid breaking sandbox use cases.
>
> I am torn.  Send me an incremental patch and I will be happy to evaluate
> it and if all is good fold the change in.  I hate breaking userspace but
> if I break it I would rather it be a clean break that is easy to spot
> and work around rather than something that almost works, and causes
> people a lot of difficulty debugging.

I'll play with it this afternoon or over the weekend.  I'm currently
on vacation, so I'll be a little slow.

>
> My expectation is that systems that are serious will have /etc/subuid
> and /etc/subgid and newuidmap and newgidmap setup.  Which is the other
> way to allow you to map your own gid.
>
>> If we want to get really fancy in the future, we could have a concept
>> of pinned groups.  That is, if you're in a userns and you're a member
>> of an unmapped group, then you can't drop that group.  (Actually, that
>> all by itself might be enough to fix this issue.)
>
> Not allowing you to drop groups really isn't enough.  One of the first
> things applications like ssh do is call setgroups(0, NULL) to drop the
> privileges granted by supplementary groups.  Further login program
> somewhere call setgroups(N, ....) and give you every group mapped
> by /etc/group.
>
> I don't think I want to run containers with every supplementary group I
> might want to drop mapped, and more than that, it would require changing
> a lot more userspace than just the userspace that just does unpriv
> containers with a single uid, and a single gid mapped.
>
> But please test and see if you really need to map your group, and send
> me an incremental patch if you see a way to do better.  Breaking
> userspace sucks.

I *know* I need the uid mapped, and I'm pretty sure I need the gid as
well.  FWIW, the code I care about won't object too strongly to a
one-time break, but it will object to needing to use subuids, since it
will have all kinds of problems if it starts to need to rely on setuid
helpers.

There's a third option: use your patch but require explicit userspace
opt-in for code that wants the setgroups-not-allowed mode.  I'll try
implementing that.

--Andy

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Eric W.Biederman]

On November 17, 2014 1:46:59 PM EST, Andy Lutomirski <luto@amacapital.net> wrote:
>On Mon, Nov 17, 2014 at 10:31 AM, Andy Lutomirski <luto@amacapital.net>
>wrote:
>> On Mon, Nov 17, 2014 at 10:06 AM, Casey Schaufler
>> <casey@schaufler-ca.com> wrote:
>>> On 11/15/2014 1:01 AM, Josh Triplett wrote:
>>>> Currently, unprivileged processes (without CAP_SETGID) cannot call
>>>> setgroups at all.  In particular, processes with a set of
>supplementary
>>>> groups cannot further drop permissions without obtaining elevated
>>>> permissions first.
>>>
>>> Has anyone put any thought into how this will interact with
>>> POSIX ACLs? I don't see that anywhere in the discussion.
>>
>> That means that user namespaces are a problem, too, and we need to
>fix
>> it.  Or we should add some control to turn unprivileged user
>namespace
>> creation on and off and document that turning it on defeats POSIX
>ACLs
>> with a group entry that is more restrictive than the other entry.
>>
>
>This is a significant enough issue that I posted it to oss-security:
>
>http://www.openwall.com/lists/oss-security/2014/11/17/19
>
>It's not at all obvious to me how to fix it.  We could disallow userns
>creation of any supplementary groups don't match fsuid, or we could
>keep negative-only groups around in the userns.
>
>It may be worth adding a sysctl to change the behavior, too.  IMO it's
>absurd to use groups to deny permissions that are otherwise available.

There is an obvious user namespace fix.  Don't allow dropping supplemental groups that are not mapped.

That will require a little bit of fancy footwork if you want to play with supplemental groups in your unprivileged user namespace.   I would like to get a grip on what hoops would be required before we add the additional restriction.  Possibly something as simple as calling sg.

I also want to look at what Tizen and any other concrete pieces of code I can find using this negative permission pattern are actually doing.   Bugs definitely exist, but I have this erie feeling that the bugs may be in instances of userspace using this negative group permission pattern.  I think we may have a hideous case of one setuid binary defeating a privilege check of another piece of code.

This issue looks like it is worth a full scale investigation.  Sigh.

Eric

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[Andy Lutomirski]

On Mon, Nov 17, 2014 at 2:41 PM, Eric W.Biederman <ebiederm@xmission.com> wrote:
>
>
> On November 17, 2014 1:46:59 PM EST, Andy Lutomirski <luto@amacapital.net> wrote:
>>On Mon, Nov 17, 2014 at 10:31 AM, Andy Lutomirski <luto@amacapital.net>
>>wrote:
>>> On Mon, Nov 17, 2014 at 10:06 AM, Casey Schaufler
>>> <casey@schaufler-ca.com> wrote:
>>>> On 11/15/2014 1:01 AM, Josh Triplett wrote:
>>>>> Currently, unprivileged processes (without CAP_SETGID) cannot call
>>>>> setgroups at all.  In particular, processes with a set of
>>supplementary
>>>>> groups cannot further drop permissions without obtaining elevated
>>>>> permissions first.
>>>>
>>>> Has anyone put any thought into how this will interact with
>>>> POSIX ACLs? I don't see that anywhere in the discussion.
>>>
>>> That means that user namespaces are a problem, too, and we need to
>>fix
>>> it.  Or we should add some control to turn unprivileged user
>>namespace
>>> creation on and off and document that turning it on defeats POSIX
>>ACLs
>>> with a group entry that is more restrictive than the other entry.
>>>
>>
>>This is a significant enough issue that I posted it to oss-security:
>>
>>http://www.openwall.com/lists/oss-security/2014/11/17/19
>>
>>It's not at all obvious to me how to fix it.  We could disallow userns
>>creation of any supplementary groups don't match fsuid, or we could
>>keep negative-only groups around in the userns.
>>
>>It may be worth adding a sysctl to change the behavior, too.  IMO it's
>>absurd to use groups to deny permissions that are otherwise available.
>
> There is an obvious user namespace fix.  Don't allow dropping supplemental groups that are not mapped.

Why exactly does this fix it?  I guess that, if a supplementary group
is in your subgid list, then we can assume that dropping it is safe?

>
> That will require a little bit of fancy footwork if you want to play with supplemental groups in your unprivileged user namespace.   I would like to get a grip on what hoops would be required before we add the additional restriction.  Possibly something as simple as calling sg.

The main hoop I can think of is that setgroups would be impossible to
call if you have an unmapped supplementary group.  This could break
all kinds of things.

>
> I also want to look at what Tizen and any other concrete pieces of code I can find using this negative permission pattern are actually doing.   Bugs definitely exist, but I have this erie feeling that the bugs may be in instances of userspace using this negative group permission pattern.  I think we may have a hideous case of one setuid binary defeating a privilege check of another piece of code.
>
> This issue looks like it is worth a full scale investigation.  Sigh.

Agreed.

>
> Eric



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups

[josh]

On Mon, Nov 17, 2014 at 02:50:10PM -0800, Andy Lutomirski wrote:
> On Mon, Nov 17, 2014 at 2:41 PM, Eric W.Biederman <ebiederm@xmission.com> wrote:
> >
> >
> > On November 17, 2014 1:46:59 PM EST, Andy Lutomirski <luto@amacapital.net> wrote:
> >>On Mon, Nov 17, 2014 at 10:31 AM, Andy Lutomirski <luto@amacapital.net>
> >>wrote:
> >>> On Mon, Nov 17, 2014 at 10:06 AM, Casey Schaufler
> >>> <casey@schaufler-ca.com> wrote:
> >>>> On 11/15/2014 1:01 AM, Josh Triplett wrote:
> >>>>> Currently, unprivileged processes (without CAP_SETGID) cannot call
> >>>>> setgroups at all.  In particular, processes with a set of
> >>supplementary
> >>>>> groups cannot further drop permissions without obtaining elevated
> >>>>> permissions first.
> >>>>
> >>>> Has anyone put any thought into how this will interact with
> >>>> POSIX ACLs? I don't see that anywhere in the discussion.
> >>>
> >>> That means that user namespaces are a problem, too, and we need to
> >>fix
> >>> it.  Or we should add some control to turn unprivileged user
> >>namespace
> >>> creation on and off and document that turning it on defeats POSIX
> >>ACLs
> >>> with a group entry that is more restrictive than the other entry.
> >>>
> >>
> >>This is a significant enough issue that I posted it to oss-security:
> >>
> >>http://www.openwall.com/lists/oss-security/2014/11/17/19
> >>
> >>It's not at all obvious to me how to fix it.  We could disallow userns
> >>creation of any supplementary groups don't match fsuid, or we could
> >>keep negative-only groups around in the userns.
> >>
> >>It may be worth adding a sysctl to change the behavior, too.  IMO it's
> >>absurd to use groups to deny permissions that are otherwise available.
> >
> > There is an obvious user namespace fix.  Don't allow dropping supplemental groups that are not mapped.
> 
> Why exactly does this fix it?  I guess that, if a supplementary group
> is in your subgid list, then we can assume that dropping it is safe?

Considering that one of the fixes I'd like to add is "allow mapping
groups that you have in your supplemental group list", no, that fix
doesn't suffice. :)

> > That will require a little bit of fancy footwork if you want to play with supplemental groups in your unprivileged user namespace.   I would like to get a grip on what hoops would be required before we add the additional restriction.  Possibly something as simple as calling sg.
> 
> The main hoop I can think of is that setgroups would be impossible to
> call if you have an unmapped supplementary group.  This could break
> all kinds of things.

Agreed.

- Josh Triplett

[PATCH manpages] getgroups.2: Document unprivileged setgroups calls

[Josh Triplett]

Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
This should probably also include appropriate documentation for what
kernel introduces this behavior.

 man2/getgroups.2 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/man2/getgroups.2 b/man2/getgroups.2
index 373c204..edca37c 100644
--- a/man2/getgroups.2
+++ b/man2/getgroups.2
@@ -81,9 +81,10 @@ to be used in a further call to
 .PP
 .BR setgroups ()
 sets the supplementary group IDs for the calling process.
-Appropriate privileges (Linux: the
+Any process may drop groups from its list, but adding groups requires
+appropriate privileges (Linux: the
 .B CAP_SETGID
-capability) are required.
+capability).
 The
 .I size
 argument specifies the number of supplementary group IDs
-- 
2.1.3