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

[Amir Goldstein <amir73il@gmail.com>]

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

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

Thanks,
Amir.

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