[RFC PATCH 0/2] Extend AUDIT_EXE and AUDIT_DIR to more filter types

[RFC PATCH 0/2] Extend AUDIT_EXE and AUDIT_DIR to more filter types

[Ondrej Mosnacek]

This patch set extends the previous AUDIT_EXE patch by also doing a similar
thing with the AUDIT_DIR field.

I am sending it as RFC since this change requires passing audit_context to
audit_filter and I'm not sure if I should also pass it when doing the
AUDIT_FILTER_USER filtering. The call site does not have the ctx variable,
although I suppose it could be extracted from the current task somehow, but I'm
not sure if it even makes sense to use it in that place. I am not enabling
AUDIT_DIR for AUDIT_FILTER_USER in this patch, but if it makes sense I will do
that in the final patch.

Paul/Richard, please advise. See the FIXME in the second patch for the
problematic location.

Ondrej Mosnacek (2):
  audit: allow other filter list types for AUDIT_EXE
  [WIP] audit: allow other filter list types for AUDIT_DIR

 kernel/audit.c       |  5 +++--
 kernel/audit.h       | 32 +++++++++++++++++++++++++++++++-
 kernel/audit_tree.c  |  4 +++-
 kernel/auditfilter.c | 13 ++++++++++---
 kernel/auditsc.c     | 28 ----------------------------
 5 files changed, 47 insertions(+), 35 deletions(-)

-- 
2.17.0

[RFC PATCH 1/2] audit: allow other filter list types for AUDIT_EXE

[Ondrej Mosnacek]

This patch removes the restriction of the AUDIT_EXE field to only
SYSCALL filter and teaches audit_filter to recognize this field.

This makes it possible to write rule lists such as:

    auditctl -a exit,always [some general rule]
    # Filter out events with executable name /bin/exe1 or /bin/exe2:
    auditctl -a exclude,always -F exe=/bin/exe1
    auditctl -a exclude,always -F exe=/bin/exe2

See: https://github.com/linux-audit/audit-kernel/issues/54

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 kernel/auditfilter.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index eaa320148d97..6db9847ca031 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -428,8 +428,6 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
 	case AUDIT_EXE:
 		if (f->op != Audit_not_equal && f->op != Audit_equal)
 			return -EINVAL;
-		if (entry->rule.listnr != AUDIT_FILTER_EXIT)
-			return -EINVAL;
 		break;
 	}
 	return 0;
@@ -1360,6 +1358,11 @@ int audit_filter(int msgtype, unsigned int listtype)
 							f->type, f->op, f->lsm_rule, NULL);
 				}
 				break;
+			case AUDIT_EXE:
+				result = audit_exe_compare(current, e->rule.exe);
+				if (f->op == Audit_not_equal)
+					result = !result;
+				break;
 			default:
 				goto unlock_and_return;
 			}
-- 
2.17.0

[RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR

[Ondrej Mosnacek]

This patch allows the AUDIR_DIR field to be used also with the exclude
filter.

Not-yet-signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 kernel/audit.c       |  5 +++--
 kernel/audit.h       | 32 +++++++++++++++++++++++++++++++-
 kernel/audit_tree.c  |  4 +++-
 kernel/auditfilter.c |  6 +++++-
 kernel/auditsc.c     | 28 ----------------------------
 5 files changed, 42 insertions(+), 33 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index e7478cb58079..aac5b6ecc11d 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1333,7 +1333,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 		if (!audit_enabled && msg_type != AUDIT_USER_AVC)
 			return 0;
 
-		err = audit_filter(msg_type, AUDIT_FILTER_USER);
+		// FIXME: do we need to pass the context here?
+		err = audit_filter(msg_type, AUDIT_FILTER_USER, NULL);
 		if (err == 1) { /* match or error */
 			err = 0;
 			if (msg_type == AUDIT_USER_TTY) {
@@ -1754,7 +1755,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
 	if (audit_initialized != AUDIT_INITIALIZED)
 		return NULL;
 
-	if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE)))
+	if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE, ctx)))
 		return NULL;
 
 	/* NOTE: don't ever fail/sleep on these two conditions:
diff --git a/kernel/audit.h b/kernel/audit.h
index 214e14948370..43cfeba25802 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -324,13 +324,43 @@ extern void audit_kill_trees(struct list_head *list);
 #define audit_kill_trees(list) BUG()
 #endif
 
+struct audit_tree_refs {
+	struct audit_tree_refs *next;
+	struct audit_chunk *c[31];
+};
+
+/* A utility function to match tree refs: */
+static inline int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree)
+{
+#ifdef CONFIG_AUDIT_TREE
+	struct audit_tree_refs *p;
+	int n;
+	if (!tree)
+		return 0;
+	/* full ones */
+	for (p = ctx->first_trees; p != ctx->trees; p = p->next) {
+		for (n = 0; n < 31; n++)
+			if (audit_tree_match(p->c[n], tree))
+				return 1;
+	}
+	/* partial */
+	if (p) {
+		for (n = ctx->tree_count; n < 31; n++)
+			if (audit_tree_match(p->c[n], tree))
+				return 1;
+	}
+#endif
+	return 0;
+}
+
 extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len);
 
 extern pid_t audit_sig_pid;
 extern kuid_t audit_sig_uid;
 extern u32 audit_sig_sid;
 
-extern int audit_filter(int msgtype, unsigned int listtype);
+extern int audit_filter(int msgtype, unsigned int listtype,
+			struct audit_context *ctx);
 
 #ifdef CONFIG_AUDITSYSCALL
 extern int audit_signal_info(int sig, struct task_struct *t);
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 67e6956c0b61..d4d36389c3d7 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -675,9 +675,11 @@ void audit_trim_trees(void)
 
 int audit_make_tree(struct audit_krule *rule, char *pathname, u32 op)
 {
+	if (krule->listnr != AUDIT_FILTER_EXIT &&
+			krule->listnr != AUDIT_FILTER_TYPE)
+		return -EINVAL;
 
 	if (pathname[0] != '/' ||
-	    rule->listnr != AUDIT_FILTER_EXIT ||
 	    op != Audit_equal ||
 	    rule->inode_f || rule->watch || rule->tree)
 		return -EINVAL;
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 6db9847ca031..e1d70cb77ea3 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -1309,7 +1309,7 @@ int audit_compare_dname_path(const char *dname, const char *path, int parentlen)
 	return strncmp(p, dname, dlen);
 }
 
-int audit_filter(int msgtype, unsigned int listtype)
+int audit_filter(int msgtype, unsigned int listtype, struct audit_context *ctx)
 {
 	struct audit_entry *e;
 	int ret = 1; /* Audit by default */
@@ -1363,6 +1363,10 @@ int audit_filter(int msgtype, unsigned int listtype)
 				if (f->op == Audit_not_equal)
 					result = !result;
 				break;
+			case AUDIT_DIR:
+				if (ctx)
+					result = match_tree_refs(ctx, e->rule.tree);
+				break;
 			default:
 				goto unlock_and_return;
 			}
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index ceb1c4596c51..0d00b9354886 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -125,11 +125,6 @@ struct audit_aux_data_bprm_fcaps {
 	struct audit_cap_data	new_pcap;
 };
 
-struct audit_tree_refs {
-	struct audit_tree_refs *next;
-	struct audit_chunk *c[31];
-};
-
 static int audit_match_perm(struct audit_context *ctx, int mask)
 {
 	unsigned n;
@@ -286,29 +281,6 @@ static void free_tree_refs(struct audit_context *ctx)
 	}
 }
 
