Re: [PATCH v2 1/1] fanotify: introduce new event flag FAN_EXEC

[Matthew Bobrowski <mbobrowski@mbobrowski.org>]

Hi Amir,

On 27/9/18 11:57 pm, Amir Goldstein wrote:
> [cc: linux-api]
> 
> On Thu, Sep 27, 2018 at 4:05 PM Matthew Bobrowski
> <mbobrowski@mbobrowski.org> wrote:
>>
>> This is a reduced version of a patch that I originally submitted a while ago.
>>
>> In short, the fanotify API currently does not provide any means for user space
>> programs to receive events specifically when a file has been opened with the
>> intent to be executed. The FAN_EXEC flag will be set within the event mask when
>> a object has been opened with one of the open flags being __FMODE_EXEC.
>>
>> Linux is used as an Operating System in some products, with an environment that
>> can be certified under the Common Criteria Operating System Protection Profile
>> (OSPP). This is a formal threat model for a class of technology. It requires
>> specific countermeasures to mitigate threats. It requires documentation to
>> explain how a product implements these countermeasures. It requires proof via a
>> test suite to demonstrate that the requirements are met, observed and checked by
>> an independent qualified third party. The latest set of requirements for OSPP
>> v4.2 can be found here:
>>
>> https://www.niap-ccevs.org/Profile/Info.cfm?PPID=424&id=424
>>
>> If you look on page 58, you will see the following requirement:
>>
>> FPT_SRP_EXT.1 Software Restriction Policies   FPT_SRP_EXT.1.1
>>
>> The OS shall restrict execution to only programs which match an administrator
>> specified [selection:
>>         file path,
>>         file digital signature,
>>         version,
>>         hash,
>>         [assignment: other characteristics]
>> ]
>>
>> This patch is to help aid in meeting this requirement.
>>
>> Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>
>>
>> ---
>>
>> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
>> index 94b52157bf8d..295549479be7 100644
>> --- a/fs/notify/fanotify/fanotify.c
>> +++ b/fs/notify/fanotify/fanotify.c
>> @@ -204,6 +204,7 @@ static int fanotify_handle_event(struct fsnotify_group *group,
>>         BUILD_BUG_ON(FAN_OPEN_PERM != FS_OPEN_PERM);
>>         BUILD_BUG_ON(FAN_ACCESS_PERM != FS_ACCESS_PERM);
>>         BUILD_BUG_ON(FAN_ONDIR != FS_ISDIR);
>> +       BUILD_BUG_ON(FAN_EXEC != FS_EXEC);
>>
>>         if (!fanotify_should_send_event(iter_info, mask, data, data_type))
>>                 return 0;
>> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
>> index ababdbfab537..e22324f4642c 100644
>> --- a/fs/notify/fsnotify.c
>> +++ b/fs/notify/fsnotify.c
>> @@ -386,7 +386,7 @@ static __init int fsnotify_init(void)
>>  {
>>         int ret;
>>
>> -       BUG_ON(hweight32(ALL_FSNOTIFY_EVENTS) != 23);
>> +       BUG_ON(hweight32(ALL_FSNOTIFY_EVENTS) != 24);
>>
>>         ret = init_srcu_struct(&fsnotify_mark_srcu);
>>         if (ret)
>> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
>> index fd1ce10553bf..aad174c81322 100644
>> --- a/include/linux/fsnotify.h
>> +++ b/include/linux/fsnotify.h
>> @@ -216,6 +216,9 @@ static inline void fsnotify_open(struct file *file)
>>         if (S_ISDIR(inode->i_mode))
>>                 mask |= FS_ISDIR;
>>
>> +       if (file->f_flags & __FMODE_EXEC)
>> +               mask |= FS_EXEC;
>> +
> 
> I think that may be breaking existing programs that do not expect to see
> this bit in the event mask (i.e. if they only requested to see FAN_OPEN).

A very good point and is definitely something that did cross my mind while
writing this patch.

> A possible mitigation would be to set a group flag FAN_ENABLE_EXEC
> on the first time that user requests the FAN_EXEC event and then
> compute the FAN_ALL_OUTGOING_EVENTS at runtime based on that
> group flag (see example with FAN_ONDIR in my dev branch [1]).
> Unlike my example, I don't think you have to expose the init flag
> FAN_ENABLE_EXEC to users, because it can be set implicitly on the
> first FAN_EXEC mark request.

Ah yes, I think this is quite elegant and could actually work well. Let me
review your suggestions and write an alternative patch using this
particular approach.

> 
>>         fsnotify_parent(path, NULL, mask);
>>         fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
>>  }
>> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
>> index b8f4182f42f1..edcdf2fd2328 100644
>> --- a/include/linux/fsnotify_backend.h
>> +++ b/include/linux/fsnotify_backend.h
>> @@ -38,6 +38,7 @@
>>  #define FS_DELETE              0x00000200      /* Subfile was deleted */
>>  #define FS_DELETE_SELF         0x00000400      /* Self was deleted */
>>  #define FS_MOVE_SELF           0x00000800      /* Self was moved */
>> +#define FS_EXEC                0x00001000      /* File was executed */
>>
>>  #define FS_UNMOUNT             0x00002000      /* inode on umount fs */
>>  #define FS_Q_OVERFLOW          0x00004000      /* Event queued overflowed */
>> @@ -62,7 +63,8 @@
>>  #define FS_EVENTS_POSS_ON_CHILD   (FS_ACCESS | FS_MODIFY | FS_ATTRIB |\
>>                                    FS_CLOSE_WRITE | FS_CLOSE_NOWRITE | FS_OPEN |\
>>                                    FS_MOVED_FROM | FS_MOVED_TO | FS_CREATE |\
>> -                                  FS_DELETE | FS_OPEN_PERM | FS_ACCESS_PERM)
>> +                                  FS_DELETE | FS_OPEN_PERM | FS_ACCESS_PERM |\
>> +                                  FS_EXEC)
>>
>>  #define FS_MOVE                        (FS_MOVED_FROM | FS_MOVED_TO)
>>
>> @@ -75,7 +77,8 @@
>>                              FS_UNMOUNT | FS_Q_OVERFLOW | FS_IN_IGNORED | \
>>                              FS_OPEN_PERM | FS_ACCESS_PERM | FS_EXCL_UNLINK | \
>>                              FS_ISDIR | FS_IN_ONESHOT | FS_DN_RENAME | \
>> -                            FS_DN_MULTISHOT | FS_EVENT_ON_CHILD)
>> +                            FS_DN_MULTISHOT | FS_EVENT_ON_CHILD |\
>> +                            FS_EXEC)
>>
>>  struct fsnotify_group;
>>  struct fsnotify_event;
>> diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
>> index 74247917de04..a850670333fd 100644
>> --- a/include/uapi/linux/fanotify.h
>> +++ b/include/uapi/linux/fanotify.h
>> @@ -10,7 +10,7 @@
>>  #define FAN_CLOSE_WRITE                0x00000008      /* Writtable file closed */
>>  #define FAN_CLOSE_NOWRITE      0x00000010      /* Unwrittable file closed */
>>  #define FAN_OPEN               0x00000020      /* File was opened */
>> -
>> +#define FAN_EXEC               0x00001000      /* File was executed */
>>  #define FAN_Q_OVERFLOW         0x00004000      /* Event queued overflowed */
>>
>>  #define FAN_OPEN_PERM          0x00010000      /* File open in perm check */
>> @@ -69,7 +69,8 @@
>>  #define FAN_ALL_EVENTS (FAN_ACCESS |\
>>                         FAN_MODIFY |\
>>                         FAN_CLOSE |\
>> -                       FAN_OPEN)
>> +                       FAN_OPEN |\
>> +                       FAN_EXEC)
>>
> 
> It *might* be better not to change the uapi exposed value of
> FAN_ALL_EVENTS. I think in retrospect that exposing it was a mistake,
> because no user program should be using this value, although for
> brevity, some programs might be using this value and if said program
> is recompiled with new headers, said program won't run on old kernels.
> An alternative approach is to use new constants for internal kernel use,
> and leave the old legacy uapi constants frozen in the past. see suggested
> implementation on my dev branch [2].

Yes, agreed.

> 
> Thanks,
> Amir.
> 
> [1] https://github.com/amir73il/linux/commit/6fca47dfb793d1c00935e2f91d47ba209f61c4e8
> [2] https://github.com/amir73il/linux/commit/24e970b4756ef9435cdbc8f35c29ff5ab65cfa81
> 

-- 

Thanks,
Matthew Bobrowski