unprivileged user can drop supplementary groups

Hi Craig, interesting finding, thanks for reporting it to us.

Hi,

thanks for pointing this out.

would you mind posting a patch to fix this?

It looks like a real bug, except I'm a bit confused as to how this features is supposed to be used in the first place. The point of the setgroups=deny feature was to not regress the case where you use a negative group acl to deny a user from reading a file, right? But near as I can tell you can only enable setgroups=deny when creating a new user namespace, so such an admin is in any case required to step through new hoops to not regress functionality, which would be unacceptable.

In any case, my feeling on the bug is that it is CVE-worthy, but does not need to be secret (since the workaround is chmod 000 /usr/bin/newgidmap or apt purge uidmap. Does that seem reasonable?

Well, there is the slight problem that every single Ubuntu server install comes with newgidmap installed today so I think we do need to be careful with releasing a fix for this as there may well be users of LXC/LXD that also rely on negative groups for access control on their system.

Your suggestion to chmod 000 newgidmap would break those users and is certainly not something that'd be suitable for a security upload.

@stgraber,

Still trying to wrap my head around whether this currently actually
works. Can you verify that you can use setgroups=deny with a negative
acl in the initial user_ns to prevent a user doing the equivalent of
lxc-usernsexec -m b:0:$(id -u):1 to get around the acl?

I think you could but we don't have any PAM module that lets you do that today.

Oh, actually, according to https://lwn.net/Articles/626665/ the setgroups file is a namespace specific knob, so flipping it to deny would actually prevent setgroups by anyone who's in the host namespace...

Thinking about this problem a bit, it certainly sounds like for the normal case we'd want newgidmap to flip setgroups to false prior to writing the map, effectively preventing this issue.

The obvious problem is that we have very legitimate use cases where we absolutely do want newgidmap to write a map to /proc/PID/gid_map and keep setgroups allowed. That's what we need for fully unprivileged LXC containers.

My current thought here is that we'd effectively need a way to record in what case this is okay, so that an administrator can apply this on a per-user basis. My initial plan there was to add an extra column to /etc/subgid to control that, but it actually seems to be unrelated to the map used and more of a per-user attribute that should be tracked separately.

I agree that in order to preserve the ability of unprivileged containers to call setgroups we need some root-owned config file (possibly a column in /etc/subgid, possibly a new file) that controls which users newgidmap *won't* set /proc/$pid/setgroups to deny for.

Serge, I'm not much of a C programmer but I'd be happy to try writing a patch once we decide on the solution.

Eric W. Biederman contributed this via email:

> The short answer is that if you want negative acls to work don't try and
> apply them to a user in /etc/subuid or /etc/subgid.
>
> To my knowledge there is not a good solution to this problem.
>
> As for setting setgroups to deny that is the default setting if you do
> nothing. Allow can't be set until the gid map is set. Plus there
> are some inheritence rules that ensure if your parent has deny set you
> always will have deny set.
>
> To date in my experience negative group acls are a theoretical construct
> that no one actually uses.

Given that there's no clear solution to this problem I'm going to make
this bug public, so others can know that subtracting permissions via group
membership isn't perfect.

Thanks

Thanks for replying Eric, but I'm having trouble reproducing what you've posted. I can't write the gid map until I've written deny to /prod/$pid/setgroups, not the other way around. There might be some nuance I've missed.

Also, newgidmap will allow a user to map their own GID to 0 in the user namespace, even when there is no entry for that user in /etc/subgid.

What if newgidmap wrote "deny" to /proc/$pid/setgroups unless the user is whitelisted in some config file, probably separate from /etc/subgid, as Stéphane suggested?

> Thanks for replying Eric, but I'm having trouble reproducing what you've
> posted. I can't write the gid map until I've written deny to
> /prod/$pid/setgroups, not the other way around. There might be some nuance
> I've missed.

Yes, this is a security feature. setgroups must be written to *before* gid_map
(the reason for this is explained further in user_namespaces(7)). And only
privileged users are allowed to write to gid_map if setgroups is set to allow.

> Also, newgidmap will allow a user to map their own GID to 0 in the user
> namespace, even when there is no entry for that user in /etc/subgid.

This is something that is generally required for a container to function, and
isn't fundamentally a security issue because users are already allowed to do
that without privileges (this is how rootless containers and LXC unprivileged
containers work) -- *unless* in this mode newgidmap is setting setgroups=allow
(in which case this is a major security problem).

> What if newgidmap wrote "deny" to /proc/$pid/setgroups unless the user is
> whitelisted in some config file, probably separate from /etc/subgid, as
> Stéphane suggested?

:+1: I'd prefer if we implemented this by changing the /etc/subgid schema so
that rather than having the format

   user:id:id_cnt

It has the format

   user:id:id_cnt[:flagA,flagB,...]

And we define flags "allow_setgroups" and "deny_setgrouops" (with
"deny_setgroups" being the default). This way, administrators can be *explicit*
about the denial, we don't add any new configuration files, and it's backwards
compatible (with security being opt-out).

I imagine making deny_setgroups might be a *bit* contentious, but in the worst
case we could have a migration script that asks users (or just add a document
about it to the logfile for the upgrade).

> And we define flags "allow_setgroups" and "deny_setgrouops" (with
"deny_setgroups" being the default).

I think allow_setgropus should be the default for keeping compatibility.

However, useradd(8) may print warning for the default configuration.

This sounds acceptable to me. Issues or (even better) PRs against
github.com/shadow-maint/shadow would be great :)

