linux-kernel - Re: [PATCH 5/5] overlayfs: Use vfs_getxattr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b62a4ca9-3f75-b662-108b-1dec85c40ba1@schaufler-ca.com>
Date:	Tue, 5 Jul 2016 13:29:39 -0700
From:	Casey Schaufler <casey@...aufler-ca.com>
To:	Vivek Goyal <vgoyal@...hat.com>, miklos@...redi.hu,
	sds@...ho.nsa.gov, linux-kernel@...r.kernel.org,
	linux-unionfs@...r.kernel.org,
	linux-security-module@...r.kernel.org
Cc:	dwalsh@...hat.com, dhowells@...hat.com, pmoore@...hat.com,
	viro@...IV.linux.org.uk, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode

On 7/5/2016 8:50 AM, Vivek Goyal wrote:
> ovl_getxattr() currently uses vfs_getxattr() on realinode. This fails
> if mounter does not have DAC/MAC permission to access getxattr.
>
> Specifically this becomes a problem when selinux is trying to initialize
> overlay inode and does ->getxattr(overlay_inode). A task might trigger
> initialization of overlay inode and we will access real inode xattr in the
> context of mounter and if mounter does not have permissions, then inode
> selinux context initialization fails and inode is labeled as unlabeled_t.
>
> One way to deal with it is to let SELinux do getxattr checks both on
> overlay inode and underlying inode and overlay can call vfs_getxattr_noperm()
> to make sure when selinux is trying to initialize label on inode, it does
> not go through checks on lower levels and initialization is successful.
> And after inode initialization, SELinux will make sure task has getatttr
> permission.
>
> One issue with this approach is that it does not work for directories as
> d_real() returns the overlay dentry for directories and not the underlying
> directory dentry.
>
> Another way to deal with it to introduce another function pointer in
> inode_operations, say getxattr_noperm(), which is responsible to get
> xattr without any checks. SELinux initialization code will call this
> first if it is available on inode. So user space code path will call
> ->getxattr() and that will go through checks and SELinux internal
> initialization will call ->getxattr_noperm() and that will not
> go through checks.
>
> For now, I am just converting ovl_getxattr() to get xattr without
> any checks on underlying inode. That means it is possible for
> a task to get xattr of a file/dir on lower/upper through overlay mount
> while it is not possible outside overlay mount.
>
> If this is a major concern, I can look into implementing getxattr_noperm().

This is a major concern.

>
> Signed-off-by: Vivek Goyal <vgoyal@...hat.com>
> ---
>  fs/overlayfs/inode.c  |  7 +------
>  fs/xattr.c            | 28 +++++++++++++++++++---------
>  include/linux/xattr.h |  1 +
>  3 files changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 36dfd86..a5d3320 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -233,16 +233,11 @@ ssize_t ovl_getxattr(struct dentry *dentry, struct inode *inode,
>  		     const char *name, void *value, size_t size)
>  {
>  	struct dentry *realdentry = ovl_dentry_real(dentry);
> -	ssize_t sz;
> -	const struct cred *old_cred;
>  
>  	if (ovl_is_private_xattr(name))
>  		return -ENODATA;
>  
> -	old_cred = ovl_override_creds(dentry->d_sb);
> -	sz = vfs_getxattr(realdentry, name, value, size);
> -	revert_creds(old_cred);
> -	return size;
> +	return vfs_getxattr_noperm(realdentry, name, value, size);
>  }
>  
>  ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size)
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 4beafc4..973e18c 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -209,19 +209,11 @@ vfs_getxattr_alloc(struct dentry *dentry, const char *name, char **xattr_value,
>  }
>  
>  ssize_t
> -vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size)
> +vfs_getxattr_noperm(struct dentry *dentry, const char *name, void *value, size_t size)
>  {
>  	struct inode *inode = dentry->d_inode;
>  	int error;
>  
> -	error = xattr_permission(inode, name, MAY_READ);
> -	if (error)
> -		return error;
> -
> -	error = security_inode_getxattr(dentry, name);
> -	if (error)
> -		return error;
> -
>  	if (!strncmp(name, XATTR_SECURITY_PREFIX,
>  				XATTR_SECURITY_PREFIX_LEN)) {
>  		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> @@ -242,6 +234,24 @@ nolsm:
>  
>  	return error;
>  }
> +EXPORT_SYMBOL_GPL(vfs_getxattr_noperm);
> +
> +ssize_t
> +vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size)
> +{
> +	struct inode *inode = dentry->d_inode;
> +	int error;
> +
> +	error = xattr_permission(inode, name, MAY_READ);
> +	if (error)
> +		return error;
> +
> +	error = security_inode_getxattr(dentry, name);
> +	if (error)
> +		return error;
> +
> +	return vfs_getxattr_noperm(dentry, name, value, size);
> +}
>  EXPORT_SYMBOL_GPL(vfs_getxattr);
>  
>  ssize_t
> diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> index 94079ba..832a6b6 100644
> --- a/include/linux/xattr.h
> +++ b/include/linux/xattr.h
> @@ -47,6 +47,7 @@ struct xattr {
>  
>  ssize_t xattr_getsecurity(struct inode *, const char *, void *, size_t);
>  ssize_t vfs_getxattr(struct dentry *, const char *, void *, size_t);
> +ssize_t vfs_getxattr_noperm(struct dentry *, const char *, void *, size_t);
>  ssize_t vfs_listxattr(struct dentry *d, char *list, size_t size);
>  int __vfs_setxattr_noperm(struct dentry *, const char *, const void *, size_t, int);
>  int vfs_setxattr(struct dentry *, const char *, const void *, size_t, int);