From: Serge Hallyn <serge.hallyn@ubuntu.com> To: Aristeu Rozanski <aris@redhat.com> Cc: linux-kernel@vger.kernel.org, "Eric W. Biederman" <ebiederm@xmission.com>, amorgan@redhat.com, cgroups@vger.kernel.org Subject: Re: [PATCH 0/4] Rebase device_cgroup v2 patchset Date: Tue, 14 May 2013 11:22:38 -0500 [thread overview] Message-ID: <20130514162238.GA9056@sergelap> (raw) In-Reply-To: <20130514155111.GJ680@redhat.com> Quoting Aristeu Rozanski (aris@redhat.com): > On Tue, May 14, 2013 at 10:05:39AM -0500, Serge Hallyn wrote: > > so now that the device cgroup properly respects hierarchy, not allowing > > a cgroup to be given greater permission than its parent, should we consider > > relaxing the capability checks? > > > > There are two capable(CAP_SYS_ADMIN) checks in deice_cgroup.c: one in > > devcgroup_can_attach() to protect changing another task's cgroup, and > > one in devcgroup_update_access() to protect writes to the devices.allow > > and devices.deny files. > > > > I think the first should be changed to a check for ns_capable() to > > the victim's user_ns. Something like > > > > --- a/security/device_cgroup.c > > +++ b/security/device_cgroup.c > > @@ -70,10 +70,16 @@ static int devcgroup_can_attach(struct cgroup *new_cgrp, > > struct cgroup_taskset *set) > > { > > struct task_struct *task = cgroup_taskset_first(set); > > + struct user_namespace *ns; > > + int ret = -EPERM; > > > > - if (current != task && !capable(CAP_SYS_ADMIN)) > > - return -EPERM; > > - return 0; > > + if (current == task) > > + return 0; > > + > > + ns = userns_get(task);; > > + ret = ns_capable(ns, CAP_SYS_ADMIN) ? 0 : -EPERM; > > + put_user_ns(ns); > > + return ret; > > } > > wouldn't this allow a userns root to move a task in the same userns into > a parent cgroup? I believe than anything but moving down the hierarchy > would be very complicated to verify (how far up can you go). But only if they are able to open the tasks file for writing, which they shouldn't be able to do, right? > > For the second, the hierarchy support should let us ignore concerns > > about unprivileged users escalating privilege, but I'm trying to decide > > whether we need to worry about the sendmail capability class of bugs. > > You have a pointer for more information on those? Darn - unfortunately the best description of it, which was at http://userweb.kernel.org/~morgan/sendmail-capabilities-war-story.html is no longer there since userweb was taken down, and it was never captured by archive.org. There's a brief description in http://lwn.net/Articles/280279/ at the paragraph starting with "The memory of the sendmail-capabilities bug from 2000..." > > My sense is actually the answer is no, and we can drop the capable() > > check altogether. The reason is that while userspace frequently doesn't > > properly handle a failing system call due to unexpected lack of partial > > privilege, I wouldn't expect any setuid root program to ignore failure > > to open or mknod a device file (and proceed into a bad failure mode). > > Does this sound rasonable, or a recipe for disaster? > > The second case sounds ok to me -serge
WARNING: multiple messages have this Message-ID (diff)
From: Serge Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> To: Aristeu Rozanski <aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>, amorgan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Subject: Re: [PATCH 0/4] Rebase device_cgroup v2 patchset Date: Tue, 14 May 2013 11:22:38 -0500 [thread overview] Message-ID: <20130514162238.GA9056@sergelap> (raw) In-Reply-To: <20130514155111.GJ680-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> Quoting Aristeu Rozanski (aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org): > On Tue, May 14, 2013 at 10:05:39AM -0500, Serge Hallyn wrote: > > so now that the device cgroup properly respects hierarchy, not allowing > > a cgroup to be given greater permission than its parent, should we consider > > relaxing the capability checks? > > > > There are two capable(CAP_SYS_ADMIN) checks in deice_cgroup.c: one in > > devcgroup_can_attach() to protect changing another task's cgroup, and > > one in devcgroup_update_access() to protect writes to the devices.allow > > and devices.deny files. > > > > I think the first should be changed to a check for ns_capable() to > > the victim's user_ns. Something like > > > > --- a/security/device_cgroup.c > > +++ b/security/device_cgroup.c > > @@ -70,10 +70,16 @@ static int devcgroup_can_attach(struct cgroup *new_cgrp, > > struct cgroup_taskset *set) > > { > > struct task_struct *task = cgroup_taskset_first(set); > > + struct user_namespace *ns; > > + int ret = -EPERM; > > > > - if (current != task && !capable(CAP_SYS_ADMIN)) > > - return -EPERM; > > - return 0; > > + if (current == task) > > + return 0; > > + > > + ns = userns_get(task);; > > + ret = ns_capable(ns, CAP_SYS_ADMIN) ? 0 : -EPERM; > > + put_user_ns(ns); > > + return ret; > > } > > wouldn't this allow a userns root to move a task in the same userns into > a parent cgroup? I believe than anything but moving down the hierarchy > would be very complicated to verify (how far up can you go). But only if they are able to open the tasks file for writing, which they shouldn't be able to do, right? > > For the second, the hierarchy support should let us ignore concerns > > about unprivileged users escalating privilege, but I'm trying to decide > > whether we need to worry about the sendmail capability class of bugs. > > You have a pointer for more information on those? Darn - unfortunately the best description of it, which was at http://userweb.kernel.org/~morgan/sendmail-capabilities-war-story.html is no longer there since userweb was taken down, and it was never captured by archive.org. There's a brief description in http://lwn.net/Articles/280279/ at the paragraph starting with "The memory of the sendmail-capabilities bug from 2000..." > > My sense is actually the answer is no, and we can drop the capable() > > check altogether. The reason is that while userspace frequently doesn't > > properly handle a failing system call due to unexpected lack of partial > > privilege, I wouldn't expect any setuid root program to ignore failure > > to open or mknod a device file (and proceed into a bad failure mode). > > Does this sound rasonable, or a recipe for disaster? > > The second case sounds ok to me -serge
next prev parent reply other threads:[~2013-05-14 16:22 UTC|newest] Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top 2012-10-22 13:45 [PATCH 0/4] Rebase device_cgroup v2 patchset Aristeu Rozanski 2012-10-22 13:45 ` [PATCH 1/4] cgroup: fix invalid rcu dereference Aristeu Rozanski 2012-10-22 16:11 ` Serge Hallyn 2012-10-22 16:11 ` Serge Hallyn 2012-10-23 12:50 ` Jiri Slaby 2012-10-23 12:50 ` Jiri Slaby 2012-10-23 13:17 ` Aristeu Rozanski 2012-10-23 13:17 ` Aristeu Rozanski 2012-10-23 13:53 ` Jiri Slaby 2012-10-23 13:53 ` Jiri Slaby 2012-10-22 13:45 ` [PATCH 2/4] device_cgroup: rename deny_all to behavior Aristeu Rozanski 2012-10-22 16:12 ` Serge Hallyn 2012-10-22 13:45 ` [PATCH 3/4] device_cgroup: stop using simple_strtoul() Aristeu Rozanski 2012-10-22 16:14 ` Serge Hallyn 2012-10-22 16:14 ` Serge Hallyn 2012-10-22 13:45 ` [PATCH 4/4] device_cgroup: add proper checking when changing default behavior Aristeu Rozanski 2012-10-22 16:16 ` Serge Hallyn 2012-10-22 16:16 ` Serge Hallyn 2012-10-22 19:58 ` [PATCH 0/4] Rebase device_cgroup v2 patchset Andrew Morton 2012-10-22 19:58 ` Andrew Morton 2012-10-22 20:14 ` Aristeu Rozanski 2012-10-22 20:14 ` Aristeu Rozanski 2013-05-14 15:05 ` Serge Hallyn 2013-05-14 15:51 ` Aristeu Rozanski 2013-05-14 15:51 ` Aristeu Rozanski 2013-05-14 16:22 ` Serge Hallyn [this message] 2013-05-14 16:22 ` Serge Hallyn 2013-05-14 21:02 ` Eric W. Biederman 2013-05-14 21:02 ` Eric W. Biederman 2013-05-16 1:14 ` Serge E. Hallyn 2013-05-16 1:14 ` Serge E. Hallyn 2013-05-16 1:23 ` Serge E. Hallyn 2013-05-16 1:23 ` Serge E. Hallyn
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=20130514162238.GA9056@sergelap \ --to=serge.hallyn@ubuntu.com \ --cc=amorgan@redhat.com \ --cc=aris@redhat.com \ --cc=cgroups@vger.kernel.org \ --cc=ebiederm@xmission.com \ --cc=linux-kernel@vger.kernel.org \ /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.