Default ACLs and SGID bit on directories

Default ACLs and SGID bit on directories

[Jan Kara]

Hello,

commit 073931017b49d "posix_acl: Clear SGID bit when setting file
permissions" started clearing SGID bit when ACLs are changed for an inode
if user is not member of the owning group (or has appropriate
capabilities). Now this is in line with what chmod does. However this has
caused a regression which one of our customer noticed: Suppose you have a
directory DIR with mode 02777 with default ACLs and belongs to a
group GROUP you are not member of. You can create subdir SUB in DIR, it
will be again owned by GROUP. Previously it will also have SGID bit set,
however after commit 073931017b49d, the SGID bit gets cleared as a
side-effect of inheritance of default ACLs.

Now it is relatively easy (although a bit ugly) to restore SGID bit after
ACLs got inherited and Lance has a patch for this. However I wonder: If we
let SGID bit be inherited, user can then modify default ACLs of DIR/SUB
arbitrarily and this has no effect on the SGID bit. So further files and
directories created under SUB can have arbitrary set of ACLs and owning
group GROUP. I guess this is in line with the fact that the mode of file /
directory under DIR can be arbitrary even without ACLs but I wanted to
check whether someone does not see any problem with this.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: Default ACLs and SGID bit on directories

[Andreas Gruenbacher]

On Wed, Jun 14, 2017 at 3:19 PM, Jan Kara <jack@suse.cz> wrote:
> commit 073931017b49d "posix_acl: Clear SGID bit when setting file
> permissions" started clearing SGID bit when ACLs are changed for an inode
> if user is not member of the owning group (or has appropriate
> capabilities). Now this is in line with what chmod does. However this has
> caused a regression which one of our customer noticed: Suppose you have a
> directory DIR with mode 02777 with default ACLs and belongs to a
> group GROUP you are not member of. You can create subdir SUB in DIR, it
> will be again owned by GROUP. Previously it will also have SGID bit set,
> however after commit 073931017b49d, the SGID bit gets cleared as a
> side-effect of inheritance of default ACLs.

That's clearly a bug.

> Now it is relatively easy (although a bit ugly) to restore SGID bit after
> ACLs got inherited and Lance has a patch for this.

How about a patch that doesn't incorrectly clear the SGID bit instead?

We will also need an xfstests test case.

> However I wonder: If we
> let SGID bit be inherited, user can then modify default ACLs of DIR/SUB
> arbitrarily and this has no effect on the SGID bit. So further files and
> directories created under SUB can have arbitrary set of ACLs and owning
> group GROUP. I guess this is in line with the fact that the mode of file /
> directory under DIR can be arbitrary even without ACLs but I wanted to
> check whether someone does not see any problem with this.

So fixing this bug won't create any additional problems, but the
behavior of chmod(2) and open(2) is still inconsistent, with or
without ACLs.

I wonder if it would make sense to change the open(2) and mknod(2)
behavior to knock out S_ISGID when the owner isn't a member in the
owning group, just like chmod(2) does. Any thoughts?

[mkdir(2) filters out S_ISUID and S_ISGID from the create mode, so it
isn't affected.]

Thanks,
Andreas

Re: Default ACLs and SGID bit on directories

[Jan Kara]

On Wed 14-06-17 19:53:42, Andreas Gruenbacher wrote:
> On Wed, Jun 14, 2017 at 3:19 PM, Jan Kara <jack@suse.cz> wrote:
> > commit 073931017b49d "posix_acl: Clear SGID bit when setting file
> > permissions" started clearing SGID bit when ACLs are changed for an inode
> > if user is not member of the owning group (or has appropriate
> > capabilities). Now this is in line with what chmod does. However this has
> > caused a regression which one of our customer noticed: Suppose you have a
> > directory DIR with mode 02777 with default ACLs and belongs to a
> > group GROUP you are not member of. You can create subdir SUB in DIR, it
> > will be again owned by GROUP. Previously it will also have SGID bit set,
> > however after commit 073931017b49d, the SGID bit gets cleared as a
> > side-effect of inheritance of default ACLs.
> 
> That's clearly a bug.
> 
> > Now it is relatively easy (although a bit ugly) to restore SGID bit after
> > ACLs got inherited and Lance has a patch for this.
> 
> How about a patch that doesn't incorrectly clear the SGID bit instead?

Well, I was considering that as well and it seemed non-trivial. Looking at
it again today maybe we could do that. If I understand the code right, it
would mean factoring out posix_acl_update_mode() out of each filesystem's
xattr setting function so that we can modify ACLs without updating mode
(that should be already uptodate from posix_acl_create()), right? OK, I
think we can do that.

> We will also need an xfstests test case.

Yup.

> > However I wonder: If we
> > let SGID bit be inherited, user can then modify default ACLs of DIR/SUB
> > arbitrarily and this has no effect on the SGID bit. So further files and
> > directories created under SUB can have arbitrary set of ACLs and owning
> > group GROUP. I guess this is in line with the fact that the mode of file /
> > directory under DIR can be arbitrary even without ACLs but I wanted to
> > check whether someone does not see any problem with this.
> 
> So fixing this bug won't create any additional problems, but the
> behavior of chmod(2) and open(2) is still inconsistent, with or
> without ACLs.

Yes, that's basically what triggered my uncertainty about how to proceed.

> I wonder if it would make sense to change the open(2) and mknod(2)
> behavior to knock out S_ISGID when the owner isn't a member in the
> owning group, just like chmod(2) does. Any thoughts?
> 
> [mkdir(2) filters out S_ISUID and S_ISGID from the create mode, so it
> isn't affected.]

Well, mkdir filters out S_ISUID and S_ISGID from the mode but still S_ISGID
can be set by inheriting it from the parent directory including owning
group creating user is not member of. And our customer's bug shows, there
are setups which depend on this behavior at least for directories. So
unless someone finds really good security reason why we need to clear
S_ISGID in such cases on dir / file creation, I think we have to keep
current behavior as "interface consistency" is not good enough reason to
break userspace...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR