devices cgroup: allow can_attach() if ns_capable

RFC PATCH Applied Serge Hallyn 4 participants 14 messages Jul 23, 2013 – Nov 4, 2013

Subsystems (2)

Security (LSM) Capabilities

lore

Share & tools

lore

Serge Hallyn

author

2013-07-23 18:16 UTC #0 lore raw

RFC PATCH 1/2 devices cgroup: allow can_attach() if ns_capable

We allow a task to change its own devices cgroup, or to change other tasks' cgroups if it has CAP_SYS_ADMIN.

Also allow task A to change task B's cgroup if task A has CAP_SYS_ADMIN with respect to task B - meaning A is root in the same userns, or A created B's userns.

Signed-off-by Serge Hallyn <[email protected]>

---
 security/device_cgroup.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

security/device_cgroup.c +7 -4

index 9760ecb6..8f5386ea 100644
@@ -74,11 +74,14 @@ struct cgroup_subsys devices_subsys;
 static int devcgroup_can_attach(struct cgroup *new_cgrp,
 				struct cgroup_taskset *set)
 {
-	struct task_struct *task = cgroup_taskset_first(set);
+	struct task_struct *t = cgroup_taskset_first(set);
+	int ret = 0;
 
-	if (current != task && !capable(CAP_SYS_ADMIN))
-		return -EPERM;
-	return 0;
+	rcu_read_lock();
+	if (current != t && !ns_capable(__task_cred(t)->user_ns, CAP_SYS_ADMIN))
+		ret = -EPERM;
+	rcu_read_unlock();
+	return ret;
 }
 
 /*

Signature · 1 line

1.7.9.5

Serge Hallyn

author

2013-07-23 18:18 UTC #1 lore raw

RFC PATCH 2/2 capabilities: allow nice if we are privileged

We allow task A to change B's nice level if it has a supserset of B's privileges, or of it has CAP_SYS_NICE. Also allow it if A has CAP_SYS_NICE with respect to B - meaning it is root in the same namespace, or it created B's namespace.

Signed-off-by Serge Hallyn <[email protected]>

---
 security/commoncap.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

security/commoncap.c +4 -4

index d78b003..ef98b56 100644
@@ -768,16 +768,16 @@ int cap_task_fix_setuid(struct cred *new, const struct cred *old, int flags)
  */
 static int cap_safe_nice(struct task_struct *p)
 {
-	int is_subset;
+	int is_subset, ret = 0;
 
 	rcu_read_lock();
 	is_subset = cap_issubset(__task_cred(p)->cap_permitted,
 				 current_cred()->cap_permitted);
+	if (!is_subset && !ns_capable(__task_cred(p)->user_ns, CAP_SYS_NICE))
+		ret = -EPERM;
 	rcu_read_unlock();
 
-	if (!is_subset && !capable(CAP_SYS_NICE))
-		return -EPERM;
-	return 0;
+	return ret;
 }
 
 /**

Signature · 1 line

1.7.9.5

Eric W. Biederman

Reviewed-by on v1 2/2

2013-07-24 08:07 UTC #2 lore raw

Re: [RFC PATCH 2/2] capabilities: allow nice if we are privileged

Quoted 4 lines · Serge Hallyn

We allow task A to change B's nice level if it has a supserset of B's privileges, or of it has CAP_SYS_NICE. Also allow it if A has CAP_SYS_NICE with respect to B - meaning it is root in the same namespace, or it created B's namespace.

This patch looks good to me.

We already have this logic elsewhere in the kernel so I don't expect this will make things worse.

Reviewed-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

Quoted 30 lines

Signed-off-by Serge Hallyn <[email protected]>

---
 security/commoncap.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

security/commoncap.c +4 -4

index d78b003..ef98b56 100644
@@ -768,16 +768,16 @@ int cap_task_fix_setuid(struct cred *new, const struct cred *old, int flags)
  */
 static int cap_safe_nice(struct task_struct *p)
 {
-	int is_subset;
+	int is_subset, ret = 0;
 
 	rcu_read_lock();
 	is_subset = cap_issubset(__task_cred(p)->cap_permitted,
 				 current_cred()->cap_permitted);
+	if (!is_subset && !ns_capable(__task_cred(p)->user_ns, CAP_SYS_NICE))
+		ret = -EPERM;
 	rcu_read_unlock();
 
-	if (!is_subset && !capable(CAP_SYS_NICE))
-		return -EPERM;
-	return 0;
+	return ret;
 }
 
 /**

Tejun Heo

2013-07-23 18:30 UTC #3 lore raw

In reply to Serge Hallyn

Quoted 6 lines · Serge Hallyn

We allow a task to change its own devices cgroup, or to change other tasks' cgroups if it has CAP_SYS_ADMIN.
Also allow task A to change task B's cgroup if task A has CAP_SYS_ADMIN with respect to task B - meaning A is root in the same userns, or A created B's userns.

As discussed multpile times, cgroup isn't gonna support delegating cgroup management directly into containers, so this doesn't really jive with where we're heading.

Thanks.

Signature · 1 line

tejun

Serge Hallyn

author

2013-07-23 18:38 UTC #4 lore raw

Quoted 11 lines · Tejun Heo

Quoted 6 lines · Serge Hallyn
We allow a task to change its own devices cgroup, or to change other tasks' cgroups if it has CAP_SYS_ADMIN.
Also allow task A to change task B's cgroup if task A has CAP_SYS_ADMIN with respect to task B - meaning A is root in the same userns, or A created B's userns.
As discussed multpile times, cgroup isn't gonna support delegating cgroup management directly into containers, so this doesn't really jive with where we're heading.

This doesn't delegate it into the container. It allows me, on the host, to set the cgroup for a container.

thanks,
-serge

Tejun Heo

2013-07-23 18:50 UTC #5 lore raw

Quoted 2 lines · Serge Hallyn

This doesn't delegate it into the container. It allows me, on the host, to set the cgroup for a container.

Hmmm? I'm a bit confused. Isn't the description saying that the patch allows pseudo-root in userns to change cgroup membership even if it isn't actually root?

Besides, I find the whole check rather bogus and would actually much prefer just nuking the check and just follow the standard permission checks.

Thanks.

--
tejun

Serge Hallyn

author

2013-07-23 19:04 UTC #6 lore raw

Quoted 7 lines · Tejun Heo

Quoted 2 lines · Serge Hallyn
This doesn't delegate it into the container. It allows me, on the host, to set the cgroup for a container.
Hmmm? I'm a bit confused. Isn't the description saying that the patch allows pseudo-root in userns to change cgroup membership even if it isn't actually root?

If task A is uid 1000 on the host, and creates task B as uid X in a new user namespace, then task A, still being uid 1000 on the host, is privileged with respect to B and his namespace - i.e. ns_capable(B->userns, CAP_SYS_ADMIN) is true.

Quoted 3 lines

Besides, I find the whole check rather bogus and would actually much prefer just nuking the check and just follow the standard permission checks.

I'd be ok with that - but there's one case I'm not sure about: If PAM sets me up with /sys/fs/cgroup/devices/serge owned by me, then if I'm thinking right, removing can_attach would mean I could move init into /sys/fs/cgroup/devices/serge...

Is there something else stopping that from happening?

-serge

Tejun Heo

2013-07-23 19:12 UTC #7 lore raw

Hello,

Quoted 4 lines · Serge Hallyn

If task A is uid 1000 on the host, and creates task B as uid X in a new user namespace, then task A, still being uid 1000 on the host, is privileged with respect to B and his namespace - i.e. ns_capable(B->userns, CAP_SYS_ADMIN) is true.

Well, that also is the exact type of priv delegation we're moving away from, so....

Quoted 10 lines

Quoted 3 lines
Besides, I find the whole check rather bogus and would actually much prefer just nuking the check and just follow the standard permission checks.
I'd be ok with that - but there's one case I'm not sure about: If PAM sets me up with /sys/fs/cgroup/devices/serge owned by me, then if I'm thinking right, removing can_attach would mean I could move init into /sys/fs/cgroup/devices/serge...
Is there something else stopping that from happening?

If PAM is giving out perms on cgroup directory, the whole system is prone to DoS in various ways anyway. It's already utterly broken, so kinda moot point. If there are people actually doing that in the wild, we can conditionalize it on cgroup_sane_behavior().

Thanks.

Signature · 1 line

tejun

Serge Hallyn

author

2013-07-23 19:28 UTC #8 lore raw

Quoted 10 lines · Tejun Heo

Hello,
Quoted 4 lines · Serge Hallyn
If task A is uid 1000 on the host, and creates task B as uid X in a new user namespace, then task A, still being uid 1000 on the host, is privileged with respect to B and his namespace - i.e. ns_capable(B->userns, CAP_SYS_ADMIN) is true.
Well, that also is the exact type of priv delegation we're moving away from, so....

I think that's unreasonable, but I guess I'll have to go reread the old thread.

Quoted 9 lines

Quoted 9 lines
Quoted 3 lines
Besides, I find the whole check rather bogus and would actually much prefer just nuking the check and just follow the standard permission checks.
I'd be ok with that - but there's one case I'm not sure about: If PAM sets me up with /sys/fs/cgroup/devices/serge owned by me, then if I'm thinking right, removing can_attach would mean I could move init into /sys/fs/cgroup/devices/serge...

Quoted 4 lines

Quoted 1 line
Is there something else stopping that from happening?
If PAM is giving out perms on cgroup directory, the whole system is prone to DoS in various ways anyway. It's already utterly broken, so

If we have decent enforcement of hierarchy for devices.{allow,deny}, which we now do, then I don't see why this has to be the case.

Quoted 2 lines

kinda moot point. If there are people actually doing that in the wild, we can conditionalize it on cgroup_sane_behavior().

Guess we'll stop using cgroups for now.

-serge

Tejun Heo

2013-07-23 19:39 UTC #9 lore raw

Hello, Serge.

Quoted 5 lines · Serge Hallyn

Quoted 2 lines
Well, that also is the exact type of priv delegation we're moving away from, so....
I think that's unreasonable, but I guess I'll have to go reread the old thread.

Yeah, please do. I think the case is pretty strong for disallowing delegation of cgroup directories to !root (or whatever CAP it should be) users. It's inherently unsafe for some controllers and ends up leaking kernel implementation details into regular binaries thus cementing those leaked details as APIs, which is a giant no-no.

Quoted 2 lines

If we have decent enforcement of hierarchy for devices.{allow,deny}, which we now do, then I don't see why this has to be the case.

If you think about devcg in isolation, maybe, but please keep in mind that devcg itself is already somewhat abusing cgroup and many other controllers shouldn't be allowed to delegate and we're headed for one unified hierarchy.

Quoted 4 lines

Quoted 2 lines
kinda moot point. If there are people actually doing that in the wild, we can conditionalize it on cgroup_sane_behavior().
Guess we'll stop using cgroups for now.

If you're delegating cgroup accesses to !root users, yes, STOP, please. It'd be doing a lot more harm to the whole kernel and its maintainability than whatever extra features / benefits it may be bringing.

Thanks.

--
tejun

Serge E. Hallyn

maintainer

2013-11-04 21:51 UTC #10 lore raw

Quoted 12 lines · Tejun Heo

Hello, Serge.
Quoted 9 lines · Serge Hallyn
Quoted 7 lines · Serge Hallyn
If task A is uid 1000 on the host, and creates task B as uid X in a new user namespace, then task A, still being uid 1000 on the host, is privileged with respect to B and his namespace - i.e. ns_capable(B->userns, CAP_SYS_ADMIN) is true.
Well, that also is the exact type of priv delegation we're moving away from, so....

Quoted 8 lines

Quoted 2 lines
I think that's unreasonable, but I guess I'll have to go reread the old thread.
Yeah, please do. I think the case is pretty strong for disallowing delegation of cgroup directories to !root (or whatever CAP it should be) users. It's inherently unsafe for some controllers and ends up leaking kernel implementation details into regular binaries thus cementing those leaked details as APIs, which is a giant no-no.

Hi Tejun,

So to come back to this... At plumbers, I believe you were not present at the 'let me contain that for you' session. There the "delegation is not safe" topic came up. When pressed, the only example that came up was (IIRC) having to do with an rt scheduling issue which, all agreed, was a bug in the implementation which should be fixed, rather than a valid reason to say that delegation of cgroups should be fundamentally not supported.

Do you have a list of such issues which you see with delegation? That is, cases where, if ownership of a subtree is granted to a non-root user, that user can affect tasks owned by other users who are in other cgroups?

-serge

Tejun Heo

2013-11-04 22:06 UTC #11 lore raw

Hello,

Quoted 4 lines · Serge E. Hallyn

Do you have a list of such issues which you see with delegation? That is, cases where, if ownership of a subtree is granted to a non-root user, that user can affect tasks owned by other users who are in other cgroups?

A lot of security is about logistics and cgroup simply doesn't have them - depth, number of cgroups quota, even config changes or subdirectory operations which involve RCU operations can easily be used for DoS attacks. Just think about how much complexity and effort need to be spent on making and maintaining anything properly delegatable to !priv users. cgroup has never spent such design or implementation effort - e.g. take a look at how event_control thing is implemented, it's extremely easy to trigger OOM if you give that out to !priv users.

cgroup has *never* been safe to give out to !priv users and it is highly unlikely to be in any foreseeable future. It will be a big new giant feature which I frankly don't think is worth the risk or effort. Think of it as giving out sysctl or firewall rule control to !priv users. Giving out subset of those controls do make sense in terms of function but we don't do that and don't have infrastructure to support such usage. cgroup at this stage isn't that different. If you insist on doing that, you can but it is severely compromising in terms of security and it'll stay that way for the foreseeable future.

Thanks.

Signature · 1 line

tejun

Serge E. Hallyn

maintainer

2013-10-23 00:41 UTC #12 lore raw

In reply to Tejun Heo

Quoted 11 lines · Tejun Heo

Quoted 2 lines · Serge Hallyn
This doesn't delegate it into the container. It allows me, on the host, to set the cgroup for a container.
Hmmm? I'm a bit confused. Isn't the description saying that the patch allows pseudo-root in userns to change cgroup membership even if it isn't actually root?
Besides, I find the whole check rather bogus and would actually much prefer just nuking the check and just follow the standard permission checks.

Can we please nuke it like this then?

From b840083ec8fa1f0645ae925c79db3dc51edd019c Mon Sep 17 00:00:00 2001
From: Serge Hallyn <serge.hallyn-GeWIH/[email protected]>
Date: Wed, 23 Oct 2013 01:34:00 +0200
Subject: [PATCH 1/1] device_cgroup: remove can_attach

It is really only wanting to duplicate a check which is already done by the cgroup subsystem.

With this patch, user jdoe still cannot move pid 1 into a devices cgroup he owns, but now he can move his own other tasks into devices cgroups.

Signed-off-by Serge Hallyn <serge.hallyn-GeWIH/[email protected]>

Cc Aristeu Rozanski <[email protected]>

Cc Tejun Heo <[email protected]>

---
 security/device_cgroup.c | 11 -----------
 1 file changed, 11 deletions(-)

security/device_cgroup.c +0 ~~-11~~

index efc6f68..a37c054 100644
@@ -64,16 +64,6 @@ static inline struct dev_cgroup *task_devcgroup(struct task_struct *task)
 
 struct cgroup_subsys devices_subsys;
 
-static int devcgroup_can_attach(struct cgroup_subsys_state *new_css,
-				struct cgroup_taskset *set)
-{
-	struct task_struct *task = cgroup_taskset_first(set);
-
-	if (current != task && !capable(CAP_SYS_ADMIN))
-		return -EPERM;
-	return 0;
-}
-
 /*
  * called under devcgroup_mutex
  */

