Re: Inconsistencies with trusted.SGI_ACL_{FILE,DEFAULT}

[Brian Foster <bfoster@redhat.com>]

On Sat, Oct 24, 2015 at 03:58:04PM +0200, Andreas Gruenbacher wrote:
> On Sat, Oct 24, 2015 at 2:57 PM, Brian Foster <bfoster@redhat.com> wrote:
> > On Fri, Oct 23, 2015 at 03:52:54PM +0200, Andreas Gruenbacher wrote:
> >> Hello,
> >>
> >> The usual way of manipulating a file's POSIX ACL is through the
> >> system.posix_acl_{access,default} xattrs. Setting
> >> system.posix_acl_access also sets the permission bits in the file
> >> mode. The acls are cached in inode->i_acl and inode->i_default_acl.
> >>
> >> On XFS, POSIX ACLs are also exposed as trusted.SGI_ACL_{FILE,DEFAULT}
> >> xattrs in a different value format. However, setting these xattrs does
> >> not update inode->i_{,default_}acl, and setting trusted.SGI_ACL_FILE
> >> does not update the file mode; things can get out of sync:
> >>
> >
> > It looks like the posix_acl_* values are virtual xattrs on XFS. They get
> > translated to the SGI_ACL_* names before the acl code calls down into
> > the xattr code. The result is cached into the inode via set_cached_acl()
> > before the call returns.
> 
> The posix_acl_* attributes are handled by the vfs
> posix_acl_*_xattr_handler handlers which talk to the filesystem using
> the get_acl and set_acl inode operations. We would need such handlers
> for SGI_ACL_*, installed before xfs_xattr_trusted_handler in
> xfs_xattr_handlers.
> 

Yes, but the translation all occurs on the XFS side. I'm not following
the last bit... are you suggesting a special xattr handler that uses
"trusted.SGI_FILE" as a prefix? I'm not sure that matters either way so
long as those xattrs are trapped appropriately, but feel free to send a
patch. :)

> > The xattr code doesn't know anything about this so I suspect this is the
> > reason for the inconsistency. A direct xattr update just updates the
> > data on-disk and has no knowledge of the ACLs cached to the inode, the
> > latter of which is probably what is returned after the setxattr.
> >
> >>   $ touch f
> >>   $ setfacl -m u:agruenba:rw f
> >>   $ ls -l f
> >>   -rw-rw-r--+ 1 root root 0 Oct 23 15:04 f
> >>   $ getfattr -m- -d f
> >>   # file: f
> >>   security.selinux="unconfined_u:object_r:user_tmp_t:s0"
> >>   system.posix_acl_access=0sAgAAAAEABgD/////AgAGAOgDAAAEAAQA/////xAABgD/////IAAEAP////8=
> >>   trusted.SGI_ACL_FILE=0sAAAABQAAAAH/////AAYAAAAAAAIAAAPoAAYAAAAAAAT/////AAQAAAAAABD/////AAYAAAAAACD/////AAQAAA==
> >>
> >>   $ chmod 0 f
> >>   $ setfattr -n trusted.SGI_ACL_FILE -v
> >> 0sAAAABQAAAAH/////AAYAAAAAAAIAAAPoAAYAAAAAAAT/////AAQAAAAAABD/////AAYAAAAAACD/////AAQAAA==
> >> f
> >>   $ ls -l f
> >>   ----------+ 1 root root 0 Oct 23 15:04 /var/tmp/f
> >>   $ getfacl f
> >>   # file: f
> >>   # owner: root
> >>   # group: root
> >>   user::---
> >>   user:agruenba:rw-        #effective:---
> >>   group::r--            #effective:---
> >>   mask::---
> >>   other::---
> >>   $ getfattr -m- -d f
> >>   # file: f
> >>   security.selinux="unconfined_u:object_r:user_tmp_t:s0"
> >>   system.posix_acl_access=0sAgAAAAEAAAD/////AgAGAOgDAAAEAAQA/////xAAAAD/////IAAAAP////8=
> >>   trusted.SGI_ACL_FILE=0sAAAABQAAAAH/////AAYAAAAAAAIAAAPoAAYAAAAAAAT/////AAQAAAAAABD/////AAYAAAAAACD/////AAQAAA==
> >>
> >> Here, the file mode and the reported value of system.posix_acl_access
> >> are both wrong; trusted.SGI_ACL_FILE corresponds to what's stored on
> >> disk.
> >>
> >> Access to trusted.* attributes is limited to users capable of
> >> CAP_SYS_ADMIN so ordinary users cannot cause this kind of damage, but
> >> this still deserves fixing.
> >>
> >
> > Not sure there's a real use case for this, but it looks like we could
> > invalidate the cached ACLs if those xattrs are modified directly via the
> > xattr interface.
> 
> POSIX ACLs should not have been exposed twice in the first place, but
> those SGI_ACL_* xattrs have been around for a very long time and we
> cannot get rid of them now. It's likely that some applications will
> back up some or all of an inode's xattrs and later restore them.
> 

I wasn't suggesting to get rid of them.

> > Care to test the (compile tested only) hunk below?
> 
> That won't update the file mode when setting a SGI_ACL_* attribute.
> 

Hmm, perhaps this is not sufficient if the mode has to be updated as
well. I suppose we could try to do that as well in this path, but that
implies verification of the data (already in on-disk format) as well.
There's nothing stopping somebody from doing 'setattr -n
trusted.SGI_FILE_ACCESS -v 0 <file>,' after all. The previous patch
wasn't really intended to address that.

> > An alternative could be to just disallow setting these xattrs directly.
> 
> Probably not because that would cause applications to fail in
> unexpected new ways.
> 

I suppose a backup/restore application might want to set these, but I'm
not aware of any other sane usage given they're in a filesystem specific
format at this point. We'd probably have to take a look at xfsdump, see
how it handles this, then see if there's a clean way to run through
necessary acl bits if we're called via setxattr().

Brian

> Thanks,
> Andreas

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs