Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758511AbeAIPK5 (ORCPT + 1 other); Tue, 9 Jan 2018 10:10:57 -0500 Received: from mail-yw0-f179.google.com ([209.85.161.179]:45630 "EHLO mail-yw0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751846AbeAIPKz (ORCPT ); Tue, 9 Jan 2018 10:10:55 -0500 X-Google-Smtp-Source: ACJfBosfarMBBRyr2ta0tWAn1gftlU8adGpK2/JA0ypWg8UQuSR2ijSFfY1zQI6qM0umo1XNtoMxcTyBUXISqHkfU+w= MIME-Version: 1.0 In-Reply-To: <20180105192407.GF22430@wotan.suse.de> References: <20180105192407.GF22430@wotan.suse.de> From: Dongsu Park Date: Tue, 9 Jan 2018 16:10:54 +0100 Message-ID: Subject: Re: [PATCH 03/11] fs: Allow superblock owner to change ownership of inodes To: "Luis R. Rodriguez" Cc: LKML , Linux Containers , Alban Crequy , "Eric W . Biederman" , Miklos Szeredi , Seth Forshee , Sargun Dhillon , linux-fsdevel@vger.kernel.org, Alexander Viro , Kees Cook Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: Hi, On Fri, Jan 5, 2018 at 8:24 PM, Luis R. Rodriguez wrote: > On Fri, Dec 22, 2017 at 03:32:27PM +0100, Dongsu Park wrote: >> 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; > > I think this patch would read much better and easier to review if it was > split up by first adding the helpers, and then extending them afterwards. I'm fine with splitting it up into multiple patches, if the original author Eric agrees. >> /* 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; > > Are we sure proc is the only special one? How was it observed first that this was > require for proc? Has anyone tried fuzzing by trying this op with a slew of other > filesystems on all files? >From my limited knowledge about procfs, I suppose that procfs is a little different from ordinary filesystems. Procfs is not exactly namespaced, it has many inconsistencies. Some files under /proc should be owned by the global root, regardless of user namespaces. That's why we need to handle such special cases for proc. As it has been historically like that since the beginning, it's hard to change it fundamentally. However, you have good points. Other than procfs, there could be other filesystems that have potential issues when relaxing privileges. Question is how we can be sure that there's no hidden issues. From my understanding, usually we could run testsuites like LTP (https://github.com/linux-test-project/ltp.git) to avoid such regressions. Today I have run LTP tests for fs & containers, with the patchset included. It seemed to work fine without failures. Obviously it doesn't mean that it's completely bug-free, when we are talking about unknown issues. Please let me know if there are other good ways to figure out potential issues. Thanks, Dongsu > Luis