[fmac-discuss] [PATCH] Move fmac_vnode_access() hook to zfs_zaccess()

[Mark Shellenbaum]

Stephen Smalley wrote:
> Extend fmac_vnode_access() to understand ACE masks as well as
> traditional modes, and move the calls to it from fop_access() and
> zfs_create() into zfs_zaccess().  The access functions of other
> filesystems would need to be similarly instrumented, although they can
> pass conventional modes rather than ACE masks since fmac_vnode_access()
> understands both and distinguishes them based on flags.
> 
> Note btw that extending fmac_vnode_access() to understand ACE masks is
> required regardless of whether we call it from fop_access() or
> zfs_zaccess() since callers of VOP_ACCESS() can pass ACE masks (as in
> the nfs4 and smbsrv cases).
> 
> fmac_vnode_access() is only called if the normal zfs_zaccess() logic
> would grant the access (i.e. either the base DAC logic or the secpolicy
> hook authorized it).  The secpolicy hook is not allowed to override a
> FMAC denial.
> 
> One immediate benefit of taking the FMAC hook into zfs_zaccess is that
> we get proper checking against the base file when a named attribute is
> accessed, e.g. runat /etc/passwd cp /tmp/foo attr.1 fails as expected
> with a setattr denial.  We also gain more complete coverage of file
> operations.
> 
> I looked at moving the fmac_vnode_access() hook inside of
> zfs_zaccess_common(), but it seemed better to do it in zfs_zaccess()
> because:
> 1) We can then place it after both the base DAC logic and the privilege
> check.
> 2) We do not gain any benefit from checking all calls to
> zfs_zaccess_common() since other callers like zfs_zaccess_delete() and
> _rename() short-circuit their checking if access to the directory is
> granted, whereas we want checking against the target file in all cases
> and thus still need our separate hooks for those operations.
> 
> Some of the other FMAC checks may be obsoleted by taking
> fmac_vnode_access() into zfs_zaccess(), but this requires a case-by-case
> review and testing as callers of zfs_zaccess() do not always honor a
> denial from it (can be overridden by other secpolicy calls), as in the
> setattr case.
> 
> This is relative to the prior patch for the create hooks.

> +
> +#define	ACE_GETATTR_MASK (ACE_READ_NAMED_ATTRS | ACE_READ_ATTRIBUTES | \
> +    ACE_READ_ACL)
> +#define	ACE_SETATTR_MASK (ACE_WRITE_NAMED_ATTRS | ACE_WRITE_ATTRIBUTES | \
> +    ACE_WRITE_ACL | ACE_WRITE_OWNER)
> +

A standard VOP_GETATTR() would only require ACE_READ_ATTRIBUTES, whereas 
a VOP_GETSECATTR() would require ACE_READ_ACL.  ACE_READ_NAMED_ATTRS is 
used for "search" permission in the hidden extended attribute directory.

In the setattr case it really depends on what operation is being 
performed as to what permissions *may* be required.

   -Mark