From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:55416 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726748AbeJCWeC (ORCPT ); Wed, 3 Oct 2018 18:34:02 -0400 Date: Wed, 3 Oct 2018 17:45:03 +0200 From: Jan Kara To: Matthew Bobrowski Cc: Jan Kara , Amir Goldstein , Steve Grubb , linux-fsdevel Subject: Re: [PATCH v2 1/1] fanotify: introduce new event flag FAN_EXEC Message-ID: <20181003154503.GE24030@quack2.suse.cz> References: <20181001105803.GG3913@quack2.suse.cz> <20181002092448.GA4135@quack2.suse.cz> <0bf5f2ed-3860-8b6b-0256-1d9c9edb699b@mbobrowski.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <0bf5f2ed-3860-8b6b-0256-1d9c9edb699b@mbobrowski.org> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue 02-10-18 21:50:24, Matthew Bobrowski wrote: > On 02/10/18 19:24, Jan Kara wrote: > > On Mon 01-10-18 17:01:23, Amir Goldstein wrote: > >> On Mon, Oct 1, 2018 at 1:58 PM Jan Kara wrote: > >>> > >>> On Thu 27-09-18 23:05:14, 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 > >>> > >>> I agree with Amir's points wrt API so I won't repeat those. But I have one > >>> more API question: > >>> > >>> You implement FS_EXEC as a flag that can get set for certain FAN_OPEN > >>> events. That is a new API concept for fanotify. So far you can only request > >>> event of a certain type and then you get the same flag back when the event > >>> happens. There is also a case of FAN_ONDIR where you can restrict set of > >>> events only to events on a particular inode type but that's again > >>> different. Hence my question: Is there a good reason why we don't create > >>> FAN_OPEN_EXEC event that would trigger only on executable opens? > >>> > >>> If someone is interested only in executable opens, he'd have less events to > >>> care about. OTOH additional FS_EXEC flag is probably more flexible (e.g. > >>> you can easily implement equivalent of FAN_OPEN_NOEXEC in userspace if you > >>> wished). Just the inconsistency of the FS_EXEC and e.g. how we handle > >>> FAN_CLOSE_NOWRITE & FAN_CLOSE_WRITE is bothering me... > >>> > >> > >> I understand why the inconsitency is bothering you, but IMO it is too late > >> to change that. By trying to be more consistent in the *implementation* of > >> the flags, we will end up confusing users instead of making their life easy > >> by sticking to FAN_OPEN sematics they are used to. > >> > >> IMO we need to report FAN_OPEN for every open like we do now. > >> and additionally report FAN_OPEN_EXEC for open for exec. > > > > Fully agreed. > > > >> Then user can implement FAN_OPEN_NOEXEC by setting ignore mask > >> with FAN_OPEN_EXEC. > > > > Good point. > > > >> You can make the analogy to the compound event FAN_CLOSE. > >> If user sets a mask to the compound event FAN_CLOSE and sets ignore > >> mask with FAN_CLOSE_WRITE, then user effectively implemented > >> FAN_CLOSE_NOWRITE with similar semantics to implementation of > >> FAN_OPEN_NOEXEC. > >> > >> Similarly, if user requests FAN_OPEN|FAN_CLOSE and then checks > >> (event->mask & FAN_OPEN) or (event->mask & FAN_CLOSE) it has > >> similar meaning. Testing (event->mask & FAN_OPEN_EXEC) and > >> (event->mask & FAN_CLOSE_WRITE) in this case is similarly informative. > >> > >> Honestly, I can't think of an application interested only in > >> FAN_CLOSE_NOWRITE nor only in FAN_OPEN_NOEXEC, but the > >> functionality is available for both. The fact that user *can* implement the > >> former without ignore mask and cannot implement to latter without > >> ignore mask is IMO the neccesary evil we need to carry for historic API > >> decisions. > > > > So it seems we are in full agreement that we don't want "optional event > > flags" as Matthew implemented it currently and rather want new event type > > FAN_OPEN_EXEC? > > So, what we're proposing here is to actually *not* go with amending the > additional _bonus_ flag to existing event(s) and to explicitly define a new > event type i.e. FAN_OPEN_EXEC? Yes. > I can't really see why we wouldn't go with the solution that I've originally > proposed. It basically provides the same capabilities as what FAN_OPEN_EXEC > would, but at the same time also offers the user the additional flexibility, > which is nice? Well, as Amir explained in his reply both your variant and new FAN_OPEN_EXEC have the same level of flexibility. > I agree that using FAN_OPEN_EXEC may possibly be cleaner from a user > perspective, but just not entirely sure at this point whether it would be > adding some additional unnecessary complexity throughout the API. I think new FAN_OPEN_EXEC event would be actually simpler for the API. There's no need for new fanotify_init() flag with that, explanations to the user in the manpage how it behaves etc. It is just another event. The fact that it is a subtype of FAN_OPEN may result in a need to do some masking in fanotify_handle_event() but that's easy to do. > Either way though, looking for guidance and feedback from yourself and Amir. Sure, let us sort it out with Amir first and then we'll give you some guidance where to go further :) Honza -- Jan Kara SUSE Labs, CR