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

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

[Heinrich Schuchardt]

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.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 fs/notify/fanotify/fanotify_user.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 4e565c8..5d22a20 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -62,6 +62,8 @@ static int create_fd(struct fsnotify_group *group,
 {
 	int client_fd;
 	struct file *new_file;
+	int mask;
+	int ret;
 
 	pr_debug("%s: group=%p event=%p\n", __func__, group, event);
 
@@ -75,11 +77,19 @@ static int create_fd(struct fsnotify_group *group,
 	 */
 	/* it's possible this event was an overflow event.  in that case dentry and mnt
 	 * are NULL;  That's fine, just don't call dentry open */
-	if (event->path.dentry && event->path.mnt)
-		new_file = dentry_open(&event->path,
-				       group->fanotify_data.f_flags | FMODE_NONOTIFY,
-				       current_cred());
-	else
+	if (event->path.dentry && event->path.mnt) {
+		/* check permissions before granting access to file */
+		mask = MAY_READ;
+		if (group->fanotify_data.f_flags & (O_RDWR | O_WRONLY))
+			mask |= MAY_WRITE;
+		ret = inode_permission(event->path.dentry->d_inode, mask);
+		if (ret)
+			new_file = ERR_PTR(ret);
+		else
+			new_file = dentry_open(&event->path,
+					       group->fanotify_data.f_flags | FMODE_NONOTIFY,
+					       current_cred());
+	} else
 		new_file = ERR_PTR(-EOVERFLOW);
 	if (IS_ERR(new_file)) {
 		/*
-- 
1.9.1

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

[Jan Kara]

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...

								Honza
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  fs/notify/fanotify/fanotify_user.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 4e565c8..5d22a20 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -62,6 +62,8 @@ static int create_fd(struct fsnotify_group *group,
>  {
>  	int client_fd;
>  	struct file *new_file;
> +	int mask;
> +	int ret;
>  
>  	pr_debug("%s: group=%p event=%p\n", __func__, group, event);
>  
> @@ -75,11 +77,19 @@ static int create_fd(struct fsnotify_group *group,
>  	 */
>  	/* it's possible this event was an overflow event.  in that case dentry and mnt
>  	 * are NULL;  That's fine, just don't call dentry open */
> -	if (event->path.dentry && event->path.mnt)
> -		new_file = dentry_open(&event->path,
> -				       group->fanotify_data.f_flags | FMODE_NONOTIFY,
> -				       current_cred());
> -	else
> +	if (event->path.dentry && event->path.mnt) {
> +		/* check permissions before granting access to file */
> +		mask = MAY_READ;
> +		if (group->fanotify_data.f_flags & (O_RDWR | O_WRONLY))
> +			mask |= MAY_WRITE;
> +		ret = inode_permission(event->path.dentry->d_inode, mask);
> +		if (ret)
> +			new_file = ERR_PTR(ret);
> +		else
> +			new_file = dentry_open(&event->path,
> +					       group->fanotify_data.f_flags | FMODE_NONOTIFY,
> +					       current_cred());
> +	} else
>  		new_file = ERR_PTR(-EOVERFLOW);
>  	if (IS_ERR(new_file)) {
>  		/*
> -- 
> 1.9.1
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

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

[Michael Kerrisk (man-pages)]

On Tue, Apr 22, 2014 at 3:40 PM, Jan Kara <jack@suse.cz> 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...

If I understand correctly, passing to an unprivileged process is the
point. The point is I think that supposedly one only needs to
CAP_SYS_ADMIN to use fanotify. However, once you have that capability,
then you implicitly get the effect of CAP_DAC_READ_SEARCH and
CAP_DAC_OVERRIDE as well.

Cheers,

Michael


>>  fs/notify/fanotify/fanotify_user.c | 20 +++++++++++++++-----
>>  1 file changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
>> index 4e565c8..5d22a20 100644
>> --- a/fs/notify/fanotify/fanotify_user.c
>> +++ b/fs/notify/fanotify/fanotify_user.c
>> @@ -62,6 +62,8 @@ static int create_fd(struct fsnotify_group *group,
>>  {
>>       int client_fd;
>>       struct file *new_file;
>> +     int mask;
>> +     int ret;
>>
>>       pr_debug("%s: group=%p event=%p\n", __func__, group, event);
>>
>> @@ -75,11 +77,19 @@ static int create_fd(struct fsnotify_group *group,
>>        */
>>       /* it's possible this event was an overflow event.  in that case dentry and mnt
>>        * are NULL;  That's fine, just don't call dentry open */
>> -     if (event->path.dentry && event->path.mnt)
>> -             new_file = dentry_open(&event->path,
>> -                                    group->fanotify_data.f_flags | FMODE_NONOTIFY,
>> -                                    current_cred());
>> -     else
>> +     if (event->path.dentry && event->path.mnt) {
>> +             /* check permissions before granting access to file */
>> +             mask = MAY_READ;
>> +             if (group->fanotify_data.f_flags & (O_RDWR | O_WRONLY))
>> +                     mask |= MAY_WRITE;
>> +             ret = inode_permission(event->path.dentry->d_inode, mask);
>> +             if (ret)
>> +                     new_file = ERR_PTR(ret);
>> +             else
>> +                     new_file = dentry_open(&event->path,
>> +                                            group->fanotify_data.f_flags | FMODE_NONOTIFY,
>> +                                            current_cred());
>> +     } else
>>               new_file = ERR_PTR(-EOVERFLOW);
>>       if (IS_ERR(new_file)) {
>>               /*
>> --
>> 1.9.1
>>
> --
> Jan Kara <jack@suse.cz>
> SUSE Labs, CR



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

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

[Michael Kerrisk (man-pages)]

On Tue, Apr 22, 2014 at 3:50 PM, Michael Kerrisk (man-pages)
<mtk.manpages@gmail.com> wrote:
> On Tue, Apr 22, 2014 at 3:40 PM, Jan Kara <jack@suse.cz> 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...
>
> If I understand correctly, passing to an unprivileged process is the

sorry: s/is the/is not the/

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

[Jan Kara]

On Tue 22-04-14 15:50:26, Michael Kerrisk (man-pages) wrote:
> On Tue, Apr 22, 2014 at 3:40 PM, Jan Kara <jack@suse.cz> 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...
> 
> If I understand correctly, passing to an unprivileged process is the
> point. The point is I think that supposedly one only needs to
> CAP_SYS_ADMIN to use fanotify. However, once you have that capability,
> then you implicitly get the effect of CAP_DAC_READ_SEARCH and
> CAP_DAC_OVERRIDE as well.
  Ah, OK. Thanks for explanation. Then I'm OK with the patch. So feel free
to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> >>  fs/notify/fanotify/fanotify_user.c | 20 +++++++++++++++-----
> >>  1 file changed, 15 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> >> index 4e565c8..5d22a20 100644
> >> --- a/fs/notify/fanotify/fanotify_user.c
> >> +++ b/fs/notify/fanotify/fanotify_user.c
> >> @@ -62,6 +62,8 @@ static int create_fd(struct fsnotify_group *group,
> >>  {
> >>       int client_fd;
> >>       struct file *new_file;
> >> +     int mask;
> >> +     int ret;
> >>
> >>       pr_debug("%s: group=%p event=%p\n", __func__, group, event);
> >>
> >> @@ -75,11 +77,19 @@ static int create_fd(struct fsnotify_group *group,
> >>        */
> >>       /* it's possible this event was an overflow event.  in that case dentry and mnt
> >>        * are NULL;  That's fine, just don't call dentry open */
> >> -     if (event->path.dentry && event->path.mnt)
> >> -             new_file = dentry_open(&event->path,
> >> -                                    group->fanotify_data.f_flags | FMODE_NONOTIFY,
> >> -                                    current_cred());
> >> -     else
> >> +     if (event->path.dentry && event->path.mnt) {
> >> +             /* check permissions before granting access to file */
> >> +             mask = MAY_READ;
> >> +             if (group->fanotify_data.f_flags & (O_RDWR | O_WRONLY))
> >> +                     mask |= MAY_WRITE;
> >> +             ret = inode_permission(event->path.dentry->d_inode, mask);
> >> +             if (ret)
> >> +                     new_file = ERR_PTR(ret);
> >> +             else
> >> +                     new_file = dentry_open(&event->path,
> >> +                                            group->fanotify_data.f_flags | FMODE_NONOTIFY,
> >> +                                            current_cred());
> >> +     } else
> >>               new_file = ERR_PTR(-EOVERFLOW);
> >>       if (IS_ERR(new_file)) {
> >>               /*
> >> --
> >> 1.9.1
> >>
> > --
> > Jan Kara <jack@suse.cz>
> > SUSE Labs, CR
> 
> 
> 
> -- 
> Michael Kerrisk
> Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
> Linux/UNIX System Programming Training: http://man7.org/training/
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

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

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

[Jan Kara]

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@suse.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@suse.cz>
SUSE Labs, CR

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

[Jan Kara]

On Tue 22-04-14 16:07:47, Jan Kara wrote:
> On Tue 22-04-14 15:50:26, Michael Kerrisk (man-pages) wrote:
> > On Tue, Apr 22, 2014 at 3:40 PM, Jan Kara <jack@suse.cz> 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...
> > 
> > If I understand correctly, passing to an unprivileged process is the
> > point. The point is I think that supposedly one only needs to
> > CAP_SYS_ADMIN to use fanotify. However, once you have that capability,
> > then you implicitly get the effect of CAP_DAC_READ_SEARCH and
> > CAP_DAC_OVERRIDE as well.
>   Ah, OK. Thanks for explanation. Then I'm OK with the patch. So feel free
> to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
  Hum, when digging more around this code, I've found out that
fanotify_mark() checks whether it has a read permission to a watched file
when creating the mark (in fanotify_find_path()). So I don't think it's
really worth it to recheck the permissions when creating a file descriptor
for the event. Sure it may be somewhat surprising that read fd is created
after a process doesn't have access to the file anymore but OTOH it is
similar to a situation where the process has opened the file long time ago.

								Honza

> > >>  fs/notify/fanotify/fanotify_user.c | 20 +++++++++++++++-----
> > >>  1 file changed, 15 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > >> index 4e565c8..5d22a20 100644
> > >> --- a/fs/notify/fanotify/fanotify_user.c
> > >> +++ b/fs/notify/fanotify/fanotify_user.c
> > >> @@ -62,6 +62,8 @@ static int create_fd(struct fsnotify_group *group,
> > >>  {
> > >>       int client_fd;
> > >>       struct file *new_file;
> > >> +     int mask;
> > >> +     int ret;
> > >>
> > >>       pr_debug("%s: group=%p event=%p\n", __func__, group, event);
> > >>
> > >> @@ -75,11 +77,19 @@ static int create_fd(struct fsnotify_group *group,
> > >>        */
> > >>       /* it's possible this event was an overflow event.  in that case dentry and mnt
> > >>        * are NULL;  That's fine, just don't call dentry open */
> > >> -     if (event->path.dentry && event->path.mnt)
> > >> -             new_file = dentry_open(&event->path,
> > >> -                                    group->fanotify_data.f_flags | FMODE_NONOTIFY,
> > >> -                                    current_cred());
> > >> -     else
> > >> +     if (event->path.dentry && event->path.mnt) {
> > >> +             /* check permissions before granting access to file */
> > >> +             mask = MAY_READ;
> > >> +             if (group->fanotify_data.f_flags & (O_RDWR | O_WRONLY))
> > >> +                     mask |= MAY_WRITE;
> > >> +             ret = inode_permission(event->path.dentry->d_inode, mask);
> > >> +             if (ret)
> > >> +                     new_file = ERR_PTR(ret);
> > >> +             else
> > >> +                     new_file = dentry_open(&event->path,
> > >> +                                            group->fanotify_data.f_flags | FMODE_NONOTIFY,
> > >> +                                            current_cred());
> > >> +     } else
> > >>               new_file = ERR_PTR(-EOVERFLOW);
> > >>       if (IS_ERR(new_file)) {
> > >>               /*
> > >> --
> > >> 1.9.1
> > >>
> > > --
> > > Jan Kara <jack@suse.cz>
> > > SUSE Labs, CR
> > 
> > 
> > 
> > -- 
> > Michael Kerrisk
> > Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
> > Linux/UNIX System Programming Training: http://man7.org/training/
> -- 
> Jan Kara <jack@suse.cz>
> SUSE Labs, CR
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

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

[Heinrich Schuchardt]

On 24.04.2014 11:04, Jan Kara wrote:
> On Tue 22-04-14 16:07:47, Jan Kara wrote:
>> On Tue 22-04-14 15:50:26, Michael Kerrisk (man-pages) wrote:
>>> On Tue, Apr 22, 2014 at 3:40 PM, Jan Kara <jack@suse.cz> 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...
>>>
>>> If I understand correctly, passing to an unprivileged process is the
>>> point. The point is I think that supposedly one only needs to
>>> CAP_SYS_ADMIN to use fanotify. However, once you have that capability,
>>> then you implicitly get the effect of CAP_DAC_READ_SEARCH and
>>> CAP_DAC_OVERRIDE as well.
>>    Ah, OK. Thanks for explanation. Then I'm OK with the patch. So feel free
>> to add:
>>
>> Reviewed-by: Jan Kara <jack@suse.cz>
>    Hum, when digging more around this code, I've found out that
> fanotify_mark() checks whether it has a read permission to a watched file
> when creating the mark (in fanotify_find_path()). So I don't think it's
> really worth it to recheck the permissions when creating a file .gnupg/secring.gpgdescriptor
> for the event. Sure it may be somewhat surprising that read fd is created
> after a process doesn't have access to the file anymore but OTOH it is
> similar to a situation where the process has opened the file long time ago.
>

fanotify_mark checks for the read authorization for the marked object,
not for the object for which the event occurs.

This means a listener may have read authorization for /home and mark 
this mount.
Afterwards, while you sign a git tag, it will receive a FAN_OPEN event
and use the file descriptor supplied in the event to overwrite your
/home/jankara/.gnupg/secring.pgp
though the file is chmod 600 and the listener is neither root nor you.

>
>>>>>   fs/notify/fanotify/fanotify_user.c | 20 +++++++++++++++-----
>>>>>   1 file changed, 15 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
>>>>> index 4e565c8..5d22a20 100644
>>>>> --- a/fs/notify/fanotify/fanotify_user.c
>>>>> +++ b/fs/notify/fanotify/fanotify_user.c
>>>>> @@ -62,6 +62,8 @@ static int create_fd(struct fsnotify_group *group,
>>>>>   {
>>>>>        int client_fd;
>>>>>        struct file *new_file;
>>>>> +     int mask;
>>>>> +     int ret;
>>>>>
>>>>>        pr_debug("%s: group=%p event=%p\n", __func__, group, event);
>>>>>
>>>>> @@ -75,11 +77,19 @@ static int create_fd(struct fsnotify_group *group,
>>>>>         */
>>>>>        /* it's possible this event was an overflow event.  in that case dentry and mnt
>>>>>         * are NULL;  That's fine, just don't call dentry open */
>>>>> -     if (event->path.dentry && event->path.mnt)
>>>>> -             new_file = dentry_open(&event->path,
>>>>> -                                    group->fanotify_data.f_flags | FMODE_NONOTIFY,
>>>>> -                                    current_cred());
>>>>> -     else
>>>>> +     if (event->path.dentry && event->path.mnt) {
>>>>> +             /* check permissions before granting access to file */
>>>>> +             mask = MAY_READ;
>>>>> +             if (group->fanotify_data.f_flags & (O_RDWR | O_WRONLY))
>>>>> +                     mask |= MAY_WRITE;
>>>>> +             ret = inode_permission(event->path.dentry->d_inode, mask);
>>>>> +             if (ret)
>>>>> +                     new_file = ERR_PTR(ret);
>>>>> +             else
>>>>> +                     new_file = dentry_open(&event->path,
>>>>> +                                            group->fanotify_data.f_flags | FMODE_NONOTIFY,
>>>>> +                                            current_cred());
>>>>> +     } else
>>>>>                new_file = ERR_PTR(-EOVERFLOW);
>>>>>        if (IS_ERR(new_file)) {
>>>>>                /*
>>>>> --
>>>>> 1.9.1
>>>>>
>>>> --
>>>> Jan Kara <jack@suse.cz>
>>>> SUSE Labs, CR
>>>
>>>
>>>
>>> --
>>> Michael Kerrisk
>>> Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
>>> Linux/UNIX System Programming Training: http://man7.org/training/
>> --
>> Jan Kara <jack@suse.cz>
>> SUSE Labs, CR

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

[Michael Kerrisk (man-pages)]

On Thu, Apr 24, 2014 at 3:54 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 24.04.2014 11:04, Jan Kara wrote:
>>
>> On Tue 22-04-14 16:07:47, Jan Kara wrote:
>>>
>>> On Tue 22-04-14 15:50:26, Michael Kerrisk (man-pages) wrote:
>>>>
>>>> On Tue, Apr 22, 2014 at 3:40 PM, Jan Kara <jack@suse.cz> 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...
>>>>
>>>>
>>>> If I understand correctly, passing to an unprivileged process is the
>>>> point. The point is I think that supposedly one only needs to
>>>> CAP_SYS_ADMIN to use fanotify. However, once you have that capability,
>>>> then you implicitly get the effect of CAP_DAC_READ_SEARCH and
>>>> CAP_DAC_OVERRIDE as well.
>>>
>>>    Ah, OK. Thanks for explanation. Then I'm OK with the patch. So feel
>>> free
>>> to add:
>>>
>>> Reviewed-by: Jan Kara <jack@suse.cz>
>>
>>    Hum, when digging more around this code, I've found out that
>> fanotify_mark() checks whether it has a read permission to a watched file
>> when creating the mark (in fanotify_find_path()). So I don't think it's
>> really worth it to recheck the permissions when creating a file
>> .gnupg/secring.gpgdescriptor
>>
>> for the event. Sure it may be somewhat surprising that read fd is created
>> after a process doesn't have access to the file anymore but OTOH it is
>> similar to a situation where the process has opened the file long time
>> ago.
>>
>
> fanotify_mark checks for the read authorization for the marked object,
> not for the object for which the event occurs.
>
> This means a listener may have read authorization for /home and mark this
> mount.
> Afterwards, while you sign a git tag, it will receive a FAN_OPEN event
> and use the file descriptor supplied in the event to overwrite your
> /home/jankara/.gnupg/secring.pgp

Sweet!



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

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

[Jan Kara]

On Thu 24-04-14 15:54:06, Heinrich Schuchardt wrote:
> On 24.04.2014 11:04, Jan Kara wrote:
> >On Tue 22-04-14 16:07:47, Jan Kara wrote:
> >>On Tue 22-04-14 15:50:26, Michael Kerrisk (man-pages) wrote:
> >>>On Tue, Apr 22, 2014 at 3:40 PM, Jan Kara <jack@suse.cz> 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...
> >>>
> >>>If I understand correctly, passing to an unprivileged process is the
> >>>point. The point is I think that supposedly one only needs to
> >>>CAP_SYS_ADMIN to use fanotify. However, once you have that capability,
> >>>then you implicitly get the effect of CAP_DAC_READ_SEARCH and
> >>>CAP_DAC_OVERRIDE as well.
> >>   Ah, OK. Thanks for explanation. Then I'm OK with the patch. So feel free
> >>to add:
> >>
> >>Reviewed-by: Jan Kara <jack@suse.cz>
> >   Hum, when digging more around this code, I've found out that
> >fanotify_mark() checks whether it has a read permission to a watched file
> >when creating the mark (in fanotify_find_path()). So I don't think it's
> >really worth it to recheck the permissions when creating a file .gnupg/secring.gpgdescriptor
> >for the event. Sure it may be somewhat surprising that read fd is created
> >after a process doesn't have access to the file anymore but OTOH it is
> >similar to a situation where the process has opened the file long time ago.
> >
> 
> fanotify_mark checks for the read authorization for the marked object,
> not for the object for which the event occurs.
> 
> This means a listener may have read authorization for /home and mark
> this mount.
> Afterwards, while you sign a git tag, it will receive a FAN_OPEN event
> and use the file descriptor supplied in the event to overwrite your
> /home/jankara/.gnupg/secring.pgp
> though the file is chmod 600 and the listener is neither root nor you.
  Ah, right. Thanks for explaining this to me. I'm not really too excited
about this as a security issue because once the process has CAP_SYS_ADMIN
it basically owns the machine (it is sadly one of those capabilities which
is too broad) but I agree checking permissions when creating the fd is
reasonable.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

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

[Michael Kerrisk (man-pages)]

>> This means a listener may have read authorization for /home and mark
>> this mount.
>> Afterwards, while you sign a git tag, it will receive a FAN_OPEN event
>> and use the file descriptor supplied in the event to overwrite your
>> /home/jankara/.gnupg/secring.pgp
>> though the file is chmod 600 and the listener is neither root nor you.
>   Ah, right. Thanks for explaining this to me. I'm not really too excited
> about this as a security issue because once the process has CAP_SYS_ADMIN
> it basically owns the machine (it is sadly one of those capabilities which
> is too broad)

Yup. https://lwn.net/Articles/486306/

> but I agree checking permissions when creating the fd is
> reasonable.
>
>                                                                 Honza
> --
> Jan Kara <jack@suse.cz>
> SUSE Labs, CR



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/