-static int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree)
-{
-#ifdef CONFIG_AUDIT_TREE
-	struct audit_tree_refs *p;
-	int n;
-	if (!tree)
-		return 0;
-	/* full ones */
-	for (p = ctx->first_trees; p != ctx->trees; p = p->next) {
-		for (n = 0; n < 31; n++)
-			if (audit_tree_match(p->c[n], tree))
-				return 1;
-	}
-	/* partial */
-	if (p) {
-		for (n = ctx->tree_count; n < 31; n++)
-			if (audit_tree_match(p->c[n], tree))
-				return 1;
-	}
-#endif
-	return 0;
-}
-
 static int audit_compare_uid(kuid_t uid,
 			     struct audit_names *name,
 			     struct audit_field *f,
-- 
2.17.0

Re: [RFC PATCH 1/2] audit: allow other filter list types for AUDIT_EXE

[Richard Guy Briggs]

On 2018-05-30 10:45, Ondrej Mosnacek wrote:
> This patch removes the restriction of the AUDIT_EXE field to only
> SYSCALL filter and teaches audit_filter to recognize this field.
> 
> This makes it possible to write rule lists such as:
> 
>     auditctl -a exit,always [some general rule]
>     # Filter out events with executable name /bin/exe1 or /bin/exe2:
>     auditctl -a exclude,always -F exe=/bin/exe1
>     auditctl -a exclude,always -F exe=/bin/exe2
> 
> See: https://github.com/linux-audit/audit-kernel/issues/54
> 
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

Reviewed-by: Richard Guy Briggs <rgb@redhat.com>

> ---
>  kernel/auditfilter.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index eaa320148d97..6db9847ca031 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -428,8 +428,6 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
>  	case AUDIT_EXE:
>  		if (f->op != Audit_not_equal && f->op != Audit_equal)
>  			return -EINVAL;
> -		if (entry->rule.listnr != AUDIT_FILTER_EXIT)
> -			return -EINVAL;
>  		break;
>  	}
>  	return 0;
> @@ -1360,6 +1358,11 @@ int audit_filter(int msgtype, unsigned int listtype)
>  							f->type, f->op, f->lsm_rule, NULL);
>  				}
>  				break;
> +			case AUDIT_EXE:
> +				result = audit_exe_compare(current, e->rule.exe);
> +				if (f->op == Audit_not_equal)
> +					result = !result;
> +				break;
>  			default:
>  				goto unlock_and_return;
>  			}
> -- 
> 2.17.0
> 

- 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: [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR

[Richard Guy Briggs]

On 2018-05-30 10:45, Ondrej Mosnacek wrote:
> This patch allows the AUDIR_DIR field to be used also with the exclude
> filter.
> 
> Not-yet-signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  kernel/audit.c       |  5 +++--
>  kernel/audit.h       | 32 +++++++++++++++++++++++++++++++-
>  kernel/audit_tree.c  |  4 +++-
>  kernel/auditfilter.c |  6 +++++-
>  kernel/auditsc.c     | 28 ----------------------------
>  5 files changed, 42 insertions(+), 33 deletions(-)
> 
> diff --git a/kernel/audit.c b/kernel/audit.c
> index e7478cb58079..aac5b6ecc11d 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1333,7 +1333,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>  		if (!audit_enabled && msg_type != AUDIT_USER_AVC)
>  			return 0;
>  
> -		err = audit_filter(msg_type, AUDIT_FILTER_USER);
> +		// FIXME: do we need to pass the context here?
> +		err = audit_filter(msg_type, AUDIT_FILTER_USER, NULL);
>  		if (err == 1) { /* match or error */
>  			err = 0;
>  			if (msg_type == AUDIT_USER_TTY) {
> @@ -1754,7 +1755,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
>  	if (audit_initialized != AUDIT_INITIALIZED)
>  		return NULL;
>  
> -	if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE)))
> +	if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE, ctx)))
>  		return NULL;
>  
>  	/* NOTE: don't ever fail/sleep on these two conditions:
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 214e14948370..43cfeba25802 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -324,13 +324,43 @@ extern void audit_kill_trees(struct list_head *list);
>  #define audit_kill_trees(list) BUG()
>  #endif
>  
> +struct audit_tree_refs {
> +	struct audit_tree_refs *next;
> +	struct audit_chunk *c[31];
> +};
> +
> +/* A utility function to match tree refs: */
> +static inline int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree)
> +{
> +#ifdef CONFIG_AUDIT_TREE
> +	struct audit_tree_refs *p;
> +	int n;
> +	if (!tree)
> +		return 0;
> +	/* full ones */
> +	for (p = ctx->first_trees; p != ctx->trees; p = p->next) {
> +		for (n = 0; n < 31; n++)
> +			if (audit_tree_match(p->c[n], tree))
> +				return 1;
> +	}
> +	/* partial */
> +	if (p) {
> +		for (n = ctx->tree_count; n < 31; n++)
> +			if (audit_tree_match(p->c[n], tree))
> +				return 1;
> +	}
> +#endif
> +	return 0;
> +}
> +
>  extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len);
>  
>  extern pid_t audit_sig_pid;
>  extern kuid_t audit_sig_uid;
>  extern u32 audit_sig_sid;
>  
> -extern int audit_filter(int msgtype, unsigned int listtype);
> +extern int audit_filter(int msgtype, unsigned int listtype,
> +			struct audit_context *ctx);
>  
>  #ifdef CONFIG_AUDITSYSCALL
>  extern int audit_signal_info(int sig, struct task_struct *t);
> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index 67e6956c0b61..d4d36389c3d7 100644
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
> @@ -675,9 +675,11 @@ void audit_trim_trees(void)
>  
>  int audit_make_tree(struct audit_krule *rule, char *pathname, u32 op)
>  {
> +	if (krule->listnr != AUDIT_FILTER_EXIT &&
> +			krule->listnr != AUDIT_FILTER_TYPE)
> +		return -EINVAL;
>  
>  	if (pathname[0] != '/' ||
> -	    rule->listnr != AUDIT_FILTER_EXIT ||
>  	    op != Audit_equal ||
>  	    rule->inode_f || rule->watch || rule->tree)
>  		return -EINVAL;
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 6db9847ca031..e1d70cb77ea3 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -1309,7 +1309,7 @@ int audit_compare_dname_path(const char *dname, const char *path, int parentlen)
>  	return strncmp(p, dname, dlen);
>  }
>  
> -int audit_filter(int msgtype, unsigned int listtype)
> +int audit_filter(int msgtype, unsigned int listtype, struct audit_context *ctx)
>  {
>  	struct audit_entry *e;
>  	int ret = 1; /* Audit by default */
> @@ -1363,6 +1363,10 @@ int audit_filter(int msgtype, unsigned int listtype)
>  				if (f->op == Audit_not_equal)
>  					result = !result;
>  				break;
> +			case AUDIT_DIR:
> +				if (ctx)
> +					result = match_tree_refs(ctx, e->rule.tree);

I don't see why you need to send a context to audit_filter, since the
rest of the function assumes the current task you can just use
audit_context() to hand to match_tree_refs().  You could even check that
ctx isn't NULL in match_tree_refs() to hide that code from
audit_filter().

> +				break;
>  			default:
>  				goto unlock_and_return;
>  			}
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index ceb1c4596c51..0d00b9354886 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -125,11 +125,6 @@ struct audit_aux_data_bprm_fcaps {
>  	struct audit_cap_data	new_pcap;
>  };
>  
> -struct audit_tree_refs {
> -	struct audit_tree_refs *next;
> -	struct audit_chunk *c[31];
> -};
> -
>  static int audit_match_perm(struct audit_context *ctx, int mask)
>  {
>  	unsigned n;
> @@ -286,29 +281,6 @@ static void free_tree_refs(struct audit_context *ctx)
>  	}
>  }
>  
> -static int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree)
> -{
> -#ifdef CONFIG_AUDIT_TREE
> -	struct audit_tree_refs *p;
> -	int n;
> -	if (!tree)
> -		return 0;
> -	/* full ones */
> -	for (p = ctx->first_trees; p != ctx->trees; p = p->next) {
> -		for (n = 0; n < 31; n++)
> -			if (audit_tree_match(p->c[n], tree))
> -				return 1;
> -	}
> -	/* partial */
> -	if (p) {
> -		for (n = ctx->tree_count; n < 31; n++)
> -			if (audit_tree_match(p->c[n], tree))
> -				return 1;
> -	}
> -#endif
> -	return 0;
> -}
> -

Why did you move match_tree_refs() out of auditsc.c?