Indeed the default should be the more permissible. (I won't accept
patches which require changes to the container runtime.)

On Mon, Jan 15, 2018 at 9:13 AM, Akihiro Suda <email address hidden> wrote:
>> And we define flags "allow_setgroups" and "deny_setgrouops" (with
> "deny_setgroups" being the default).
>
>
> I think allow_setgropus should be the default for keeping compatibility.
>
> However, useradd(8) may print warning for the default configuration.
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1729357
>
> Title:
> unprivileged user can drop supplementary groups
>
> Status in shadow package in Ubuntu:
> Confirmed
>
> Bug description:
> Distribution: Ubuntu 16.04.3 LTS
> Kernel: 4.4.0-97-generic
> uidmap package version: 1:4.2-3.1ubuntu5.3
>
> The newgidmap setuid executable allows any user to write a single
> mapping line to the gid_map of a process whose identity is the same as
> the calling process, as long as that mapping line maps the process's
> own GID outside of the user namespace to GID 0 inside the user
> namespace.
>
> Newgidmap will write the mapping regardless of the content of
> /proc/$process_being_mapped/setgroups, which will initially contain
> the string "allow". After this mapping is performed, and also after
> the process' uid_map is written with newuidmap, the process in the
> user namespace will be able to use the setgroups system call to drop
> supplementary groups.
>
> This is possible even if there is no entry for the user in
> /etc/subgid, because no subordinate GIDs are actually being used.
>
> This allows any user to circumvent the use of supplementary groups as
> blacklists, e.g. for some file owned by root:blacklist with permission
> bits 0604 (octal). Normally any process whose identity included the
> group "blacklist" in its supplementary groups would not be able to
> read that file. By performing this exploit using newgidmap, they can
> drop all supplementary groups and read that file.
>
> If newgidmap was not available, unprivileged users would not be able
> to write a process's gid_map until writing "deny" to
> /proc/$pid/setgroups. A fix for this might be for newgidmap to check
> the content of /proc/$process_being_mapped/setgroups is "deny", but we
> have not tried to patch this ourselves.
>
> An example using 2 login shells for a user named "someone" on Ubuntu
> Xenial, with the uidmap package installed:
>
> Shell 1
>
> someone@ubuntu-xenial:~$ id
> uid=1001(someone) gid=1001(someone) groups=1001(someone),1002(restricted)
>
> someone@ubuntu-xenial:~$ ls -al /tmp/should_restrict
> -rw----r-- 1 root restricted 8 Nov 1 12:23 /tmp/should_restrict
>
> someone@ubuntu-xenial:~$ cat /tmp/should_restrict
> cat: /tmp/should_restrict: Permission denied
>
> someone@ubuntu-xenial:~$ unshare -U --setgroups allow #
> /proc/self/setgroups already contains 'allow', but let's be explicit
>
> nobody@ubuntu-xenial:~$ echo $$
> 1878
>
> Sh...

Serge: I will submit a patch later today. However, I just thought that it's probably better that "allow_setgroups" should be "ignore_setgroups" and we retain the current behaviour (we don't write anything to /proc/$pid/setgroups) -- which allows a user (or runtime) to explicitly disable setgroups even if they would normally have the right to use setgroups.

Does that sound okay to you? I will make the default ignore_setgroups, but IMO we should change the default for `useradd` to (by default) set deny_setgroups.

Oh, and we should definitely get a CVE assigned IMO.

@cyphar

Did you submit patch/CVE?

