From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-yw1-f68.google.com ([209.85.161.68]:46503 "EHLO mail-yw1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726953AbeI0UPh (ORCPT ); Thu, 27 Sep 2018 16:15:37 -0400 MIME-Version: 1.0 References: In-Reply-To: From: Amir Goldstein Date: Thu, 27 Sep 2018 16:57:02 +0300 Message-ID: Subject: Re: [PATCH v2 1/1] fanotify: introduce new event flag FAN_EXEC To: Matthew Bobrowski Cc: Jan Kara , Steve Grubb , linux-fsdevel , linux-api@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: [cc: linux-api] On Thu, Sep 27, 2018 at 4:05 PM Matthew Bobrowski 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 > > --- > > 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