KEYS: allow changing key ownership with CAP_SYS_ADMIN in a NS

Submitted by Dimitri John Ledkov on Oct. 3, 2017, 2:45 a.m.

Details

Message ID 20171003024528.28242-1-xnox@ubuntu.com
State New
Series "KEYS: allow changing key ownership with CAP_SYS_ADMIN in a NS"
Headers show

Commit Message

Dimitri John Ledkov Oct. 3, 2017, 2:45 a.m.
Currently, changing key ownership from one namespaced uid/gid to
another namespaced uid/gid is only allowed by processes that have
CAP_SYS_ADMIN in the intial namespace. Fix the capability check to
also check the capability in the current capability.

Fixes: https://github.com/systemd/systemd/issues/6281
Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com>
---

 Dear containers mailing list,

 There is now userspace code that uses kernel keyring, in
 user-namespaces, and tries to chown the keyrings from one namespaced
 uid/gid to another. See systemd keyring code to store invocation id
 in src/core/execute.c.

 This code fails under system user-namespace containers, such as
 OpenVZ, LXC, LXD.

 Setup for a reproducer:
 ## enter a user-names, however you like. Using lxd as an example
 $ lxc init ubunt-daily:a test-keyring
 $ lxc exec test-keyring bash
 # apt install keyutils

 Reproducer:
 # keyctl session
 Joined session keyring: 556756508
 # keyctl chown 556756508 1000
 keyctl_chown: Permission denied
 # keyctl chown 556756508 0
 # echo $?

 The permission denied is unexpected, and this patch resolves
 this. I've tested this patch by recompiling Ubuntu kernel with this
 patch applied and testing above in a VM.

 Some prior art. When user namespaces were written in 2011, code to
 switch from capable to ns_capable was written, but eventually not
 merged due to siding on a being more conservative, as mentioned in
 the pull request in 2012. Given that user-namespaces are more mature
 now, and actually in active use, it is time to lax capability checks
 to probe user namespace rather than initial naspace.

 Please consider applying this patch.

 References to prior art:
 https://lists.onap.org/pipermail/containers/2011-September/028137.html
 https://github.com/torvalds/linux/commit/437589a74b6a590d175f86cf9f7b2efcee7765e7

      Ultimately there will be work to relax privlige checks from
     "capable(FOO)" to "ns_capable(user_ns, FOO)" where it is safe
     allowing root in a user names to do those things that today we
     only forbid to non-root users because it will confuse suid root
     applications.

 security/keys/keyctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index ab0b337c84b4..dc554bb80325 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -822,65 +822,65 @@  long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
 	struct key_user *newowner, *zapowner = NULL;
 	struct key *key;
 	key_ref_t key_ref;
 	long ret;
 	kuid_t uid;
 	kgid_t gid;
 
 	uid = make_kuid(current_user_ns(), user);
 	gid = make_kgid(current_user_ns(), group);
 	ret = -EINVAL;
 	if ((user != (uid_t) -1) && !uid_valid(uid))
 		goto error;
 	if ((group != (gid_t) -1) && !gid_valid(gid))
 		goto error;
 
 	ret = 0;
 	if (user == (uid_t) -1 && group == (gid_t) -1)
 		goto error;
 
 	key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL,
 				  KEY_NEED_SETATTR);
 	if (IS_ERR(key_ref)) {
 		ret = PTR_ERR(key_ref);
 		goto error;
 	}
 
 	key = key_ref_to_ptr(key_ref);
 
 	/* make the changes with the locks held to prevent chown/chown races */
 	ret = -EACCES;
 	down_write(&key->sem);
 
