[PATCH 1/1] Fanotify: Introduce a permissive mode

[PATCH 1/1] Fanotify: Introduce a permissive mode

[Steve Grubb]

Hello,

The fanotify interface can be used as an access control subsystem. If
for some reason the policy is bad, there is potentially no good way to
recover the system. This patch introduces a new command line variable,
fanotify_enforce, to allow overriding the access decision from user
space. The initialization status is recorded as an audit event so that
there is a record of being in permissive mode for the security officer.

Signed-off-by: sgrubb <sgrubb@redhat.com>
---
 Documentation/admin-guide/kernel-parameters.txt |  7 +++++
 fs/notify/fanotify/fanotify.c                   | 42 +++++++++++++++++++++++--
 include/uapi/linux/audit.h                      |  1 +
 3 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 7737ab5..84c0e78 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1141,6 +1141,13 @@
 			Format: <interval>,<probability>,<space>,<times>
 			See also Documentation/fault-injection/.
 
+	fanotify_enforce=[FANOTIFY] Enable or disable enforcement of policy
+			decisions at boot time.
+			Format: { "0" | "1" }
+			0 -- disable enforcement.
+			1 -- enable enforcement.
+			Default value is 1 (enforcing).
+
 	floppy=		[HW]
 			See Documentation/blockdev/floppy.txt.
 
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 2fa99ae..cab5c2b 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -9,9 +9,43 @@
 #include <linux/sched/user.h>
 #include <linux/types.h>
 #include <linux/wait.h>
+#include <linux/audit.h>
 
 #include "fanotify.h"
 
+
+#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
+/*
+ * This variable determines if the decisions made by user space listener
+ * will be enforced or overridden for system recovery
+ */
+static unsigned int enforcing_mode = 1;
+
+
+/* Record status of the fanotify sunsystem */
+static int __init fanotify_init(void)
+{
+	audit_log(NULL, GFP_KERNEL, AUDIT_FANOTIFY_STATUS,
+		"state=initialized fanotify_enforce=%u res=1",
+		enforcing_mode);
+	return 0;
+}
+late_initcall(fanotify_init);
+
+static int __init set_fanotify_enforce(char *str)
+{
+	long val;
+
+	if (kstrtol(str, 0, &val) == 0) {
+		enforcing_mode = !!val;
+		pr_info("fanotify initialized with fanotify_enforce=%u\n",
+			enforcing_mode);
+	}
+	return 1;
+}
+__setup("fanotify_enforce=", set_fanotify_enforce);
+#endif
+
 static bool should_merge(struct fsnotify_event *old_fsn,
 			 struct fsnotify_event *new_fsn)
 {
@@ -88,9 +122,12 @@ static int fanotify_get_response(struct fsnotify_group *group,
 	}
 	event->response = 0;
 
-	pr_debug("%s: group=%p event=%p about to return ret=%d\n", __func__,
-		 group, event, ret);
-	
+	pr_debug("%s: group=%p event=%p about to return ret=%d enforce=%u\n",
+		 __func__, group, event, ret, enforcing_mode);
+
+	if (unlikely(!enforcing_mode))
+		ret = 0;
+
 	return ret;
 }
 #endif
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 0714a66..9560627 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -112,6 +112,7 @@
 #define AUDIT_FEATURE_CHANGE	1328	/* audit log listing feature changes */
 #define AUDIT_REPLACE		1329	/* Replace auditd if this packet unanswerd */
 #define AUDIT_KERN_MODULE	1330	/* Kernel Module events */
+#define AUDIT_FANOTIFY_STATUS	1331	/* Fanotify enforcing status */
 
 #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
 #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */
-- 
2.9.4

Re: [PATCH 1/1] Fanotify: Introduce a permissive mode

[Amir Goldstein]

On Mon, Aug 14, 2017 at 5:04 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> Hello,
>
> The fanotify interface can be used as an access control subsystem. If
> for some reason the policy is bad, there is potentially no good way to
> recover the system. This patch introduces a new command line variable,
> fanotify_enforce, to allow overriding the access decision from user
> space. The initialization status is recorded as an audit event so that
> there is a record of being in permissive mode for the security officer.

:-/ overriding the security access decision sounds like a bad practice
*if* at all this method is acceptable overriding access decision should
probably be accompanied with pr_warn_ratelimited and a big warning
for fanotify_init with FAN_CLASS_{,PRE_}CONTENT priority.

If the proposed kernel param is acceptable by others, I would prefer
that it prevents setting up FAN_CLASS_{,PRE_}CONTENT priority
watches, instead of setting them up and ignoring the user daemon response.

B.T.W Jan,

I hope I am not out of line to propose:

--- a/MAINTAINERS
+++ b/MAINTAINERS

 FANOTIFY
-M:     Eric Paris <eparis@redhat.com>
+M:     Jan Kara <jack@suse.com>
+R:     Amir Goldstein <amir73il@gmail.com>
+L:     linux-fsdevel@vger.kernel.org
 S:     Maintained
 F:     fs/notify/fanotify/
 F:     include/linux/fanotify.h

