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