Re: [fuse-devel] [PATCH v4 4/5] fuse: Support privileged xattrs only with a mount option

[Seth Forshee]

On Tue, Oct 14, 2014 at 02:19:19PM -0700, Andy Lutomirski wrote:
> On Tue, Oct 14, 2014 at 2:13 PM, Eric W. Biederman
> <ebiederm@xxxxxxxxxxxx> wrote:
> > Seth Forshee <seth.forshee@xxxxxxxxxxxxx> writes:
> >
> >> On Tue, Oct 14, 2014 at 01:01:02PM -0700, Eric W. Biederman wrote:
> >>> Michael j Theall <mtheall@xxxxxxxxxx> writes:
> >>>
> >>> > Seth Forshee <seth.forshee@xxxxxxxxxxxxx> wrote on 10/14/2014 09:25:55 AM:
> >>> >
> >>> >> From: Seth Forshee <seth.forshee@xxxxxxxxxxxxx>
> >>> >> To: Miklos Szeredi <miklos@xxxxxxxxxx>
> >>> >> Cc: fuse-devel@xxxxxxxxxxxxxxxxxxxxx, "Serge H. Hallyn"
> >>> >> <serge.hallyn@xxxxxxxxxx>, linux-kernel@xxxxxxxxxxxxxxx, Seth
> >>> >> Forshee <seth.forshee@xxxxxxxxxxxxx>, "Eric W. Biederman"
> >>> >> <ebiederm@xxxxxxxxxxxx>, linux-fsdevel@xxxxxxxxxxxxxxx
> >>> >> Date: 10/14/2014 09:27 AM
> >>> >> Subject: [fuse-devel] [PATCH v4 4/5] fuse: Support privileged xattrs
> >>> >> only with a mount option
> >>> >>
> >>> >> Allowing unprivileged users to provide arbitrary xattrs via fuse
> >>> >> mounts bypasses the normal restrictions on setting xattrs. Such
> >>> >> mounts should be restricted to reading and writing xattrs in the
> >>> >> user.* namespace.
> >>> >>
> >>> >
> >>> > Can you explain how the normal restrictions on setting xattrs are
> >>> > bypassed?
> >>>
> >>> If the fuse server is not run by root.  Which is a large part of the
> >>> point of fuse.
> >>
> >> So the server could for example return trusted.* xattrs which were not
> >> set by a privileged user.
> >>
> >>> > My filesystem still needs security.* and system.*, and it looks like
> >>> > xattr_permission already prevents non-privileged users from accessing
> >>> > trusted.*
> >>>
> >>> If the filesystem is mounted with nosuid (typical of a non-privileged
> >>> mount of fuse) then the security.* attributes are ignored.
> >>
> >> That I wasn't aware of. In fact I still haven't found where this
> >> restriction is implemented.
> >
> > My memory may be have been incomplete.  What I was thinking of is
> > security/commoncap.c the MNT_NOSUID check in get_file_caps.
> >
> > Upon inspection that appears limited to file capabilities, and while
> > there are a few other MNT_NOSUID checks under security the feel far from
> > complete.
> >
> > Sigh.
> >
> > This deserves a hard look because if MNT_NOSUID is not sufficient than
> > it may be possible for me to insert a usb stick with an extN filesystem
> > with the right labels having it auto-mounted nosuid and subvert the
> > security of something like selinux.
> 
> It's this code in selinux/hooks.c:
> 
>     if ((bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) ||
>         (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS))
>         new_tsec->sid = old_tsec->sid;
> 
> 
> One might argue that this should actually generate -EPERM instead of
> ignoring the label, but it should be safe against untrusted media.
> 
> >
> >> Nonetheless, a userns mount could be done without nosuid (though that
> >> mount will also be unaccessible outside of that namespace).
> >>
> >>> >> It's difficult though to tell whether a mount is being performed
> >>> >> on behalf of an unprivileged user since fuse mounts are ususally
> >>> >> done via a suid root helper. Thus a new mount option,
> >>> >> privileged_xattrs, is added to indicated that xattrs from other
> >>> >> namespaces are allowed. This option can only be supplied by
> >>> >> system-wide root; supplying the option as an unprivileged user
> >>> >> will cause the mount to fail.
> >>> >
> >>> > I can't say I'm convinced that this is the right direction to head.
> >>>
> >>> With respect to defaults we could keep the current default if you
> >>> have the global CAP_SYS_ADMIN privilege when the mount takes place
> >>> and then avoid breaking anything.
> >>
> >> Except that unprivileged mounts are normally done by a suid root helper,
> >> which is why I've required both global CAP_SYS_ADMIN and a mount option
> >> to get the current default behavior.
> >
> > If nosuid is sufficient that may break existing setups for no good
> > reason.
> >
> > Shrug.  I won't have much time for a bit but I figured I would highlight
> > the potential security hole in existing setups.  So someone with time
> > this week can look at that.
> 
> I think I have a better solution.  I'll send it out.

To be honest I don't understand enough about how selinux uses security
labels to know what level of paranoia is appropriate, so I wrote this
out of an excess of paranoia. If the patch you sent restricts things
sufficiently then I'm perfectly happy to see this patch dropped.

And really with fuse all of this is really excess paranoia because (if
my other patches are applied at least) the fuse mount will be
inaccessible to any user outside the user namespace from which it was
mounted or its descendants.

Thanks,
Seth

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