| Message ID | 3663877.NZSPRKlUQW@x2 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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.
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
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);
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 */
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(-)