Problem with setxattr on sockfs with Smack after 971df15bd54ad46e907046ff33750a137b2f0096

Problem with setxattr on sockfs with Smack after 971df15bd54ad46e907046ff33750a137b2f0096

[Casey Schaufler]

Smack has always used extended attributes to identify
the security information used to make access control
decisions on packet delivery. The two attributes
security.SMACK64_IPIN and security.SMACK64_IPOUT
contain the label used for inbound and outbound
checks respectively. A process with CAP_MAC_ADMIN can
change these values using fsetxattr() to allow a
privileged service to communicate more openly than
is allowed under the strict Smack policy.

After the xattr rework the fsetxattr() call still
sets the Smack attribute correctly, because the
smack_inode_setxattr() hook is still getting called,
but it returns EOPNOTSUPP. I believe that this is
either a result of the attribute name being unknown
to sockfs (as mentioned in the commit message) or
one of the other changes made in the process of the
xattr rework. I haven't finished the bisect yet,
but I'm reasonably certain the issue arises here.

Should I add the Smack attributes to the list of
attributes sockfs acknowledges? Is there a better
approach?

Thank you.

Re: Problem with setxattr on sockfs with Smack after 971df15bd54ad46e907046ff33750a137b2f0096

[Casey Schaufler]

On 10/31/2016 2:55 PM, Casey Schaufler wrote:
> Smack has always used extended attributes to identify
> the security information used to make access control
> decisions on packet delivery. The two attributes
> security.SMACK64_IPIN and security.SMACK64_IPOUT
> contain the label used for inbound and outbound
> checks respectively. A process with CAP_MAC_ADMIN can
> change these values using fsetxattr() to allow a
> privileged service to communicate more openly than
> is allowed under the strict Smack policy.
>
> After the xattr rework the fsetxattr() call still
> sets the Smack attribute correctly, because the
> smack_inode_setxattr() hook is still getting called,
> but it returns EOPNOTSUPP. I believe that this is
> either a result of the attribute name being unknown
> to sockfs (as mentioned in the commit message) or
> one of the other changes made in the process of the
> xattr rework. I haven't finished the bisect yet,
> but I'm reasonably certain the issue arises here.

The final results from bisect say that the problem hit
in 6c6ef9f26e598fb977f60935e109cd5b266c941a -
xattr: stop calling {get,set,remove}xattr inode operations


>
> Should I add the Smack attributes to the list of
> attributes sockfs acknowledges? Is there a better
> approach?
>
> Thank you.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCH] Re: Problem with setxattr on sockfs with Smack after 971df15bd54ad46e907046ff33750a137b2f0096

[Andreas Gruenbacher]

Casey,

does this patch help?

(The way how security xattrs are handled by LSM is pretty ugly.  I'm not
convinced that it doesn't break something else, yet.)

Thanks,
Andreas
---
 fs/xattr.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index 3368659..bf09836 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -183,11 +183,13 @@ 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;
+	}
+	if (issec && error == -EOPNOTSUPP) {
+		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
+
 		error = security_inode_setsecurity(inode, suffix, value,
 						   size, flags);
 		if (!error)
-- 
2.7.4

Re: [PATCH] Re: Problem with setxattr on sockfs with Smack after 971df15bd54ad46e907046ff33750a137b2f0096

[Casey Schaufler]

On 11/2/2016 12:34 PM, Andreas Gruenbacher wrote:
> Casey,
>
> does this patch help?

My tests pass with this patch applied.

>
> (The way how security xattrs are handled by LSM is pretty ugly.

I'm open to suggestions.

>   I'm not
> convinced that it doesn't break something else, yet.)
>
> Thanks,
> Andreas
> ---
>  fs/xattr.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 3368659..bf09836 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -183,11 +183,13 @@ 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;
> +	}
> +	if (issec && error == -EOPNOTSUPP) {
> +		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> +
>  		error = security_inode_setsecurity(inode, suffix, value,
>  						   size, flags);
>  		if (!error)

[PATCH] xattr: Fix setting security xattrs on sockfs

[Andreas Gruenbacher]

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?

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@redhat.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
-- 
2.7.4

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

[Casey Schaufler]

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@redhat.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

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

[Andreas Gruenbacher]

On Thu, Nov 3, 2016 at 4:51 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> 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.

Ah, I forgot to register the new sockfs xattr handler.  Does this help?

Thanks,
Andreas

---
 net/socket.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/socket.c b/net/socket.c
index 816392a..9820725 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -357,6 +357,7 @@ static const struct xattr_handler sockfs_security_xattr_handler = {
 
 static const struct xattr_handler *sockfs_xattr_handlers[] = {
 	&sockfs_xattr_handler,
+	&sockfs_security_xattr_handler,
 	NULL
 };
 
-- 
2.7.4

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

[Casey Schaufler]

On 11/3/2016 9:00 AM, Andreas Gruenbacher wrote:
> On Thu, Nov 3, 2016 at 4:51 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>> 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.
> Ah, I forgot to register the new sockfs xattr handler.  Does this help?

Yes. With this addition it looks good.

>
> Thanks,
> Andreas
>
> ---
>  net/socket.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/net/socket.c b/net/socket.c
> index 816392a..9820725 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -357,6 +357,7 @@ static const struct xattr_handler sockfs_security_xattr_handler = {
>  
>  static const struct xattr_handler *sockfs_xattr_handlers[] = {
>  	&sockfs_xattr_handler,
> +	&sockfs_security_xattr_handler,
>  	NULL
>  };
>