Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757288AbdLWDRo (ORCPT ); Fri, 22 Dec 2017 22:17:44 -0500 Received: from h2.hallyn.com ([78.46.35.8]:53440 "EHLO h2.hallyn.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756885AbdLWDRl (ORCPT ); Fri, 22 Dec 2017 22:17:41 -0500 Date: Fri, 22 Dec 2017 21:17:39 -0600 From: "Serge E. Hallyn" To: Dongsu Park Cc: linux-kernel@vger.kernel.org, containers@lists.linux-foundation.org, Alban Crequy , "Eric W . Biederman" , Miklos Szeredi , Seth Forshee , Sargun Dhillon , linux-fsdevel@vger.kernel.org, Alexander Viro , "Luis R. Rodriguez" , Kees Cook Subject: Re: [PATCH 03/11] fs: Allow superblock owner to change ownership of inodes Message-ID: <20171223031739.GC6837@mail.hallyn.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6498 Lines: 178 On Fri, Dec 22, 2017 at 03:32:27PM +0100, Dongsu Park wrote: > From: Eric W. Biederman > > Allow users with CAP_SYS_CHOWN over the superblock of a filesystem to Note it is CAP_CHOWN > chown files. 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. > > For the proc filesystem this relaxation of permissions is not safe, as > some files are owned by users (particularly GLOBAL_ROOT_UID) outside > of the control of the mounter of the proc and that would be unsafe to > grant chown access to. So update setattr on proc to disallow changing > files whose uids or gids are outside of proc's s_user_ns. > > 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. > > Patch v4 is available: https://patchwork.kernel.org/patch/8944611/ > > Cc: linux-fsdevel@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: Alexander Viro > Cc: "Luis R. Rodriguez" > Cc: Kees Cook > Inspired-by: Seth Forshee > Signed-off-by: Eric W. Biederman > [saf: Resolve conflicts caused by s/inode_change_ok/setattr_prepare/] > Signed-off-by: Dongsu Park Reviewed-by: Serge Hallyn > --- > fs/attr.c | 34 ++++++++++++++++++++++++++-------- > fs/proc/base.c | 7 +++++++ > fs/proc/generic.c | 7 +++++++ > fs/proc/proc_sysctl.c | 7 +++++++ > 4 files changed, 47 insertions(+), 8 deletions(-) > > diff --git a/fs/attr.c b/fs/attr.c > index 12ffdb6f..bf8e94f3 100644 > --- a/fs/attr.c > +++ b/fs/attr.c > @@ -18,6 +18,30 @@ > #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 (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 (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 +76,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. */ > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 31934cb9..9d50ec92 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -665,10 +665,17 @@ int proc_setattr(struct dentry *dentry, struct iattr *attr) > { > int error; > struct inode *inode = d_inode(dentry); > + struct user_namespace *s_user_ns; > > if (attr->ia_valid & ATTR_MODE) > return -EPERM; > > + /* Don't let anyone mess with weird proc files */ > + s_user_ns = inode->i_sb->s_user_ns; > + if (!kuid_has_mapping(s_user_ns, inode->i_uid) || > + !kgid_has_mapping(s_user_ns, inode->i_gid)) > + return -EPERM; > + > error = setattr_prepare(dentry, attr); > if (error) > return error; > diff --git a/fs/proc/generic.c b/fs/proc/generic.c > index 793a6757..527d46c8 100644 > --- a/fs/proc/generic.c > +++ b/fs/proc/generic.c > @@ -106,8 +106,15 @@ static int proc_notify_change(struct dentry *dentry, struct iattr *iattr) > { > struct inode *inode = d_inode(dentry); > struct proc_dir_entry *de = PDE(inode); > + struct user_namespace *s_user_ns; > int error; > > + /* Don't let anyone mess with weird proc files */ > + s_user_ns = inode->i_sb->s_user_ns; > + if (!kuid_has_mapping(s_user_ns, inode->i_uid) || > + !kgid_has_mapping(s_user_ns, inode->i_gid)) > + return -EPERM; > + > error = setattr_prepare(dentry, iattr); > if (error) > return error; > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > index c5cbbdff..0f9562d1 100644 > --- a/fs/proc/proc_sysctl.c > +++ b/fs/proc/proc_sysctl.c > @@ -802,11 +802,18 @@ static int proc_sys_permission(struct inode *inode, int mask) > static int proc_sys_setattr(struct dentry *dentry, struct iattr *attr) > { > struct inode *inode = d_inode(dentry); > + struct user_namespace *s_user_ns; > int error; > > if (attr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID)) > return -EPERM; > > + /* Don't let anyone mess with weird proc files */ > + s_user_ns = inode->i_sb->s_user_ns; > + if (!kuid_has_mapping(s_user_ns, inode->i_uid) || > + !kgid_has_mapping(s_user_ns, inode->i_gid)) > + return -EPERM; > + > error = setattr_prepare(dentry, attr); > if (error) > return error; > -- > 2.13.6