linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dongsu Park <dongsu@kinvolk.io>
To: "Luis R. Rodriguez" <mcgrof@kernel.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Linux Containers <containers@lists.linux-foundation.org>,
	Alban Crequy <alban@kinvolk.io>,
	"Eric W . Biederman" <ebiederm@xmission.com>,
	Miklos Szeredi <mszeredi@redhat.com>,
	Seth Forshee <seth.forshee@canonical.com>,
	Sargun Dhillon <sargun@sargun.me>,
	linux-fsdevel@vger.kernel.org,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Kees Cook <keescook@chromium.org>
Subject: Re: [PATCH 03/11] fs: Allow superblock owner to change ownership of inodes
Date: Tue, 9 Jan 2018 16:10:54 +0100	[thread overview]
Message-ID: <CANxcAMvDQFH0g5PPnVZ3p2Tei04N+8fNf0pk02DrfTkBHjjrPQ@mail.gmail.com> (raw)
In-Reply-To: <20180105192407.GF22430@wotan.suse.de>

Hi,

On Fri, Jan 5, 2018 at 8:24 PM, Luis R. Rodriguez <mcgrof@kernel.org> 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 <linux/evm.h>
>>  #include <linux/ima.h>
>>
>> +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

  reply	other threads:[~2018-01-09 15:10 UTC|newest]

