Date:	Mon, 2 Nov 2015 18:02:52 +0000
From:	Serge Hallyn <serge.hallyn@...ntu.com>
To:	Dirk Steinmetz <public@...tdrjgfuzkfg.com>
Cc:	Seth Forshee <seth.forshee@...onical.com>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Andy Lutomirski <luto@...capital.net>,
	Kees Cook <keescook@...omium.org>,
	Serge Hallyn <serge.hallyn@...onical.com>
Subject: Re: [PATCH] namei: permit linking with CAP_FOWNER in userns

Quoting Dirk Steinmetz (public@...tdrjgfuzkfg.com):
> On Wed, 28 Oct 2015 17:33:10 +0000, Serge Hallyn wrote:
> > Quoting Dirk Steinmetz (public@...tdrjgfuzkfg.com):
> > > On Tue, 27 Oct 2015 20:28:02 +0000, Serge Hallyn wrote:
> > > > Quoting Dirk Steinmetz (public@...tdrjgfuzkfg.com):
> > > > > On Tue, 27 Oct 2015 09:33:44 -0500, Seth Forshee wrote:
> > > > > > I did want to point what seems to be an inconsistency in how
> > > > > > capabilities in user namespaces are handled with respect to inodes. When
> > > > > > I started looking at this my initial thought was to replace
> > > > > > capable(CAP_FOWNER) with capable_wrt_inode_uidgid(inode, CAP_FOWNER). On
> > > > > > the face of it this should be equivalent to what's done here, but it
> > > > > > turns out that capable_wrt_inode_uidgid requires that the inode's uid
> > > > > > and gid are both mapped into the namespace whereas
> > > > > > inode_owner_or_capable only requires the uid be mapped. I'm not sure how
> > > > > > significant that is, but it seems a bit odd.
> > > > > 
> > > > > I agree that this seems odd. I've chosen inode_owner_or_capable over
> > > > > capable_wrt_inode_uidgid(inode, CAP_FOWNER) as it seemed consistent:
> > > > > a privileged user (with CAP_SETUID) can impersonate the owner UID and thus
> > > > > bypass the check completely; this also matches the documented behavior of
> > > > > CAP_FOWNER: "Bypass permission checks on operations that normally require
> > > > > the filesystem UID of the process to match the UID of the file".
> > > > > 
> > > > > However, thinking about it I get the feeling that checking the gid seems
> > > > > reasonable as well. This is, however, independently of user namespaces.
> > > > > Consider the following scenario in any namespace, including the init one:
> > > > > - A file has the setgid and user/group executable bits set, and is owned
> > > > >   by user:group.
> > > > > - The user 'user' is not in the group 'group', and does not have any
> > > > >   capabilities.
> > > > > - The user 'user' hardlinks the file. The permission check will succeed,
> > > > >   as the user is the owner of the file.
> > > > > - The file is replaced with a newer version (for example fixing a security
> > > > >   issue)
> > > > > - Now user can still use the hardlink-pinned version to execute the file
> > > > >   as 'user:group' (and for example exploit the security issue).
> > > > > I would have expected the user to not be able to hardlink, as he lacks
> > > > > CAP_FSETID, and thus is not allowed to chmod, change or move the file
> > > > > without loosing the setgid bit. So it is impossible for him to make a non-
> > > > > hardlink copy with the setgid bit set -- why should he be able to make a
> > > > > hardlinked one?
> > > > 
> > > > Yeah, this sounds sensible.  It allows a user without access to 'disk',
> > > > for instance, to become that group.
> > > > 
> > > > > It seems to me as if may_linkat would additionally require a check
> > > > > verifying that either
> > > > > - not both setgid and group executable bit set
> > > > > - fsgid == owner gid
> > > > > - capable_wrt_inode_uidgid(CAP_FSETID) -- or CAP_FOWNER, depending on
> > > > >   whether to adapt chmod's behavior or keeping everything hardlink-
> > > > >   related in CAP_FOWNER; I don't feel qualified enough to pick ;)
> > > > 
> > > > In particular just changing it is not ok since people who are using file
> > > > capabilities to grant what they currently need would be stuck with a
> > > > mysterious new failure.
> > > 
> > > Is there any use case (besides exploiting hardlinks with malicious intent)
> > > that would be broken when changing this? There are some (imho) rather
> > > unlikely conditions to be met in order to observe changed behavior:
> > 
> > The simplest example would be if I wanted to run a very quick program to
> > just add the symbolic link.  Let's say the link /usr/sbin/uuidd were owned
> > by root:disk and setuid and setgid.  The proposed change would force me
> > to bind in both the root user and disk group, whereas without it I can
> > just bind in only the root user.
> While root usually has CAP_FSETID and CAP_FOWNER, which would still permit
> linking in this case, I agree that the change could be visible when
> performing specific maintenance tasks in some rare setups.
> 
> > We've already dealt with such regressions and iirc agreed that they were
> > worthwhile.
> Would you prefer to not fix the issue at all, then? Or would you prefer to

No.  I think I was saying I think it's worth adding the 'gid must be mapped'
requirement.

And I was saying that changing the capability needed is not ok.

> add a new value on /proc/sys/fs/protected_hardlinks -- which would still
> cause the symptoms you describe on distributions using the new value, but
> would be more easy to change for users knowing that this is an issue?
> 
> I personally still favor changing the behavior and documentation over a new
> value there, as -- once identified by the user -- the user can easily adapt

I agree.

Note the difference - changing the capability required to link the
file can affect (probably rare, but definately) normal, non-user-namespace
setups.  Changing the link requirements in a user namespace so that gid
must be mapped only affects a case which we've previously said should not
be supported.

Linus may still disagree - not changing what userspace can do is pretty
fundamental, but this was purely a missed security fix iiuc.

> his usage or disable protected hardlinks globally, depending on the actual
> requirements. And another value will not improve the abilities of the user
> to identify protected hardlinks as reason for the changed behavior.

Agreed, and more to the point changing the sysctl required woudl still be
breaking userspace.  We may as well require the proper, safe setup, rather
than requiring a unsafe sysctl.

-serge
--
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/

Powered by blists - more mailing lists