>  static int audit_compare_uid(kuid_t uid,
>  			     struct audit_names *name,
>  			     struct audit_field *f,
> -- 
> 2.17.0
> 

- 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: [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR

[Ondrej Mosnacek]

2018-05-31 22:52 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>:
> On 2018-05-30 10:45, Ondrej Mosnacek wrote:
>> This patch allows the AUDIR_DIR field to be used also with the exclude
>> filter.
>>
>> Not-yet-signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>> ---
>>  kernel/audit.c       |  5 +++--
>>  kernel/audit.h       | 32 +++++++++++++++++++++++++++++++-
>>  kernel/audit_tree.c  |  4 +++-
>>  kernel/auditfilter.c |  6 +++++-
>>  kernel/auditsc.c     | 28 ----------------------------
>>  5 files changed, 42 insertions(+), 33 deletions(-)
>>
>> diff --git a/kernel/audit.c b/kernel/audit.c
>> index e7478cb58079..aac5b6ecc11d 100644
>> --- a/kernel/audit.c
>> +++ b/kernel/audit.c
>> @@ -1333,7 +1333,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>>               if (!audit_enabled && msg_type != AUDIT_USER_AVC)
>>                       return 0;
>>
>> -             err = audit_filter(msg_type, AUDIT_FILTER_USER);
>> +             // FIXME: do we need to pass the context here?
>> +             err = audit_filter(msg_type, AUDIT_FILTER_USER, NULL);
>>               if (err == 1) { /* match or error */
>>                       err = 0;
>>                       if (msg_type == AUDIT_USER_TTY) {
>> @@ -1754,7 +1755,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
>>       if (audit_initialized != AUDIT_INITIALIZED)
>>               return NULL;
>>
>> -     if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE)))
>> +     if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE, ctx)))
>>               return NULL;
>>
>>       /* NOTE: don't ever fail/sleep on these two conditions:
>> diff --git a/kernel/audit.h b/kernel/audit.h
>> index 214e14948370..43cfeba25802 100644
>> --- a/kernel/audit.h
>> +++ b/kernel/audit.h
>> @@ -324,13 +324,43 @@ extern void audit_kill_trees(struct list_head *list);
>>  #define audit_kill_trees(list) BUG()
>>  #endif
>>
>> +struct audit_tree_refs {
>> +     struct audit_tree_refs *next;
>> +     struct audit_chunk *c[31];
>> +};
>> +
>> +/* A utility function to match tree refs: */
>> +static inline int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree)
>> +{
>> +#ifdef CONFIG_AUDIT_TREE
>> +     struct audit_tree_refs *p;
>> +     int n;
>> +     if (!tree)
>> +             return 0;
>> +     /* full ones */
>> +     for (p = ctx->first_trees; p != ctx->trees; p = p->next) {
>> +             for (n = 0; n < 31; n++)
>> +                     if (audit_tree_match(p->c[n], tree))
>> +                             return 1;
>> +     }
>> +     /* partial */
>> +     if (p) {
>> +             for (n = ctx->tree_count; n < 31; n++)
>> +                     if (audit_tree_match(p->c[n], tree))
>> +                             return 1;
>> +     }
>> +#endif
>> +     return 0;
>> +}
>> +
>>  extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len);
>>
>>  extern pid_t audit_sig_pid;
>>  extern kuid_t audit_sig_uid;
>>  extern u32 audit_sig_sid;
>>
>> -extern int audit_filter(int msgtype, unsigned int listtype);
>> +extern int audit_filter(int msgtype, unsigned int listtype,
>> +                     struct audit_context *ctx);
>>
>>  #ifdef CONFIG_AUDITSYSCALL
>>  extern int audit_signal_info(int sig, struct task_struct *t);
>> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
>> index 67e6956c0b61..d4d36389c3d7 100644
>> --- a/kernel/audit_tree.c
>> +++ b/kernel/audit_tree.c
>> @@ -675,9 +675,11 @@ void audit_trim_trees(void)
>>
>>  int audit_make_tree(struct audit_krule *rule, char *pathname, u32 op)
>>  {
>> +     if (krule->listnr != AUDIT_FILTER_EXIT &&
>> +                     krule->listnr != AUDIT_FILTER_TYPE)
>> +             return -EINVAL;
>>
>>       if (pathname[0] != '/' ||
>> -         rule->listnr != AUDIT_FILTER_EXIT ||
>>           op != Audit_equal ||
>>           rule->inode_f || rule->watch || rule->tree)
>>               return -EINVAL;
>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
>> index 6db9847ca031..e1d70cb77ea3 100644
>> --- a/kernel/auditfilter.c
>> +++ b/kernel/auditfilter.c
>> @@ -1309,7 +1309,7 @@ int audit_compare_dname_path(const char *dname, const char *path, int parentlen)
>>       return strncmp(p, dname, dlen);
>>  }
>>
>> -int audit_filter(int msgtype, unsigned int listtype)
>> +int audit_filter(int msgtype, unsigned int listtype, struct audit_context *ctx)
>>  {
>>       struct audit_entry *e;
>>       int ret = 1; /* Audit by default */
>> @@ -1363,6 +1363,10 @@ int audit_filter(int msgtype, unsigned int listtype)
>>                               if (f->op == Audit_not_equal)
>>                                       result = !result;
>>                               break;
>> +                     case AUDIT_DIR:
>> +                             if (ctx)
>> +                                     result = match_tree_refs(ctx, e->rule.tree);
>
> I don't see why you need to send a context to audit_filter, since the
> rest of the function assumes the current task you can just use
> audit_context() to hand to match_tree_refs().  You could even check that
> ctx isn't NULL in match_tree_refs() to hide that code from
> audit_filter().

I wasn't sure if I could do that, since audit_filter didn't need the
context until now and it is called from two places:

audit_log_start  --  which accepts context as an argument (doesn't get
it from task, and it can be called with ctx == NULL).
audit_receive_msg  --  this function doesn't work with context at all,
so I wasn't sure if audit_filter should consider it being NULL or if
it should get it from the current task. My hunch is the second option
is the right one, but the first one is safer (AUDIT_DIR will simply
never be checked here). I don't have such insight into the logic of
audit_context's lifetime, so I need someone to tell me what makes more
sense here.

>
>> +                             break;
>>                       default:
>>                               goto unlock_and_return;
>>                       }
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index ceb1c4596c51..0d00b9354886 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -125,11 +125,6 @@ struct audit_aux_data_bprm_fcaps {
>>       struct audit_cap_data   new_pcap;
>>  };
>>
>> -struct audit_tree_refs {
>> -     struct audit_tree_refs *next;
>> -     struct audit_chunk *c[31];
>> -};
>> -
>>  static int audit_match_perm(struct audit_context *ctx, int mask)
>>  {
>>       unsigned n;
>> @@ -286,29 +281,6 @@ static void free_tree_refs(struct audit_context *ctx)
>>       }
>>  }
>>
>> -static int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree)
>> -{
>> -#ifdef CONFIG_AUDIT_TREE
>> -     struct audit_tree_refs *p;
>> -     int n;
>> -     if (!tree)
>> -             return 0;
>> -     /* full ones */
>> -     for (p = ctx->first_trees; p != ctx->trees; p = p->next) {
>> -             for (n = 0; n < 31; n++)
>> -                     if (audit_tree_match(p->c[n], tree))
>> -                             return 1;
>> -     }
>> -     /* partial */
>> -     if (p) {
>> -             for (n = ctx->tree_count; n < 31; n++)
>> -                     if (audit_tree_match(p->c[n], tree))
>> -                             return 1;
>> -     }
>> -#endif
>> -     return 0;
>> -}
>> -
>
> Why did you move match_tree_refs() out of auditsc.c?

Because now it can be called from both 'audit_filter_rules()' (defined
in auditsc.c) and 'audit_filter()' (defined in auditfilter.c).

I'm actually playing with the idea of unifying the filtering logic in
these two functions, where sharing this function wouldn't be
necessary. However, that is quite a big change (a lot of LOC being
moved around) so I'd prefer the simple & dirty approach now and keep
the cleanup for a later patch.

Thanks for the comments!

