[PATCH] KEYS: allow changing key ownership with CAP_SYS_ADMIN in a NS

[Seth Forshee]

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 at 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 at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team