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

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

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@suse.cz>
>

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