| Message ID | 1970763.eSnnJ2BxMO@x2 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 5, 2017 at 2:32 PM, Steve Grubb <sgrubb@redhat.com> wrote: > 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. > > It would be used something like this in user space code: > > response.response = FAN_DENY | FAN_AUDIT; > write(fd, &response, sizeof(struct fanotify_response)); > > When the syscall ends, the audit system will record the decision as a > AUDIT_FANOTIFY auxiliary record to denote that the reason this event > occurred is the result of an access control decision from fanotify > rather than DAC or MAC policy. > > A sample event looks like this: > > type=PATH msg=audit(1504310584.332:290): item=0 name="./evil-ls" > inode=1319561 dev=fc:03 mode=0100755 ouid=1000 ogid=1000 rdev=00:00 > obj=unconfined_u:object_r:user_home_t:s0 nametype=NORMAL > type=CWD msg=audit(1504310584.332:290): cwd="/home/sgrubb" > type=SYSCALL msg=audit(1504310584.332:290): arch=c000003e syscall=2 > success=no exit=-1 a0=32cb3fca90 a1=0 a2=43 a3=8 items=1 ppid=901 > pid=959 auid=1000 uid=1000 gid=1000 euid=1000 suid=1000 > fsuid=1000 egid=1000 sgid=1000 fsgid=1000 tty=pts1 ses=3 comm="bash" > exe="/usr/bin/bash" subj=unconfined_u:unconfined_r:unconfined_t: > s0-s0:c0.c1023 key=(null) > type=FANOTIFY msg=audit(1504310584.332:290): resp=2 > > Signed-off-by: sgrubb <sgrubb@redhat.com> > --- > fs/notify/fanotify/fanotify.c | 8 +++++++- > fs/notify/fanotify/fanotify_user.c | 2 +- > include/linux/audit.h | 7 +++++++ > include/uapi/linux/audit.h | 1 + > include/uapi/linux/fanotify.h | 2 ++ > kernel/auditsc.c | 6 ++++++ > 6 files changed, 24 insertions(+), 2 deletions(-) The audit bits look fine to me, although considering the timing, this really shouldn't be merged anywhere until after the current merge window closes. Acked-by: Paul Moore <paul@paul-moore.com> > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > index 2fa99ae..1968d21 100644 > --- a/fs/notify/fanotify/fanotify.c > +++ b/fs/notify/fanotify/fanotify.c > @@ -9,6 +9,7 @@ > #include <linux/sched/user.h> > #include <linux/types.h> > #include <linux/wait.h> > +#include <linux/audit.h> > > #include "fanotify.h" > > @@ -78,7 +79,7 @@ static int fanotify_get_response(struct fsnotify_group *group, > fsnotify_finish_user_wait(iter_info); > out: > /* userspace responded, convert to something usable */ > - switch (event->response) { > + switch (event->response & ~FAN_AUDIT) { > case FAN_ALLOW: > ret = 0; > break; > @@ -86,6 +87,11 @@ static int fanotify_get_response(struct fsnotify_group *group, > default: > ret = -EPERM; > } > + > + /* Check if the response should be audited */ > + if (event->response & FAN_AUDIT) > + audit_fanotify(event->response & ~FAN_AUDIT); > + > event->response = 0; > > pr_debug("%s: group=%p event=%p about to return ret=%d\n", __func__, > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index 907a481..b983b5c 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -179,7 +179,7 @@ static int process_access_response(struct fsnotify_group *group, > * userspace can send a valid response or we will clean it up after the > * timeout > */ > - switch (response) { > + switch (response & ~FAN_AUDIT) { > case FAN_ALLOW: > case FAN_DENY: > break; > diff --git a/include/linux/audit.h b/include/linux/audit.h > index 2150bdc..bf55732 100644 > --- a/include/linux/audit.h > +++ b/include/linux/audit.h > @@ -360,6 +360,7 @@ extern int __audit_log_bprm_fcaps(struct linux_binprm *bprm, > extern void __audit_log_capset(const struct cred *new, const struct cred *old); > extern void __audit_mmap_fd(int fd, int flags); > extern void __audit_log_kern_module(char *name); > +extern void __audit_fanotify(unsigned int response); > > static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp) > { > @@ -456,6 +457,12 @@ static inline void audit_log_kern_module(char *name) > __audit_log_kern_module(name); > } > > +static inline void audit_fanotify(unsigned int response) > +{ > + if (!audit_dummy_context()) > + __audit_fanotify(response); > +} > + > extern int audit_n_rules; > extern int audit_signals; > #else /* CONFIG_AUDITSYSCALL */ > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > index 0714a66..221f8b7 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 1331 /* Fanotify access decision */ > > #define AUDIT_AVC 1400 /* SE Linux avc denial or grant */ > #define AUDIT_SELINUX_ERR 1401 /* Internal SE Linux Errors */ > diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h > index 030508d..5681e44 100644 > --- a/include/uapi/linux/fanotify.h > +++ b/include/uapi/linux/fanotify.h > @@ -99,6 +99,8 @@ struct fanotify_response { > /* Legit userspace responses to a _PERM event */ > #define FAN_ALLOW 0x01 > #define FAN_DENY 0x02 > +#define FAN_AUDIT 0x80 /* Bit mask to create audit record for result */ > + > /* No fd set in event */ > #define FAN_NOFD -1 > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index 3260ba2..1725f73 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -2390,6 +2390,12 @@ void __audit_log_kern_module(char *name) > context->type = AUDIT_KERN_MODULE; > } > > +void __audit_fanotify(unsigned int response) > +{ > + audit_log(current->audit_context, GFP_ATOMIC, > + AUDIT_FANOTIFY, "resp=%u", response); > +} > + > static void audit_log_task(struct audit_buffer *ab) > { > kuid_t auid, uid; > -- > 2.9.5 > > > -- > Linux-audit mailing list > Linux-audit@redhat.com > https://www.redhat.com/mailman/listinfo/linux-audit
On 2017-09-05 14:32, Steve Grubb wrote: > 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. > > It would be used something like this in user space code: > > response.response = FAN_DENY | FAN_AUDIT; > write(fd, &response, sizeof(struct fanotify_response)); > > When the syscall ends, the audit system will record the decision as a > AUDIT_FANOTIFY auxiliary record to denote that the reason this event > occurred is the result of an access control decision from fanotify > rather than DAC or MAC policy. > > A sample event looks like this: > > type=PATH msg=audit(1504310584.332:290): item=0 name="./evil-ls" > inode=1319561 dev=fc:03 mode=0100755 ouid=1000 ogid=1000 rdev=00:00 > obj=unconfined_u:object_r:user_home_t:s0 nametype=NORMAL > type=CWD msg=audit(1504310584.332:290): cwd="/home/sgrubb" > type=SYSCALL msg=audit(1504310584.332:290): arch=c000003e syscall=2 > success=no exit=-1 a0=32cb3fca90 a1=0 a2=43 a3=8 items=1 ppid=901 > pid=959 auid=1000 uid=1000 gid=1000 euid=1000 suid=1000 > fsuid=1000 egid=1000 sgid=1000 fsgid=1000 tty=pts1 ses=3 comm="bash" > exe="/usr/bin/bash" subj=unconfined_u:unconfined_r:unconfined_t: > s0-s0:c0.c1023 key=(null) > type=FANOTIFY msg=audit(1504310584.332:290): resp=2 > > Signed-off-by: sgrubb <sgrubb@redhat.com> In the AUDIT_FANOTIFY record, there is a new field "resp". Should this not conform to the existing field types "res" or "result"? Other than that, Reviewed-by: Richard Guy Briggs <rgb@redhat.com> > --- > fs/notify/fanotify/fanotify.c | 8 +++++++- > fs/notify/fanotify/fanotify_user.c | 2 +- > include/linux/audit.h | 7 +++++++ > include/uapi/linux/audit.h | 1 + > include/uapi/linux/fanotify.h | 2 ++ > kernel/auditsc.c | 6 ++++++ > 6 files changed, 24 insertions(+), 2 deletions(-) > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > index 2fa99ae..1968d21 100644 > --- a/fs/notify/fanotify/fanotify.c > +++ b/fs/notify/fanotify/fanotify.c > @@ -9,6 +9,7 @@ > #include <linux/sched/user.h> > #include <linux/types.h> > #include <linux/wait.h> > +#include <linux/audit.h> > > #include "fanotify.h" > > @@ -78,7 +79,7 @@ static int fanotify_get_response(struct fsnotify_group *group, > fsnotify_finish_user_wait(iter_info); > out: > /* userspace responded, convert to something usable */ > - switch (event->response) { > + switch (event->response & ~FAN_AUDIT) { > case FAN_ALLOW: > ret = 0; > break; > @@ -86,6 +87,11 @@ static int fanotify_get_response(struct fsnotify_group *group, > default: > ret = -EPERM; > } > + > + /* Check if the response should be audited */ > + if (event->response & FAN_AUDIT) > + audit_fanotify(event->response & ~FAN_AUDIT); > + > event->response = 0; > > pr_debug("%s: group=%p event=%p about to return ret=%d\n", __func__, > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index 907a481..b983b5c 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -179,7 +179,7 @@ static int process_access_response(struct fsnotify_group *group, > * userspace can send a valid response or we will clean it up after the > * timeout > */ > - switch (response) { > + switch (response & ~FAN_AUDIT) { > case FAN_ALLOW: > case FAN_DENY: > break; > diff --git a/include/linux/audit.h b/include/linux/audit.h > index 2150bdc..bf55732 100644 > --- a/include/linux/audit.h > +++ b/include/linux/audit.h > @@ -360,6 +360,7 @@ extern int __audit_log_bprm_fcaps(struct linux_binprm *bprm, > extern void __audit_log_capset(const struct cred *new, const struct cred *old); > extern void __audit_mmap_fd(int fd, int flags); > extern void __audit_log_kern_module(char *name); > +extern void __audit_fanotify(unsigned int response); > > static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp) > { > @@ -456,6 +457,12 @@ static inline void audit_log_kern_module(char *name) > __audit_log_kern_module(name); > } > > +static inline void audit_fanotify(unsigned int response) > +{ > + if (!audit_dummy_context()) > + __audit_fanotify(response); > +} > + > extern int audit_n_rules; > extern int audit_signals; > #else /* CONFIG_AUDITSYSCALL */ > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > index 0714a66..221f8b7 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 1331 /* Fanotify access decision */ > > #define AUDIT_AVC 1400 /* SE Linux avc denial or grant */ > #define AUDIT_SELINUX_ERR 1401 /* Internal SE Linux Errors */ > diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h > index 030508d..5681e44 100644 > --- a/include/uapi/linux/fanotify.h > +++ b/include/uapi/linux/fanotify.h > @@ -99,6 +99,8 @@ struct fanotify_response { > /* Legit userspace responses to a _PERM event */ > #define FAN_ALLOW 0x01 > #define FAN_DENY 0x02 > +#define FAN_AUDIT 0x80 /* Bit mask to create audit record for result */ > + > /* No fd set in event */ > #define FAN_NOFD -1 > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index 3260ba2..1725f73 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -2390,6 +2390,12 @@ void __audit_log_kern_module(char *name) > context->type = AUDIT_KERN_MODULE; > } > > +void __audit_fanotify(unsigned int response) > +{ > + audit_log(current->audit_context, GFP_ATOMIC, > + AUDIT_FANOTIFY, "resp=%u", response); > +} > + > static void audit_log_task(struct audit_buffer *ab) > { > kuid_t auid, uid; > -- > 2.9.5 > > > -- > Linux-audit mailing list > Linux-audit@redhat.com > https://www.redhat.com/mailman/listinfo/linux-audit - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
On Tue 05-09-17 14:32:07, Steve Grubb wrote: > 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? Or is it that for CC requirements you have to implement some deamon which will arbitrate access using fanotify and you need to have decisions logged using kernel audit interface? And another design question: If you need all responses by some daemon logged, wouldn't it be more logical to make FAN_AUDIT a property of fanotify instance (i.e., a flag to fanotify_init(2))? Or maybe a property of fanotify mark (i.e., a flag to fanotify_mark(2))? > It would be used something like this in user space code: > > response.response = FAN_DENY | FAN_AUDIT; > write(fd, &response, sizeof(struct fanotify_response)); > > When the syscall ends, the audit system will record the decision as a > AUDIT_FANOTIFY auxiliary record to denote that the reason this event > occurred is the result of an access control decision from fanotify > rather than DAC or MAC policy. > > A sample event looks like this: > > type=PATH msg=audit(1504310584.332:290): item=0 name="./evil-ls" > inode=1319561 dev=fc:03 mode=0100755 ouid=1000 ogid=1000 rdev=00:00 > obj=unconfined_u:object_r:user_home_t:s0 nametype=NORMAL > type=CWD msg=audit(1504310584.332:290): cwd="/home/sgrubb" > type=SYSCALL msg=audit(1504310584.332:290): arch=c000003e syscall=2 > success=no exit=-1 a0=32cb3fca90 a1=0 a2=43 a3=8 items=1 ppid=901 > pid=959 auid=1000 uid=1000 gid=1000 euid=1000 suid=1000 > fsuid=1000 egid=1000 sgid=1000 fsgid=1000 tty=pts1 ses=3 comm="bash" > exe="/usr/bin/bash" subj=unconfined_u:unconfined_r:unconfined_t: > s0-s0:c0.c1023 key=(null) > type=FANOTIFY msg=audit(1504310584.332:290): resp=2 > > Signed-off-by: sgrubb <sgrubb@redhat.com> <snip> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index 3260ba2..1725f73 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -2390,6 +2390,12 @@ void __audit_log_kern_module(char *name) > context->type = AUDIT_KERN_MODULE; > } > > +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. Honza
On Wed, Sep 6, 2017 at 12:18 PM, Jan Kara <jack@suse.cz> wrote: > On Tue 05-09-17 14:32:07, Steve Grubb wrote: >> 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? Or is it that for CC > requirements you have to implement some deamon which will arbitrate access > using fanotify and you need to have decisions logged using kernel audit > interface? > > And another design question: If you need all responses by some daemon > logged, wouldn't it be more logical to make FAN_AUDIT a property of > fanotify instance (i.e., a flag to fanotify_init(2))? Or maybe a property > of fanotify mark (i.e., a flag to fanotify_mark(2))? > Even if the use case is auditing blocklisted files, the change of ABI on the response fd should be opt-in by a new flag to fanotify_init(), something like FAN_CAN_AUDIT. In other words, your new daemon that responds with FAN_AUDIT must fail to start when running on an old kernel that doesn't support the FAN_AUDIT response, unless it knows how to fall back and call fanotify_init() again without FAN_CAN_AUDIT and then not respond with FAN_AUDIT. Other than that, I agree with Jan that setting FAN_AUDIT by default for all permission responses at fanotify_init() and maybe fanotify_mark() sounds useful. IMO, it makes most sense in proximity to defining the notification class, e.g.: fanotify_init(FAN_CLASS_CONTENT|FAN_PERM_AUDIT, 0); Amir.
On Tuesday, September 5, 2017 11:24:49 PM EDT Richard Guy Briggs wrote: > On 2017-09-05 14:32, Steve Grubb wrote: > > 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. > > > > It would be used something like this in user space code: > > response.response = FAN_DENY | FAN_AUDIT; > > write(fd, &response, sizeof(struct fanotify_response)); > > > > When the syscall ends, the audit system will record the decision as a > > AUDIT_FANOTIFY auxiliary record to denote that the reason this event > > occurred is the result of an access control decision from fanotify > > rather than DAC or MAC policy. > > > > A sample event looks like this: > > > > type=PATH msg=audit(1504310584.332:290): item=0 name="./evil-ls" > > inode=1319561 dev=fc:03 mode=0100755 ouid=1000 ogid=1000 rdev=00:00 > > obj=unconfined_u:object_r:user_home_t:s0 nametype=NORMAL > > type=CWD msg=audit(1504310584.332:290): cwd="/home/sgrubb" > > type=SYSCALL msg=audit(1504310584.332:290): arch=c000003e syscall=2 > > success=no exit=-1 a0=32cb3fca90 a1=0 a2=43 a3=8 items=1 ppid=901 > > pid=959 auid=1000 uid=1000 gid=1000 euid=1000 suid=1000 > > fsuid=1000 egid=1000 sgid=1000 fsgid=1000 tty=pts1 ses=3 comm="bash" > > exe="/usr/bin/bash" subj=unconfined_u:unconfined_r:unconfined_t: > > s0-s0:c0.c1023 key=(null) > > type=FANOTIFY msg=audit(1504310584.332:290): resp=2 > > > > Signed-off-by: sgrubb <sgrubb@redhat.com> > > In the AUDIT_FANOTIFY record, there is a new field "resp". Should this > not conform to the existing field types "res" or "result"? The values used by fanotify are either 1 & 2 rather than 0 & 1. So, it would need record specific translation. It's easier to just pick a new field name and key off of that. -Steve > Other than that, > Reviewed-by: Richard Guy Briggs <rgb@redhat.com> > > > --- > > > > fs/notify/fanotify/fanotify.c | 8 +++++++- > > fs/notify/fanotify/fanotify_user.c | 2 +- > > include/linux/audit.h | 7 +++++++ > > include/uapi/linux/audit.h | 1 + > > include/uapi/linux/fanotify.h | 2 ++ > > kernel/auditsc.c | 6 ++++++ > > 6 files changed, 24 insertions(+), 2 deletions(-) > > > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > > index 2fa99ae..1968d21 100644 > > --- a/fs/notify/fanotify/fanotify.c > > +++ b/fs/notify/fanotify/fanotify.c > > @@ -9,6 +9,7 @@ > > > > #include <linux/sched/user.h> > > #include <linux/types.h> > > #include <linux/wait.h> > > > > +#include <linux/audit.h> > > > > #include "fanotify.h" > > > > @@ -78,7 +79,7 @@ static int fanotify_get_response(struct fsnotify_group > > *group,> > > fsnotify_finish_user_wait(iter_info); > > > > out: > > /* userspace responded, convert to something usable */ > > > > - switch (event->response) { > > + switch (event->response & ~FAN_AUDIT) { > > > > case FAN_ALLOW: > > ret = 0; > > break; > > > > @@ -86,6 +87,11 @@ static int fanotify_get_response(struct fsnotify_group > > *group,> > > default: > > ret = -EPERM; > > > > } > > > > + > > + /* Check if the response should be audited */ > > + if (event->response & FAN_AUDIT) > > + audit_fanotify(event->response & ~FAN_AUDIT); > > + > > > > event->response = 0; > > > > pr_debug("%s: group=%p event=%p about to return ret=%d\n", __func__, > > > > diff --git a/fs/notify/fanotify/fanotify_user.c > > b/fs/notify/fanotify/fanotify_user.c index 907a481..b983b5c 100644 > > --- a/fs/notify/fanotify/fanotify_user.c > > +++ b/fs/notify/fanotify/fanotify_user.c > > @@ -179,7 +179,7 @@ static int process_access_response(struct > > fsnotify_group *group,> > > * userspace can send a valid response or we will clean it up after the > > * timeout > > */ > > > > - switch (response) { > > + switch (response & ~FAN_AUDIT) { > > > > case FAN_ALLOW: > > > > case FAN_DENY: > > break; > > > > diff --git a/include/linux/audit.h b/include/linux/audit.h > > index 2150bdc..bf55732 100644 > > --- a/include/linux/audit.h > > +++ b/include/linux/audit.h > > @@ -360,6 +360,7 @@ extern int __audit_log_bprm_fcaps(struct linux_binprm > > *bprm,> > > extern void __audit_log_capset(const struct cred *new, const struct cred > > *old); extern void __audit_mmap_fd(int fd, int flags); > > extern void __audit_log_kern_module(char *name); > > > > +extern void __audit_fanotify(unsigned int response); > > > > static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp) > > { > > > > @@ -456,6 +457,12 @@ static inline void audit_log_kern_module(char *name) > > > > __audit_log_kern_module(name); > > > > } > > > > +static inline void audit_fanotify(unsigned int response) > > +{ > > + if (!audit_dummy_context()) > > + __audit_fanotify(response); > > +} > > + > > > > extern int audit_n_rules; > > extern int audit_signals; > > #else /* CONFIG_AUDITSYSCALL */ > > > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > > index 0714a66..221f8b7 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 1331 /* Fanotify access decision */ > > > > #define AUDIT_AVC 1400 /* SE Linux avc denial or grant */ > > #define AUDIT_SELINUX_ERR 1401 /* Internal SE Linux Errors */ > > > > diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h > > index 030508d..5681e44 100644 > > --- a/include/uapi/linux/fanotify.h > > +++ b/include/uapi/linux/fanotify.h > > @@ -99,6 +99,8 @@ struct fanotify_response { > > > > /* Legit userspace responses to a _PERM event */ > > #define FAN_ALLOW 0x01 > > #define FAN_DENY 0x02 > > > > +#define FAN_AUDIT 0x80 /* Bit mask to create audit record for result */ > > + > > > > /* No fd set in event */ > > #define FAN_NOFD -1 > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > index 3260ba2..1725f73 100644 > > --- a/kernel/auditsc.c > > +++ b/kernel/auditsc.c > > @@ -2390,6 +2390,12 @@ void __audit_log_kern_module(char *name) > > > > context->type = AUDIT_KERN_MODULE; > > > > } > > > > +void __audit_fanotify(unsigned int response) > > +{ > > + audit_log(current->audit_context, GFP_ATOMIC, > > + AUDIT_FANOTIFY, "resp=%u", response); > > +} > > + > > > > static void audit_log_task(struct audit_buffer *ab) > > { > > > > kuid_t auid, uid; > > - RGB > > -- > Richard Guy Briggs <rgb@redhat.com> > Sr. S/W Engineer, Kernel Security, Base Operating Systems > Remote, Ottawa, Red Hat Canada > IRC: rgb, SunRaycer > Voice: +1.647.777.2635, Internal: (81) 32635
On Wednesday, September 6, 2017 5:18:22 AM EDT Jan Kara wrote: > On Tue 05-09-17 14:32:07, Steve Grubb wrote: > > 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. > 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. > And another design question: If you need all responses by some daemon > logged, wouldn't it be more logical to make FAN_AUDIT a property of > fanotify instance (i.e., a flag to fanotify_init(2))? Or maybe a property > of fanotify mark (i.e., a flag to fanotify_mark(2))? It needs to be selectively audited. Meaning that most of the time it stays out of the way, but when an admin wants the information they have the ability make it happen. The example code below is overly simplistic. You would normally calculate the access decision and then check if auditing is requested and then 'or' in the audit bit if needed. > > It would be used something like this in user space code: > > response.response = FAN_DENY | FAN_AUDIT; > > write(fd, &response, sizeof(struct fanotify_response)); > > > > When the syscall ends, the audit system will record the decision as a > > AUDIT_FANOTIFY auxiliary record to denote that the reason this event > > occurred is the result of an access control decision from fanotify > > rather than DAC or MAC policy. > > > > A sample event looks like this: > > > > type=PATH msg=audit(1504310584.332:290): item=0 name="./evil-ls" > > inode=1319561 dev=fc:03 mode=0100755 ouid=1000 ogid=1000 rdev=00:00 > > obj=unconfined_u:object_r:user_home_t:s0 nametype=NORMAL > > type=CWD msg=audit(1504310584.332:290): cwd="/home/sgrubb" > > type=SYSCALL msg=audit(1504310584.332:290): arch=c000003e syscall=2 > > success=no exit=-1 a0=32cb3fca90 a1=0 a2=43 a3=8 items=1 ppid=901 > > pid=959 auid=1000 uid=1000 gid=1000 euid=1000 suid=1000 > > fsuid=1000 egid=1000 sgid=1000 fsgid=1000 tty=pts1 ses=3 comm="bash" > > exe="/usr/bin/bash" subj=unconfined_u:unconfined_r:unconfined_t: > > s0-s0:c0.c1023 key=(null) > > type=FANOTIFY msg=audit(1504310584.332:290): resp=2 > > > > Signed-off-by: sgrubb <sgrubb@redhat.com> > > <snip> > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > index 3260ba2..1725f73 100644 > > --- a/kernel/auditsc.c > > +++ b/kernel/auditsc.c > > @@ -2390,6 +2390,12 @@ void __audit_log_kern_module(char *name) > > > > context->type = AUDIT_KERN_MODULE; > > > > } > > > > +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. -Steve
On Wednesday, September 6, 2017 7:11:48 AM EDT Amir Goldstein wrote: > On Wed, Sep 6, 2017 at 12:18 PM, Jan Kara <jack@suse.cz> wrote: > > On Tue 05-09-17 14:32:07, Steve Grubb wrote: > >> 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? Or is it that for CC > > requirements you have to implement some deamon which will arbitrate access > > using fanotify and you need to have decisions logged using kernel audit > > interface? > > > > And another design question: If you need all responses by some daemon > > logged, wouldn't it be more logical to make FAN_AUDIT a property of > > fanotify instance (i.e., a flag to fanotify_init(2))? Or maybe a property > > of fanotify mark (i.e., a flag to fanotify_mark(2))? > > Even if the use case is auditing blocklisted files, the change of ABI on the > response fd should be opt-in by a new flag to fanotify_init(), something > like FAN_CAN_AUDIT. > > In other words, your new daemon that responds with FAN_AUDIT must > fail to start when running on an old kernel that doesn't support the > FAN_AUDIT response, unless it knows how to fall back and call > fanotify_init() again without > FAN_CAN_AUDIT and then not respond with FAN_AUDIT. OK. That's easy enough to add and makes sense. When a daemon tries to audit on a kernel that doesn't, the response is rejected and the system eventually hangs. Killing the daemon fixes it. > Other than that, I agree with Jan that setting FAN_AUDIT by default for > all permission responses at fanotify_init() and maybe fanotify_mark() > sounds useful. > IMO, it makes most sense in proximity to defining the notification class, > e.g.: fanotify_init(FAN_CLASS_CONTENT|FAN_PERM_AUDIT, 0); Its supposed to be selective. The idea is to not force a blanket policy but let the daemon determine by the policy its enforcing whether or not the admin wanted the events. So, the design places exact control in the daemon that responds. For example, the admin may want events only associated with a specific user, directory, file type, file hash, specific file content, etc. but not others. -Steve
On 2017-09-06 10:20, Steve Grubb wrote: > On Tuesday, September 5, 2017 11:24:49 PM EDT Richard Guy Briggs wrote: > > On 2017-09-05 14:32, Steve Grubb wrote: > > > 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. > > > > > > It would be used something like this in user space code: > > > response.response = FAN_DENY | FAN_AUDIT; > > > write(fd, &response, sizeof(struct fanotify_response)); > > > > > > When the syscall ends, the audit system will record the decision as a > > > AUDIT_FANOTIFY auxiliary record to denote that the reason this event > > > occurred is the result of an access control decision from fanotify > > > rather than DAC or MAC policy. > > > > > > A sample event looks like this: > > > > > > type=PATH msg=audit(1504310584.332:290): item=0 name="./evil-ls" > > > inode=1319561 dev=fc:03 mode=0100755 ouid=1000 ogid=1000 rdev=00:00 > > > obj=unconfined_u:object_r:user_home_t:s0 nametype=NORMAL > > > type=CWD msg=audit(1504310584.332:290): cwd="/home/sgrubb" > > > type=SYSCALL msg=audit(1504310584.332:290): arch=c000003e syscall=2 > > > success=no exit=-1 a0=32cb3fca90 a1=0 a2=43 a3=8 items=1 ppid=901 > > > pid=959 auid=1000 uid=1000 gid=1000 euid=1000 suid=1000 > > > fsuid=1000 egid=1000 sgid=1000 fsgid=1000 tty=pts1 ses=3 comm="bash" > > > exe="/usr/bin/bash" subj=unconfined_u:unconfined_r:unconfined_t: > > > s0-s0:c0.c1023 key=(null) > > > type=FANOTIFY msg=audit(1504310584.332:290): resp=2 > > > > > > Signed-off-by: sgrubb <sgrubb@redhat.com> > > > > In the AUDIT_FANOTIFY record, there is a new field "resp". Should this > > not conform to the existing field types "res" or "result"? > > The values used by fanotify are either 1 & 2 rather than 0 & 1. So, it would > need record specific translation. It's easier to just pick a new field name > and key off of that. Ah, fair enough. That's unfortunate. > -Steve > > > Other than that, > > Reviewed-by: Richard Guy Briggs <rgb@redhat.com> > > > > > --- > > > > > > fs/notify/fanotify/fanotify.c | 8 +++++++- > > > fs/notify/fanotify/fanotify_user.c | 2 +- > > > include/linux/audit.h | 7 +++++++ > > > include/uapi/linux/audit.h | 1 + > > > include/uapi/linux/fanotify.h | 2 ++ > > > kernel/auditsc.c | 6 ++++++ > > > 6 files changed, 24 insertions(+), 2 deletions(-) > > > > > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > > > index 2fa99ae..1968d21 100644 > > > --- a/fs/notify/fanotify/fanotify.c > > > +++ b/fs/notify/fanotify/fanotify.c > > > @@ -9,6 +9,7 @@ > > > > > > #include <linux/sched/user.h> > > > #include <linux/types.h> > > > #include <linux/wait.h> > > > > > > +#include <linux/audit.h> > > > > > > #include "fanotify.h" > > > > > > @@ -78,7 +79,7 @@ static int fanotify_get_response(struct fsnotify_group > > > *group,> > > > fsnotify_finish_user_wait(iter_info); > > > > > > out: > > > /* userspace responded, convert to something usable */ > > > > > > - switch (event->response) { > > > + switch (event->response & ~FAN_AUDIT) { > > > > > > case FAN_ALLOW: > > > ret = 0; > > > break; > > > > > > @@ -86,6 +87,11 @@ static int fanotify_get_response(struct fsnotify_group > > > *group,> > > > default: > > > ret = -EPERM; > > > > > > } > > > > > > + > > > + /* Check if the response should be audited */ > > > + if (event->response & FAN_AUDIT) > > > + audit_fanotify(event->response & ~FAN_AUDIT); > > > + > > > > > > event->response = 0; > > > > > > pr_debug("%s: group=%p event=%p about to return ret=%d\n", __func__, > > > > > > diff --git a/fs/notify/fanotify/fanotify_user.c > > > b/fs/notify/fanotify/fanotify_user.c index 907a481..b983b5c 100644 > > > --- a/fs/notify/fanotify/fanotify_user.c > > > +++ b/fs/notify/fanotify/fanotify_user.c > > > @@ -179,7 +179,7 @@ static int process_access_response(struct > > > fsnotify_group *group,> > > > * userspace can send a valid response or we will clean it up after the > > > * timeout > > > */ > > > > > > - switch (response) { > > > + switch (response & ~FAN_AUDIT) { > > > > > > case FAN_ALLOW: > > > > > > case FAN_DENY: > > > break; > > > > > > diff --git a/include/linux/audit.h b/include/linux/audit.h > > > index 2150bdc..bf55732 100644 > > > --- a/include/linux/audit.h > > > +++ b/include/linux/audit.h > > > @@ -360,6 +360,7 @@ extern int __audit_log_bprm_fcaps(struct linux_binprm > > > *bprm,> > > > extern void __audit_log_capset(const struct cred *new, const struct cred > > > *old); extern void __audit_mmap_fd(int fd, int flags); > > > extern void __audit_log_kern_module(char *name); > > > > > > +extern void __audit_fanotify(unsigned int response); > > > > > > static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp) > > > { > > > > > > @@ -456,6 +457,12 @@ static inline void audit_log_kern_module(char *name) > > > > > > __audit_log_kern_module(name); > > > > > > } > > > > > > +static inline void audit_fanotify(unsigned int response) > > > +{ > > > + if (!audit_dummy_context()) > > > + __audit_fanotify(response); > > > +} > > > + > > > > > > extern int audit_n_rules; > > > extern int audit_signals; > > > #else /* CONFIG_AUDITSYSCALL */ > > > > > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > > > index 0714a66..221f8b7 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 1331 /* Fanotify access decision */ > > > > > > #define AUDIT_AVC 1400 /* SE Linux avc denial or grant */ > > > #define AUDIT_SELINUX_ERR 1401 /* Internal SE Linux Errors */ > > > > > > diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h > > > index 030508d..5681e44 100644 > > > --- a/include/uapi/linux/fanotify.h > > > +++ b/include/uapi/linux/fanotify.h > > > @@ -99,6 +99,8 @@ struct fanotify_response { > > > > > > /* Legit userspace responses to a _PERM event */ > > > #define FAN_ALLOW 0x01 > > > #define FAN_DENY 0x02 > > > > > > +#define FAN_AUDIT 0x80 /* Bit mask to create audit record for result */ > > > + > > > > > > /* No fd set in event */ > > > #define FAN_NOFD -1 > > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > > index 3260ba2..1725f73 100644 > > > --- a/kernel/auditsc.c > > > +++ b/kernel/auditsc.c > > > @@ -2390,6 +2390,12 @@ void __audit_log_kern_module(char *name) > > > > > > context->type = AUDIT_KERN_MODULE; > > > > > > } > > > > > > +void __audit_fanotify(unsigned int response) > > > +{ > > > + audit_log(current->audit_context, GFP_ATOMIC, > > > + AUDIT_FANOTIFY, "resp=%u", response); > > > +} > > > + > > > > > > static void audit_log_task(struct audit_buffer *ab) > > > { > > > > > > kuid_t auid, uid; > > > > - RGB > > > > -- > > Richard Guy Briggs <rgb@redhat.com> > > Sr. S/W Engineer, Kernel Security, Base Operating Systems > > Remote, Ottawa, Red Hat Canada > > IRC: rgb, SunRaycer > > Voice: +1.647.777.2635, Internal: (81) 32635 > > - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
On Wed 06-09-17 10:35:32, Steve Grubb wrote: > On Wednesday, September 6, 2017 5:18:22 AM EDT Jan Kara wrote: > > On Tue 05-09-17 14:32:07, Steve Grubb wrote: > > > 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. > > 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... > > > +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
Hello Jan, On Wednesday, September 6, 2017 12:48:21 PM EDT Jan Kara wrote: > On Wed 06-09-17 10:35:32, Steve Grubb wrote: > > On Wednesday, September 6, 2017 5:18:22 AM EDT Jan Kara wrote: > > > 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? Not really. For CC, the mechanism just needs to be available. > That seems pretty hard to do to me and even it done, it sounds like quite a > bit of duplication? AFAIK, there is only one that needs to get patched. It's totally opt in. > 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? There can be a lot of information to wade through. Normally, we don't parse events in the kernel or user space. They are in a race to keep events flowing so that the kernel stays fast and responsive. There are buffer limits where if we too far behind we start losing events. The decision to log should be rare. So, if we get lots of events that don't need to be logged, it will slow down the whole kernel. But besides the performance side of it, the audit subsystem has part of the information to make a decision on whether or not this one should be logged. It doesn't know the same information as the daemon that is deciding to grant access. Only the daemon granting access knows if this one file alone should get an audit record. And based on the fanotify API there is no way to pass along additional information that could be taken into account by the audit subsystem for its decision. > 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... We have control over that, too. You can audit all EPERM events or you can be selective about getting them from a specific directory, application, user, or MAC label. To get this kind of granularity would mean making another filter in the kernel and loading a set of rules which duplicates, for the most part, the rules the access daemon has. Then we'd still need the identifier saying the event originated from the fanotify subsystem. This leads to a lot more complexity. As it stands, the patch set adds 24 lines of code. So, its much more minimalistic and places the decision in the only thing that truly knows if an event is needed. But let's examine that a bit. The user space daemon could also directly log an event through the user space API. But, it would need to gather a lot of information to fully identify the subject and objects in the event. There is a size limit on how big an event could be. So, if we have a file that is at PATH_MAX and has control characters in it, we would need 8192 bytes to log the filename. Add in the MAC labels and other house keeping and we have less than 100 bytes to log information. This also means we need to make new parsers and design reporting to make sense of this new record format. So, due to these size limits, it more robust to generate the record in the kernel and coopt all the reporting mechanisms that are already in place. So, to sum it up, doing it this was is better performance for the kernel, only needs 24 or so additional lines of code in the kernel, only 4 lines in the user space daemon, and 4 lines in the user space audit code. Its the simplest approach with the best targeting of events. Does this help? > > > > +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. Sure. I'll add that into v2 of the patch. Thanks, -Steve
On Wed 06-09-17 13:34:32, Steve Grubb wrote: > Hello Jan, > > On Wednesday, September 6, 2017 12:48:21 PM EDT Jan Kara wrote: > > On Wed 06-09-17 10:35:32, Steve Grubb wrote: > > > On Wednesday, September 6, 2017 5:18:22 AM EDT Jan Kara wrote: > > > > 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? > > Not really. For CC, the mechanism just needs to be available. > > > That seems pretty hard to do to me and even it done, it sounds like quite a > > bit of duplication? > > AFAIK, there is only one that needs to get patched. It's totally opt in. I see. Thanks for explanation. But still, for this feature to make a real difference, you'll have to implement FAN_AUDIT (and corresponding filtering) in all programs using fanotify permission events on your system, won't you? Otherwise decisions from those programs won't get logged and you'll have incomplete information which presumably breaks audit requirements. > > 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? > > There can be a lot of information to wade through. Normally, we don't parse > events in the kernel or user space. They are in a race to keep events flowing > so that the kernel stays fast and responsive. There are buffer limits where if > we too far behind we start losing events. The decision to log should be rare. > So, if we get lots of events that don't need to be logged, it will slow down > the whole kernel. > > But besides the performance side of it, the audit subsystem has part of the > information to make a decision on whether or not this one should be logged. It > doesn't know the same information as the daemon that is deciding to grant > access. Only the daemon granting access knows if this one file alone should > get an audit record. And based on the fanotify API there is no way to pass > along additional information that could be taken into account by the audit > subsystem for its decision. Ok, I think I'm starting to understand this. The audit event about fanotify refusing the access is generally a supplemental information to another event informing about access being denied, isn't it? So you want to log it if and only if denied access event will be logged. Am I getting it right? So the application handling fanotify permission events would parse audit rules in /etc/audit.rules, decide whether its decision would lead to event being logged and if yes, it would set FAN_AUDIT in its response so that supplemental information is also logged. Right? > > 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... > > We have control over that, too. You can audit all EPERM events or you can be > selective about getting them from a specific directory, application, user, or > MAC label. To get this kind of granularity would mean making another filter in > the kernel and loading a set of rules which duplicates, for the most part, the > rules the access daemon has. Then we'd still need the identifier saying the > event originated from the fanotify subsystem. This leads to a lot more > complexity. OK, understood. But with FAN_AUDIT design, you still have to duplicate this functionality - in each application using fanotify permission events which wants to be conformant to CC criteria if I understand things right. Sure it is different from duplicating in the kernel, you can have shared libraries helping with this etc. But still it doesn't look like an ideal situation? One idea I had was: Couldn't we store fanotify decision in audit_context and then if we find event needs to be emitted, we also additionally emit the fact that fanotify is the reason? > As it stands, the patch set adds 24 lines of code. So, its much more > minimalistic and places the decision in the only thing that truly knows if an > event is needed. But let's examine that a bit. > > The user space daemon could also directly log an event through the user space > API. But, it would need to gather a lot of information to fully identify the > subject and objects in the event. There is a size limit on how big an event > could be. So, if we have a file that is at PATH_MAX and has control characters > in it, we would need 8192 bytes to log the filename. Add in the MAC labels and > other house keeping and we have less than 100 bytes to log information. > > This also means we need to make new parsers and design reporting to make sense > of this new record format. So, due to these size limits, it more robust to > generate the record in the kernel and coopt all the reporting mechanisms that > are already in place. > > So, to sum it up, doing it this was is better performance for the kernel, only > needs 24 or so additional lines of code in the kernel, only 4 lines in the > user space daemon, and 4 lines in the user space audit code. Its the simplest > approach with the best targeting of events. > > Does this help? Thanks for detailed explanation! So I'm not at all concerned about complexity of the kernel patch - you are right it is trivial. My concern is more that this adds userspace visible API so once we add it, we have to maintain it forever. So I'd like to get the API right (so that we don't have to add new API for the same thing in a few years) - filesystem notification interfaces are an area where we particularly suffer from API design mistakes - even the third incarnation of the API (fanotify) is not ideal... I understand the difficulty of associating fanotify response with the object (and thus other audit events) from userspace. So I agree that doesn't look like an easier way to go. On the other hand bear in mind there can be several processes mediating access through fanotify and you can end up with supplemental messages like (expanding your example): type=FANOTIFY msg=audit(1504310584.332:290): resp=1 type=FANOTIFY msg=audit(1504310584.332:290): resp=1 type=FANOTIFY msg=audit(1504310584.332:290): resp=1 type=FANOTIFY msg=audit(1504310584.332:290): resp=2 (or possibly without the FAN_ALLOW messages - do we want to log those?) and you have no way to determine which process actually denied the access. I'm not sure whether this matters or not but I can imagine some users complaining about this so I wanted to point that out. Honza
Hello Jan, > > On Thursday, September 7, 2017 6:18:05 AM EDT Jan Kara wrote: > On Wed 06-09-17 13:34:32, Steve Grubb wrote: > > On Wednesday, September 6, 2017 12:48:21 PM EDT Jan Kara wrote: > > > On Wed 06-09-17 10:35:32, Steve Grubb wrote: > > > > On Wednesday, September 6, 2017 5:18:22 AM EDT Jan Kara wrote: > > > > > 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? > > > > Not really. For CC, the mechanism just needs to be available. > > > > > That seems pretty hard to do to me and even it done, it sounds like > > > quite a bit of duplication? > > > > AFAIK, there is only one that needs to get patched. It's totally opt in. > > I see. Thanks for explanation. But still, for this feature to make a real > difference, you'll have to implement FAN_AUDIT (and corresponding > filtering) in all programs using fanotify permission events on your system, > won't you? In real life, perhaps. For common criteria, the developer defines a baseline of applications that make up the Target Of Evaluation. One would normally make sure all applications that make up the TOE correctly implement the security features and demonstrate this with a test suite. So, in this case, I know of only one application that needs patching. Out of curiosity, what other applications would need patching that you know of? > Otherwise decisions from those programs won't get logged and > you'll have incomplete information which presumably breaks audit > requirements. Usually systems can be defined where everything is consistent. For example, passwd and shadow-utils are patched for auditing and libuser is not. > > > 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? > > > > There can be a lot of information to wade through. Normally, we don't > > parse events in the kernel or user space. They are in a race to keep > > events flowing so that the kernel stays fast and responsive. There are > > buffer limits where if we too far behind we start losing events. The > > decision to log should be rare. So, if we get lots of events that don't > > need to be logged, it will slow down the whole kernel. > > > > But besides the performance side of it, the audit subsystem has part of > > the information to make a decision on whether or not this one should be > > logged. It doesn't know the same information as the daemon that is > > deciding to grant access. Only the daemon granting access knows if this > > one file alone should get an audit record. And based on the fanotify API > > there is no way to pass along additional information that could be taken > > into account by the audit subsystem for its decision. > > Ok, I think I'm starting to understand this. The audit event about fanotify > refusing the access is generally a supplemental information to another > event informing about access being denied, isn't it? So you want to log it > if and only if denied access event will be logged. Am I getting it right? No, it could be when an access is granted, too. Some people are paranoid and may want information on approvals and denials for specific kinds of files or users. Again, its not a blanket policy where everything denied must be recorded. We should leave that decision to the admin to determine what he wants recorded. > So the application handling fanotify permission events would parse audit > rules in /etc/audit.rules, decide whether its decision would lead to event > being logged and if yes, it would set FAN_AUDIT in its response so that > supplemental information is also logged. Right? I wouldn't imagine it like that. The way I see it, the daemon that determines access has its own set of rules. In those rules there would be some syntax about attributes of the file to match against and then what to do if it matches. It could either be deny, approve, deny with audit, or approve with audit. In the case of a virus scanner, the rule is implicit any signature match is denial. In this way, the daemon and audit system do not need to know anything about each other. AppArmor and Selinux are the same way. They have their own rules and they decide whether or not an audit event should be generated. > > > 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... > > > > We have control over that, too. You can audit all EPERM events or you can > > be selective about getting them from a specific directory, application, > > user, or MAC label. To get this kind of granularity would mean making > > another filter in the kernel and loading a set of rules which duplicates, > > for the most part, the rules the access daemon has. Then we'd still need > > the identifier saying the event originated from the fanotify subsystem. > > This leads to a lot more complexity. > > OK, understood. But with FAN_AUDIT design, you still have to duplicate this > functionality - in each application using fanotify permission events which > wants to be conformant to CC criteria if I understand things right. Sure it > is different from duplicating in the kernel, you can have shared libraries > helping with this etc. But still it doesn't look like an ideal situation? To add this capability to other apps should be small effort. I really don't like the audit all denies, because it removes the selectivity from the admin. They can't decide based on storage requirements how much they want to audit. So, I really advocate keeping the decision to audit in the rules of the application granting or denying access. > One idea I had was: Couldn't we store fanotify decision in audit_context > and then if we find event needs to be emitted, we also additionally emit > the fact that fanotify is the reason? That is kind of how the patch works. When the user space daemon sees an access that the admin thought was important, it tags the decision wit an audit bit which in turn causes a call into the audit code to add an auxiliary record to the context and if the event has not be determined to be of interest to the audit system to override that and saying this is of interest generate the event on syscall exit. > > As it stands, the patch set adds 24 lines of code. So, its much more > > minimalistic and places the decision in the only thing that truly knows if > > an event is needed. But let's examine that a bit. > > > > The user space daemon could also directly log an event through the user > > space API. But, it would need to gather a lot of information to fully > > identify the subject and objects in the event. There is a size limit on > > how big an event could be. So, if we have a file that is at PATH_MAX and > > has control characters in it, we would need 8192 bytes to log the > > filename. Add in the MAC labels and other house keeping and we have less > > than 100 bytes to log information. > > > > This also means we need to make new parsers and design reporting to make > > sense of this new record format. So, due to these size limits, it more > > robust to generate the record in the kernel and coopt all the reporting > > mechanisms that are already in place. > > > > So, to sum it up, doing it this was is better performance for the kernel, > > only needs 24 or so additional lines of code in the kernel, only 4 lines > > in the user space daemon, and 4 lines in the user space audit code. Its > > the simplest approach with the best targeting of events. > > > > Does this help? > > Thanks for detailed explanation! So I'm not at all concerned about > complexity of the kernel patch - you are right it is trivial. My concern is > more that this adds userspace visible API so once we add it, we have to > maintain it forever. So I'd like to get the API right (so that we don't > have to add new API for the same thing in a few years) - filesystem > notification interfaces are an area where we particularly suffer from API > design mistakes - even the third incarnation of the API (fanotify) is not > ideal... OK. I have some more ideas on improvements that I'll share in time. > I understand the difficulty of associating fanotify response with the > object (and thus other audit events) from userspace. So I agree that > doesn't look like an easier way to go. On the other hand bear in mind there > can be several processes mediating access through fanotify and you can end > up with supplemental messages like (expanding your example): > > type=FANOTIFY msg=audit(1504310584.332:290): resp=1 > type=FANOTIFY msg=audit(1504310584.332:290): resp=1 > type=FANOTIFY msg=audit(1504310584.332:290): resp=1 > type=FANOTIFY msg=audit(1504310584.332:290): resp=2 > > (or possibly without the FAN_ALLOW messages - do we want to log those?) and > you have no way to determine which process actually denied the access. I'm > not sure whether this matters or not but I can imagine some users > complaining about this so I wanted to point that out. I was also thinking about that and I think we can add that to the event easily. One other thing that I think could be helpful is if the daemon could also write a reason code or something like the rule number that its enforcing. Would that be something useful? (I could imagine a security officer wanting the rule association.) If so, then maybe we can carve out more bits of the response to be used by the daemon for a reason code? -Steve
Hello Steve, On Thu 07-09-17 11:47:35, Steve Grubb wrote: > > > On Thursday, September 7, 2017 6:18:05 AM EDT Jan Kara wrote: > > On Wed 06-09-17 13:34:32, Steve Grubb wrote: > > > On Wednesday, September 6, 2017 12:48:21 PM EDT Jan Kara wrote: > > > > On Wed 06-09-17 10:35:32, Steve Grubb wrote: > > > > > On Wednesday, September 6, 2017 5:18:22 AM EDT Jan Kara wrote: > > > > > > 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? > > > > > > Not really. For CC, the mechanism just needs to be available. > > > > > > > That seems pretty hard to do to me and even it done, it sounds like > > > > quite a bit of duplication? > > > > > > AFAIK, there is only one that needs to get patched. It's totally opt in. > > > > I see. Thanks for explanation. But still, for this feature to make a real > > difference, you'll have to implement FAN_AUDIT (and corresponding > > filtering) in all programs using fanotify permission events on your system, > > won't you? > > In real life, perhaps. For common criteria, the developer defines a baseline > of applications that make up the Target Of Evaluation. One would normally make > sure all applications that make up the TOE correctly implement the security > features and demonstrate this with a test suite. So, in this case, I know of > only one application that needs patching. > > Out of curiosity, what other applications would need patching that you know > of? I've never used Audit for security certifications so I don't really know. But I've used it several times for debugging system behavior and there it would be handy to have all applications supported. But we have ftrace for that these days. I can only imagine paranoid admin wanting to know whether his $favourite virus scanner refused some access. And it would be nice if all such scanners were automatically supported instead to having to add support for each of them. > > > > 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? > > > > > > There can be a lot of information to wade through. Normally, we don't > > > parse events in the kernel or user space. They are in a race to keep > > > events flowing so that the kernel stays fast and responsive. There are > > > buffer limits where if we too far behind we start losing events. The > > > decision to log should be rare. So, if we get lots of events that don't > > > need to be logged, it will slow down the whole kernel. > > > > > > But besides the performance side of it, the audit subsystem has part of > > > the information to make a decision on whether or not this one should be > > > logged. It doesn't know the same information as the daemon that is > > > deciding to grant access. Only the daemon granting access knows if this > > > one file alone should get an audit record. And based on the fanotify API > > > there is no way to pass along additional information that could be taken > > > into account by the audit subsystem for its decision. > > > > Ok, I think I'm starting to understand this. The audit event about fanotify > > refusing the access is generally a supplemental information to another > > event informing about access being denied, isn't it? So you want to log it > > if and only if denied access event will be logged. Am I getting it right? > > No, it could be when an access is granted, too. Some people are paranoid and > may want information on approvals and denials for specific kinds of files or > users. Again, its not a blanket policy where everything denied must be > recorded. We should leave that decision to the admin to determine what he > wants recorded. > > > So the application handling fanotify permission events would parse audit > > rules in /etc/audit.rules, decide whether its decision would lead to event > > being logged and if yes, it would set FAN_AUDIT in its response so that > > supplemental information is also logged. Right? > > I wouldn't imagine it like that. The way I see it, the daemon that determines > access has its own set of rules. In those rules there would be some syntax > about attributes of the file to match against and then what to do if it > matches. It could either be deny, approve, deny with audit, or approve with > audit. In the case of a virus scanner, the rule is implicit any signature > match is denial. > > In this way, the daemon and audit system do not need to know anything about > each other. AppArmor and Selinux are the same way. They have their own rules > and they decide whether or not an audit event should be generated. OK, I can see why this is interesting. But then the audit event should have enough information to be useful on its own, shouldn't it? Because currently it is only context-id and response, which is useless on its own... > > One idea I had was: Couldn't we store fanotify decision in audit_context > > and then if we find event needs to be emitted, we also additionally emit > > the fact that fanotify is the reason? > > That is kind of how the patch works. When the user space daemon sees an access > that the admin thought was important, it tags the decision wit an audit bit > which in turn causes a call into the audit code to add an auxiliary record to > the context and if the event has not be determined to be of interest to the > audit system to override that and saying this is of interest generate the > event on syscall exit. OK, understood. So the only place where we differ is whether the process processing fanotify permission events decide about logging the event or whether kernel should decide about logging on its own. My though was that we could have something like another filesystem event type - currently we can decide about logging reads, writes, execute, ... now we could also decide about logging of fanotify decisions but I'm not sure whether this would reasonably tie into audit philosophy. So I'm still not 100% convinced putting decision about logging the event into application is a great idea (after all we don't put burden of logging denied access due to permissions e.g. to FUSE daemon which denied the access) but I'm now less opposed to it ;) > > I understand the difficulty of associating fanotify response with the > > object (and thus other audit events) from userspace. So I agree that > > doesn't look like an easier way to go. On the other hand bear in mind there > > can be several processes mediating access through fanotify and you can end > > up with supplemental messages like (expanding your example): > > > > type=FANOTIFY msg=audit(1504310584.332:290): resp=1 > > type=FANOTIFY msg=audit(1504310584.332:290): resp=1 > > type=FANOTIFY msg=audit(1504310584.332:290): resp=1 > > type=FANOTIFY msg=audit(1504310584.332:290): resp=2 > > > > (or possibly without the FAN_ALLOW messages - do we want to log those?) and > > you have no way to determine which process actually denied the access. I'm > > not sure whether this matters or not but I can imagine some users > > complaining about this so I wanted to point that out. > > I was also thinking about that and I think we can add that to the event > easily. One other thing that I think could be helpful is if the daemon could So it is easy to add inode / file where the event happened (which would be IMHO useful if the events should be mostly standalone), adding PID of the process whose access is mediated is easy as well (that's just the running process in whose context we generate the event). Adding PID of the process which decided about access is more difficult (we have only file descriptor where we send event) however we could attach that information internally to fanotify_event() in process_access_response() and then pick it up when generating audit event. > also write a reason code or something like the rule number that its enforcing. > Would that be something useful? (I could imagine a security officer wanting > the rule association.) If so, then maybe we can carve out more bits of the > response to be used by the daemon for a reason code? I'd be wary of adding blanket "reason code". Without a clear meaning there would be inconsistencies among applications and so it would be useless. If you have more concrete proposal, we can talk about it. Honza
Hello Jan, On Friday, September 8, 2017 6:55:45 AM EDT Jan Kara wrote: > Hello Steve, > > On Thu 07-09-17 11:47:35, Steve Grubb wrote: > > > > On Thursday, September 7, 2017 6:18:05 AM EDT Jan Kara wrote: > > > On Wed 06-09-17 13:34:32, Steve Grubb wrote: > > > > On Wednesday, September 6, 2017 12:48:21 PM EDT Jan Kara wrote: > > > > > On Wed 06-09-17 10:35:32, Steve Grubb wrote: > > > > > > On Wednesday, September 6, 2017 5:18:22 AM EDT Jan Kara wrote: > > > > > > > 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? > > > > > > > > Not really. For CC, the mechanism just needs to be available. > > > > > > > > > That seems pretty hard to do to me and even it done, it sounds like > > > > > quite a bit of duplication? > > > > > > > > AFAIK, there is only one that needs to get patched. It's totally opt > > > > in. > > > > > > I see. Thanks for explanation. But still, for this feature to make a > > > real difference, you'll have to implement FAN_AUDIT (and corresponding > > > filtering) in all programs using fanotify permission events on your > > > system, won't you? > > > > In real life, perhaps. For common criteria, the developer defines a > > baseline of applications that make up the Target Of Evaluation. One would > > normally make sure all applications that make up the TOE correctly > > implement the security features and demonstrate this with a test suite. > > So, in this case, I know of only one application that needs patching. > > > > Out of curiosity, what other applications would need patching that you > > know of? > > I've never used Audit for security certifications so I don't really know. > But I've used it several times for debugging system behavior and there it > would be handy to have all applications supported. But we have ftrace for > that these days. > > I can only imagine paranoid admin wanting to know whether his $favourite > virus scanner refused some access. And it would be nice if all such > scanners were automatically supported instead to having to add support for > each of them. But to set the flag on, the virus scanner software has to call fanotify_init and ask for it. So, I don't think there is a magic bullet. I'm not proposing a sysctl, so there is no one place for an admin to turn it on. I think that we can make the facility available and they (virus scanner developers) can opt in on their next product update. > > > > > 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? > > > > > > > > There can be a lot of information to wade through. Normally, we don't > > > > parse events in the kernel or user space. They are in a race to keep > > > > events flowing so that the kernel stays fast and responsive. There are > > > > buffer limits where if we too far behind we start losing events. The > > > > decision to log should be rare. So, if we get lots of events that > > > > don't need to be logged, it will slow down the whole kernel. > > > > > > > > But besides the performance side of it, the audit subsystem has part > > > > of the information to make a decision on whether or not this one > > > > should be logged. It doesn't know the same information as the daemon > > > > that is deciding to grant access. Only the daemon granting access > > > > knows if this one file alone should get an audit record. And based on > > > > the fanotify API there is no way to pass along additional information > > > > that could be taken into account by the audit subsystem for its > > > > decision. > > > > > > Ok, I think I'm starting to understand this. The audit event about > > > fanotify refusing the access is generally a supplemental information to > > > another event informing about access being denied, isn't it? So you want > > > to log it if and only if denied access event will be logged. Am I > > > getting it right? > > > > No, it could be when an access is granted, too. Some people are paranoid > > and may want information on approvals and denials for specific kinds of > > files or users. Again, its not a blanket policy where everything denied > > must be recorded. We should leave that decision to the admin to determine > > what he wants recorded. > > > > > So the application handling fanotify permission events would parse audit > > > rules in /etc/audit.rules, decide whether its decision would lead to > > > event being logged and if yes, it would set FAN_AUDIT in its response so > > > that supplemental information is also logged. Right? > > > > I wouldn't imagine it like that. The way I see it, the daemon that > > determines access has its own set of rules. In those rules there would be > > some syntax about attributes of the file to match against and then what > > to do if it matches. It could either be deny, approve, deny with audit, > > or approve with audit. In the case of a virus scanner, the rule is > > implicit any signature match is denial. > > > > In this way, the daemon and audit system do not need to know anything > > about each other. AppArmor and Selinux are the same way. They have their > > own rules and they decide whether or not an audit event should be > > generated. > > OK, I can see why this is interesting. But then the audit event should have > enough information to be useful on its own, shouldn't it? Because currently > it is only context-id and response, which is useless on its own... Agreed. Way back in the original email, I gave the event that is triggered: type=PATH msg=audit(1504310584.332:290): item=0 name="./evil-ls" inode=1319561 dev=fc:03 mode=0100755 ouid=1000 ogid=1000 rdev=00:00 obj=unconfined_u:object_r:user_home_t:s0 nametype=NORMAL type=CWD msg=audit(1504310584.332:290): cwd="/home/sgrubb" type=SYSCALL msg=audit(1504310584.332:290): arch=c000003e syscall=2 success=no exit=-1 a0=32cb3fca90 a1=0 a2=43 a3=8 items=1 ppid=901 pid=959 auid=1000 uid=1000 gid=1000 euid=1000 suid=1000 fsuid=1000 egid=1000 sgid=1000 fsgid=1000 tty=pts1 ses=3 comm="bash" exe="/usr/bin/bash" subj=unconfined_u:unconfined_r:unconfined_t: s0-s0:c0.c1023 key=(null) type=FANOTIFY msg=audit(1504310584.332:290): resp=2 This ^^^ is complete information. You have the user, program, file, devvice, inode, security labels, modes, owners, tty, login session, and a record indicating this is the result of fanotify. > > > One idea I had was: Couldn't we store fanotify decision in audit_context > > > and then if we find event needs to be emitted, we also additionally emit > > > the fact that fanotify is the reason? > > > > That is kind of how the patch works. When the user space daemon sees an > > access that the admin thought was important, it tags the decision wit an > > audit bit which in turn causes a call into the audit code to add an > > auxiliary record to the context and if the event has not be determined to > > be of interest to the audit system to override that and saying this is of > > interest generate the event on syscall exit. > > OK, understood. So the only place where we differ is whether the process > processing fanotify permission events decide about logging the event or > whether kernel should decide about logging on its own. My though was that > we could have something like another filesystem event type - currently we > can decide about logging reads, writes, execute, ... now we could also > decide about logging of fanotify decisions but I'm not sure whether this > would reasonably tie into audit philosophy. I don't think so. What I've got above is complete information. Wrt to logging it always, there really can be a lot of normal system activity. > So I'm still not 100% convinced putting decision about logging the event > into application is a great idea (after all we don't put burden of logging > denied access due to permissions e.g. to FUSE daemon which denied the > access) but I'm now less opposed to it ;) Hmm. That is something I haven't looked into yet. But if anything makes information flow control decisions, it is subject to needing to be auditable. One of these days I need to visit policyKit and see about adding audit there. > > > I understand the difficulty of associating fanotify response with the > > > object (and thus other audit events) from userspace. So I agree that > > > doesn't look like an easier way to go. On the other hand bear in mind > > > there can be several processes mediating access through fanotify and you > > > can end up with supplemental messages like (expanding your example): > > > > > > type=FANOTIFY msg=audit(1504310584.332:290): resp=1 > > > type=FANOTIFY msg=audit(1504310584.332:290): resp=1 > > > type=FANOTIFY msg=audit(1504310584.332:290): resp=1 > > > type=FANOTIFY msg=audit(1504310584.332:290): resp=2 > > > > > > (or possibly without the FAN_ALLOW messages - do we want to log those?) > > > and you have no way to determine which process actually denied the > > > access. I'm not sure whether this matters or not but I can imagine some > > > users complaining about this so I wanted to point that out. > > > > I was also thinking about that and I think we can add that to the event > > easily. One other thing that I think could be helpful is if the daemon > > could > > So it is easy to add inode / file where the event happened (which would be > IMHO useful if the events should be mostly standalone), adding PID of the > process whose access is mediated is easy as well (that's just the running > process in whose context we generate the event). Adding PID of the process > which decided about access is more difficult (we have only file descriptor > where we send event) however we could attach that information internally to > fanotify_event() in process_access_response() and then pick it up when > generating audit event. That would be helpful for other reasons and is along the lines of some other suggestions that I would like to make in the near future. > > also write a reason code or something like the rule number that its > > enforcing. Would that be something useful? (I could imagine a security > > officer wanting the rule association.) If so, then maybe we can carve out > > more bits of the response to be used by the daemon for a reason code? > > I'd be wary of adding blanket "reason code". Without a clear meaning there > would be inconsistencies among applications and so it would be useless. If > you have more concrete proposal, we can talk about it. I'll think about it and if I come up with something that could be widely applicable, I'll mention it. So, let's punt that one for another time. To sum things up, I think you suggested changing the memory allocation and making a flag that is passed so that the program can detect that this is supported or not. Let me know if we have a general agreement and I'll send an updated patch. Best Regards, -Steve
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index 2fa99ae..1968d21 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -9,6 +9,7 @@ #include <linux/sched/user.h> #include <linux/types.h> #include <linux/wait.h> +#include <linux/audit.h> #include "fanotify.h" @@ -78,7 +79,7 @@ static int fanotify_get_response(struct fsnotify_group *group, fsnotify_finish_user_wait(iter_info); out: /* userspace responded, convert to something usable */ - switch (event->response) { + switch (event->response & ~FAN_AUDIT) { case FAN_ALLOW: ret = 0; break; @@ -86,6 +87,11 @@ static int fanotify_get_response(struct fsnotify_group *group, default: ret = -EPERM; } + + /* Check if the response should be audited */ + if (event->response & FAN_AUDIT) + audit_fanotify(event->response & ~FAN_AUDIT); + event->response = 0; pr_debug("%s: group=%p event=%p about to return ret=%d\n", __func__, diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index 907a481..b983b5c 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -179,7 +179,7 @@ static int process_access_response(struct fsnotify_group *group, * userspace can send a valid response or we will clean it up after the * timeout */ - switch (response) { + switch (response & ~FAN_AUDIT) { case FAN_ALLOW: case FAN_DENY: break; diff --git a/include/linux/audit.h b/include/linux/audit.h index 2150bdc..bf55732 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -360,6 +360,7 @@ extern int __audit_log_bprm_fcaps(struct linux_binprm *bprm, extern void __audit_log_capset(const struct cred *new, const struct cred *old); extern void __audit_mmap_fd(int fd, int flags); extern void __audit_log_kern_module(char *name); +extern void __audit_fanotify(unsigned int response); static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp) { @@ -456,6 +457,12 @@ static inline void audit_log_kern_module(char *name) __audit_log_kern_module(name); } +static inline void audit_fanotify(unsigned int response) +{ + if (!audit_dummy_context()) + __audit_fanotify(response); +} + extern int audit_n_rules; extern int audit_signals; #else /* CONFIG_AUDITSYSCALL */ diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h index 0714a66..221f8b7 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 1331 /* Fanotify access decision */ #define AUDIT_AVC 1400 /* SE Linux avc denial or grant */ #define AUDIT_SELINUX_ERR 1401 /* Internal SE Linux Errors */ diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h index 030508d..5681e44 100644 --- a/include/uapi/linux/fanotify.h +++ b/include/uapi/linux/fanotify.h @@ -99,6 +99,8 @@ struct fanotify_response { /* Legit userspace responses to a _PERM event */ #define FAN_ALLOW 0x01 #define FAN_DENY 0x02 +#define FAN_AUDIT 0x80 /* Bit mask to create audit record for result */ + /* No fd set in event */ #define FAN_NOFD -1 diff --git a/kernel/auditsc.c b/kernel/auditsc.c index 3260ba2..1725f73 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -2390,6 +2390,12 @@ void __audit_log_kern_module(char *name) context->type = AUDIT_KERN_MODULE; } +void __audit_fanotify(unsigned int response) +{ + audit_log(current->audit_context, GFP_ATOMIC, + AUDIT_FANOTIFY, "resp=%u", response); +} + static void audit_log_task(struct audit_buffer *ab) { kuid_t auid, uid;
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. It would be used something like this in user space code: response.response = FAN_DENY | FAN_AUDIT; write(fd, &response, sizeof(struct fanotify_response)); When the syscall ends, the audit system will record the decision as a AUDIT_FANOTIFY auxiliary record to denote that the reason this event occurred is the result of an access control decision from fanotify rather than DAC or MAC policy. A sample event looks like this: type=PATH msg=audit(1504310584.332:290): item=0 name="./evil-ls" inode=1319561 dev=fc:03 mode=0100755 ouid=1000 ogid=1000 rdev=00:00 obj=unconfined_u:object_r:user_home_t:s0 nametype=NORMAL type=CWD msg=audit(1504310584.332:290): cwd="/home/sgrubb" type=SYSCALL msg=audit(1504310584.332:290): arch=c000003e syscall=2 success=no exit=-1 a0=32cb3fca90 a1=0 a2=43 a3=8 items=1 ppid=901 pid=959 auid=1000 uid=1000 gid=1000 euid=1000 suid=1000 fsuid=1000 egid=1000 sgid=1000 fsgid=1000 tty=pts1 ses=3 comm="bash" exe="/usr/bin/bash" subj=unconfined_u:unconfined_r:unconfined_t: s0-s0:c0.c1023 key=(null) type=FANOTIFY msg=audit(1504310584.332:290): resp=2 Signed-off-by: sgrubb <sgrubb@redhat.com> --- fs/notify/fanotify/fanotify.c | 8 +++++++- fs/notify/fanotify/fanotify_user.c | 2 +- include/linux/audit.h | 7 +++++++ include/uapi/linux/audit.h | 1 + include/uapi/linux/fanotify.h | 2 ++ kernel/auditsc.c | 6 ++++++ 6 files changed, 24 insertions(+), 2 deletions(-)