From: Andy Lutomirski <luto@amacapital.net> To: "Eric W. Biederman" <ebiederm@xmission.com> Cc: Linux Containers <containers@lists.linux-foundation.org>, Josh Triplett <josh@joshtriplett.org>, Andrew Morton <akpm@linux-foundation.org>, Kees Cook <keescook@chromium.org>, Michael Kerrisk-manpages <mtk.manpages@gmail.com>, Linux API <linux-api@vger.kernel.org>, linux-man <linux-man@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, LSM <linux-security-module@vger.kernel.org>, Casey Schaufler <casey@schaufler-ca.com>, "Serge E. Hallyn" <serge@hallyn.com>, Richard Weinberger <richard@nod.at> Subject: Re: [CFT][PATCH] userns: Avoid problems with negative groups Date: Thu, 27 Nov 2014 12:52:16 -0800 [thread overview] Message-ID: <CALCETrUuWDq2akKfb50AiPHeDDWzPW7ijz1QwnuNiskyZbBEfA@mail.gmail.com> (raw) In-Reply-To: <87lhmwwpey.fsf_-_@x220.int.ebiederm.org> On Thu, Nov 27, 2014 at 8:59 AM, Eric W. Biederman <ebiederm@xmission.com> wrote: > > Classic unix permission checks have an interesting feature. The group > permissions for a file can be set to less than the other permissions > on a file. Occassionally this is used deliberately to give a certain > group of users fewer permissions than the default. > > Overlooking negative groups has resulted in the permission checks for > setting up a group mapping in a user namespace to be too lax. Tighten > the permission checks in new_idmap_permitted to ensure that mapping > uids and gids into user namespaces without privilege will not result > in new combinations of credentials being available to the users. > > When setting mappings without privilege only the creator of the user > namespace is interesting as all other users that have CAP_SETUID over > the user namespace will also have CAP_SETUID over the user namespaces > parent. So the scope of the unprivileged check is reduced to just > the case where cred->euid is the namespace creator. > > For setting a uid mapping without privilege only euid is considered as > setresuid can set uid, suid and fsuid from euid without privielege > making any combination of uids possible with user namespaces already > possible without them. > > For setting a gid mapping without privilege only egid on a credential > without supplementary groups is condsidered, as setresgid can set gid, > sgid and fsgid from egid without privilege. The requirement for no > supplementary groups is because CAP_SETUID in a user namespace allows > supplementary groups to be cleared, which unfortunately means allowing > a credential with supplementary groups would allow new combinations > of credentials to exist, and thus would allow defeating negative > groups without permission. > > This change should break userspace by the minimal amount needed > to fix this issue. > > This should fix CVE-2014-8989. I think this is both unnecessarily restrictive and that it doesn't fix the bug. For example, I can exploit CVE-2014-8989 without ever writing a uid map or a gid map. IIUC, the only real issue is that user namespaces allow groups to be dropped using setgroups that wouldn't otherwise be dropped. Can we get away with adding a per-user-ns flag that determines whether setgroups can be used? setgroups would be unusable until the gid_map has been written and then it would become usable if and only if the parent userns could use setgroups and the opener of gid_map was privileged. If we wanted to allow finer-grained control, we could allow writing control lines like: options +setgroups or options -setgroups in gid_map, or we could add user_ns_flags that can only be written once and only before either uid_map or gid_map is written. --Andy > > Cc: stable@vger.kernel.org > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > --- > > If people can test and review this change and verify this and verify it > doesn't break anything I can't help breaking I will appreciate it. > > kernel/user_namespace.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c > index aa312b0dc3ec..b338c42d04fd 100644 > --- a/kernel/user_namespace.c > +++ b/kernel/user_namespace.c > @@ -812,16 +812,24 @@ static bool new_idmap_permitted(const struct file *file, > struct user_namespace *ns, int cap_setid, > struct uid_gid_map *new_map) > { > - /* Allow mapping to your own filesystem ids */ > - if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1)) { > + const struct cred *cred = file->f_cred; > + > + /* Allow mapping an id without CAP_SETUID and CAP_SETGID when > + * allowing the root of the user namespace CAP_SETUID or > + * CAP_SETGID restricted to just that id will not change the > + * set of credentials available that user. > + */ > + if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1) && > + uid_eq(ns->owner, cred->euid)) { > u32 id = new_map->extent[0].lower_first; > if (cap_setid == CAP_SETUID) { > kuid_t uid = make_kuid(ns->parent, id); > - if (uid_eq(uid, file->f_cred->fsuid)) > + if (uid_eq(uid, cred->euid)) > return true; > } else if (cap_setid == CAP_SETGID) { > kgid_t gid = make_kgid(ns->parent, id); > - if (gid_eq(gid, file->f_cred->fsgid)) > + if (gid_eq(gid, cred->egid) && > + (cred->group_info->ngroups == 0)) > return true; > } > } > -- > 1.9.1 > -- Andy Lutomirski AMA Capital Management, LLC
WARNING: multiple messages have this Message-ID (diff)
From: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> To: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> Cc: Linux Containers <containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org>, Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org>, Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>, Michael Kerrisk-manpages <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, Linux API <linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, linux-man <linux-man-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, LSM <linux-security-module-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Casey Schaufler <casey-iSGtlc1asvQWG2LlvL+J4A@public.gmane.org>, "Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>, Richard Weinberger <richard-/L3Ra7n9ekc@public.gmane.org> Subject: Re: [CFT][PATCH] userns: Avoid problems with negative groups Date: Thu, 27 Nov 2014 12:52:16 -0800 [thread overview] Message-ID: <CALCETrUuWDq2akKfb50AiPHeDDWzPW7ijz1QwnuNiskyZbBEfA@mail.gmail.com> (raw) In-Reply-To: <87lhmwwpey.fsf_-_-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org> On Thu, Nov 27, 2014 at 8:59 AM, Eric W. Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote: > > Classic unix permission checks have an interesting feature. The group > permissions for a file can be set to less than the other permissions > on a file. Occassionally this is used deliberately to give a certain > group of users fewer permissions than the default. > > Overlooking negative groups has resulted in the permission checks for > setting up a group mapping in a user namespace to be too lax. Tighten > the permission checks in new_idmap_permitted to ensure that mapping > uids and gids into user namespaces without privilege will not result > in new combinations of credentials being available to the users. > > When setting mappings without privilege only the creator of the user > namespace is interesting as all other users that have CAP_SETUID over > the user namespace will also have CAP_SETUID over the user namespaces > parent. So the scope of the unprivileged check is reduced to just > the case where cred->euid is the namespace creator. > > For setting a uid mapping without privilege only euid is considered as > setresuid can set uid, suid and fsuid from euid without privielege > making any combination of uids possible with user namespaces already > possible without them. > > For setting a gid mapping without privilege only egid on a credential > without supplementary groups is condsidered, as setresgid can set gid, > sgid and fsgid from egid without privilege. The requirement for no > supplementary groups is because CAP_SETUID in a user namespace allows > supplementary groups to be cleared, which unfortunately means allowing > a credential with supplementary groups would allow new combinations > of credentials to exist, and thus would allow defeating negative > groups without permission. > > This change should break userspace by the minimal amount needed > to fix this issue. > > This should fix CVE-2014-8989. I think this is both unnecessarily restrictive and that it doesn't fix the bug. For example, I can exploit CVE-2014-8989 without ever writing a uid map or a gid map. IIUC, the only real issue is that user namespaces allow groups to be dropped using setgroups that wouldn't otherwise be dropped. Can we get away with adding a per-user-ns flag that determines whether setgroups can be used? setgroups would be unusable until the gid_map has been written and then it would become usable if and only if the parent userns could use setgroups and the opener of gid_map was privileged. If we wanted to allow finer-grained control, we could allow writing control lines like: options +setgroups or options -setgroups in gid_map, or we could add user_ns_flags that can only be written once and only before either uid_map or gid_map is written. --Andy > > Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> > --- > > If people can test and review this change and verify this and verify it > doesn't break anything I can't help breaking I will appreciate it. > > kernel/user_namespace.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c > index aa312b0dc3ec..b338c42d04fd 100644 > --- a/kernel/user_namespace.c > +++ b/kernel/user_namespace.c > @@ -812,16 +812,24 @@ static bool new_idmap_permitted(const struct file *file, > struct user_namespace *ns, int cap_setid, > struct uid_gid_map *new_map) > { > - /* Allow mapping to your own filesystem ids */ > - if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1)) { > + const struct cred *cred = file->f_cred; > + > + /* Allow mapping an id without CAP_SETUID and CAP_SETGID when > + * allowing the root of the user namespace CAP_SETUID or > + * CAP_SETGID restricted to just that id will not change the > + * set of credentials available that user. > + */ > + if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1) && > + uid_eq(ns->owner, cred->euid)) { > u32 id = new_map->extent[0].lower_first; > if (cap_setid == CAP_SETUID) { > kuid_t uid = make_kuid(ns->parent, id); > - if (uid_eq(uid, file->f_cred->fsuid)) > + if (uid_eq(uid, cred->euid)) > return true; > } else if (cap_setid == CAP_SETGID) { > kgid_t gid = make_kgid(ns->parent, id); > - if (gid_eq(gid, file->f_cred->fsgid)) > + if (gid_eq(gid, cred->egid) && > + (cred->group_info->ngroups == 0)) > return true; > } > } > -- > 1.9.1 > -- Andy Lutomirski AMA Capital Management, LLC
next prev parent reply other threads:[~2014-11-27 20:52 UTC|newest] Thread overview: 73+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-11-15 9:00 [PATCH 1/2] groups: Factor out a function to set a pre-sorted group list Josh Triplett 2014-11-15 9:00 ` Josh Triplett 2014-11-15 9:01 ` [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups Josh Triplett 2014-11-15 15:37 ` Eric W. Biederman 2014-11-15 15:37 ` Eric W. Biederman 2014-11-15 19:29 ` Josh Triplett 2014-11-15 19:29 ` Josh Triplett 2014-11-15 20:06 ` Andy Lutomirski 2014-11-15 20:06 ` Andy Lutomirski 2014-11-15 20:20 ` Josh Triplett 2014-11-15 20:20 ` Josh Triplett 2014-11-16 2:05 ` Theodore Ts'o 2014-11-16 2:05 ` Theodore Ts'o 2014-11-16 2:35 ` Josh Triplett 2014-11-16 2:35 ` Josh Triplett 2014-11-16 3:08 ` Eric W. Biederman 2014-11-16 3:08 ` Eric W. Biederman 2014-11-16 5:07 ` Josh Triplett 2014-11-16 5:07 ` Josh Triplett 2014-11-16 13:32 ` Theodore Ts'o 2014-11-16 13:32 ` Theodore Ts'o 2014-11-16 15:42 ` Andy Lutomirski 2014-11-16 15:42 ` Andy Lutomirski 2014-11-16 19:12 ` Josh Triplett 2014-11-16 19:12 ` Josh Triplett 2014-11-16 19:09 ` Josh Triplett 2014-11-16 19:09 ` Josh Triplett 2014-11-16 3:40 ` Theodore Ts'o 2014-11-16 3:40 ` Theodore Ts'o 2014-11-16 4:52 ` Josh Triplett 2014-11-16 4:52 ` Josh Triplett 2014-11-17 11:37 ` One Thousand Gnomes 2014-11-17 11:37 ` One Thousand Gnomes 2014-11-17 18:07 ` Andy Lutomirski 2014-11-17 18:07 ` Andy Lutomirski 2014-11-17 22:11 ` Eric W.Biederman 2014-11-17 22:11 ` Eric W.Biederman 2014-11-17 22:22 ` Andy Lutomirski 2014-11-17 22:22 ` Andy Lutomirski 2014-11-17 22:37 ` josh 2014-11-17 22:37 ` josh-iaAMLnmF4UmaiuxdJuQwMA 2014-11-18 0:56 ` Casey Schaufler 2014-11-17 18:06 ` Casey Schaufler 2014-11-17 18:31 ` Andy Lutomirski 2014-11-17 18:31 ` Andy Lutomirski 2014-11-17 18:46 ` Andy Lutomirski 2014-11-17 18:51 ` Casey Schaufler [not found] ` <546A43CE.2030706-iSGtlc1asvQWG2LlvL+J4A@public.gmane.org> 2014-11-27 16:59 ` [CFT][PATCH] userns: Avoid problems with negative groups Eric W. Biederman 2014-11-27 16:59 ` Eric W. Biederman 2014-11-27 20:52 ` Andy Lutomirski [this message] 2014-11-27 20:52 ` Andy Lutomirski 2014-11-28 5:21 ` Eric W. Biederman 2014-11-28 5:21 ` Eric W. Biederman [not found] ` <87wq6frjcw.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org> 2014-11-28 5:22 ` [CFT][PATCH v2] " Eric W. Biederman 2014-11-28 5:22 ` Eric W. Biederman 2014-11-28 15:11 ` [CFT][PATCH] " Andy Lutomirski 2014-11-28 15:11 ` Andy Lutomirski 2014-11-28 15:11 ` Andy Lutomirski [not found] ` <CALCETrX2s-7iaLMEKLQsExTEp3JyoAPQG44p0v5wkeED3-6dQA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2014-11-28 16:34 ` Eric W. Biederman 2014-11-28 16:34 ` Eric W. Biederman 2014-11-28 16:34 ` Eric W. Biederman [not found] ` <874mtjp9m1.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org> 2014-11-28 17:11 ` Andy Lutomirski 2014-11-28 17:11 ` Andy Lutomirski [not found] ` <CALCETrUuWDq2akKfb50AiPHeDDWzPW7ijz1QwnuNiskyZbBEfA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2014-11-28 5:21 ` Eric W. Biederman [not found] ` <87lhmwwpey.fsf_-_-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org> 2014-11-27 20:52 ` Andy Lutomirski 2014-11-17 22:41 ` [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups Eric W.Biederman 2014-11-17 22:41 ` Eric W.Biederman 2014-11-17 22:50 ` Andy Lutomirski 2014-11-17 22:50 ` Andy Lutomirski 2014-11-17 23:13 ` josh 2014-11-17 23:13 ` josh-iaAMLnmF4UmaiuxdJuQwMA 2014-11-15 9:01 ` [PATCH manpages] getgroups.2: Document unprivileged setgroups calls Josh Triplett 2014-11-15 9:01 ` Josh Triplett
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CALCETrUuWDq2akKfb50AiPHeDDWzPW7ijz1QwnuNiskyZbBEfA@mail.gmail.com \ --to=luto@amacapital.net \ --cc=akpm@linux-foundation.org \ --cc=casey@schaufler-ca.com \ --cc=containers@lists.linux-foundation.org \ --cc=ebiederm@xmission.com \ --cc=josh@joshtriplett.org \ --cc=keescook@chromium.org \ --cc=linux-api@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-man@vger.kernel.org \ --cc=linux-security-module@vger.kernel.org \ --cc=mtk.manpages@gmail.com \ --cc=richard@nod.at \ --cc=serge@hallyn.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.