Re: [PATCH 1/1] fanotify: check permissions when creating file descriptor

[Heinrich Schuchardt]

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@xxxxxxx>

Hello Jan, hello Eric

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.

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?

Best regards

Heinrich Schuchardt
--
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/