Re: [PATCH v2 3/5] attr: use consistent sgid stripping checks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> > > @@ -721,10 +721,10 @@ int chown_common(const struct path *path, uid_t user, gid_t group)
> > >                 return -EINVAL;
> > >         if ((group != (gid_t)-1) && !setattr_vfsgid(&newattrs, gid))
> > >                 return -EINVAL;
> > > -       if (!S_ISDIR(inode->i_mode))
> > > -               newattrs.ia_valid |=
> > > -                       ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_KILL_PRIV;
> > >         inode_lock(inode);
> > > +       if (!S_ISDIR(inode->i_mode))
> > > +               newattrs.ia_valid |= ATTR_KILL_SUID | ATTR_KILL_PRIV |
> > > +                                    should_remove_sgid(mnt_userns, inode);
> >
> > This is making me stop and wonder:
> > 1. This has !S_ISDIR, should_remove_suid() has S_ISREG and
> >     setattr_drop_sgid() has neither - is this consistent?
>
> I thought about that. It'very likely redundant since we deal with that
> in other parts but I need to verify all callers before we can remove
> that.
>
> > 2. SUID and PRIV are removed unconditionally and SGID is
> >     removed conditionally - this is not a change of behavior
> >     (at least for non-overlayfs), but is it desired???
>
> It looks that way but it isn't. The setgid bit was only killed
> unconditionally for S_IXGRP. We continue to do that. But it was always
> removed conditionally for ~S_IXGRP. The difference between this patchset
> and earlier is that it was done in settattr_prepare() or setattr_copy()
> before this change.
>
> IOW, we raised ATTR_KILL_SGID unconditionally but then only
> conditionally obeyed it in setattr_{prepare,copy}() whereas now we
> conditionally raise ATTR_KILL_SGID. That's surely a slight change but it
> just means that we don't cause bugs for filesystems that roll their own
> prepare or copy helpers and is just nicer overall.
>

Yes, that sounds right.

The point that I was trying to make and failed to articulate myself was
that chown_common() raises ATTR_KILL_SUID unconditionally,
while should_remove_suid() raises ATTR_KILL_SUID conditional
to !capable(CAP_FSETID).

Is this inconsistency in stripping SUID desired?

According to man page (I think that) it is:

"When the owner or group of an executable file is changed by an
 unprivileged user, the S_ISUID and S_ISGID mode bits are cleared.
 POSIX does not specify whether this also  should  happen  when  root
 does the chown(); the Linux behavior depends on the kernel version,
 and since Linux 2.2.13, root is treated like other users..."

So special casing SUID stripping in chown() looks intentional,
but maybe it is worth a comment.

The paragraph above *may* be interpreted that chown() should strip
S_SGID|S_IXGRP regardless of CAP_FSETID, which, as you say,
has not been the case for a while.

"...In case of a non-group-executable file (i.e., one for which the
 S_IXGRP bit is not set) the S_ISGID bit indicates mandatory locking,
 and is not cleared by a chown().
 When the owner or group of an executable file is changed (by any user),
 all capability sets for the file are cleared."

This part sounds aligned with the code.

Thanks,
Amir.



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux