Message-ID: <20140424100115.GK17824@quack.suse.cz>
Date:	Thu, 24 Apr 2014 12:01:15 +0200
From:	Jan Kara <jack@...e.cz>
To:	Heinrich Schuchardt <xypron.glpk@....de>
Cc:	Jan Kara <jack@...e.cz>,
	"Michael Kerrisk (man-pages)" <mtk.manpages@...il.com>,
	Eric Paris <eparis@...hat.com>,
	lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] fanotify: check permissions when creating file
 descriptor

On Tue 22-04-14 22:51:58, Heinrich Schuchardt wrote:
> On 22.04.2014 16:07, Jan Kara wrote:
> >>>On Sat 19-04-14 22:53:53, Heinrich Schuchardt wrote:
> >>>>When monitoring a directory or a mount with the fanotify API
> >>>>the call to fanotify_init checks,
> >>>>  * the process has cap_sys_admin capability
> >>>>
> >>>>The call to fanotify_mark checks,
> >>>>  * the process has read authorization for directory or mount
> >>>>
> >>>>A directory or mount may contain files for which the process
> >>>>has no read or write authorization.
> >>>>Yet when reading from the fanotify file descriptor, structures
> >>>>fanotify_event_metadata are returned, which contain a file
> >>>>descriptor for these files, and will allow to read or write.
> >>>>
> >>>>The patch adds an authorization check for read and write
> >>>>permission. In case of missing permission, reading from the
> >>>>fanotify file descriptor returns EACCES.
> >>>   OK, am I right you are concerned about a situation where fanotify group
> >>>descriptor is passed to an unpriviledged process which handles all the
> >>>incoming events? I'm asking because the permission checking can be
> >>>relatively expensive (think of acls) so we better do it for a reason.
> >>>I'd prefer to hear from Eric what the original intention regarding
> >>>permissions was...
> >
> >Reviewed-by: Jan Kara <jack@...e.cz>
> >
> 
> before applying the patch I think another problem has to be solved.
> 
> fanotify_read can have one of the following outcomes:
> 
> 1) Everything works fine one or multiple struct fanotify_event_metadata
> are returned. fanotify_event_metadata->fd references the concerned
> files.
> 2) An overflow occured. fanotify_event_metadata->fd is set to FAN_NOFD.
> 3) An error occured. fanotify_read returns no
> struct fanotify_event_metadata but fails with an error code.
> 
> This means any error in create_fd (called by fanotify read) may lead to
> lost events, if the error does not occur in the first event handled by
> fanotify_read.
> And not only events are lost, but also references to file descriptors
> are lost.
> 
> Of cause create_fd can already fail with EMFILE, if no more file
> descriptors are available. (Not a good situation to lose references
> to file descriptors?)
> 
> If we add the patch to check permissions errors in create_fd will
> be much more of an issue. A malware might force such errors to
> occur by writing to a file with chmod 200.
  Yeah, so as I wrote in another email I don't think permission checking in
create_fd() is needed after all but still the problem you raised is valid.

> Hence we have to rethink how errors are to be handled.
> 
> Instead of having fanotify_read returning an error code it could
> set the concerned fanotify_event_metadata->fd to the
> negative value of the error code, and return this
> fanotify_event_metadata as the last event.
> 
> Unfortunately this might break existing code, if this code only
> checks fanotify_event_metadata->fd against FAN_NOFD.
> 
> Another solution would be to simply set
> fanotify_event_metadata->fd = FAN_NOFD
> and errno to the error code.
> 
> This would not break any existing code, because such code already
> has to be able to handle FAN_NOFD. And the error relating to
> FANO_NOFD can be recovered from errno.
> 
> What is your idea how fanotify_read should gracefully handle a
> situation were the last of many returned events has a problem?
  I think the best behavior would be the following:
If we are filling out the first event, return the error which happened. If
we are filling out second or further event, leave the problematic event in
the queue and only return events succesfully copied.

The implementation isn't trivial but should be doable - you can peek at the
head of the event queue, try to open a file, and only if that succeeded remove
the event from the queue. To avoid races with other readers removing event
from the head of the queue, you have to hold group->notification_mutex
while doing this. A more lightweight alternative is to just peek at the
head of the queue, take reference to 'path', drop notification_mutex, open
the file described by path. Then you take notification_mutex again and
recheck whether the head event has the same path you opened - if yes, you
proceed with what you have, if no, you close the file and try again.

Do you want to look into this or should I cook up some patch for it?

								Honza
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
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