Message ID | 1506366664-4557-1-git-send-email-xnox@ubuntu.com |
---|---|
State | New |
Headers | show |
Series | KEYS: allow changing key ownership with CAP_SYS_ADMIN in a NS | expand |
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@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@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On 26 September 2017 at 12:05, Seth Forshee <seth.forshee@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@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@lists.ubuntu.com >> https://lists.ubuntu.com/mailman/listinfo/kernel-team
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++;
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@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. Regards, Dimitri. security/keys/keyctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)