[PATCH] fuse: Only allow read/writing user xattrs

[PATCH] fuse: Only allow read/writing user xattrs

[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.

Cc: Miklos Szeredi <miklos@szeredi.hu>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---

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

 fs/fuse/dir.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 45a27c4..260de56 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -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

Re: [PATCH] fuse: Only allow read/writing user xattrs

[Eric Paris]

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

On Fri, Oct 5, 2012 at 6:50 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> In the context of unprivileged mounts supporting anything except
> xattrs with the "user." prefix seems foolish.  Return -EOPNOSUPP
> for all other types of xattrs.
>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>
> Do we really have a kernel bug as blantant as this appears?
>
>  fs/fuse/dir.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 45a27c4..260de56 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fuse: Only allow read/writing user xattrs

[Eric W. Biederman]

Eric Paris <eparis@parisplace.org> writes:

> 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

Re: [PATCH] fuse: Only allow read/writing user xattrs

[Eric Paris]

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

On Sat, Oct 6, 2012 at 11:34 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Eric Paris <eparis@parisplace.org> writes:
>
>> 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

Re: [PATCH] fuse: Only allow read/writing user xattrs

[Eric W. Biederman]

Eric Paris <eparis@parisplace.org> writes:

> 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

Re: [PATCH] fuse: Only allow read/writing user xattrs

[Serge Hallyn]

Quoting Eric W. Biederman (ebiederm@xmission.com):
> Eric Paris <eparis@parisplace.org> writes:
> 
> > 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.

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

Re: [PATCH] fuse: Only allow read/writing user xattrs

[Eric Paris]

On Sat, Oct 6, 2012 at 7:42 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Eric Paris <eparis@parisplace.org> writes:
>
>> 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.

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.