-	if (!capable(CAP_SYS_ADMIN)) {
+	if (!ns_capable(current_user_ns(), CAP_SYS_ADMIN)) {
 		/* only the sysadmin can chown a key to some other UID */
 		if (user != (uid_t) -1 && !uid_eq(key->uid, uid))
 			goto error_put;
 
 		/* only the sysadmin can set the key's GID to a group other
 		 * than one of those that the current process subscribes to */
 		if (group != (gid_t) -1 && !gid_eq(gid, key->gid) && !in_group_p(gid))
 			goto error_put;
 	}
 
 	/* change the UID */
 	if (user != (uid_t) -1 && !uid_eq(uid, key->uid)) {
 		ret = -ENOMEM;
 		newowner = key_user_lookup(uid);
 		if (!newowner)
 			goto error_put;
 
 		/* transfer the quota burden to the new user */
 		if (test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) {
 			unsigned maxkeys = uid_eq(uid, GLOBAL_ROOT_UID) ?
 				key_quota_root_maxkeys : key_quota_maxkeys;
 			unsigned maxbytes = uid_eq(uid, GLOBAL_ROOT_UID) ?
 				key_quota_root_maxbytes : key_quota_maxbytes;
 
 			spin_lock(&newowner->lock);
 			if (newowner->qnkeys + 1 >= maxkeys ||
 			    newowner->qnbytes + key->quotalen >= maxbytes ||
 			    newowner->qnbytes + key->quotalen <
 			    newowner->qnbytes)
 				goto quota_overrun;
 
 			newowner->qnkeys++;

Comments

Eric W. Biederman Oct. 3, 2017, 3:30 a.m.
Dimitri John Ledkov <xnox@ubuntu.com> writes:

> Currently, changing key ownership from one namespaced uid/gid to
> another namespaced uid/gid is only allowed by processes that have
> CAP_SYS_ADMIN in the intial namespace. Fix the capability check to
> also check the capability in the current capability.

Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>

I won't deny the issue, but unless I am misreading something this
will allow me to change the the uid of any key simply by unsharing
a user namespace.  At which point there is no point in having a
permission check at all.


> Fixes: https://github.com/systemd/systemd/issues/6281
> Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com>
> ---
>
>  Dear containers mailing list,
>
>  There is now userspace code that uses kernel keyring, in
>  user-namespaces, and tries to chown the keyrings from one namespaced
>  uid/gid to another. See systemd keyring code to store invocation id
>  in src/core/execute.c.
>
>  This code fails under system user-namespace containers, such as
>  OpenVZ, LXC, LXD.
>
>  Setup for a reproducer:
>  ## enter a user-names, however you like. Using lxd as an example
>  $ lxc init ubunt-daily:a test-keyring
>  $ lxc exec test-keyring bash
>  # apt install keyutils
>
>  Reproducer:
>  # keyctl session
>  Joined session keyring: 556756508
>  # keyctl chown 556756508 1000
>  keyctl_chown: Permission denied
>  # keyctl chown 556756508 0
>  # echo $?
>
>  The permission denied is unexpected, and this patch resolves
>  this. I've tested this patch by recompiling Ubuntu kernel with this
>  patch applied and testing above in a VM.
>
>  Some prior art. When user namespaces were written in 2011, code to
>  switch from capable to ns_capable was written, but eventually not
>  merged due to siding on a being more conservative, as mentioned in
>  the pull request in 2012. Given that user-namespaces are more mature
>  now, and actually in active use, it is time to lax capability checks
>  to probe user namespace rather than initial naspace.
>
>  Please consider applying this patch.
>
>  References to prior art:
>  https://lists.onap.org/pipermail/containers/2011-September/028137.html
>  https://github.com/torvalds/linux/commit/437589a74b6a590d175f86cf9f7b2efcee7765e7
>
>       Ultimately there will be work to relax privlige checks from
>      "capable(FOO)" to "ns_capable(user_ns, FOO)" where it is safe
>      allowing root in a user names to do those things that today we
>      only forbid to non-root users because it will confuse suid root
>      applications.
>
>  security/keys/keyctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index ab0b337c84b4..dc554bb80325 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -822,65 +822,65 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
>  	struct key_user *newowner, *zapowner = NULL;
>  	struct key *key;
>  	key_ref_t key_ref;
>  	long ret;
>  	kuid_t uid;
>  	kgid_t gid;
>  
>  	uid = make_kuid(current_user_ns(), user);
>  	gid = make_kgid(current_user_ns(), group);
>  	ret = -EINVAL;
>  	if ((user != (uid_t) -1) && !uid_valid(uid))
>  		goto error;
>  	if ((group != (gid_t) -1) && !gid_valid(gid))
>  		goto error;
>  
>  	ret = 0;
>  	if (user == (uid_t) -1 && group == (gid_t) -1)
>  		goto error;
>  
>  	key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL,
>  				  KEY_NEED_SETATTR);
>  	if (IS_ERR(key_ref)) {
>  		ret = PTR_ERR(key_ref);
>  		goto error;
>  	}
>  
>  	key = key_ref_to_ptr(key_ref);
>  
>  	/* make the changes with the locks held to prevent chown/chown races */
>  	ret = -EACCES;
>  	down_write(&key->sem);
>  
> -	if (!capable(CAP_SYS_ADMIN)) {
> +	if (!ns_capable(current_user_ns(), CAP_SYS_ADMIN)) {
>  		/* only the sysadmin can chown a key to some other UID */
>  		if (user != (uid_t) -1 && !uid_eq(key->uid, uid))
>  			goto error_put;
>  
>  		/* only the sysadmin can set the key's GID to a group other
>  		 * than one of those that the current process subscribes to */
>  		if (group != (gid_t) -1 && !gid_eq(gid, key->gid) && !in_group_p(gid))
>  			goto error_put;
>  	}
>  
>  	/* change the UID */
>  	if (user != (uid_t) -1 && !uid_eq(uid, key->uid)) {
>  		ret = -ENOMEM;
>  		newowner = key_user_lookup(uid);
>  		if (!newowner)
>  			goto error_put;
>  
>  		/* transfer the quota burden to the new user */
>  		if (test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) {
>  			unsigned maxkeys = uid_eq(uid, GLOBAL_ROOT_UID) ?
>  				key_quota_root_maxkeys : key_quota_maxkeys;
>  			unsigned maxbytes = uid_eq(uid, GLOBAL_ROOT_UID) ?
>  				key_quota_root_maxbytes : key_quota_maxbytes;
>  
>  			spin_lock(&newowner->lock);
>  			if (newowner->qnkeys + 1 >= maxkeys ||
>  			    newowner->qnbytes + key->quotalen >= maxbytes ||
>  			    newowner->qnbytes + key->quotalen <
>  			    newowner->qnbytes)
>  				goto quota_overrun;
>  
>  			newowner->qnkeys++;
Serge E. Hallyn Oct. 3, 2017, 4:11 a.m.
On Mon, Oct 02, 2017 at 10:30:43PM -0500, Eric W. Biederman wrote:
> Dimitri John Ledkov <xnox@ubuntu.com> writes:
> 
> > Currently, changing key ownership from one namespaced uid/gid to
> > another namespaced uid/gid is only allowed by processes that have
> > CAP_SYS_ADMIN in the intial namespace. Fix the capability check to
> > also check the capability in the current capability.
> 
> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> 
> I won't deny the issue, but unless I am misreading something this
> will allow me to change the the uid of any key simply by unsharing
> a user namespace.  At which point there is no point in having a
> permission check at all.

Right so without having looked closely, at the very least you need to
verify that the ucrrent user is privileged over key->{uid,gid} and
over @user and @group.  Now the latter is I *think* being done
implicitly by the make_kuid(current_user_ns, user) at the top.  So
you need to further verify that key->uid and key->gid are mapped into
current_user_ns.

That *may* be sufficient.

-serge
Eric W. Biederman Oct. 3, 2017, 2:51 p.m.
"Serge E. Hallyn" <serge@hallyn.com> writes:

> On Mon, Oct 02, 2017 at 10:30:43PM -0500, Eric W. Biederman wrote:
>> Dimitri John Ledkov <xnox@ubuntu.com> writes:
>> 
>> > Currently, changing key ownership from one namespaced uid/gid to
>> > another namespaced uid/gid is only allowed by processes that have
>> > CAP_SYS_ADMIN in the intial namespace. Fix the capability check to
>> > also check the capability in the current capability.
>> 
>> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> 
>> I won't deny the issue, but unless I am misreading something this
>> will allow me to change the the uid of any key simply by unsharing
>> a user namespace.  At which point there is no point in having a
>> permission check at all.
>
> Right so without having looked closely, at the very least you need to
> verify that the ucrrent user is privileged over key->{uid,gid} and
> over @user and @group.  Now the latter is I *think* being done
> implicitly by the make_kuid(current_user_ns, user) at the top.  So
> you need to further verify that key->uid and key->gid are mapped into
> current_user_ns.
>
> That *may* be sufficient.

Yes.  It sounds like either we need to change something in the
implementation of keys so they have a clear user namespace owner
or implement capable_wrt_key_uidgid.

The latter is tricky so at the very least I would prefer it have a
function of it's own.  Just so people don't handroll the necessary
pattern incorrectly at different places.

Eric
Serge E. Hallyn Oct. 3, 2017, 2:56 p.m.
On Tue, Oct 03, 2017 at 09:51:14AM -0500, Eric W. Biederman wrote:
> "Serge E. Hallyn" <serge@hallyn.com> writes:
> 
> > On Mon, Oct 02, 2017 at 10:30:43PM -0500, Eric W. Biederman wrote:
> >> Dimitri John Ledkov <xnox@ubuntu.com> writes:
> >> 
> >> > Currently, changing key ownership from one namespaced uid/gid to
> >> > another namespaced uid/gid is only allowed by processes that have
> >> > CAP_SYS_ADMIN in the intial namespace. Fix the capability check to
> >> > also check the capability in the current capability.
> >> 
> >> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> >> 
> >> I won't deny the issue, but unless I am misreading something this
> >> will allow me to change the the uid of any key simply by unsharing
> >> a user namespace.  At which point there is no point in having a
> >> permission check at all.
> >
> > Right so without having looked closely, at the very least you need to
> > verify that the ucrrent user is privileged over key->{uid,gid} and
> > over @user and @group.  Now the latter is I *think* being done
> > implicitly by the make_kuid(current_user_ns, user) at the top.  So
> > you need to further verify that key->uid and key->gid are mapped into
> > current_user_ns.
> >
> > That *may* be sufficient.
> 
> Yes.  It sounds like either we need to change something in the
> implementation of keys so they have a clear user namespace owner
> or implement capable_wrt_key_uidgid.
> 
> The latter is tricky so at the very least I would prefer it have a
> function of it's own.  Just so people don't handroll the necessary
> pattern incorrectly at different places.

Right, capable_wrt_inode_uid_gid was of course what I had in mind :)

Dimitri, do you want to post something along those lines?
David Howells Oct. 3, 2017, 3:04 p.m.
Eric W. Biederman <ebiederm@xmission.com> wrote:

> Yes.  It sounds like either we need to change something in the
> implementation of keys so they have a clear user namespace owner
> or implement capable_wrt_key_uidgid.

I'm thinking on the lines of making keys belong to a namespace in some way,
and automatically invalidating them when the owning namespace is deleted.
This will cause all links to them to be gc'd and thence the keys themselves.

David
Dimitri John Ledkov Oct. 14, 2017, 8:59 p.m.
On 3 October 2017 at 04:30, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Dimitri John Ledkov <xnox@ubuntu.com> writes:
>
>> Currently, changing key ownership from one namespaced uid/gid to
>> another namespaced uid/gid is only allowed by processes that have
>> CAP_SYS_ADMIN in the intial namespace. Fix the capability check to
>> also check the capability in the current capability.
>
> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> I won't deny the issue, but unless I am misreading something this
> will allow me to change the the uid of any key simply by unsharing
> a user namespace.  At which point there is no point in having a
> permission check at all.
>

Reading the code I do not believe this is true. But I'm not sure how
we can prove this.

My understanding is that chown operation is change of source uid/guid
to some target uid/guid.

Source uid/gid are validated by calling lookup_user_key() and this is
where the source ACL is asserted. I am not sure about the flags
requested, but it looks to me that ctx.cred = get_current_cred() is
used in key_task_permission() already and thus already requires the
owner of the key, or be in the group, or the other permissions to
allow setattr permission, and on top of that LSM module must ok this
too. What I'm not sure about is if the "other" permissions check is
correctly namespaced, since "other" permissions should not imho leak
between user namespaces.

If lookup_user_key() is not allowed by get_current_cred() the function
bails out, even before doing capability check.

The capability check validates the target uid/gid only. And it decides
whether the change of source -> target is deemed to be permissible,
when the current process is otherwise allowed to tinker with the
source key. We already look up target uid/gid relative current user
namespace thus both target & source uid/gid are from the same
namespace. And we then allow change of group by the key owner, within
the groups one is in, and we only allow uid change for CAP_SYS_ADMIN.
However the CAP_SYS_ADMIN we demand here is the one from initial
namespace, rather than from current user namespace.

Does above assessment addresses your immediate concern?
Is my understanding of this code flawed, especially the bit about the
security of lookup_user_key()?

I do want to write test cases to assert current behavior and
experimentally assert change of behavior with my patch applied. Is
there existing test-suite / unit-tests for changing key permissions?
(e.g. some keyctl test-suite and/or some namespace tests?) Any tips /
pointers of where that should live, or if there are any existing unit
tests w.r.t. to this functionality?

Regards,

Dimitri.

>
>> Fixes: https://github.com/systemd/systemd/issues/6281
>> Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com>
>> ---
>>
>>  Dear containers mailing list,
>>
>>  There is now userspace code that uses kernel keyring, in
>>  user-namespaces, and tries to chown the keyrings from one namespaced
>>  uid/gid to another. See systemd keyring code to store invocation id
>>  in src/core/execute.c.
>>
>>  This code fails under system user-namespace containers, such as
>>  OpenVZ, LXC, LXD.
>>
>>  Setup for a reproducer:
>>  ## enter a user-names, however you like. Using lxd as an example
>>  $ lxc init ubunt-daily:a test-keyring
>>  $ lxc exec test-keyring bash
>>  # apt install keyutils
>>
>>  Reproducer:
>>  # keyctl session
>>  Joined session keyring: 556756508
>>  # keyctl chown 556756508 1000
>>  keyctl_chown: Permission denied
>>  # keyctl chown 556756508 0
>>  # echo $?
>>
>>  The permission denied is unexpected, and this patch resolves
>>  this. I've tested this patch by recompiling Ubuntu kernel with this
>>  patch applied and testing above in a VM.
>>
>>  Some prior art. When user namespaces were written in 2011, code to
>>  switch from capable to ns_capable was written, but eventually not
>>  merged due to siding on a being more conservative, as mentioned in
>>  the pull request in 2012. Given that user-namespaces are more mature
>>  now, and actually in active use, it is time to lax capability checks
>>  to probe user namespace rather than initial naspace.
>>
>>  Please consider applying this patch.
>>
>>  References to prior art:
>>  https://lists.onap.org/pipermail/containers/2011-September/028137.html
>>  https://github.com/torvalds/linux/commit/437589a74b6a590d175f86cf9f7b2efcee7765e7
>>
>>       Ultimately there will be work to relax privlige checks from
>>      "capable(FOO)" to "ns_capable(user_ns, FOO)" where it is safe
>>      allowing root in a user names to do those things that today we
>>      only forbid to non-root users because it will confuse suid root
>>      applications.
>>
>>  security/keys/keyctl.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
>> index ab0b337c84b4..dc554bb80325 100644
>> --- a/security/keys/keyctl.c
>> +++ b/security/keys/keyctl.c
>> @@ -822,65 +822,65 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
>>       struct key_user *newowner, *zapowner = NULL;
>>       struct key *key;
>>       key_ref_t key_ref;
>>       long ret;
>>       kuid_t uid;
>>       kgid_t gid;
>>
>>       uid = make_kuid(current_user_ns(), user);
>>       gid = make_kgid(current_user_ns(), group);
>>       ret = -EINVAL;
>>       if ((user != (uid_t) -1) && !uid_valid(uid))
>>               goto error;
>>       if ((group != (gid_t) -1) && !gid_valid(gid))
>>               goto error;
>>
>>       ret = 0;
>>       if (user == (uid_t) -1 && group == (gid_t) -1)
>>               goto error;
>>
>>       key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL,
>>                                 KEY_NEED_SETATTR);
>>       if (IS_ERR(key_ref)) {
>>               ret = PTR_ERR(key_ref);
>>               goto error;
>>       }
>>
>>       key = key_ref_to_ptr(key_ref);
>>
>>       /* make the changes with the locks held to prevent chown/chown races */
>>       ret = -EACCES;
>>       down_write(&key->sem);
>>
>> -     if (!capable(CAP_SYS_ADMIN)) {
>> +     if (!ns_capable(current_user_ns(), CAP_SYS_ADMIN)) {
>>               /* only the sysadmin can chown a key to some other UID */
>>               if (user != (uid_t) -1 && !uid_eq(key->uid, uid))
>>                       goto error_put;
>>
>>               /* only the sysadmin can set the key's GID to a group other
>>                * than one of those that the current process subscribes to */
>>               if (group != (gid_t) -1 && !gid_eq(gid, key->gid) && !in_group_p(gid))
>>                       goto error_put;
>>       }
>>
>>       /* change the UID */
>>       if (user != (uid_t) -1 && !uid_eq(uid, key->uid)) {
>>               ret = -ENOMEM;
>>               newowner = key_user_lookup(uid);
>>               if (!newowner)
>>                       goto error_put;
>>
>>               /* transfer the quota burden to the new user */
>>               if (test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) {
>>                       unsigned maxkeys = uid_eq(uid, GLOBAL_ROOT_UID) ?
>>                               key_quota_root_maxkeys : key_quota_maxkeys;
>>                       unsigned maxbytes = uid_eq(uid, GLOBAL_ROOT_UID) ?
>>                               key_quota_root_maxbytes : key_quota_maxbytes;
>>
>>                       spin_lock(&newowner->lock);
>>                       if (newowner->qnkeys + 1 >= maxkeys ||
>>                           newowner->qnbytes + key->quotalen >= maxbytes ||
>>                           newowner->qnbytes + key->quotalen <
>>                           newowner->qnbytes)
>>                               goto quota_overrun;
>>
>>                       newowner->qnkeys++;