[PATCH] KEYS: allow changing key ownership with CAP_SYS_ADMIN in a NS
Eric W. Biederman
ebiederm at xmission.com
Tue Oct 3 03:30:43 UTC 2017
Dimitri John Ledkov <xnox at 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 at 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 at 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++;
More information about the Containers
mailing list