On Tue, Feb 22, 2022 at 05:54:07PM +0300, Andrey Zhadchenko wrote:
>
>
> On 2/22/22 15:36, Christian Brauner wrote:
> > On Tue, Feb 22, 2022 at 01:23:31PM +0100, Christian Brauner wrote:
> > > On Tue, Feb 22, 2022 at 02:19:16PM +0300, Andrey Zhadchenko wrote:
> > > > On 2/22/22 13:24, Christian Brauner wrote:
> > > > > On Tue, Feb 22, 2022 at 09:33:40AM +0100, Christoph Hellwig wrote:
> > > > > > On Mon, Feb 21, 2022 at 09:22:18PM +0300, Andrey Zhadchenko wrote:
> > > > > > > xfs_fileattr_set() handles idmapped mounts correctly and do not drop this
> > > > > > > bits.
> > > > > > > Unfortunately chown syscall results in different callstask:
> > > > > > > i_op->xfs_vn_setattr()->...->xfs_setattr_nonsize() which checks if process
> > > > > > > has CAP_FSETID capable in init_user_ns rather than mntns userns.
> > > > > >
> > > > > > Can you add an xfstests the exercises this path?
> > > > > >
> > > > > > The fix itself looks good:
> > > > > >
> > > > > > Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> > > > >
> > > > > So for anything other than directories the s{g,u}id bits are cleared on
> > > > > every chown in notify_change() by the vfs; even for the root user (Also
> > > > > documented on chown(2) manpage).
> > > >
> > > > Only exception - chown preserves setgid bit set on a non-group-executable
> > > > file (also documented there) but do not take root privileges into account at
> > > > vfs level.
> > > >
> > > > >
> > > > > So the only scenario were this change would be relevant is for
> > > > > directories afaict:
> > > > >
> > > > > 1. So ext4 has the behavior:
> > > > >
> > > > > ubuntu@f2-vm|~
> > > > > > mkdir suid.dir
> > > > > ubuntu@f2-vm|~
> > > > > > perms ./suid.dir
> > > > > drwxrwxr-x 775 (1000:1000) ./suid.dir
> > > > > ubuntu@f2-vm|~
> > > > > > chmod u+s ./suid.dir/
> > > > > ubuntu@f2-vm|~
> > > > > > perms ./suid.dir
> > > > > drwsrwxr-x 4775 (1000:1000) ./suid.dir
> > > > > ubuntu@f2-vm|~
> > > > > > chmod g+s ./suid.dir/
> > > > > ubuntu@f2-vm|~
> > > > > > perms ./suid.dir
> > > > > drwsrwsr-x 6775 (1000:1000) ./suid.dir
> > > > > ubuntu@f2-vm|~
> > > > > > chown 1000:1000 ./suid.dir/
> > > > > ubuntu@f2-vm|~
> > > > > > perms ./suid.dir/
> > > > > drwsrwsr-x 6775 (1000:1000) ./suid.dir/
> > > > > meaning that both s{g,u}id bits are retained for directories. (Just to
> > > > > make this explicit: changing {g,u}id to the same {g,u}id still ends up
> > > > > calling into the filesystem.)
> > > > >
> > > > > 2. Whereas xfs currently has:
> > > > >
> > > > > brauner@wittgenstein|~
> > > > > > mkdir suid.dir
> > > > > brauner@wittgenstein|~
> > > > > > perms ./suid.dir
> > > > > drwxrwxr-x 775 ./suid.dir
> > > > > brauner@wittgenstein|~
> > > > > > chmod u+s ./suid.dir/
> > > > > brauner@wittgenstein|~
> > > > > > perms ./suid.dir
> > > > > drwsrwxr-x 4775 ./suid.dir
> > > > > brauner@wittgenstein|~
> > > > > > chmod g+s ./suid.dir/
> > > > > brauner@wittgenstein|~
> > > > > > perms ./suid.dir
> > > > > drwsrwsr-x 6775 ./suid.dir
> > > > > brauner@wittgenstein|~
> > > > > > chown 1000:1000 ./suid.dir/
> > > > > brauner@wittgenstein|~
> > > > > > perms ./suid.dir/
> > > > > drwxrwxr-x 775 ./suid.dir/
> > > > > meaning that both s{g,u}id bits are cleared for directories.
> > > > >
> > > > > Since the vfs will always ensure that s{g,u}id bits are stripped for
> > > > > anything that isn't a directory in the vfs:
> > > > > - ATTR_KILL_S{G,U}ID is raised in chown_common():
> > > > >
> > > > > if (!S_ISDIR(inode->i_mode))
> > > > > newattrs.ia_valid |=
> > > > > ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_KILL_PRIV;
> > > > >
> > > > > - and then in notify_change() we'll get the bits stripped and ATTR_MODE
> > > > > raised:
> > > > >
> > > > > if (ia_valid & ATTR_KILL_SUID) {
> > > > > if (mode & S_ISUID) {
> > > > > ia_valid = attr->ia_valid |= ATTR_MODE;
> > > > > attr->ia_mode = (inode->i_mode & ~S_ISUID);
> > > > > }
> > > > > }
> > > > > if (ia_valid & ATTR_KILL_SGID) {
> > > > > if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
> > > >
> > > > So SGID is not killed if there is no S_IXGRP (yet no capability check)
> > > >
> > > > Actually I do not really understand why do kernel expects filesystems to
> > > > further apply restrictions with CAP_FSETID. Why not kill it here since we
> > > > have all info?
> > >
> > > Some filesystems do treat the sgid behavior of directories special (some
> > > network filesystems do where they send that information to the server
> > > before updating the inode afair). So I'd rather not do that in there as
> > > we're risking breaking expectations and it's a very sensitive change.
> > >
> > > Plus, the logic is encapsulated in the vfs generic setattr_copy() helper
> > > which nearly all filesystems call.
> > >
> > > >
> > > > > if (!(ia_valid & ATTR_MODE)) {
> > > > > ia_valid = attr->ia_valid |= ATTR_MODE;
> > > > > attr->ia_mode = inode->i_mode;
> > > > > }
> > > > > attr->ia_mode &= ~S_ISGID;
> > > > > }
> > > > > }
> > > > >
> > > > > we can change this codepath to just mirror setattr_copy() or switch
> > > > > fully to setattr_copy() (if feasible).
> > > > >
> > > > > Because as of right now the code seems to imply that the xfs code itself
> > > > > is responsible for stripping s{g,u}id bits for all files whereas it is
> > > > > the vfs that does it for any non-directory. So I'd propose to either try
> > > > > and switch that code to setattr_copy() or to do open-code the
> > > > > setattr_copy() check:
>
> I did some more research on it and seems like modes are already stripped
> enough.
>
> notify_change() -> inode->i_op->setattr() -> xfs_vn_setattr() ->
> xfs_vn_change_ok() -> prepare_setattr()
> which has the following:
> if (!in_group_p((ia_valid & ATTR_GID) ? attr->ia_gid :
> i_gid_into_mnt(mnt_userns, inode)) &&
> !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID))
> attr->ia_mode &= ~S_ISGID;
>
> After xfs_vn_change_ok() xfs_setattr_nonsize() is finally called and
> additionally strips sgid and suid.
>
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 09211e1d08ad..7fda5ff3ef17 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -767,16 +767,6 @@ xfs_setattr_nonsize(
> gid = (mask & ATTR_GID) ? iattr->ia_gid : igid;
> uid = (mask & ATTR_UID) ? iattr->ia_uid : iuid;
>
> - /*
> - * CAP_FSETID overrides the following restrictions:
> - *
> - * The set-user-ID and set-group-ID bits of a file will be
> - * cleared upon successful return from chown()
> - */
> - if ((inode->i_mode & (S_ISUID|S_ISGID)) &&
> - !capable(CAP_FSETID))
> - inode->i_mode &= ~(S_ISUID|S_ISGID);
> -
> /*
> * Change the ownerships and register quota modifications
> * in the transaction.
>
>
> root@debian:/mnt/xfs# unshare -U --map-root
> root@debian:/mnt/xfs# touch testfile
> root@debian:/mnt/xfs# chmod g+s testfile
> root@debian:/mnt/xfs# ls -la testfile
> -rw-r-Sr-- 1 root root 0 Feb 22 14:46 testfile
> root@debian:/mnt/xfs# chown 0:0 testfile
> root@debian:/mnt/xfs# ls -la testfile
> -rw-r-Sr-- 1 root root 0 Feb 22 14:46 testfile
> root@debian:/mnt/xfs#
>
> root@debian:/mnt/xfs# mkdir testdir
> root@debian:/mnt/xfs# chmod u+s testdir
> root@debian:/mnt/xfs# chmod u+g testdir
> root@debian:/mnt/xfs#
> root@debian:/mnt/xfs# ls -la
> total 4
> drwxr-xr-x 4 root root 50 Feb 22 14:47 .
> drwxr-xr-x 4 root root 4096 Feb 22 13:45 ..
> drwsr-sr-x 2 root root 6 Feb 22 14:42 test1
> drwsr-xr-x 2 root root 6 Feb 22 14:47 testdir
> -rw-r-Sr-- 1 root root 0 Feb 22 14:46 testfile
> root@debian:/mnt/xfs# chown 0:0 testdir
> root@debian:/mnt/xfs# ls -la
> total 4
> drwxr-xr-x 4 root root 50 Feb 22 14:47 .
> drwxr-xr-x 4 root root 4096 Feb 22 13:45 ..
> drwsr-sr-x 2 root root 6 Feb 22 14:42 test1
> drwsr-xr-x 2 root root 6 Feb 22 14:47 testdir
> -rw-r-Sr-- 1 root root 0 Feb 22 14:46 testfile
>
> > > > >
> > > > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > > > > index b79b3846e71b..ff55b31521a2 100644
> > > > > --- a/fs/xfs/xfs_iops.c
> > > > > +++ b/fs/xfs/xfs_iops.c
> > > > > @@ -748,9 +748,13 @@ xfs_setattr_nonsize(
> > > > > * The set-user-ID and set-group-ID bits of a file will be
> > > > > * cleared upon successful return from chown()
> > > > > */
> > > > > - if ((inode->i_mode & (S_ISUID|S_ISGID)) &&
> > > > > - !capable(CAP_FSETID))
> > > > > - inode->i_mode &= ~(S_ISUID|S_ISGID);
> > > > > + if (iattr->ia_valid & ATTR_MODE) {
> > > > > + umode_t mode = iattr->ia_mode;
> > > > > + if (!in_group_p(i_gid_into_mnt(mnt_userns, inode)) &&
> > > > > + !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID))
> > > > > + mode &= ~S_ISGID;
> > > > > + inode->i_mode = mode;
> > > > > + }
> > > > >
> > > > > /*
> > > > > * Change the ownerships and register quota modifications
> > > > >
> > > > > which aligns xfs with ext4 and any other filesystem. Any thoughts on
> > > > > this?
> > > > >
> > > > > For @Andrey specifically: the tests these should go into:
> > > > >
> > > > > https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/tree/src/idmapped-mounts/idmapped-mounts.c
> > > > >
> > > > > Note that there are already setgid inheritance tests and set*id
> > > > > execution tests in there.
> > > > > You should be able to copy a lot of code from them. Could you please add
> > > > > the test I sketched above and ideally also a test that the set{g,u}id
> > > > > bits are stripped during chown for regular files?
> > > > Thanks for the link. To clarify what tests and result you expect:
> > > > - for directory chown we expect to preserve s{g,u}id
> > > > - for regfile chown we expect to preserve S_ISGID only if S_IXGRP is absent
> > > > and CAP_FSETID is present
> > >
> > > So specifically for chown():
> > > 1. if regfile
> > > -> strip suid bit unconditionally
> > > -> strip sgid bit if inode has sgid bit and file is group-executable
> > > 2. if directory
> > > -> strip sgid bit if inode's gid is neither among the caller's groups
> > > nor is the caller capable wrt to that inode
> > > The behavior described in 2. is encoded in the vfs generic
> > > setattr_copy() helper. And that is what we should see.
> > >
> > > The behavior of ext4 and btrfs is what we should see afaict as both use
> > > setattr_copy().
> > >
> > > >
> > > > JFYI: I found out this problem while running LTP (specifically
> > > > syscalls/chown02 test) on idmapped XFS. Maybe I will be able to find more,
> > > > who knows.
> > >
> > > Hm, if you look above, then you can see that the failure (or difference
> > > in behavior) you're reporting is independent of idmapped mounts. An
> > > ext4 directory shows different behavior than an xfs directory on a
> > > regular system without any idmapped mounts used. So I'm not clear how
> > > that's specifically related to idmapped mounts yet.
>
> I guess my commit message is pretty poor. Initially I found out that chown()
> on idmapped xfs (+userns) drops sgid unconditionally on regfiles. I did not
> thought about directories at all. It is good that you pointed it out.
> The problem is indeed independent from idmapping. However I thought this
> belonged to it since most of the checks were updated with idmapped series.
> Thanks for the explanation
No, not at all. It's perfectly fine to not know all the details right of
the bat! Thank your for bringing this up so I could take a closer look
at this.
>
> >
> > So for example, in order to cause the sgid bit stripped while it should
> > be preserved if xfs were to use setattr_copy() I can simply do:
> >
> > brauner@wittgenstein|~/src/git/linux/ltp/testcases/kernel/syscalls/chown|master %=
> > > unshare -U --map-root
> > root@wittgenstein|~/src/git/linux/ltp/testcases/kernel/syscalls/chown|master %=
> > > PATH=$PATH:$PWD ./chown02
> > tst_memutils.c:157: TWARN: Can't adjust score, even with capabilities!?
> > tst_test.c:1455: TINFO: Timeout per run is 0h 05m 00s
> > chown02.c:45: TPASS: chown(testfile1, 0, 0) passed
> > chown02.c:45: TPASS: chown(testfile2, 0, 0) passed
> > chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
> >
> > Summary:
> > passed 2
> > failed 1
> > broken 0
> > skipped 0
> > warnings 1
> >
> > There's no idmapped mounts here in play. The caller simply has been
> > placed in a new user namespace and thus they fail the current
> > capable(CAP_FSETID) check which will cause xfs to strip the sgid bit >
> > Now trying the same with ext4:
> >
> > ubuntu@f2-vm:~/src/git/linux/ltp/testcases/kernel/syscalls/chown$ unshare -U --map-root
> > root@f2-vm:~/src/git/linux/ltp/testcases/kernel/syscalls/chown# PATH=$PATH:$PWD ./chown02
> > tst_memutils.c:157: TWARN: Can't adjust score, even with capabilities!?
> > tst_test.c:1455: TINFO: Timeout per run is 0h 05m 00s
> > chown02.c:45: TPASS: chown(testfile1, 0, 0) passed
> > chown02.c:45: TPASS: chown(testfile2, 0, 0) passed
> >
> > Summary:
> > passed 2
> > failed 0
> > broken 0
> > skipped 0
> > warnings 1
> >
> > it passes since ext4 uses setattr_copy() and thus the capability is
> > checked for in the caller's user namespace.
> >
> > >
> > > Fwiw, one part in your commit message is a bit misleading:
> > >
> > > > > > > has CAP_FSETID capable in init_user_ns rather than mntns userns.
> > >
> > > that's not what capable_wrt_to_inode_uidgid() does. What it does is to
> > > check whether the caller is capable in their current user namespace.
> > > That's how capable_wrt_to_inode_uidgid() has always worked.
> > > The mnt_userns is only used to idmap the inode's {g,u}id. So if the
> > > caller has CAP_FSETID in its current userns and the inode's {g,u}id have
> > > a valid mapping in the mnt's userns the caller is considered privileged
> > > over that inode.
> > >
>