I had a preliminary patch written, but it was getting quite complicated (shadow's codebase is much more complicated than I expected -- and the /etc/subgid parsing code is intertwined with the parsing code for all of the other /etc/... files). I am working on it though.

I've also email the SUSE Security team about getting a CVE assigned, though I'm not sure if it's better that we get it assigned or that the Ubuntu folks get it assigned.

I've just sent a request for a CVE. I'm working on the patch now. My current plan is that allow_setgroups will be the default for all mappings that are present in /etc/subgid -- but any "implicit" mappings (like mapping your own group) will be deny_setgroups by default (because that's the biggest security issue coming out of this bug -- that *any* user can drop groups, not just the ones with subgids set up).

On Thu, Feb 15, 2018 at 11:29:03AM -0000, Aleksa Sarai wrote:
> I've just sent a request for a CVE. I'm working on the patch now. My

I assume the CVE will at least be correctly attributed to Craig.

Christian

Yes, of course. "Craig Furman (Pivotal)" is in the credits. I also
added Akihiro Suda (for suggesting him that it was a newgidmap bug)
and myself (for working on a fix for it), but if Craig prefers I can
just make him the only credit.

On Thu, Feb 15, 2018 at 11:00 PM, Christian Brauner
<email address hidden> wrote:
> On Thu, Feb 15, 2018 at 11:29:03AM -0000, Aleksa Sarai wrote:
>> I've just sent a request for a CVE. I'm working on the patch now. My
>
> I assume the CVE will at least be correctly attributed to Craig.
>
> Christian
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1729357
>
> Title:
> unprivileged user can drop supplementary groups
>
> Status in shadow package in Ubuntu:
> Confirmed
>
> Bug description:
> Distribution: Ubuntu 16.04.3 LTS
> Kernel: 4.4.0-97-generic
> uidmap package version: 1:4.2-3.1ubuntu5.3
>
> The newgidmap setuid executable allows any user to write a single
> mapping line to the gid_map of a process whose identity is the same as
> the calling process, as long as that mapping line maps the process's
> own GID outside of the user namespace to GID 0 inside the user
> namespace.
>
> Newgidmap will write the mapping regardless of the content of
> /proc/$process_being_mapped/setgroups, which will initially contain
> the string "allow". After this mapping is performed, and also after
> the process' uid_map is written with newuidmap, the process in the
> user namespace will be able to use the setgroups system call to drop
> supplementary groups.
>
> This is possible even if there is no entry for the user in
> /etc/subgid, because no subordinate GIDs are actually being used.
>
> This allows any user to circumvent the use of supplementary groups as
> blacklists, e.g. for some file owned by root:blacklist with permission
> bits 0604 (octal). Normally any process whose identity included the
> group "blacklist" in its supplementary groups would not be able to
> read that file. By performing this exploit using newgidmap, they can
> drop all supplementary groups and read that file.
>
> If newgidmap was not available, unprivileged users would not be able
> to write a process's gid_map until writing "deny" to
> /proc/$pid/setgroups. A fix for this might be for newgidmap to check
> the content of /proc/$process_being_mapped/setgroups is "deny", but we
> have not tried to patch this ourselves.
>
> An example using 2 login shells for a user named "someone" on Ubuntu
> Xenial, with the uidmap package installed:
>
> Shell 1
>
> someone@ubuntu-xenial:~$ id
> uid=1001(someone) gid=1001(someone) groups=1001(someone),1002(restricted)
>
> someone@ubuntu-xenial:~$ ls -al /tmp/should_restrict
> -rw----r-- 1 root restricted 8 Nov 1 12:23 /tmp/should_restrict
>
> someone@ubuntu-xenial:~$ cat /tmp/should_restrict
> cat: /tmp/should_restrict: Permission denied
>
> someone@ubuntu-xenial:~$ unshare -U --setgroups allow #
> /proc/self/setgroups already contains 'allow', but let's be explicit
>
> nobody@ubuntu-xenial:~$ echo $$
> 1878
>
> Shell 2
>
> someone@ubuntu-xenia...

Thanks for the credit! I did highlight that the bug was in newgidmap in my initial report, by the way.

Aleksa, thanks for asking for a CVE? How did you go about this? This is new territory to me.

On Thu, Feb 15, 2018 at 11:30 PM, Craig Furman
<email address hidden> wrote:
> Thanks for the credit! I did highlight that the bug was in newgidmap in
> my initial report, by the way.

No problem -- you found the issue after all. Sorry for getting the timeline
wrong, did you want me to change the credits at all? It's your call.

> Aleksa, thanks for asking for a CVE? How did you go about this? This is
> new territory to me.

You just submit the online form at https://cveform.mitre.org/. You can also go
through the project if the project is registered with MITRE. (Canonical is
registered for example, but since this bug affects all distributions and not
just Ubuntu I felt it made more sense to just submit directly.)

There didn't appear to be any way for me to add you to Cc in the form (I could
only provide a single contact address), but I can forward the mails to you.

--
Aleksa Sarai (cyphar)
www.cyphar.com

It's really not a problem, I'm happy to leave it as it is.

https://github.com/shadow-maint/shadow/pull/97 is my proposed patch. It currently only deals with the immediate security issue of allowing users that don't have

  % echo "$(whoami):$(id -g):1" >> /etc/setgid

... set up. I've tested this with a couple of different setups and it appears to preserve behaviour when you're mapping subgid'd groups, but it restricts setgroups if the mapping is a fallback one. I was working on a patch for the flags code, but there's a lot of magic in the parsing code for that -- so I will work on that separately.

CVE-2018-7169 is assigned for this issue.

https://github.com/shadow-maint/shadow/pull/99 includes the allow_setgroups/deny_setgroups feature that we discussed earlier.

@stgraber @mdeslaur - I'd considered making a release for Ubuntu... but this is the negative acl thing... Your opinions appreciated.