Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups
On 11/17/2014 2:37 PM, j...@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
>> wrote:
>>>
>>> On November 17, 2014 1:07:30 PM EST, Andy Lutomirski
>>> wrote:
On Nov 17, 2014 3:37 AM, "One Thousand Gnomes"
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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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
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 > wrote: > > > > > > On November 17, 2014 1:46:59 PM EST, Andy Lutomirski > > wrote: > >>On Mon, Nov 17, 2014 at 10:31 AM, Andy Lutomirski > >>wrote: > >>> On Mon, Nov 17, 2014 at 10:06 AM, Casey Schaufler > >>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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
On Mon, Nov 17, 2014 at 2:41 PM, Eric W.Biederman wrote: > > > On November 17, 2014 1:46:59 PM EST, Andy Lutomirski > wrote: >>On Mon, Nov 17, 2014 at 10:31 AM, Andy Lutomirski >>wrote: >>> On Mon, Nov 17, 2014 at 10:06 AM, Casey Schaufler >>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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
On November 17, 2014 1:46:59 PM EST, Andy Lutomirski wrote: >On Mon, Nov 17, 2014 at 10:31 AM, Andy Lutomirski >wrote: >> On Mon, Nov 17, 2014 at 10:06 AM, Casey Schaufler >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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
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
> wrote:
> >
> >
> > On November 17, 2014 1:07:30 PM EST, Andy Lutomirski
> > wrote:
> >>On Nov 17, 2014 3:37 AM, "One Thousand Gnomes"
> >> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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
On Mon, Nov 17, 2014 at 2:11 PM, Eric W.Biederman wrote:
>
>
> On November 17, 2014 1:07:30 PM EST, Andy Lutomirski
> wrote:
>>On Nov 17, 2014 3:37 AM, "One Thousand Gnomes"
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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
On November 17, 2014 1:07:30 PM EST, Andy Lutomirski
wrote:
>On Nov 17, 2014 3:37 AM, "One Thousand Gnomes"
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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
On 11/17/2014 10:46 AM, Andy Lutomirski wrote:
> On Mon, Nov 17, 2014 at 10:31 AM, Andy Lutomirski wrote:
>> On Mon, Nov 17, 2014 at 10:06 AM, Casey Schaufler
>> 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
#include
#include
#include
#include
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
---
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 < g
Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups
On Mon, Nov 17, 2014 at 10:31 AM, Andy Lutomirski wrote:
> On Mon, Nov 17, 2014 at 10:06 AM, Casey Schaufler
> 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
>>> #include
>>> #include
>>> #include
>>> #include
>>>
>>> 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
>>> ---
>>> 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;
>>> +
Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups
On Mon, Nov 17, 2014 at 10:06 AM, Casey Schaufler
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
>> #include
>> #include
>> #include
>> #include
>>
>> 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
>> ---
>> 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->g
Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups
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
> #include
> #include
> #include
> #include
>
> 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
> ---
> 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 > NG
Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups
On Nov 17, 2014 3:37 AM, "One Thousand Gnomes"
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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
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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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
On Sun, Nov 16, 2014 at 5:32 AM, 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. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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
On Sat, Nov 15, 2014 at 09:08:07PM -0600, Eric W. Biederman wrote: > Josh Triplett writes: > > On November 15, 2014 6:05:11 PM PST, Theodore Ts'o 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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
Josh Triplett writes: > On November 15, 2014 6:05:11 PM PST, Theodore Ts'o 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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
On November 15, 2014 6:05:11 PM PST, Theodore Ts'o 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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
On Sat, Nov 15, 2014 at 12:06:20PM -0800, Andy Lutomirski wrote: > On Sat, Nov 15, 2014 at 11:29 AM, Josh Triplett wrote: > > On Sat, Nov 15, 2014 at 09:37:27AM -0600, Eric W. Biederman wrote: > >> Josh Triplett 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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
On Sat, Nov 15, 2014 at 11:29 AM, Josh Triplett wrote: > On Sat, Nov 15, 2014 at 09:37:27AM -0600, Eric W. Biederman wrote: >> Josh Triplett 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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
On Sat, Nov 15, 2014 at 09:37:27AM -0600, Eric W. Biederman wrote: > Josh Triplett 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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
Josh Triplett 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
> #include
> #include
> #include
> #include
>
> 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
> ---
> 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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordom
