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@xxxxxxxxxxxx> wrote:
> Andy Lutomirski <luto@xxxxxxxxxxxxxx> writes:
>
>> On Thu, Nov 27, 2014 at 9:21 PM, Eric W. Biederman
>> <ebiederm@xxxxxxxxxxxx> wrote:
>>> Andy Lutomirski <luto@xxxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/