Skip to contentSkip to conversation

fuse: Only allow read/writing user xattrs

PATCH NAK Eric W. Biederman 3 participants 7 messages Oct 5, 2012 – Oct 8, 2012

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 fuse: Only allow read/writing user xattrs

In the context of unprivileged mounts supporting anything except xattrs with the "user." prefix seems foolish. Return -EOPNOSUPP for all other types of xattrs.

---

Do we really have a kernel bug as blantant as this appears?

 fs/fuse/dir.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
fs/fuse/dir.c +7 -0
index 45a27c4..260de56 100644
@@ -13,6 +13,7 @@
 #include <linux/sched.h>
 #include <linux/namei.h>
 #include <linux/slab.h>
+#include <linux/xattr.h>
 
 #if BITS_PER_LONG >= 64
 static inline void fuse_dentry_settime(struct dentry *entry, u64 time)
@@ -1537,6 +1538,9 @@ static int fuse_setxattr(struct dentry *entry, const char *name,
 	if (fc->no_setxattr)
 		return -EOPNOTSUPP;
 
+	if (strncmp(name, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN) != 0)
+		return -EOPNOTSUPP;
+
 	req = fuse_get_req(fc);
 	if (IS_ERR(req))
 		return PTR_ERR(req);
@@ -1576,6 +1580,9 @@ static ssize_t fuse_getxattr(struct dentry *entry, const char *name,
 	if (fc->no_getxattr)
 		return -EOPNOTSUPP;
 
+	if (strncmp(name, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN) != 0)
+		return -EOPNOTSUPP;
+
 	req = fuse_get_req(fc);
 	if (IS_ERR(req))
 		return PTR_ERR(req);
Signature · 1 line
1.7.5.4
#1 lore raw

NAK, this makes no sense... We are working to support SELinux
attrs on fuse, why shouldn't we?

Quoted 53 lines · Eric W. Biederman

In the context of unprivileged mounts supporting anything except xattrs with the "user." prefix seems foolish. Return -EOPNOSUPP for all other types of xattrs.

---

Do we really have a kernel bug as blantant as this appears?

 fs/fuse/dir.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
fs/fuse/dir.c +7 -1
index 45a27c4..260de56 100644
@@ -13,6 +13,7 @@
 #include <linux/sched.h>
 #include <linux/namei.h>
 #include <linux/slab.h>
+#include <linux/xattr.h>
 
 #if BITS_PER_LONG >= 64
 static inline void fuse_dentry_settime(struct dentry *entry, u64 time)
@@ -1537,6 +1538,9 @@ static int fuse_setxattr(struct dentry *entry, const char *name,
        if (fc->no_setxattr)
                return -EOPNOTSUPP;
 
+       if (strncmp(name, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN) != 0)
+               return -EOPNOTSUPP;
+
        req = fuse_get_req(fc);
        if (IS_ERR(req))
                return PTR_ERR(req);
@@ -1576,6 +1580,9 @@ static ssize_t fuse_getxattr(struct dentry *entry, const char *name,
        if (fc->no_getxattr)
                return -EOPNOTSUPP;
 
+       if (strncmp(name, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN) != 0)
+               return -EOPNOTSUPP;
+
        req = fuse_get_req(fc);
        if (IS_ERR(req))
                return PTR_ERR(req);
--

1.7.5.4

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#2 lore raw
Quoted 2 lines · Eric Paris

NAK, this makes no sense... We are working to support SELinux
attrs on fuse, why shouldn't we?

Certainly there is a logical disconnect here.

What is the sense in security labels when anyone can arbitrarily choose the security label they want and change the security label whenever they want?

What is the point in a security label when you can not trust it?

Eric

#3 lore raw

Why trust uids or rwx bits. Might as well do away with those as well, right?

Quoted 14 lines · Eric W. Biederman
Quoted 2 lines · Eric Paris

NAK, this makes no sense... We are working to support SELinux
attrs on fuse, why shouldn't we?

Certainly there is a logical disconnect here.

What is the sense in security labels when anyone can arbitrarily choose the security label they want and change the security label whenever they want?

What is the point in a security label when you can not trust it?

Eric

#4 lore raw
Quoted 2 lines · Eric Paris

Why trust uids or rwx bits. Might as well do away with those as well, right?

Lying to your own userspace processes (which you can do with LD_PRELOAD) is rather different than lying to the selinux or the smack modules.

What I am saying with my patch is that fuse is remarkably non-nuanced in how it interacts with extended attributes, and that it appears very clear that there are bugs in the area of unprivileged mounts that need to be addressed.

I am happy to hear about better solutions. Telling me it's not a bug and sticking your head in the sand is quite amusing.

Eric

#5 lore raw
Quoted 8 lines · Eric W. Biederman
Quoted 2 lines · Eric Paris

Why trust uids or rwx bits. Might as well do away with those as well, right?

Lying to your own userspace processes (which you can do with LD_PRELOAD) is rather different than lying to the selinux or the smack modules.

Quoted 7 lines

What I am saying with my patch is that fuse is remarkably non-nuanced in how it interacts with extended attributes, and that it appears very clear that there are bugs in the area of unprivileged mounts that need to be addressed.

I am happy to hear about better solutions. Telling me it's not a bug and sticking your head in the sand is quite amusing.

I'm not terribly familiar with the ways fuse modules can be loaded. Would it be possible to, at load time, based on the (selinux) credentials of the user loading the module, either allow the loader to specify that security.* and trusted.* xattrs may be used, or, if user is unprivileged, ignore the xattrs and use a default based on the user's credentials?

-serge

#6 lore raw
In reply to Eric W. Biederman
Quoted 8 lines · Eric W. Biederman
Quoted 2 lines · Eric Paris

Why trust uids or rwx bits. Might as well do away with those as well, right?

Lying to your own userspace processes (which you can do with LD_PRELOAD) is rather different than lying to the selinux or the smack modules.

Quoted 7 lines

What I am saying with my patch is that fuse is remarkably non-nuanced in how it interacts with extended attributes, and that it appears very clear that there are bugs in the area of unprivileged mounts that need to be addressed.

I am happy to hear about better solutions. Telling me it's not a bug and sticking your head in the sand is quite amusing.

I'm not sure how to fix it. But breaking things that do work today for untrusted user mounts isn't right or acceptable. Maybe serge is onto something. Or maybe the best solution is to require LSM policy to just disallow all unpriv (from init namespace PoV) mounts.