Re: [PATCH 1/1] Fanotify: Introduce a permissive mode

[Jan Kara]

On Tue 15-08-17 12:19:50, Amir Goldstein wrote:
> On Mon, Aug 14, 2017 at 5:04 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > Hello,
> >
> > The fanotify interface can be used as an access control subsystem. If
> > for some reason the policy is bad, there is potentially no good way to
> > recover the system. This patch introduces a new command line variable,
> > fanotify_enforce, to allow overriding the access decision from user
> > space. The initialization status is recorded as an audit event so that
> > there is a record of being in permissive mode for the security officer.
> 
> :-/ overriding the security access decision sounds like a bad practice
> *if* at all this method is acceptable overriding access decision should
> probably be accompanied with pr_warn_ratelimited and a big warning
> for fanotify_init with FAN_CLASS_{,PRE_}CONTENT priority.
> 
> If the proposed kernel param is acceptable by others, I would prefer
> that it prevents setting up FAN_CLASS_{,PRE_}CONTENT priority
> watches, instead of setting them up and ignoring the user daemon response.

Agreed. You need CAP_SYS_ADMIN to be able to set up watches for access
control. If you have applications with CAP_SYS_ADMIN you don't trust, just
don't run them or fix bugs in them. Kernel parameter is not the right way
to fix broken applications with administrative priviledges.

> I hope I am not out of line to propose:
> 
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> 
>  FANOTIFY
> -M:     Eric Paris <eparis@redhat.com>
> +M:     Jan Kara <jack@suse.com>
> +R:     Amir Goldstein <amir73il@gmail.com>
> +L:     linux-fsdevel@vger.kernel.org
>  S:     Maintained
>  F:     fs/notify/fanotify/
>  F:     include/linux/fanotify.h

Yeah, I'll queue it up and the same for inotify & dnotify.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/1] Fanotify: Introduce a permissive mode

[Steve Grubb]

On Tuesday, August 15, 2017 6:19:50 AM EDT Amir Goldstein wrote:
> On Mon, Aug 14, 2017 at 5:04 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > Hello,
> > 
> > The fanotify interface can be used as an access control subsystem. If
> > for some reason the policy is bad, there is potentially no good way to
> > recover the system. This patch introduces a new command line variable,
> > fanotify_enforce, to allow overriding the access decision from user
> > space. The initialization status is recorded as an audit event so that
> > there is a record of being in permissive mode for the security officer.
> :
> :-/ overriding the security access decision sounds like a bad practice

Agreed, but sometimes you have to. If you are tinkering with application 
whitelisting policy and make a mistake, you just locked yourself out of the 
system or it fails booting. So, my suggested uses for this is system recovery 
and policy debugging. Hopefully the first is more clear. Let me explain the 
second one a bit.

To debug policy, I created a soft-permissive mode by asking for watches on 
file access without returning a decision. What I found was that there are 
short lived files that something requests access to and hits the queue to the 
daemon. But by the time the daemon looks at the file, it gets an EBADFD or the 
process requesting it is dead and gone. So, I have no idea what the file was 
or what was asking for it.

So, there is some utility to having the application stopped so that the daemon 
can do its checks but then throw away the answer so that more of the policy 
can be verified.


> *if* at all this method is acceptable overriding access decision should
> probably be accompanied with pr_warn_ratelimited and a big warning
> for fanotify_init with FAN_CLASS_{,PRE_}CONTENT priority.

I was hoping the audit event was a big enough warning. But something for 
dmesg/syslog is easy to add.


> If the proposed kernel param is acceptable by others, I would prefer
> that it prevents setting up FAN_CLASS_{,PRE_}CONTENT priority
> watches, instead of setting them up and ignoring the user daemon response.

Hmm. The access control policy would be targeting the FAN_CLASS_CONTENT, 
though.

-Steve

Re: [PATCH 1/1] Fanotify: Introduce a permissive mode

[Amir Goldstein]

On Tue, Aug 15, 2017 at 4:44 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Tuesday, August 15, 2017 6:19:50 AM EDT Amir Goldstein wrote:
>> On Mon, Aug 14, 2017 at 5:04 PM, Steve Grubb <sgrubb@redhat.com> wrote:
>> > Hello,
>> >
>> > The fanotify interface can be used as an access control subsystem. If
>> > for some reason the policy is bad, there is potentially no good way to
>> > recover the system. This patch introduces a new command line variable,
>> > fanotify_enforce, to allow overriding the access decision from user
>> > space. The initialization status is recorded as an audit event so that
>> > there is a record of being in permissive mode for the security officer.
>> :
>> :-/ overriding the security access decision sounds like a bad practice
>
> Agreed, but sometimes you have to. If you are tinkering with application
> whitelisting policy and make a mistake, you just locked yourself out of the
> system or it fails booting. So, my suggested uses for this is system recovery
> and policy debugging. Hopefully the first is more clear. Let me explain the
> second one a bit.
>

