linux-kernel - Re: [PATCH] xattr: Fix setting security xattrs on sockfs

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: <afcdbb40-2092-5279-665e-43999033539a@schaufler-ca.com>
Date:   Thu, 3 Nov 2016 08:51:20 -0700
From:   Casey Schaufler <casey@...aufler-ca.com>
To:     Andreas Gruenbacher <agruenba@...hat.com>,
        Al Viro <viro@...iv.linux.org.uk>
Cc:     linux-kernel@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        Stephen Smalley <sds@...ho.nsa.gov>
Subject: Re: [PATCH] xattr: Fix setting security xattrs on sockfs

On 11/3/2016 6:45 AM, Andreas Gruenbacher wrote:
> Casey, the first patch broke filesystems that support setxattr for some xattrs
> but not security xattrs.  Here's an updated patch; could you please test?

This patch does not fix the problem. I am back to EOPTNOTSUP.

>
> Al, does this look mergeable?
>
> Thanks,
> Andreas
>
> --
>
> The IOP_XATTR flag is set on sockfs because sockfs supports getting the
> "system.sockprotoname" xattr.  Commit 6c6ef9f2 started to check this
> flag for setxattr support as well.  This is wrong on sockfs because
> security xattr support there is provided by security_inode_setsecurity.
>
> Fix this by adding a security xattr handler on sockfs that returns
> -EAGAIN and by checking for -EAGAIN in setxattr.
>
> We cannot simply check for -EOPNOTSUPP in setxattr because there are
> filesystems that neither have direct security xattr support nor support
> via security_inode_setsecurity.  A more proper fix might be to move the
> call to security_inode_setsecurity into sockfs, but it's not clear to me
> if that is safe: we would end up calling security_inode_post_setxattr as
> well.
>
> Signed-off-by: Andreas Gruenbacher <agruenba@...hat.com>
> ---
>  fs/xattr.c   | 22 ++++++++++++++--------
>  net/socket.c | 14 ++++++++++++++
>  2 files changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 3368659..2d13b4e 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -170,7 +170,7 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
>  		const void *value, size_t size, int flags)
>  {
>  	struct inode *inode = dentry->d_inode;
> -	int error = -EOPNOTSUPP;
> +	int error = -EAGAIN;
>  	int issec = !strncmp(name, XATTR_SECURITY_PREFIX,
>  				   XATTR_SECURITY_PREFIX_LEN);
>  
> @@ -183,15 +183,21 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
>  			security_inode_post_setxattr(dentry, name, value,
>  						     size, flags);
>  		}
> -	} else if (issec) {
> -		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> -
> +	} else {
>  		if (unlikely(is_bad_inode(inode)))
>  			return -EIO;
> -		error = security_inode_setsecurity(inode, suffix, value,
> -						   size, flags);
> -		if (!error)
> -			fsnotify_xattr(dentry);
> +	}
> +	if (error == -EAGAIN) {
> +		error = -EOPNOTSUPP;
> +
> +		if (issec) {
> +			const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> +
> +			error = security_inode_setsecurity(inode, suffix, value,
> +							   size, flags);
> +			if (!error)
> +				fsnotify_xattr(dentry);
> +		}
>  	}
>  
>  	return error;
> diff --git a/net/socket.c b/net/socket.c
> index 5a9bf5e..816392a 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -341,6 +341,20 @@ static const struct xattr_handler sockfs_xattr_handler = {
>  	.get = sockfs_xattr_get,
>  };
>  
> +static int sockfs_security_xattr_set(const struct xattr_handler *handler,
> +				     struct dentry *dentry, struct inode *inode,
> +				     const char *suffix, const void *value,
> +				     size_t size, int flags)
> +{
> +	/* Handled by LSM. */
> +	return -EAGAIN;
> +}
> +
> +static const struct xattr_handler sockfs_security_xattr_handler = {
> +	.prefix = XATTR_SECURITY_PREFIX,
> +	.set = sockfs_security_xattr_set,
> +};
> +
>  static const struct xattr_handler *sockfs_xattr_handlers[] = {
>  	&sockfs_xattr_handler,
>  	NULL