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

[Dimitri John Ledkov]

On 26 September 2017 at 12:05, Seth Forshee <seth.forshee at 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 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.
>

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



-- 
Regards,

Dimitri.