On Wed 14-06-17 19:53:42, Andreas Gruenbacher wrote: > On Wed, Jun 14, 2017 at 3:19 PM, Jan Kara <jack@xxxxxxx> 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@xxxxxxxx> SUSE Labs, CR
