On 10/20/20 12:17 PM, Mark Salyzyn wrote:
> Because of the overlayfs getxattr recursion, the incoming inode fails
> to update the selinux sid resulting in avc denials being reported
> against a target context of u:object_r:unlabeled:s0.
>
> Solution is to respond to the XATTR_NOSECURITY flag in get xattr
> method that calls the __vfs_getxattr handler instead so that the
> context can be read in, rather than being denied with an -EACCES
> when vfs_getxattr handler is called.
>
> For the use case where access is to be blocked by the security layer.
>
> The path then would be security(dentry) ->
> __vfs_getxattr({dentry...XATTR_NOSECURITY}) ->
> handler->get({dentry...XATTR_NOSECURITY}) ->
> __vfs_getxattr({realdentry...XATTR_NOSECURITY}) ->
> lower_handler->get({realdentry...XATTR_NOSECURITY}) which
> would report back through the chain data and success as expected,
> the logging security layer at the top would have the data to
> determine the access permissions and report back to the logs and
> the caller that the target context was blocked.
>
> For selinux this would solve the cosmetic issue of the selinux log
> and allow audit2allow to correctly report the rule needed to address
> the access problem.
>
> Check impure, opaque, origin & meta xattr with no sepolicy audit
> (using __vfs_getxattr) since these operations are internal to
> overlayfs operations and do not disclose any data. This became
> an issue for credential override off since sys_admin would have
> been required by the caller; whereas would have been inherently
> present for the creator since it performed the mount.
>
> This is a change in operations since we do not check in the new
> ovl_do_getxattr function if the credential override is off or not.
> Reasoning is that the sepolicy check is unnecessary overhead,
> especially since the check can be expensive.
>
> Because for override credentials off, this affects _everyone_ that
> underneath performs private xattr calls without the appropriate
> sepolicy permissions and sys_admin capability. Providing blanket
> support for sys_admin would be bad for all possible callers.
>
> For the override credentials on, this will affect only the mounter,
> should it lack sepolicy permissions. Not considered a security
> problem since mounting by definition has sys_admin capabilities,
> but sepolicy contexts would still need to be crafted.
>
> It should be noted that there is precedence, __vfs_getxattr is used
> in other filesystems for their own internal trusted xattr management.
>
> Signed-off-by: Mark Salyzyn <[email protected]>
> To: [email protected]
> To: [email protected]
> Cc: Stephen Smalley <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: Miklos Szeredi <[email protected]>
> Cc: Jonathan Corbet <[email protected]>
> Cc: Vivek Goyal <[email protected]>
> Cc: Eric W. Biederman <[email protected]>
> Cc: Amir Goldstein <[email protected]>
> Cc: Stephen Smalley <[email protected]>
> Cc: [email protected]
>
> v17 - rebase and add inode argument to __Vfs_getxattr
>
> v16 - rebase and merge internal getxattr operations patch
>
> v15 - revert to v13 because xattr_gs_args rejected.
>
> v14 - rebase to use xattr_gs_args.
>
> v13 - rebase to use __vfs_getxattr flags option.
>
> v12 - Added back to patch series as get xattr with flag option.
>
> v11 - Squashed out of patch series and replaced with per-thread flag
> solution.
>
> v10 - Added to patch series as __get xattr method.
> ---
> fs/overlayfs/inode.c | 5 +++--
> fs/overlayfs/overlayfs.h | 6 ++++--
> fs/overlayfs/super.c | 4 ++--
> 3 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index b584dca845ba..2b14291beb86 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -378,7 +378,7 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
> }
>
> int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
> - void *value, size_t size)
> + void *value, size_t size, int flags)
> {
> ssize_t res;
> const struct cred *old_cred;
> @@ -386,7 +386,8 @@ int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
> ovl_i_dentry_upper(inode) ?: ovl_dentry_lower(dentry);
>
> old_cred = ovl_override_creds(dentry->d_sb);
> - res = vfs_getxattr(realdentry, name, value, size);
> + res = __vfs_getxattr(realdentry, d_inode(realdentry), name,
> + value, size, flags);
> revert_creds(old_cred);
> return res;
> }
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index f8880aa2ba0e..d3bf21bd60ab 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -184,7 +184,9 @@ static inline ssize_t ovl_do_getxattr(struct ovl_fs *ofs, struct dentry *dentry,
> size_t size)
> {
> const char *name = ovl_xattr(ofs, ox);
> - return vfs_getxattr(dentry, name, value, size);
> + struct ovl_inode *ip = d_inode(dentry);
struct inode *ip = d_inode(dentry);
<oops> copy and paste error from testing kernel to main