>
>>  static int audit_compare_uid(kuid_t uid,
>>                            struct audit_names *name,
>>                            struct audit_field *f,
>> --
>> 2.17.0
>>
>
> - 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

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR

[Richard Guy Briggs]

On 2018-06-01 10:12, Ondrej Mosnacek wrote:
> 2018-05-31 22:52 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>:
> > On 2018-05-30 10:45, Ondrej Mosnacek wrote:
> >> This patch allows the AUDIR_DIR field to be used also with the exclude
> >> filter.
> >>
> >> Not-yet-signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> >> ---
> >>  kernel/audit.c       |  5 +++--
> >>  kernel/audit.h       | 32 +++++++++++++++++++++++++++++++-
> >>  kernel/audit_tree.c  |  4 +++-
> >>  kernel/auditfilter.c |  6 +++++-
> >>  kernel/auditsc.c     | 28 ----------------------------
> >>  5 files changed, 42 insertions(+), 33 deletions(-)
> >>
> >> diff --git a/kernel/audit.c b/kernel/audit.c
> >> index e7478cb58079..aac5b6ecc11d 100644
> >> --- a/kernel/audit.c
> >> +++ b/kernel/audit.c
> >> @@ -1333,7 +1333,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> >>               if (!audit_enabled && msg_type != AUDIT_USER_AVC)
> >>                       return 0;
> >>
> >> -             err = audit_filter(msg_type, AUDIT_FILTER_USER);
> >> +             // FIXME: do we need to pass the context here?
> >> +             err = audit_filter(msg_type, AUDIT_FILTER_USER, NULL);
> >>               if (err == 1) { /* match or error */
> >>                       err = 0;
> >>                       if (msg_type == AUDIT_USER_TTY) {
> >> @@ -1754,7 +1755,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
> >>       if (audit_initialized != AUDIT_INITIALIZED)
> >>               return NULL;
> >>
> >> -     if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE)))
> >> +     if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE, ctx)))
> >>               return NULL;
> >>
> >>       /* NOTE: don't ever fail/sleep on these two conditions:
> >> diff --git a/kernel/audit.h b/kernel/audit.h
> >> index 214e14948370..43cfeba25802 100644
> >> --- a/kernel/audit.h
> >> +++ b/kernel/audit.h
> >> @@ -324,13 +324,43 @@ extern void audit_kill_trees(struct list_head *list);
> >>  #define audit_kill_trees(list) BUG()
> >>  #endif
> >>
> >> +struct audit_tree_refs {
> >> +     struct audit_tree_refs *next;
> >> +     struct audit_chunk *c[31];
> >> +};
> >> +
> >> +/* A utility function to match tree refs: */
> >> +static inline int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree)
> >> +{
> >> +#ifdef CONFIG_AUDIT_TREE
> >> +     struct audit_tree_refs *p;
> >> +     int n;
> >> +     if (!tree)
> >> +             return 0;
> >> +     /* full ones */
> >> +     for (p = ctx->first_trees; p != ctx->trees; p = p->next) {
> >> +             for (n = 0; n < 31; n++)
> >> +                     if (audit_tree_match(p->c[n], tree))
> >> +                             return 1;
> >> +     }
> >> +     /* partial */
> >> +     if (p) {
> >> +             for (n = ctx->tree_count; n < 31; n++)
> >> +                     if (audit_tree_match(p->c[n], tree))
> >> +                             return 1;
> >> +     }
> >> +#endif
> >> +     return 0;
> >> +}
> >> +
> >>  extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len);
> >>
> >>  extern pid_t audit_sig_pid;
> >>  extern kuid_t audit_sig_uid;
> >>  extern u32 audit_sig_sid;
> >>
> >> -extern int audit_filter(int msgtype, unsigned int listtype);
> >> +extern int audit_filter(int msgtype, unsigned int listtype,
> >> +                     struct audit_context *ctx);
> >>
> >>  #ifdef CONFIG_AUDITSYSCALL
> >>  extern int audit_signal_info(int sig, struct task_struct *t);
> >> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> >> index 67e6956c0b61..d4d36389c3d7 100644
> >> --- a/kernel/audit_tree.c
> >> +++ b/kernel/audit_tree.c
> >> @@ -675,9 +675,11 @@ void audit_trim_trees(void)
> >>
> >>  int audit_make_tree(struct audit_krule *rule, char *pathname, u32 op)
> >>  {
> >> +     if (krule->listnr != AUDIT_FILTER_EXIT &&
> >> +                     krule->listnr != AUDIT_FILTER_TYPE)
> >> +             return -EINVAL;
> >>
> >>       if (pathname[0] != '/' ||
> >> -         rule->listnr != AUDIT_FILTER_EXIT ||
> >>           op != Audit_equal ||
> >>           rule->inode_f || rule->watch || rule->tree)
> >>               return -EINVAL;
> >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> >> index 6db9847ca031..e1d70cb77ea3 100644
> >> --- a/kernel/auditfilter.c
> >> +++ b/kernel/auditfilter.c
> >> @@ -1309,7 +1309,7 @@ int audit_compare_dname_path(const char *dname, const char *path, int parentlen)
> >>       return strncmp(p, dname, dlen);
> >>  }
> >>
> >> -int audit_filter(int msgtype, unsigned int listtype)
> >> +int audit_filter(int msgtype, unsigned int listtype, struct audit_context *ctx)
> >>  {
> >>       struct audit_entry *e;
> >>       int ret = 1; /* Audit by default */
> >> @@ -1363,6 +1363,10 @@ int audit_filter(int msgtype, unsigned int listtype)
> >>                               if (f->op == Audit_not_equal)
> >>                                       result = !result;
> >>                               break;
> >> +                     case AUDIT_DIR:
> >> +                             if (ctx)
> >> +                                     result = match_tree_refs(ctx, e->rule.tree);
> >
> > I don't see why you need to send a context to audit_filter, since the
> > rest of the function assumes the current task you can just use
> > audit_context() to hand to match_tree_refs().  You could even check that
> > ctx isn't NULL in match_tree_refs() to hide that code from
> > audit_filter().
> 
> I wasn't sure if I could do that, since audit_filter didn't need the
> context until now and it is called from two places:
> 
> audit_log_start  --  which accepts context as an argument (doesn't get
> it from task, and it can be called with ctx == NULL).

Alright, if current has no context, then match_tree_refs() could test
for a NULL context and return since there are no trees to check.  Having
said that, there are cases where audit_log_start() is deliberately
handed a NULL context which should be honoured.  It appears we don't
have a choice but to pass in the context.

> audit_receive_msg  --  this function doesn't work with context at all,
> so I wasn't sure if audit_filter should consider it being NULL or if
> it should get it from the current task. My hunch is the second option
> is the right one, but the first one is safer (AUDIT_DIR will simply
> never be checked here). I don't have such insight into the logic of
> audit_context's lifetime, so I need someone to tell me what makes more
> sense here.

That is starting to work with context.  The recent FEATURE_CHANGE patch
to connect records of the same event uses current->audit_context (now
audit_context()) from audit_log_feature_change() called from
audit_set_feature() called from audit_receive_msg().

There is also a work in progress to convert all the CONFIG_CHANGE
records over.  I'm just trying to track down all the instances.

> >> +                             break;
> >>                       default:
> >>                               goto unlock_and_return;
> >>                       }
> >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> >> index ceb1c4596c51..0d00b9354886 100644
> >> --- a/kernel/auditsc.c
> >> +++ b/kernel/auditsc.c
> >> @@ -125,11 +125,6 @@ struct audit_aux_data_bprm_fcaps {
> >>       struct audit_cap_data   new_pcap;
> >>  };
> >>
> >> -struct audit_tree_refs {
> >> -     struct audit_tree_refs *next;
> >> -     struct audit_chunk *c[31];
> >> -};
> >> -
> >>  static int audit_match_perm(struct audit_context *ctx, int mask)
> >>  {
> >>       unsigned n;
> >> @@ -286,29 +281,6 @@ static void free_tree_refs(struct audit_context *ctx)
> >>       }
> >>  }
> >>
> >> -static int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree)
> >> -{
> >> -#ifdef CONFIG_AUDIT_TREE
> >> -     struct audit_tree_refs *p;
> >> -     int n;
> >> -     if (!tree)
> >> -             return 0;
> >> -     /* full ones */
> >> -     for (p = ctx->first_trees; p != ctx->trees; p = p->next) {
> >> -             for (n = 0; n < 31; n++)
> >> -                     if (audit_tree_match(p->c[n], tree))
> >> -                             return 1;
> >> -     }
> >> -     /* partial */
> >> -     if (p) {
> >> -             for (n = ctx->tree_count; n < 31; n++)
> >> -                     if (audit_tree_match(p->c[n], tree))
> >> -                             return 1;
> >> -     }
> >> -#endif
> >> -     return 0;
> >> -}
> >> -
> >
> > Why did you move match_tree_refs() out of auditsc.c?
> 
> Because now it can be called from both 'audit_filter_rules()' (defined
> in auditsc.c) and 'audit_filter()' (defined in auditfilter.c).

