linux-kernel - Re: [PATCH v2 0/3] fuse: Add support for mounts from pid/user namespaces

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140911181034.GA58733@ubuntu-hedt>
Date:	Thu, 11 Sep 2014 13:10:34 -0500
From:	Seth Forshee <seth.forshee@...onical.com>
To:	"Serge E. Hallyn" <serge@...lyn.com>
Cc:	Miklos Szeredi <miklos@...redi.hu>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Serge Hallyn <serge.hallyn@...ntu.com>,
	fuse-devel@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v2 0/3] fuse: Add support for mounts from pid/user
 namespaces

On Wed, Sep 10, 2014 at 11:42:12AM -0500, Seth Forshee wrote:
> On Wed, Sep 10, 2014 at 06:21:55PM +0200, Serge E. Hallyn wrote:
> > Quoting Seth Forshee (seth.forshee@...onical.com):
> > > On Tue, Sep 02, 2014 at 10:44:53AM -0500, Seth Forshee wrote:
> > > > Another issue mentioned by Eric was what to use for i_[ug]id if the ids
> > > > from userspace don't map into the user namespace, which is going to be a
> > > > problem for any other filesystems which become mountable from user
> > > > namespaces as well. We discussed a few options for addressing this, the
> > > > most promising of which seems to be either using INVALID_[UG]ID for
> > > > these inodes or creating vfs-wide "nobody" ids for this purpose. After
> > > > thinking about it for a while I'm favoring using the invalid ids, but
> > > > I'm hoping to solicit some more feedback.
> > > > 
> > > > For now these patches are using invalid ids if the user doesn't map into
> > > > the namespace. I went through the vfs code and found one place where
> > > > this could be handled better (addressed in patch 1 of the series). The
> > > > only other issue I found was that currently no one, not even root, can
> > > > change onwership of such inodes, but I suspect we can find a way around
> > > > this.
> > > 
> > > I started playing around with using -2 as a global nobody id. The
> > > changes below (made on top of this series) are working fine in light
> > > testing. I'm still not sure about the security implications of doing
> > > this though. Possibly the nobody id should be removed from init_user_ns
> > > to prevent any use of the id to gain unauthroized access to such files.
> > > Thoughts?
> > 
> > Can you explain the downsides of just using -1?  What are we able to do
> > (as a fuse-in-container user) when we use -2 that we can't do when it
> > uses -1?
> 
> The thing that happens with -1 is that checks like
> capable_wrt_inode_uidgid() always fail on those inodes because
> INVALID_UID isn't ever mapped into any namespace, even if you're
> system-wide root. If we use a real id this check would at least pass in
> init_user_ns, assuming that we don't have to remove -2 from init_user_ns
> for security reasons.
> 
> Or we could modify these checks so that CAP_SYS_ADMIN always gets
> permission or something like that, which might be the better way to go.

This ought to do (untested as of yet). I think I like this best, but I'm
also interested in hearing what Eric has to say.


diff --git a/fs/inode.c b/fs/inode.c
index 26753ba7b6d6..1029320ff029 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1840,6 +1840,9 @@ bool inode_owner_or_capable(const struct inode *inode)
 {
        struct user_namespace *ns;
 
+       if (capable(CAP_SYS_ADMIN))
+               return true;
+
        if (uid_eq(current_fsuid(), inode->i_uid))
                return true;
 
diff --git a/kernel/capability.c b/kernel/capability.c
index 989f5bfc57dc..a472eaa52b6a 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -438,8 +438,12 @@ EXPORT_SYMBOL(capable);
  */
 bool capable_wrt_inode_uidgid(const struct inode *inode, int cap)
 {
-       struct user_namespace *ns = current_user_ns();
+       struct user_namespace *ns;
 
+       if (capable(CAP_SYS_ADMIN))
+               return true;
+
+       ns = current_user_ns();
        return ns_capable(ns, cap) && kuid_has_mapping(ns, inode->i_uid) &&
                kgid_has_mapping(ns, inode->i_gid);
 }

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/