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