So since kernel/audit.h is included in all kernel/audit*.c files, having
the prototype in there from any kernel/audit*.c file should make the
function available to all functions in kernel/audit*.c files.

> I'm actually playing with the idea of unifying the filtering logic in
> these two functions, where sharing this function wouldn't be
> necessary. However, that is quite a big change (a lot of LOC being
> moved around) so I'd prefer the simple & dirty approach now and keep
> the cleanup for a later patch.

Some of the filtering has already been refactored and merged.  More
wouldn't hurt.

> Thanks for the comments!
> 
> >>  static int audit_compare_uid(kuid_t uid,
> >>                            struct audit_names *name,
> >>                            struct audit_field *f,
> >> --
> >> 2.17.0
> >>
> >
> > - 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
> 
> -- 
> Ondrej Mosnacek <omosnace at redhat dot com>
> Associate Software Engineer, Security Technologies
> Red Hat, Inc.

- 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: [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR

[Ondrej Mosnacek]

2018-06-01 22:05 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>:
> On 2018-06-01 10:12, Ondrej Mosnacek wrote:
>> 2018-05-31 22:52 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>:
>> > On 2018-05-30 10:45, Ondrej Mosnacek wrote:
>> >> This patch allows the AUDIR_DIR field to be used also with the exclude
>> >> filter.
>> >>
>> >> Not-yet-signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>> >> ---
>> >>  kernel/audit.c       |  5 +++--
>> >>  kernel/audit.h       | 32 +++++++++++++++++++++++++++++++-
>> >>  kernel/audit_tree.c  |  4 +++-
>> >>  kernel/auditfilter.c |  6 +++++-
>> >>  kernel/auditsc.c     | 28 ----------------------------
>> >>  5 files changed, 42 insertions(+), 33 deletions(-)
>> >>
>> >> diff --git a/kernel/audit.c b/kernel/audit.c
>> >> index e7478cb58079..aac5b6ecc11d 100644
>> >> --- a/kernel/audit.c
>> >> +++ b/kernel/audit.c
>> >> @@ -1333,7 +1333,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>> >>               if (!audit_enabled && msg_type != AUDIT_USER_AVC)
>> >>                       return 0;
>> >>
>> >> -             err = audit_filter(msg_type, AUDIT_FILTER_USER);
>> >> +             // FIXME: do we need to pass the context here?
>> >> +             err = audit_filter(msg_type, AUDIT_FILTER_USER, NULL);
>> >>               if (err == 1) { /* match or error */
>> >>                       err = 0;
>> >>                       if (msg_type == AUDIT_USER_TTY) {
>> >> @@ -1754,7 +1755,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
>> >>       if (audit_initialized != AUDIT_INITIALIZED)
>> >>               return NULL;
>> >>
>> >> -     if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE)))
>> >> +     if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE, ctx)))
>> >>               return NULL;
>> >>
>> >>       /* NOTE: don't ever fail/sleep on these two conditions:
>> >> diff --git a/kernel/audit.h b/kernel/audit.h
>> >> index 214e14948370..43cfeba25802 100644
>> >> --- a/kernel/audit.h
>> >> +++ b/kernel/audit.h
>> >> @@ -324,13 +324,43 @@ extern void audit_kill_trees(struct list_head *list);
>> >>  #define audit_kill_trees(list) BUG()
>> >>  #endif
>> >>
>> >> +struct audit_tree_refs {
>> >> +     struct audit_tree_refs *next;
>> >> +     struct audit_chunk *c[31];
>> >> +};
>> >> +
>> >> +/* A utility function to match tree refs: */
>> >> +static inline int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree)
>> >> +{
>> >> +#ifdef CONFIG_AUDIT_TREE
>> >> +     struct audit_tree_refs *p;
>> >> +     int n;
>> >> +     if (!tree)
>> >> +             return 0;
>> >> +     /* full ones */
>> >> +     for (p = ctx->first_trees; p != ctx->trees; p = p->next) {
>> >> +             for (n = 0; n < 31; n++)
>> >> +                     if (audit_tree_match(p->c[n], tree))
>> >> +                             return 1;
>> >> +     }
>> >> +     /* partial */
>> >> +     if (p) {
>> >> +             for (n = ctx->tree_count; n < 31; n++)
>> >> +                     if (audit_tree_match(p->c[n], tree))
>> >> +                             return 1;
>> >> +     }
>> >> +#endif
>> >> +     return 0;
>> >> +}
>> >> +
>> >>  extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len);
>> >>
>> >>  extern pid_t audit_sig_pid;
>> >>  extern kuid_t audit_sig_uid;
>> >>  extern u32 audit_sig_sid;
>> >>
>> >> -extern int audit_filter(int msgtype, unsigned int listtype);
>> >> +extern int audit_filter(int msgtype, unsigned int listtype,
>> >> +                     struct audit_context *ctx);
>> >>
>> >>  #ifdef CONFIG_AUDITSYSCALL
>> >>  extern int audit_signal_info(int sig, struct task_struct *t);
>> >> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
>> >> index 67e6956c0b61..d4d36389c3d7 100644
>> >> --- a/kernel/audit_tree.c
>> >> +++ b/kernel/audit_tree.c
>> >> @@ -675,9 +675,11 @@ void audit_trim_trees(void)
>> >>
>> >>  int audit_make_tree(struct audit_krule *rule, char *pathname, u32 op)
>> >>  {
>> >> +     if (krule->listnr != AUDIT_FILTER_EXIT &&
>> >> +                     krule->listnr != AUDIT_FILTER_TYPE)
>> >> +             return -EINVAL;
>> >>
>> >>       if (pathname[0] != '/' ||
>> >> -         rule->listnr != AUDIT_FILTER_EXIT ||
>> >>           op != Audit_equal ||
>> >>           rule->inode_f || rule->watch || rule->tree)
>> >>               return -EINVAL;
>> >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
>> >> index 6db9847ca031..e1d70cb77ea3 100644
>> >> --- a/kernel/auditfilter.c
>> >> +++ b/kernel/auditfilter.c
>> >> @@ -1309,7 +1309,7 @@ int audit_compare_dname_path(const char *dname, const char *path, int parentlen)
>> >>       return strncmp(p, dname, dlen);
>> >>  }
>> >>
>> >> -int audit_filter(int msgtype, unsigned int listtype)
>> >> +int audit_filter(int msgtype, unsigned int listtype, struct audit_context *ctx)
>> >>  {
>> >>       struct audit_entry *e;
>> >>       int ret = 1; /* Audit by default */
>> >> @@ -1363,6 +1363,10 @@ int audit_filter(int msgtype, unsigned int listtype)
>> >>                               if (f->op == Audit_not_equal)
>> >>                                       result = !result;
>> >>                               break;
>> >> +                     case AUDIT_DIR:
>> >> +                             if (ctx)
>> >> +                                     result = match_tree_refs(ctx, e->rule.tree);
>> >
>> > I don't see why you need to send a context to audit_filter, since the
>> > rest of the function assumes the current task you can just use
>> > audit_context() to hand to match_tree_refs().  You could even check that
>> > ctx isn't NULL in match_tree_refs() to hide that code from
>> > audit_filter().
>>
>> I wasn't sure if I could do that, since audit_filter didn't need the
>> context until now and it is called from two places:
>>
>> audit_log_start  --  which accepts context as an argument (doesn't get
>> it from task, and it can be called with ctx == NULL).
>
> Alright, if current has no context, then match_tree_refs() could test
> for a NULL context and return since there are no trees to check.  Having
> said that, there are cases where audit_log_start() is deliberately
> handed a NULL context which should be honoured.  It appears we don't
> have a choice but to pass in the context.

OK, so I'll leave that part as it is.

>
>> audit_receive_msg  --  this function doesn't work with context at all,
>> so I wasn't sure if audit_filter should consider it being NULL or if
>> it should get it from the current task. My hunch is the second option
>> is the right one, but the first one is safer (AUDIT_DIR will simply
>> never be checked here). I don't have such insight into the logic of
>> audit_context's lifetime, so I need someone to tell me what makes more
>> sense here.
>
> That is starting to work with context.  The recent FEATURE_CHANGE patch
> to connect records of the same event uses current->audit_context (now
> audit_context()) from audit_log_feature_change() called from
> audit_set_feature() called from audit_receive_msg().
>
> There is also a work in progress to convert all the CONFIG_CHANGE
> records over.  I'm just trying to track down all the instances.

OK, so does that mean that I should just pass audit_context() instead
of NULL in audit_receive_msg()? The part that calls audit_filter()
only deals with user messages
(AUDIT_USER/AUDIT_FIRST..LAST_USER_MESSAGE) -- does it make sense to
use the task's context in AUDIT_FILTER_USER (right now just with
AUDIT_DIR)? It already filters based on the task's credentials, so
perhaps it does, but I don't know...

