public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Jann Horn <jann@thejh.net>
To: Nikhilesh Reddy <reddyn@codeaurora.org>
Cc: torvalds@linux-foundation.org, Miklos Szeredi <miklos@szeredi.hu>,
	fuse-devel <fuse-devel@lists.sourceforge.net>,
	linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
	gregkh@linuxfoundation.org, linux-fsdevel@vger.kernel.org,
	viro@zeniv.linux.org.uk, Richard Weinberger <richard@nod.at>,
	"Theodore Ts'o" <tytso@mit.edu>,
	jack@suse.cz, Antonio SJ Musumeci <trapexit@spawn.link>,
	sven.utcke@gmx.de, Nikolaus Rath <nikolaus@rath.org>,
	Jann Horn <jannhorn@googlemail.com>, Mike Shal <marfey@gmail.com>
Subject: Re: [PATCH v5] fuse: Add support for passthrough read/write
Date: Wed, 3 Feb 2016 21:42:36 +0100	[thread overview]
Message-ID: <20160203204236.GA17361@pc.thejh.net> (raw)
In-Reply-To: <56B26011.70609@codeaurora.org>

[-- Attachment #1: Type: text/plain, Size: 4794 bytes --]

On Wed, Feb 03, 2016 at 12:16:17PM -0800, Nikhilesh Reddy wrote:
> On 02/03/2016 11:53 AM, Jann Horn wrote:
> >On Wed, Feb 03, 2016 at 11:05:57AM -0800, Nikhilesh Reddy wrote:
> >>Hi
> >>Thanks for your review again :)
> >>>
> >>>Uh... how do you know at this point that the file is actually writable?
> >>>Normally, e.g. vfs_write() will ensure that the file is writable, and
> >>>e.g. generic_file_write_iter() won't check for writability as far as I
> >>>can tell. This might allow someone to use the passthrough mechanism to
> >>>overwrite a file he is only allowed to read, but not write, like
> >>>/etc/passwd.
> >>
> >>I considered adding the checks ( the same ones that VFS does)  but not sure
> >>if we need to.
> >>So the user will need to construct a fuse filesystem ( that opens for
> >>O_READONLY even though the user asks for a O_RDWR from the FUSE open) and
> >>then mount it , with CAP_SYS_ADMIN  for which you need to be root but  once
> >>he has that he should be able to easily get to the files without needing to
> >>go through FUSE  right using CAP_DAC_OVERRIDE?
> >>
> >>Am i missing something? Please do help me understand.
> >>
> >>But yes if really needed I can add additional checks once i understand it
> >
> >On most Linux desktop systems, and on many servers, the userland "fuse"
> >package is installed, which ships with a setuid root helper "fusermount":
> >
> >$ ls -l /bin/fusermount
> >-rwsr-xr-x 1 root root 30800 May 21  2015 /bin/fusermount
> >
> >This setuid helper allows any user to mount FUSE filesystems anywhere he
> >wants. This works as follows: main() calls mount_fuse(), which opens
> >/dev/fuse by calling open_fuse_device(). mount_fuse() then makes sure
> >that the caller has write access to the directory he is about to mount
> >over using check_perm(), then calls mount() via do_mount(). mount_fuse()
> >returns the /dev/fuse fd, which is then sent to the invoker of fusermount
> >through a unix domain socket.
> >
> >(What the setuid binary does control are the mount options; those are
> >used to enforce that the user can't mount filesystems that are
> >accessible for other users.)
> >
> >Note that at no point, any data is sent to or read from the FUSE control
> >fd by fusermount. Therefore, the init reply that is processed in
> >process_init_reply() and determines whether passthrough will be enabled
> >is controlled by the unprivileged caller.
> >
> >This fusermount mechanism is used by pseudo-filesystems like sshfs in
> >order to allow unprivileged users to use them.
> >
> >fusermount aside, there is also an (as far as I know) pending patch with
> >the intent to make FUSE usable in user namespaces, which is going to
> >allow unprivileged users to mount FUSE filesystems even without a
> >userspace helper: https://lkml.org/lkml/2015/12/2/472
> >
> >(Note that this is very different from CUSE, which by design must never
> >be exposed to non-root code.)
> 
> Thanks for the explanation ..I am convinced :)
>  will add the checks when i send out the next version ( Probably by end of
> the week.. hopefully will be the last version :) :) )
> 
> Something on the lines of
> if (!(file->f_mode & FMODE_WRITE))
> 	return -EBADF;
> if (!(file->f_mode & FMODE_CAN_WRITE))
> 	return -EINVAL;
> 
> And
> if (!(file->f_mode & FMODE_READ))
> 	return -EBADF;
> if (!(file->f_mode & FMODE_CAN_READ))
> 	return -EINVAL;
> 	
> 
> >
> >>>Also, I think this might bypass mandatory locks, the
> >>>security_file_permission hook (which seems like a bad idea anyway
> >>>though), inotify/fsnotify and sb_start_write.
> >>>
> >>Can you please elaborate/clarify further? I am am not sure what you mean.
> 
> Can you please also explain what you meant by :
> "might bypass mandatory locks, the security_file_permission hook (which
> seems like a bad idea anyway though), inotify/fsnotify and sb_start_write."

Have a look at the stuff that goes on in vfs_write:


It checks that FMODE_WRITE is in file->f_mode (to ensure that the file is
open for writing.

It uses rw_verify_area() to check for various things; in particular, it
verifies that nobody else has taken a mandatory lock on the target of
the write operation and that no LSM wishes to prevent the write access
through the file_permission hook.

It uses file_start_write() and file_end_write() to (as far as I can
tell) prevent the filesystem from being frozen (for standby/hibernate)
while a write access is in progress.

It uses fsnotify_modify() to inform userspace that the file changed.


You might want to call one of the vfs helper functions so that you
don't have to do all of this yourself. Something like
vfs_readv/vfs_writev - but I'm not very familiar with this part.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

  reply	other threads:[~2016-02-03 20:42 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-02-01 18:56 [PATCH v5] fuse: Add support for passthrough read/write Nikhilesh Reddy
2016-02-01 19:15 ` Jann Horn
2016-02-01 19:28   ` Nikhilesh Reddy
2016-02-01 19:45     ` Jann Horn
2016-02-03 19:05       ` Nikhilesh Reddy
2016-02-03 19:56         ` Jann Horn
2016-02-02  8:10 ` Jann Horn
2016-02-03 19:05   ` Nikhilesh Reddy
2016-02-03 19:53     ` Jann Horn
2016-02-03 20:16       ` Nikhilesh Reddy
2016-02-03 20:42         ` Jann Horn [this message]
2016-03-04 12:23 ` [fuse-devel] " Andrew Karpow

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160203204236.GA17361@pc.thejh.net \
    --to=jann@thejh.net \
    --cc=fuse-devel@lists.sourceforge.net \
    --cc=gregkh@linuxfoundation.org \
    --cc=jack@suse.cz \
    --cc=jannhorn@googlemail.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marfey@gmail.com \
    --cc=miklos@szeredi.hu \
    --cc=nikolaus@rath.org \
    --cc=reddyn@codeaurora.org \
    --cc=richard@nod.at \
    --cc=sven.utcke@gmx.de \
    --cc=torvalds@linux-foundation.org \
    --cc=trapexit@spawn.link \
    --cc=tytso@mit.edu \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox