diff mbox series

KEYS: allow changing key ownership with CAP_SYS_ADMIN in a NS

Message ID 1506366664-4557-1-git-send-email-xnox@ubuntu.com
State New
Headers show
Series KEYS: allow changing key ownership with CAP_SYS_ADMIN in a NS | expand

Commit Message

Dimitri John Ledkov Sept. 25, 2017, 7:11 p.m. UTC 
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/pull/6876
Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com>
---

 Dear Ubuntu Kernel Team,

 I am consider to submit this patch upstream. Could you please review
 this patch, before I do so?

 I've generated the patch with the full function context, to point out
 that it does make_kuid/make_kgid using current_user_ns() but not the
 capability check. Which imho is silly.

 Regards,

 Dimitri.

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

Comments

Seth Forshee Sept. 26, 2017, 4:05 p.m. UTC | #1 
On Mon, Sep 25, 2017 at 08:11:04PM +0100, Dimitri John Ledkov wrote:
> 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/pull/6876
> Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com>
> ---
> 
>  Dear Ubuntu Kernel Team,
> 
>  I am consider to submit this patch upstream. Could you please review
>  this patch, before I do so?
> 
>  I've generated the patch with the full function context, to point out
>  that it does make_kuid/make_kgid using current_user_ns() but not the
>  capability check. Which imho is silly.

At first blush I think this makes sense. Since the ids are interpreted
relative to current_user_ns() then CAP_SYS_ADMIN in that ns must have
privileges wrt both of those ids. Given that I can't see what harm there
would be in allowing the chown to proceed. This seems very analgous to
what capable_wrt_inode_uidgid() checks, but for a key rather than an
inode

Having said that, I'm not all that experienced with the issue of keys in
the kernel so I may well be overlooking something important.

Seth

> 
>  Regards,
> 
>  Dimitri.
> 
>  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++;
> -- 
> 2.14.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Dimitri John Ledkov Sept. 26, 2017, 4:09 p.m. UTC | #2 
On 26 September 2017 at 12:05, Seth Forshee <seth.forshee@canonical.com> wrote:
> On Mon, Sep 25, 2017 at 08:11:04PM +0100, Dimitri John Ledkov wrote:
>> 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/pull/6876
>> Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com>
>> ---
>>
>>  Dear Ubuntu Kernel Team,
>>
>>  I am consider to submit this patch upstream. Could you please review
>>  this patch, before I do so?
>>
>>  I've generated the patch with the full function context, to point out
>>  that it does make_kuid/make_kgid using current_user_ns() but not the
>>  capability check. Which imho is silly.
>
> At first blush I think this makes sense. Since the ids are interpreted
> relative to current_user_ns() then CAP_SYS_ADMIN in that ns must have
> privileges wrt both of those ids. Given that I can't see what harm there
> would be in allowing the chown to proceed. This seems very analgous to
> what capable_wrt_inode_uidgid() checks, but for a key rather than an
> inode
>
> Having said that, I'm not all that experienced with the issue of keys in
> the kernel so I may well be overlooking something important.
>

Ack. I will try to figure out which mailing list to send this to
upstream (either namespace people or keyring people or both).

(I hope the patch formatting did not seem odd / etc)

Regards,

Dimitri.


> Seth
>
>>
>>  Regards,
>>
>>  Dimitri.
>>
>>  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++;
>> --
>> 2.14.1
>>
>>
>> --
>> kernel-team mailing list
>> kernel-team@lists.ubuntu.com
>> https://lists.ubuntu.com/mailman/listinfo/kernel-team
diff mbox series

Patch

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++;