>
>> >> +                             break;
>> >>                       default:
>> >>                               goto unlock_and_return;
>> >>                       }
>> >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> >> index ceb1c4596c51..0d00b9354886 100644
>> >> --- a/kernel/auditsc.c
>> >> +++ b/kernel/auditsc.c
>> >> @@ -125,11 +125,6 @@ struct audit_aux_data_bprm_fcaps {
>> >>       struct audit_cap_data   new_pcap;
>> >>  };
>> >>
>> >> -struct audit_tree_refs {
>> >> -     struct audit_tree_refs *next;
>> >> -     struct audit_chunk *c[31];
>> >> -};
>> >> -
>> >>  static int audit_match_perm(struct audit_context *ctx, int mask)
>> >>  {
>> >>       unsigned n;
>> >> @@ -286,29 +281,6 @@ static void free_tree_refs(struct audit_context *ctx)
>> >>       }
>> >>  }
>> >>
>> >> -static int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree)
>> >> -{
>> >> -#ifdef CONFIG_AUDIT_TREE
>> >> -     struct audit_tree_refs *p;
>> >> -     int n;
>> >> -     if (!tree)
>> >> -             return 0;
>> >> -     /* full ones */
>> >> -     for (p = ctx->first_trees; p != ctx->trees; p = p->next) {
>> >> -             for (n = 0; n < 31; n++)
>> >> -                     if (audit_tree_match(p->c[n], tree))
>> >> -                             return 1;
>> >> -     }
>> >> -     /* partial */
>> >> -     if (p) {
>> >> -             for (n = ctx->tree_count; n < 31; n++)
>> >> -                     if (audit_tree_match(p->c[n], tree))
>> >> -                             return 1;
>> >> -     }
>> >> -#endif
>> >> -     return 0;
>> >> -}
>> >> -
>> >
>> > Why did you move match_tree_refs() out of auditsc.c?
>>
>> Because now it can be called from both 'audit_filter_rules()' (defined
>> in auditsc.c) and 'audit_filter()' (defined in auditfilter.c).
>
> So since kernel/audit.h is included in all kernel/audit*.c files, having
> the prototype in there from any kernel/audit*.c file should make the
> function available to all functions in kernel/audit*.c files.

You're right, I could have just added a declaration and export the
function from auditsc.c... I will need to add a fallback definition
for when CONFIG_AUDITSYSCALL is not enabled, but in that case tasks
never have audit_context set anyway, so it's fine. I will just export
the function in the next version.

>
>> I'm actually playing with the idea of unifying the filtering logic in
>> these two functions, where sharing this function wouldn't be
>> necessary. However, that is quite a big change (a lot of LOC being
>> moved around) so I'd prefer the simple & dirty approach now and keep
>> the cleanup for a later patch.
>
> Some of the filtering has already been refactored and merged.  More
> wouldn't hurt.
>
>> Thanks for the comments!
>>
>> >>  static int audit_compare_uid(kuid_t uid,
>> >>                            struct audit_names *name,
>> >>                            struct audit_field *f,
>> >> --
>> >> 2.17.0
>> >>
>> >
>> > - 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
>>
>> --
>> Ondrej Mosnacek <omosnace at redhat dot com>
>> Associate Software Engineer, Security Technologies
>> Red Hat, Inc.
>
> - 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

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [RFC PATCH 1/2] audit: allow other filter list types for AUDIT_EXE

[Paul Moore]

On Wed, May 30, 2018 at 4:45 AM, Ondrej Mosnacek <omosnace@redhat.com> wrote:
> This patch removes the restriction of the AUDIT_EXE field to only
> SYSCALL filter and teaches audit_filter to recognize this field.
>
> This makes it possible to write rule lists such as:
>
>     auditctl -a exit,always [some general rule]
>     # Filter out events with executable name /bin/exe1 or /bin/exe2:
>     auditctl -a exclude,always -F exe=/bin/exe1
>     auditctl -a exclude,always -F exe=/bin/exe2
>
> See: https://github.com/linux-audit/audit-kernel/issues/54
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  kernel/auditfilter.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Thanks for your patience Ondrej.

Having reflected a bit on things from the recent IMA audit discussion,
my current thinking is to go ahead and merge this patch into
audit/next once the merge window closes.

> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index eaa320148d97..6db9847ca031 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -428,8 +428,6 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
>         case AUDIT_EXE:
>                 if (f->op != Audit_not_equal && f->op != Audit_equal)
>                         return -EINVAL;
> -               if (entry->rule.listnr != AUDIT_FILTER_EXIT)
> -                       return -EINVAL;
>                 break;
>         }
>         return 0;
> @@ -1360,6 +1358,11 @@ int audit_filter(int msgtype, unsigned int listtype)
>                                                         f->type, f->op, f->lsm_rule, NULL);
>                                 }
>                                 break;
> +                       case AUDIT_EXE:
> +                               result = audit_exe_compare(current, e->rule.exe);
> +                               if (f->op == Audit_not_equal)
> +                                       result = !result;
> +                               break;
>                         default:
>                                 goto unlock_and_return;
>                         }
> --
> 2.17.0

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR

[Richard Guy Briggs]

