Inconsistencies with trusted.SGI_ACL_{FILE,DEFAULT}

[Andreas Gruenbacher]

On Sat, Oct 24, 2015 at 2:57 PM, Brian Foster <bfoster at 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.

> 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.

> Care to test the (compile tested only) hunk below?

That won't update the file mode when setting a SGI_ACL_* attribute.

> An alternative could be to just disallow setting these xattrs directly.

Probably not because that would cause applications to fail in
unexpected new ways.

Thanks,
Andreas