So for first use case, my suggestion to prevent setting up high priority
watches should be sufficient to stop the offending daemon from running. Right?

> To debug policy, I created a soft-permissive mode by asking for watches on
> file access without returning a decision. What I found was that there are
> short lived files that something requests access to and hits the queue to the
> daemon. But by the time the daemon looks at the file, it gets an EBADFD or the
> process requesting it is dead and gone. So, I have no idea what the file was
> or what was asking for it.
>
> So, there is some utility to having the application stopped so that the daemon
> can do its checks but then throw away the answer so that more of the policy
> can be verified.
>
>
>> *if* at all this method is acceptable overriding access decision should
>> probably be accompanied with pr_warn_ratelimited and a big warning
>> for fanotify_init with FAN_CLASS_{,PRE_}CONTENT priority.
>
> I was hoping the audit event was a big enough warning. But something for
> dmesg/syslog is easy to add.
>

No warning is big enough if the change breaks existing apps behavior.
One of the major flaws in your suggestion is that it changes the behavior
globally. I think what you want for the debugging use case is to introduce
a new fanotify_init() flag FAN_PERMISSIVE.
Your daemon could set the new flag to opt-in for the new behavior, which
may depend on kernel parameter, or even on sysfs knob if you like.

>
>> If the proposed kernel param is acceptable by others, I would prefer
>> that it prevents setting up FAN_CLASS_{,PRE_}CONTENT priority
>> watches, instead of setting them up and ignoring the user daemon response.
>
> Hmm. The access control policy would be targeting the FAN_CLASS_CONTENT,
> though.
>

Yeh, well, that's only the solution for the first use case.

Amir.

Re: [PATCH 1/1] Fanotify: Introduce a permissive mode

[Steve Grubb]

On Tuesday, August 15, 2017 11:37:19 AM EDT Amir Goldstein wrote:
> > So, there is some utility to having the application stopped so that the
> > daemon can do its checks but then throw away the answer so that more of
> > the policy can be verified.
> > 
> >> *if* at all this method is acceptable overriding access decision should
> >> probably be accompanied with pr_warn_ratelimited and a big warning
> >> for fanotify_init with FAN_CLASS_{,PRE_}CONTENT priority.
> > 
> > I was hoping the audit event was a big enough warning. But something for
> > dmesg/syslog is easy to add.
> 
> No warning is big enough if the change breaks existing apps behavior.
> One of the major flaws in your suggestion is that it changes the behavior
> globally. I think what you want for the debugging use case is to introduce
> a new fanotify_init() flag FAN_PERMISSIVE.
> Your daemon could set the new flag to opt-in for the new behavior, which
> may depend on kernel parameter, or even on sysfs knob if you like.

Thanks for the discussion. I'm self-NAK'ing this for now.

-Steve

Re: [PATCH 1/1] Fanotify: Introduce a permissive mode

[Paul Moore]

On Mon, Aug 14, 2017 at 11:04 AM, Steve Grubb <sgrubb@redhat.com> wrote:
> Hello,
>
> The fanotify interface can be used as an access control subsystem. If
> for some reason the policy is bad, there is potentially no good way to
> recover the system. This patch introduces a new command line variable,
> fanotify_enforce, to allow overriding the access decision from user
> space. The initialization status is recorded as an audit event so that
> there is a record of being in permissive mode for the security officer.
>
> Signed-off-by: sgrubb <sgrubb@redhat.com>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |  7 +++++
>  fs/notify/fanotify/fanotify.c                   | 42 +++++++++++++++++++++++--
>  include/uapi/linux/audit.h                      |  1 +
>  3 files changed, 47 insertions(+), 3 deletions(-)

...

> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 2fa99ae..cab5c2b 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -9,9 +9,43 @@
>  #include <linux/sched/user.h>
>  #include <linux/types.h>
>  #include <linux/wait.h>
> +#include <linux/audit.h>
>
>  #include "fanotify.h"
>
> +
> +#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
> +/*
> + * This variable determines if the decisions made by user space listener
> + * will be enforced or overridden for system recovery
> + */
> +static unsigned int enforcing_mode = 1;
> +
> +
> +/* Record status of the fanotify sunsystem */
> +static int __init fanotify_init(void)
> +{
> +       audit_log(NULL, GFP_KERNEL, AUDIT_FANOTIFY_STATUS,
> +               "state=initialized fanotify_enforce=%u res=1",
> +               enforcing_mode);

I realized this has already been NAK'd, but on the chance it is
resubmitted with some tweaks I wanted to make a comment that the
"state=initialized" addition to the audit records seems a bit
redundant, the presence of a FANOTIFY_STATUS audit record should
satisfy that requirement.  Further, looking at how AUDIT_MAC_STATUS is
used (this seemed to be the closest analogue), it doesn't display a
similar state=initialized flag, the one exception being when the state
is set to disabled, which is not the case here.

> +       return 0;
> +}
> +late_initcall(fanotify_init);

-- 
paul moore
www.paul-moore.com