@@ -698,7 +688,6 @@ static struct cftype dev_cgroup_files[] = {
 
 struct cgroup_subsys devices_subsys = {
 	.name = "devices",
-	.can_attach = devcgroup_can_attach,
 	.css_alloc = devcgroup_css_alloc,
 	.css_free = devcgroup_css_free,
 	.css_online = devcgroup_online,

Signature · 1 line

1.8.3.2

Tejun Heo

Applied

2013-10-24 10:57 UTC #13 lore raw

Quoted 23 lines · Serge E. Hallyn

Quoted 11 lines · Tejun Heo
Quoted 2 lines · Serge Hallyn
This doesn't delegate it into the container. It allows me, on the host, to set the cgroup for a container.
Hmmm? I'm a bit confused. Isn't the description saying that the patch allows pseudo-root in userns to change cgroup membership even if it isn't actually root?
Besides, I find the whole check rather bogus and would actually much prefer just nuking the check and just follow the standard permission checks.
Can we please nuke it like this then?
From b840083ec8fa1f0645ae925c79db3dc51edd019c Mon Sep 17 00:00:00 2001
From: Serge Hallyn <serge.hallyn-GeWIH/[email protected]>
Date: Wed, 23 Oct 2013 01:34:00 +0200
Subject: [PATCH 1/1] device_cgroup: remove can_attach
It is really only wanting to duplicate a check which is already done by the cgroup subsystem.

Quoted 6 lines

With this patch, user jdoe still cannot move pid 1 into a devices cgroup he owns, but now he can move his own other tasks into devices cgroups.
Signed-off-by Serge Hallyn <serge.hallyn-GeWIH/[email protected]>
Cc Aristeu Rozanski <[email protected]>
Cc Tejun Heo <[email protected]>

Applied to cgroup/for-3.13. Thanks.

Signature · 1 line

tejun