On 2018-06-04 13:32, Ondrej Mosnacek wrote:
> 2018-06-01 22:05 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>:
> > On 2018-06-01 10:12, Ondrej Mosnacek wrote:
> >> 2018-05-31 22:52 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>:
> >> > On 2018-05-30 10:45, Ondrej Mosnacek wrote:
> >> >> This patch allows the AUDIR_DIR field to be used also with the exclude
> >> >> filter.
> >> >>
> >> >> Not-yet-signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> >> >> ---
> >> >>  kernel/audit.c       |  5 +++--
> >> >>  kernel/audit.h       | 32 +++++++++++++++++++++++++++++++-
> >> >>  kernel/audit_tree.c  |  4 +++-
> >> >>  kernel/auditfilter.c |  6 +++++-
> >> >>  kernel/auditsc.c     | 28 ----------------------------
> >> >>  5 files changed, 42 insertions(+), 33 deletions(-)
> >> >>
> >> >> diff --git a/kernel/audit.c b/kernel/audit.c
> >> >> index e7478cb58079..aac5b6ecc11d 100644
> >> >> --- a/kernel/audit.c
> >> >> +++ b/kernel/audit.c
> >> >> @@ -1333,7 +1333,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> >> >>               if (!audit_enabled && msg_type != AUDIT_USER_AVC)
> >> >>                       return 0;
> >> >>
> >> >> -             err = audit_filter(msg_type, AUDIT_FILTER_USER);
> >> >> +             // FIXME: do we need to pass the context here?
> >> >> +             err = audit_filter(msg_type, AUDIT_FILTER_USER, NULL);
> >> >>               if (err == 1) { /* match or error */
> >> >>                       err = 0;
> >> >>                       if (msg_type == AUDIT_USER_TTY) {
> >> >> @@ -1754,7 +1755,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
> >> >>       if (audit_initialized != AUDIT_INITIALIZED)
> >> >>               return NULL;
> >> >>
> >> >> -     if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE)))
> >> >> +     if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE, ctx)))
> >> >>               return NULL;
> >> >>
> >> >>       /* NOTE: don't ever fail/sleep on these two conditions:
> >> >> diff --git a/kernel/audit.h b/kernel/audit.h
> >> >> index 214e14948370..43cfeba25802 100644
> >> >> --- a/kernel/audit.h
> >> >> +++ b/kernel/audit.h
> >> >> @@ -324,13 +324,43 @@ extern void audit_kill_trees(struct list_head *list);
> >> >>  #define audit_kill_trees(list) BUG()
> >> >>  #endif
> >> >>
> >> >> +struct audit_tree_refs {
> >> >> +     struct audit_tree_refs *next;
> >> >> +     struct audit_chunk *c[31];
> >> >> +};
> >> >> +
> >> >> +/* A utility function to match tree refs: */
> >> >> +static inline int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree)
> >> >> +{
> >> >> +#ifdef CONFIG_AUDIT_TREE
> >> >> +     struct audit_tree_refs *p;
> >> >> +     int n;
> >> >> +     if (!tree)
> >> >> +             return 0;
> >> >> +     /* full ones */
> >> >> +     for (p = ctx->first_trees; p != ctx->trees; p = p->next) {
> >> >> +             for (n = 0; n < 31; n++)
> >> >> +                     if (audit_tree_match(p->c[n], tree))
> >> >> +                             return 1;
> >> >> +     }
> >> >> +     /* partial */
> >> >> +     if (p) {
> >> >> +             for (n = ctx->tree_count; n < 31; n++)
> >> >> +                     if (audit_tree_match(p->c[n], tree))
> >> >> +                             return 1;
> >> >> +     }
> >> >> +#endif
> >> >> +     return 0;
> >> >> +}
> >> >> +
> >> >>  extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len);
> >> >>
> >> >>  extern pid_t audit_sig_pid;
> >> >>  extern kuid_t audit_sig_uid;
> >> >>  extern u32 audit_sig_sid;
> >> >>
> >> >> -extern int audit_filter(int msgtype, unsigned int listtype);
> >> >> +extern int audit_filter(int msgtype, unsigned int listtype,
> >> >> +                     struct audit_context *ctx);
> >> >>
> >> >>  #ifdef CONFIG_AUDITSYSCALL
> >> >>  extern int audit_signal_info(int sig, struct task_struct *t);
> >> >> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> >> >> index 67e6956c0b61..d4d36389c3d7 100644
> >> >> --- a/kernel/audit_tree.c
> >> >> +++ b/kernel/audit_tree.c
> >> >> @@ -675,9 +675,11 @@ void audit_trim_trees(void)
> >> >>
> >> >>  int audit_make_tree(struct audit_krule *rule, char *pathname, u32 op)
> >> >>  {
> >> >> +     if (krule->listnr != AUDIT_FILTER_EXIT &&
> >> >> +                     krule->listnr != AUDIT_FILTER_TYPE)
> >> >> +             return -EINVAL;
> >> >>
> >> >>       if (pathname[0] != '/' ||
> >> >> -         rule->listnr != AUDIT_FILTER_EXIT ||
> >> >>           op != Audit_equal ||
> >> >>           rule->inode_f || rule->watch || rule->tree)
> >> >>               return -EINVAL;
> >> >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> >> >> index 6db9847ca031..e1d70cb77ea3 100644
> >> >> --- a/kernel/auditfilter.c
> >> >> +++ b/kernel/auditfilter.c
> >> >> @@ -1309,7 +1309,7 @@ int audit_compare_dname_path(const char *dname, const char *path, int parentlen)
> >> >>       return strncmp(p, dname, dlen);
> >> >>  }
> >> >>
> >> >> -int audit_filter(int msgtype, unsigned int listtype)
> >> >> +int audit_filter(int msgtype, unsigned int listtype, struct audit_context *ctx)
> >> >>  {
> >> >>       struct audit_entry *e;
> >> >>       int ret = 1; /* Audit by default */
> >> >> @@ -1363,6 +1363,10 @@ int audit_filter(int msgtype, unsigned int listtype)
> >> >>                               if (f->op == Audit_not_equal)
> >> >>                                       result = !result;
> >> >>                               break;
> >> >> +                     case AUDIT_DIR:
> >> >> +                             if (ctx)
> >> >> +                                     result = match_tree_refs(ctx, e->rule.tree);
> >> >
> >> > I don't see why you need to send a context to audit_filter, since the
> >> > rest of the function assumes the current task you can just use
> >> > audit_context() to hand to match_tree_refs().  You could even check that
> >> > ctx isn't NULL in match_tree_refs() to hide that code from
> >> > audit_filter().
> >>
> >> I wasn't sure if I could do that, since audit_filter didn't need the
> >> context until now and it is called from two places:
> >>
> >> audit_log_start  --  which accepts context as an argument (doesn't get
> >> it from task, and it can be called with ctx == NULL).
> >
> > Alright, if current has no context, then match_tree_refs() could test
> > for a NULL context and return since there are no trees to check.  Having
> > said that, there are cases where audit_log_start() is deliberately
> > handed a NULL context which should be honoured.  It appears we don't
> > have a choice but to pass in the context.
> 
> OK, so I'll leave that part as it is.
> 
> >
> >> audit_receive_msg  --  this function doesn't work with context at all,
> >> so I wasn't sure if audit_filter should consider it being NULL or if
> >> it should get it from the current task. My hunch is the second option
> >> is the right one, but the first one is safer (AUDIT_DIR will simply
> >> never be checked here). I don't have such insight into the logic of
> >> audit_context's lifetime, so I need someone to tell me what makes more
> >> sense here.
> >
> > That is starting to work with context.  The recent FEATURE_CHANGE patch
> > to connect records of the same event uses current->audit_context (now
> > audit_context()) from audit_log_feature_change() called from
> > audit_set_feature() called from audit_receive_msg().
> >
> > There is also a work in progress to convert all the CONFIG_CHANGE
> > records over.  I'm just trying to track down all the instances.
> 
> OK, so does that mean that I should just pass audit_context() instead
> of NULL in audit_receive_msg()?

Reluctantly, yes.

> The part that calls audit_filter()
> only deals with user messages
> (AUDIT_USER/AUDIT_FIRST..LAST_USER_MESSAGE) -- does it make sense to
> use the task's context in AUDIT_FILTER_USER (right now just with
> AUDIT_DIR)? It already filters based on the task's credentials, so
> perhaps it does, but I don't know...

I've got opinions both ways on USER messages.  I don't see the
additional information value of linking USER messages to SYSCALL
records, but I do see the single event consolidation advantage.

