Re: [PATCH 1/1] audit: Record fanotify access control decisions

On Wed 06-09-17 10:35:32, Steve Grubb wrote:

On Wednesday, September 6, 2017 5:18:22 AM EDT Jan Kara wrote:

quoted

On Tue 05-09-17 14:32:07, Steve Grubb wrote:

quoted

The fanotify interface allows user space daemons to make access

 control decisions. Under common criteria requirements, we need to
 optionally record decisions based on policy. This patch adds a bit mask,
 FAN_AUDIT, that a user space daemon can 'or' into the response decision
 which will tell the kernel that it made a decision and record it.

[Since this is API change, added linux-api to CC, also added Amir since he
works with fanotify]

Hum, probably I'm missing something here but why an application responding
to fanotify event should need to know about audit?

Common Criteria rules are that if anything can prevent a flow of information, 
then you must be able to selectively audit. Selectively audit means under 
admin direction only when they want it. Meaning they may want some denials or 
approvals but not other ones.

OK, thanks for explanation.

quoted

Or is it that for CCrequirements you have to implement some deamon which
will arbitrate access using fanotify and you need to have decisions logged
using kernel audit interface?

Yes. And even virus scanners would probably want to allow admins to pick when 
they record something being blocked.

But then if I understand it correctly, you would need to patch each and
every user of fanotify permission events to know about FAN_AUDIT to meet
those CC requirements? That seems pretty hard to do to me and even it done,
it sounds like quite a bit of duplication?

So wouldn't it be better design to pipe all fanotify access decisions to
audit subsystem which would based on policy decide whether the event should
be logged or not? I assume something like this must be working when access
is denied e.g. because of file permissions? And again I appologize for my
total ignorance of how audit works...

quoted

quoted

+void __audit_fanotify(unsigned int response)
+{
+	audit_log(current->audit_context, GFP_ATOMIC,
+		AUDIT_FANOTIFY,	"resp=%u", response);
+}

Heh, GFP_ATOMIC looks pretty crude and it can fail more often than you'd
think. In fact fanotify uses GFP_KERNEL for allocation of new event and I
don't see a reason why __audit_fanotify() couldn't use the same.

Sure, that's easy enough to change. Is that the only change needed? Is
the choice of 0x80 for the FAN_AUDIT bit OK? I wanted to leave some room
for other numbers if there ever was a new use. The number is arbitrary
and could be anything.

Yeah, 0x80 for FAN_AUDIT is OK. Also Amir's comment about fanotify_init()
flag should be reflected. But I'd really like to understand why design like
this was chosen before merging the change.

								Honza
-- 
Jan Kara [off-list ref]
SUSE Labs, CR