From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.mailbox.org ([80.241.60.212]:39896 "EHLO mx1.mailbox.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966192AbeEXWaO (ORCPT ); Thu, 24 May 2018 18:30:14 -0400 Date: Fri, 25 May 2018 00:30:08 +0200 From: Christian Brauner To: "Eric W. Biederman" Cc: Linux Containers , linux-fsdevel@vger.kernel.org, Seth Forshee , "Serge E. Hallyn" , linux-kernel@vger.kernel.org Subject: Re: [REVIEW][PATCH v2 3/6] fs: Allow superblock owner to replace invalid owners of inodes Message-ID: <20180524223008.GA17493@mailbox.org> References: <87o9h6554f.fsf@xmission.com> <20180523232538.4880-3-ebiederm@xmission.com> <87bmd6549i.fsf_-_@xmission.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <87bmd6549i.fsf_-_@xmission.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Wed, May 23, 2018 at 06:41:29PM -0500, Eric W. Biederman wrote: > > Allow users with CAP_SYS_CHOWN over the superblock of a filesystem to > chown files when inode owner is invalid. Ordinarily the > capable_wrt_inode_uidgid check is sufficient to allow access to files > but when the underlying filesystem has uids or gids that don't map to > the current user namespace it is not enough, so the chown permission > checks need to be extended to allow this case. > > Calling chown on filesystem nodes whose uid or gid don't map is > necessary if those nodes are going to be modified as writing back > inodes which contain uids or gids that don't map is likely to cause > filesystem corruption of the uid or gid fields. > > Once chown has been called the existing capable_wrt_inode_uidgid > checks are sufficient to allow the owner of a superblock to do anything > the global root user can do with an appropriate set of capabilities. > > An ordinary filesystem mountable by a userns root will limit all uids > and gids in s_user_ns or the INVALID_UID and INVALID_GID to flag all > others. So having this added permission limited to just INVALID_UID > and INVALID_GID is sufficient to handle every case on an ordinary filesystem. > > Of the virtual filesystems at least proc is known to set s_user_ns to > something other than &init_user_ns, while at the same time presenting > some files owned by GLOBAL_ROOT_UID. Those files the mounter of proc > in a user namespace should not be able to chown to get access to. > Limiting the relaxation in permission to just the minimum of allowing > changing INVALID_UID and INVALID_GID prevents problems with cases like > that. > > The original version of this patch was written by: Seth Forshee. I > have rewritten and rethought this patch enough so it's really not the > same thing (certainly it needs a different description), but he > deserves credit for getting out there and getting the conversation > started, and finding the potential gotcha's and putting up with my > semi-paranoid feedback. Ok, took me a little longer to reason about this. Acked-by: Christian Brauner > > Inspired-by: Seth Forshee > Acked-by: Seth Forshee > Signed-off-by: Eric W. Biederman > --- > > Sigh. In simplifying this change so it would not require a change to > proc (or any other similar filesystem) I accidentally introduced some > badly placed semicolons. The kbuild test robot was very nice and found > those for me. Resend with those unnecessary semicolons removed. > > fs/attr.c | 36 ++++++++++++++++++++++++++++-------- > 1 file changed, 28 insertions(+), 8 deletions(-) > > diff --git a/fs/attr.c b/fs/attr.c > index 12ffdb6fb63c..d0b4d34878fb 100644 > --- a/fs/attr.c > +++ b/fs/attr.c > @@ -18,6 +18,32 @@ > #include > #include > > +static bool chown_ok(const struct inode *inode, kuid_t uid) > +{ > + if (uid_eq(current_fsuid(), inode->i_uid) && > + uid_eq(uid, inode->i_uid)) > + return true; > + if (capable_wrt_inode_uidgid(inode, CAP_CHOWN)) > + return true; > + if (uid_eq(inode->i_uid, INVALID_UID) && > + ns_capable(inode->i_sb->s_user_ns, CAP_CHOWN)) > + return true; > + return false; > +} > + > +static bool chgrp_ok(const struct inode *inode, kgid_t gid) > +{ > + if (uid_eq(current_fsuid(), inode->i_uid) && > + (in_group_p(gid) || gid_eq(gid, inode->i_gid))) > + return true; > + if (capable_wrt_inode_uidgid(inode, CAP_CHOWN)) > + return true; > + if (gid_eq(inode->i_gid, INVALID_GID) && > + ns_capable(inode->i_sb->s_user_ns, CAP_CHOWN)) > + return true; > + return false; > +} > + > /** > * setattr_prepare - check if attribute changes to a dentry are allowed > * @dentry: dentry to check > @@ -52,17 +78,11 @@ int setattr_prepare(struct dentry *dentry, struct iattr *attr) > goto kill_priv; > > /* Make sure a caller can chown. */ > - if ((ia_valid & ATTR_UID) && > - (!uid_eq(current_fsuid(), inode->i_uid) || > - !uid_eq(attr->ia_uid, inode->i_uid)) && > - !capable_wrt_inode_uidgid(inode, CAP_CHOWN)) > + if ((ia_valid & ATTR_UID) && !chown_ok(inode, attr->ia_uid)) > return -EPERM; > > /* Make sure caller can chgrp. */ > - if ((ia_valid & ATTR_GID) && > - (!uid_eq(current_fsuid(), inode->i_uid) || > - (!in_group_p(attr->ia_gid) && !gid_eq(attr->ia_gid, inode->i_gid))) && > - !capable_wrt_inode_uidgid(inode, CAP_CHOWN)) > + if ((ia_valid & ATTR_GID) && !chgrp_ok(inode, attr->ia_gid)) > return -EPERM; > > /* Make sure a caller can chmod. */