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

[ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org (Eric W. Biederman)]

Dimitri John Ledkov <xnox-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> 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-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>

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-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
> ---
>
>  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++;