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

["Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>]

On Tue, Oct 03, 2017 at 09:51:14AM -0500, Eric W. Biederman wrote:
> "Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org> writes:
> 
> > On Mon, Oct 02, 2017 at 10:30:43PM -0500, Eric W. Biederman wrote:
> >> 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.
> >
> > Right so without having looked closely, at the very least you need to
> > verify that the ucrrent user is privileged over key->{uid,gid} and
> > over @user and @group.  Now the latter is I *think* being done
> > implicitly by the make_kuid(current_user_ns, user) at the top.  So
> > you need to further verify that key->uid and key->gid are mapped into
> > current_user_ns.
> >
> > That *may* be sufficient.
> 
> Yes.  It sounds like either we need to change something in the
> implementation of keys so they have a clear user namespace owner
> or implement capable_wrt_key_uidgid.
> 
> The latter is tricky so at the very least I would prefer it have a
> function of it's own.  Just so people don't handroll the necessary
> pattern incorrectly at different places.

Right, capable_wrt_inode_uid_gid was of course what I had in mind :)

Dimitri, do you want to post something along those lines?