Thread overview: 107+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-22 14:32 [PATCH v5 00/11] FUSE mounts from non-init user namespaces Dongsu Park
2017-12-22 14:32 ` [PATCH 01/11] block_dev: Support checking inode permissions in lookup_bdev() Dongsu Park
2017-12-22 18:59   ` Coly Li
2017-12-23 12:00     ` Dongsu Park
2017-12-23  3:03   ` Serge E. Hallyn
2017-12-22 14:32 ` [PATCH 02/11] mtd: Check permissions towards mtd block device inode when mounting Dongsu Park
2017-12-22 21:06   ` Richard Weinberger
2017-12-23 12:18     ` Dongsu Park
2017-12-23 12:56       ` Richard Weinberger
2017-12-23  3:05   ` Serge E. Hallyn
2017-12-22 14:32 ` [PATCH 03/11] fs: Allow superblock owner to change ownership of inodes Dongsu Park
2017-12-23  3:17   ` Serge E. Hallyn
2018-01-05 19:24   ` Luis R. Rodriguez
2018-01-09 15:10     ` Dongsu Park [this message]
2018-01-09 17:23       ` Luis R. Rodriguez
2018-02-13 13:18   ` Miklos Szeredi
2018-02-16 22:00     ` Eric W. Biederman
2017-12-22 14:32 ` [PATCH 04/11] fs: Don't remove suid for CAP_FSETID for userns root Dongsu Park
2017-12-23  3:26   ` Serge E. Hallyn
2017-12-23 12:38     ` Dongsu Park
2018-02-13 13:37       ` Miklos Szeredi
2017-12-22 14:32 ` [PATCH 05/11] fs: Allow superblock owner to access do_remount_sb() Dongsu Park
2017-12-23  3:30   ` Serge E. Hallyn
2017-12-22 14:32 ` [PATCH 06/11] capabilities: Allow privileged user in s_user_ns to set security.* xattrs Dongsu Park
2017-12-23  3:33   ` Serge E. Hallyn
2017-12-22 14:32 ` [PATCH 07/11] fs: Allow CAP_SYS_ADMIN in s_user_ns to freeze and thaw filesystems Dongsu Park
2017-12-23  3:39   ` Serge E. Hallyn
2018-02-14 12:28   ` Miklos Szeredi
2018-02-19 22:56     ` Eric W. Biederman
2017-12-22 14:32 ` [PATCH 08/11] fuse: Support fuse filesystems outside of init_user_ns Dongsu Park
2017-12-23  3:46   ` Serge E. Hallyn
2018-01-17 10:59   ` Alban Crequy
2018-01-17 14:29     ` Seth Forshee
2018-01-17 18:56       ` Alban Crequy
2018-01-17 19:31         ` Seth Forshee
2018-01-18 10:29           ` Alban Crequy
2018-02-12 15:57   ` Miklos Szeredi
2018-02-12 16:35     ` Eric W. Biederman
2018-02-13 10:20       ` Miklos Szeredi
2018-02-16 21:52         ` Eric W. Biederman
2018-02-20  2:12   ` Eric W. Biederman
2017-12-22 14:32 ` [PATCH 09/11] fuse: Restrict allow_other to the superblock's namespace or a descendant Dongsu Park
2017-12-23  3:50   ` Serge E. Hallyn
2018-02-19 23:16   ` Eric W. Biederman
2017-12-22 14:32 ` [PATCH 10/11] fuse: Allow user namespace mounts Dongsu Park
2017-12-23  3:51   ` Serge E. Hallyn
2018-02-14 13:44   ` Miklos Szeredi
2018-02-15  8:46     ` Miklos Szeredi
2017-12-22 14:32 ` [PATCH 11/11] evm: Don't update hmacs in user ns mounts Dongsu Park
2017-12-23  4:03   ` Serge E. Hallyn
2017-12-24  5:12     ` Mimi Zohar
2017-12-24  5:56       ` Mimi Zohar
2017-12-25  7:05 ` [PATCH v5 00/11] FUSE mounts from non-init user namespaces Eric W. Biederman
2018-01-09 15:05   ` Dongsu Park
2018-01-18 14:58     ` Alban Crequy
2018-02-19 23:09       ` Eric W. Biederman
2018-02-13 11:32 ` Miklos Szeredi
2018-02-16 21:53   ` Eric W. Biederman
2018-02-21 20:24 ` [PATCH v6 0/6] fuse: " Eric W. Biederman
2018-02-21 20:29   ` [PATCH v6 1/5] fuse: Remove the buggy retranslation of pids in fuse_dev_do_read Eric W. Biederman
2018-02-22 10:13     ` Miklos Szeredi
2018-02-22 19:04       ` Eric W. Biederman
2018-02-21 20:29   ` [PATCH v6 2/5] fuse: Fail all requests with invalid uids or gids Eric W. Biederman
2018-02-22 10:26     ` Miklos Szeredi
2018-02-22 18:15       ` Eric W. Biederman
2018-02-21 20:29   ` [PATCH v6 3/5] fuse: Support fuse filesystems outside of init_user_ns Eric W. Biederman
2018-02-21 20:29   ` [PATCH v6 4/5] fuse: Ensure posix acls are translated " Eric W. Biederman
2018-02-22 11:40     ` Miklos Szeredi
2018-02-22 19:18       ` Eric W. Biederman
2018-02-22 22:50         ` Eric W. Biederman
2018-02-26  7:47           ` Miklos Szeredi
2018-02-26 16:35             ` Eric W. Biederman
2018-02-26 21:51               ` Eric W. Biederman
2018-02-21 20:29   ` [PATCH v6 5/5] fuse: Restrict allow_other to the superblock's namespace or a descendant Eric W. Biederman
2018-02-26 23:52   ` [PATCH v7 0/7] fuse: mounts from non-init user namespaces Eric W. Biederman
2018-02-26 23:52     ` [PATCH v7 1/7] fuse: Remove the buggy retranslation of pids in fuse_dev_do_read Eric W. Biederman
2018-02-26 23:52     ` [PATCH v7 2/7] fuse: Fail all requests with invalid uids or gids Eric W. Biederman
2018-02-26 23:52     ` [PATCH v7 3/7] fs/posix_acl: Document that get_acl respects ACL_DONT_CACHE Eric W. Biederman
2018-02-27  1:13       ` Linus Torvalds
2018-02-27  2:53         ` Eric W. Biederman
2018-02-27  3:14           ` Eric W. Biederman
2018-02-27  3:41             ` Linus Torvalds
2018-03-02 19:53               ` [RFC][PATCH] fs/posix_acl: Update the comments and support lightweight cache skipping Eric W. Biederman
2018-02-27  3:36           ` [PATCH v7 3/7] fs/posix_acl: Document that get_acl respects ACL_DONT_CACHE Linus Torvalds
2018-02-26 23:52     ` [PATCH v7 4/7] fuse: Cache a NULL acl when FUSE_GETXATTR returns -ENOSYS Eric W. Biederman
2018-02-26 23:53     ` [PATCH v7 5/7] fuse: Simplfiy the posix acl handling logic Eric W. Biederman
2018-02-27  9:00       ` Miklos Szeredi
2018-03-02 21:49         ` Eric W. Biederman
2018-02-26 23:53     ` [PATCH v7 6/7] fuse: Support fuse filesystems outside of init_user_ns Eric W. Biederman
2018-02-26 23:53     ` [PATCH v7 7/7] fuse: Restrict allow_other to the superblock's namespace or a descendant Eric W. Biederman
2018-03-02 21:58     ` [PATCH v8 0/6] fuse: mounts from non-init user namespaces Eric W. Biederman
2018-03-02 21:59       ` [PATCH v8 1/6] fs/posix_acl: Update the comments and support lightweight cache skipping Eric W. Biederman
2018-03-05  9:53         ` Miklos Szeredi
2018-03-05 13:53           ` Eric W. Biederman
2018-03-02 21:59       ` [PATCH v8 2/6] fuse: Simplfiy the posix acl handling logic Eric W. Biederman
2018-03-02 21:59       ` [PATCH v8 3/6] fuse: Remove the buggy retranslation of pids in fuse_dev_do_read Eric W. Biederman
2018-03-02 21:59       ` [PATCH v8 4/6] fuse: Fail all requests with invalid uids or gids Eric W. Biederman
2018-03-02 21:59       ` [PATCH v8 5/6] fuse: Support fuse filesystems outside of init_user_ns Eric W. Biederman
2018-03-02 21:59       ` [PATCH v8 6/6] fuse: Restrict allow_other to the superblock's namespace or a descendant Eric W. Biederman
2018-03-08 21:23       ` [PATCH v9 0/4] fuse: mounts from non-init user namespaces Eric W. Biederman
2018-03-08 21:24         ` [PATCH v9 1/4] fuse: Remove the buggy retranslation of pids in fuse_dev_do_read Eric W. Biederman
2018-03-08 21:24         ` [PATCH v9 2/4] fuse: Fail all requests with invalid uids or gids Eric W. Biederman
2018-03-08 21:24         ` [PATCH v9 3/4] fuse: Support fuse filesystems outside of init_user_ns Eric W. Biederman
2018-03-08 21:24         ` [PATCH v9 4/4] fuse: Restrict allow_other to the superblock's namespace or a descendant Eric W. Biederman
2018-03-20 16:25         ` [PATCH v9 0/4] fuse: mounts from non-init user namespaces Miklos Szeredi
2018-03-20 18:27           ` Eric W. Biederman
2018-03-21  8:38             ` Miklos Szeredi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CANxcAMvDQFH0g5PPnVZ3p2Tei04N+8fNf0pk02DrfTkBHjjrPQ@mail.gmail.com \
    --to=dongsu@kinvolk.io \
    --cc=alban@kinvolk.io \
    --cc=containers@lists.linux-foundation.org \
    --cc=ebiederm@xmission.com \
    --cc=keescook@chromium.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mcgrof@kernel.org \
    --cc=mszeredi@redhat.com \
    --cc=sargun@sargun.me \
    --cc=seth.forshee@canonical.com \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).