> >> >> +                             break;
> >> >>                       default:
> >> >>                               goto unlock_and_return;
> >> >>                       }
> >> >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> >> >> index ceb1c4596c51..0d00b9354886 100644
> >> >> --- a/kernel/auditsc.c
> >> >> +++ b/kernel/auditsc.c
> >> >> @@ -125,11 +125,6 @@ struct audit_aux_data_bprm_fcaps {
> >> >>       struct audit_cap_data   new_pcap;
> >> >>  };
> >> >>
> >> >> -struct audit_tree_refs {
> >> >> -     struct audit_tree_refs *next;
> >> >> -     struct audit_chunk *c[31];
> >> >> -};
> >> >> -
> >> >>  static int audit_match_perm(struct audit_context *ctx, int mask)
> >> >>  {
> >> >>       unsigned n;
> >> >> @@ -286,29 +281,6 @@ static void free_tree_refs(struct audit_context *ctx)
> >> >>       }
> >> >>  }
> >> >>
> >> >> -static int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree)
> >> >> -{
> >> >> -#ifdef CONFIG_AUDIT_TREE
> >> >> -     struct audit_tree_refs *p;
> >> >> -     int n;
> >> >> -     if (!tree)
> >> >> -             return 0;
> >> >> -     /* full ones */
> >> >> -     for (p = ctx->first_trees; p != ctx->trees; p = p->next) {
> >> >> -             for (n = 0; n < 31; n++)
> >> >> -                     if (audit_tree_match(p->c[n], tree))
> >> >> -                             return 1;
> >> >> -     }
> >> >> -     /* partial */
> >> >> -     if (p) {
> >> >> -             for (n = ctx->tree_count; n < 31; n++)
> >> >> -                     if (audit_tree_match(p->c[n], tree))
> >> >> -                             return 1;
> >> >> -     }
> >> >> -#endif
> >> >> -     return 0;
> >> >> -}
> >> >> -
> >> >
> >> > Why did you move match_tree_refs() out of auditsc.c?
> >>
> >> Because now it can be called from both 'audit_filter_rules()' (defined
> >> in auditsc.c) and 'audit_filter()' (defined in auditfilter.c).
> >
> > So since kernel/audit.h is included in all kernel/audit*.c files, having
> > the prototype in there from any kernel/audit*.c file should make the
> > function available to all functions in kernel/audit*.c files.
> 
> You're right, I could have just added a declaration and export the
> function from auditsc.c... I will need to add a fallback definition
> for when CONFIG_AUDITSYSCALL is not enabled, but in that case tasks
> never have audit_context set anyway, so it's fine. I will just export
> the function in the next version.
> 
> >
> >> I'm actually playing with the idea of unifying the filtering logic in
> >> these two functions, where sharing this function wouldn't be
> >> necessary. However, that is quite a big change (a lot of LOC being
> >> moved around) so I'd prefer the simple & dirty approach now and keep
> >> the cleanup for a later patch.
> >
> > Some of the filtering has already been refactored and merged.  More
> > wouldn't hurt.
> >
> >> Thanks for the comments!
> >>
> >> >>  static int audit_compare_uid(kuid_t uid,
> >> >>                            struct audit_names *name,
> >> >>                            struct audit_field *f,
> >> >> --
> >> >> 2.17.0
> >> >>
> >> >
> >> > - 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
> >>
> >> --
> >> Ondrej Mosnacek <omosnace at redhat dot com>
> >> Associate Software Engineer, Security Technologies
> >> Red Hat, Inc.
> >
> > - 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
> 
> -- 
> Ondrej Mosnacek <omosnace at redhat dot com>
> Associate Software Engineer, Security Technologies
> Red Hat, Inc.

- 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: [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR

[Paul Moore]

On Fri, Jun 1, 2018 at 4:12 AM, Ondrej Mosnacek <omosnace@redhat.com> wrote:
> I'm actually playing with the idea of unifying the filtering logic in
> these two functions, where sharing this function wouldn't be
> necessary. However, that is quite a big change (a lot of LOC being
> moved around) so I'd prefer the simple & dirty approach now and keep
> the cleanup for a later patch.

[Resend as I forgot the reply-all - oops]

I've had similar thoughts in the past, and deferred the work for
exactly the same reason.

If I recall correctly, I believe part of the reason may stem from the
fact that some fields are simply not always valid when the filter is
run and this may have been a crude effort at optimization (smaller
function size).  Regardless, we might preserve some of this idea by
creating some helper functions (two?) with the different filter fields
in each (no overlap) and a couple of wrapper functions which call the
appropriate helpers ... or that just might be extra work for no noticeable
perf advantage :)

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR

[Paul Moore]

On Fri, Jun 1, 2018 at 4:05 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-06-01 10:12, Ondrej Mosnacek wrote:

...

>> audit_receive_msg  --  this function doesn't work with context at all,
>> so I wasn't sure if audit_filter should consider it being NULL or if
>> it should get it from the current task. My hunch is the second option
>> is the right one, but the first one is safer (AUDIT_DIR will simply
>> never be checked here). I don't have such insight into the logic of
>> audit_context's lifetime, so I need someone to tell me what makes more
>> sense here.

Given the nature of audit_receive_msg(), would it ever match on an
AUDIT_DIR field?  I don't think it would since there aren't really any
vfs accesses that occur in the source of sending a netlink message
down to the kernel ... am I missing something?

> That is starting to work with context.  The recent FEATURE_CHANGE patch
> to connect records of the same event uses current->audit_context (now
> audit_context()) from audit_log_feature_change() called from
> audit_set_feature() called from audit_receive_msg().
>
> There is also a work in progress to convert all the CONFIG_CHANGE
> records over.  I'm just trying to track down all the instances.

This will be a nice improvement.

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH 1/2] audit: allow other filter list types for AUDIT_EXE

[Ondrej Mosnacek]

2018-06-04 22:41 GMT+02:00 Paul Moore <paul@paul-moore.com>:
> On Wed, May 30, 2018 at 4:45 AM, Ondrej Mosnacek <omosnace@redhat.com> wrote:
>> This patch removes the restriction of the AUDIT_EXE field to only
>> SYSCALL filter and teaches audit_filter to recognize this field.
>>
>> This makes it possible to write rule lists such as:
>>
>>     auditctl -a exit,always [some general rule]
>>     # Filter out events with executable name /bin/exe1 or /bin/exe2:
>>     auditctl -a exclude,always -F exe=/bin/exe1
>>     auditctl -a exclude,always -F exe=/bin/exe2
>>
>> See: https://github.com/linux-audit/audit-kernel/issues/54
>>
>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>> ---
>>  kernel/auditfilter.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> Thanks for your patience Ondrej.
>
> Having reflected a bit on things from the recent IMA audit discussion,
> my current thinking is to go ahead and merge this patch into
> audit/next once the merge window closes.

OK, feel free to merge it independently of the DIR patch, I sent them
in series because they need to be applied in order (otherwise there
would be merge conflicts).

>
>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
>> index eaa320148d97..6db9847ca031 100644
>> --- a/kernel/auditfilter.c
>> +++ b/kernel/auditfilter.c
>> @@ -428,8 +428,6 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
>>         case AUDIT_EXE:
>>                 if (f->op != Audit_not_equal && f->op != Audit_equal)
>>                         return -EINVAL;
>> -               if (entry->rule.listnr != AUDIT_FILTER_EXIT)
>> -                       return -EINVAL;
>>                 break;
>>         }
>>         return 0;
>> @@ -1360,6 +1358,11 @@ int audit_filter(int msgtype, unsigned int listtype)
>>                                                         f->type, f->op, f->lsm_rule, NULL);
>>                                 }
>>                                 break;
>> +                       case AUDIT_EXE:
>> +                               result = audit_exe_compare(current, e->rule.exe);
>> +                               if (f->op == Audit_not_equal)
>> +                                       result = !result;
>> +                               break;
>>                         default:
>>                                 goto unlock_and_return;
>>                         }
>> --
>> 2.17.0
>
> --
> paul moore
> www.paul-moore.com



-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR

[Ondrej Mosnacek]

2018-06-05 0:19 GMT+02:00 Paul Moore <paul@paul-moore.com>:
> On Fri, Jun 1, 2018 at 4:05 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> On 2018-06-01 10:12, Ondrej Mosnacek wrote:
>
> ...
>
>>> audit_receive_msg  --  this function doesn't work with context at all,
>>> so I wasn't sure if audit_filter should consider it being NULL or if
>>> it should get it from the current task. My hunch is the second option
>>> is the right one, but the first one is safer (AUDIT_DIR will simply
>>> never be checked here). I don't have such insight into the logic of
>>> audit_context's lifetime, so I need someone to tell me what makes more
>>> sense here.
>
> Given the nature of audit_receive_msg(), would it ever match on an
> AUDIT_DIR field?  I don't think it would since there aren't really any
> vfs accesses that occur in the source of sending a netlink message
> down to the kernel ... am I missing something?

Probably not, but we still need to decide whether to pass the current
task's context or not. Both options (NULL and audit_context()) seem to
be benign for now, but I need you to pick one of those that you prefer
so I can send a final patch.

>
>> That is starting to work with context.  The recent FEATURE_CHANGE patch
>> to connect records of the same event uses current->audit_context (now
>> audit_context()) from audit_log_feature_change() called from
>> audit_set_feature() called from audit_receive_msg().
>>
>> There is also a work in progress to convert all the CONFIG_CHANGE
>> records over.  I'm just trying to track down all the instances.
>
> This will be a nice improvement.
>
> --
> paul moore
> www.paul-moore.com

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [RFC PATCH 1/2] audit: allow other filter list types for AUDIT_EXE

[Paul Moore]

On Wed, May 30, 2018 at 4:45 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> This patch removes the restriction of the AUDIT_EXE field to only
> SYSCALL filter and teaches audit_filter to recognize this field.
>
> This makes it possible to write rule lists such as:
>
>     auditctl -a exit,always [some general rule]
>     # Filter out events with executable name /bin/exe1 or /bin/exe2:
>     auditctl -a exclude,always -F exe=/bin/exe1
>     auditctl -a exclude,always -F exe=/bin/exe2
>
> See: https://github.com/linux-audit/audit-kernel/issues/54
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  kernel/auditfilter.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Merged, thanks.

-- 
paul moore
www.paul-moore.com