Dimitri John Ledkov <xnox@xxxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxxx> > --- > > 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++; _______________________________________________ Containers mailing list Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/containers
