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

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

[Steve Grubb]

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(-)

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

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

[Paul Moore]

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



-- 
paul moore
www.paul-moore.com

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

[Richard Guy Briggs]

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

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

[Steve Grubb]

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

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

[Richard Guy Briggs]

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

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

[Jan Kara]

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

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

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

[Amir Goldstein]

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.

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

[Steve Grubb]

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

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

[Steve Grubb]

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

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

[Jan Kara]

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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Steve Grubb]

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

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

[Jan Kara]

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

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

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

[Steve Grubb]

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

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

[Jan Kara]

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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Steve Grubb]

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