[PATCH] support for context based audit filtering

[Amy Griffis]

Hi Darrel,

I didn't notice this patch in my inbox until just recently.  I've put
a few comments inline.

On Fri, Feb 24, 2006 at 04:26:13PM -0600, Darrel Goeddel wrote:
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 3712295..752e2bb 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -25,6 +25,7 @@
> #include <linux/fs.h>
> #include <linux/namei.h>
> #include <linux/netlink.h>
> +#include <linux/selinux.h>
> #include "audit.h"
> 
> /* There are three lists of rules -- one to search at task creation
> @@ -50,6 +51,13 @@ static inline void audit_free_watch(stru
> 
> static inline void audit_free_rule(struct audit_entry *e)
> {
> +	int i;
> +	if (e->rule.fields)
> +		for (i = 0; i < e->rule.field_count; i++) {
> +			struct audit_field *f = &e->rule.fields[i];
> +			kfree(f->se_str);
> +			selinux_audit_rule_free(f->se_rule);
> +		}
> 	kfree(e->rule.fields);
> 	kfree(e);
> }
> @@ -192,7 +200,12 @@ static struct audit_entry *audit_rule_to
> 		f->val = rule->values[i];
> 
> 		if (f->type & AUDIT_UNUSED_BITS ||
> -		    f->type == AUDIT_WATCH) {
> +		    f->type == AUDIT_WATCH ||
> +		    f->type == AUDIT_SE_USER ||
> +		    f->type == AUDIT_SE_ROLE ||
> +		    f->type == AUDIT_SE_TYPE ||
> +		    f->type == AUDIT_SE_SEN ||
> +		    f->type == AUDIT_SE_CLR) {
> 			err = -EINVAL;
> 			goto exit_free;
> 		}
> @@ -222,7 +235,7 @@ static struct audit_entry *audit_data_to
> 	void *bufp;
> 	size_t remain = datasz - sizeof(struct audit_rule_data);
> 	int i;
> -	char *path;
> +	char *str;
> 
> 	entry = audit_to_entry_common((struct audit_rule *)data);
> 	if (IS_ERR(entry))
> @@ -241,16 +254,42 @@ static struct audit_entry *audit_data_to
> 		f->op = data->fieldflags[i] & AUDIT_OPERATORS;
> 		f->type = data->fields[i];
> 		f->val = data->values[i];
> +		f->se_str = NULL;
> +		f->se_rule = NULL;
> 		switch(f->type) {
> +		case AUDIT_SE_USER:
> +		case AUDIT_SE_ROLE:
> +		case AUDIT_SE_TYPE:
> +		case AUDIT_SE_SEN:
> +		case AUDIT_SE_CLR:
> +			str = audit_unpack_string(&bufp, &remain, f->val);
> +			if (IS_ERR(str))
> +				goto exit_free;
> +			entry->rule.buflen += f->val;
> +
> +			err = selinux_audit_rule_init(f->type, f->op, str,
> +			                              &f->se_rule);
> +			/* Keep currently invalid fields around in case they
> +			   become valid after a policy reload. */
> +			if (err == -EINVAL) {
> +				printk(KERN_WARNING "selinux audit rule for 
> item %s is invalid\n", str);
> +				err = 0;
> +			}
> +			if (err) {
> +				kfree(str);
> +				goto exit_free;
> +			} else
> +				f->se_str = str;
> +			break;
> 		case AUDIT_WATCH:
> -			path = audit_unpack_string(&bufp, &remain, f->val);
> -			if (IS_ERR(path))
> +			str = audit_unpack_string(&bufp, &remain, f->val);
> +			if (IS_ERR(str))
> 				goto exit_free;
> 			entry->rule.buflen += f->val;
> 
> -			err = audit_to_watch(path, &entry->rule, i);
> +			err = audit_to_watch(str, &entry->rule, i);
> 			if (err) {
> -				kfree(path);
> +				kfree(str);
> 				goto exit_free;
> 			}
> 			break;
> @@ -333,6 +372,14 @@ static struct audit_rule_data *audit_kru
> 			data->buflen += data->values[i] =
> 				audit_pack_string(&bufp, krule->watch->path);
> 			break;
> +		case AUDIT_SE_USER:
> +		case AUDIT_SE_ROLE:
> +		case AUDIT_SE_TYPE:
> +		case AUDIT_SE_SEN:
> +		case AUDIT_SE_CLR:
> +			data->buflen += data->values[i] =
> +				audit_pack_string(&bufp, f->se_str);
> +			break;
> 		default:
> 			data->values[i] = f->val;
> 		}
> @@ -370,6 +417,14 @@ static int audit_compare_rule(struct aud
> 			if (audit_compare_watch(a->watch, b->watch))
> 				return 1;
> 			break;
> +		case AUDIT_SE_USER:
> +		case AUDIT_SE_ROLE:
> +		case AUDIT_SE_TYPE:
> +		case AUDIT_SE_SEN:
> +		case AUDIT_SE_CLR:
> +			if (strcmp(a->fields[i].se_str, b->fields[i].se_str))
> +				return 1;
> +			break;
> 		default:
> 			if (a->fields[i].val != b->fields[i].val)
> 				return 1;
> @@ -640,6 +695,9 @@ int audit_comparator(const u32 left, con
> 	default:
> 		return -EINVAL;
> 	}
> +	/* should NEVER get here */
> +	BUG();
> +	return 0;
> }
> 
> 
> @@ -726,3 +784,143 @@ unlock_and_return:
> 	rcu_read_unlock();
> 	return result;
> }
> +
> +/* Check to see if the rule contains any selinux fields.  Returns 1 if 
> there
> +   are selinux fields specified in the rule, 0 otherwise. */
> +static inline int audit_rule_has_selinux(struct audit_krule *rule)
> +{
> +	int i;
> +
> +	for (i = 0; i < rule->field_count; i++) {
> +		struct audit_field *f = &rule->fields[i];
> +		switch (f->type) {
> +		case AUDIT_SE_USER:
> +		case AUDIT_SE_ROLE:
> +		case AUDIT_SE_TYPE:
> +		case AUDIT_SE_SEN:
> +		case AUDIT_SE_CLR:
> +			return 1;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/* Make a copy of src in dest.  This will be a deep copy with the exception
> +   of the watch - that pointer is carried over.  The selinux specific 
> fields
> +   will be updated in the copy.  The point is to be able to replace the src
> +   rule with the dest rule in the list, then free the dest rule. */
> +static inline int selinux_audit_rule_update_helper(struct audit_krule 
> *dest,
> +                                                   struct audit_krule *src)
> +{
> +	int i, err = 0;
> +
> +	dest->vers_ops = src->vers_ops;
> +	dest->flags = src->flags;
> +	dest->listnr = src->listnr;
> +	dest->action = src->action;
> +	for (i = 0; i < AUDIT_BITMASK_SIZE; i++)
> +		dest->mask[i] = src->mask[i];
> +	dest->buflen = src->buflen;
> +	dest->field_count = src->field_count;
> +
> +	/* deep copy this information, updating the se_rule fields, because
> +	   the originals will all be freed when the old rule is freed. */
> +	dest->fields = kzalloc(sizeof(struct audit_field) * 
> dest->field_count,
> +	                       GFP_ATOMIC);
> +	if (!dest->fields)
> +		return -ENOMEM;

It seems like it would be cleaner to group the memory allocations for
the rule entry and the fields into a single function, as any audit
rule must always have both.  This would allow you to retain the
assumption of the existence of fields in audit_free_rule.

> +	memcpy(dest->fields, src->fields,
> +	       sizeof(struct audit_field) * dest->field_count);
> +	for (i = 0; i < dest->field_count; i++) {
> +		struct audit_field *df = &dest->fields[i];
> +		struct audit_field *sf = &src->fields[i];
> +		switch (df->type) {
> +		case AUDIT_SE_USER:
> +		case AUDIT_SE_ROLE:
> +		case AUDIT_SE_TYPE:
> +		case AUDIT_SE_SEN:
> +		case AUDIT_SE_CLR:
> +			/* our own copy of se_str */
> +			df->se_str = kstrdup(sf->se_str, GFP_ATOMIC);

I realize failure is unlikely here, but shouldn't you check the return
value?  Later on you'll end up passing this to strcmp() which won't
like it if it's NULL.  

> +			/* our own (refreshed) copy of se_rule */
> +			err = selinux_audit_rule_init(df->type, df->op,
> +			                              df->se_str, 
> &df->se_rule);
> +			/* Keep currently invalid fields around in case they
> +			   become valid after a policy reload. */
> +			if (err == -EINVAL) {
> +				printk(KERN_WARNING "selinux audit rule for 
> item %s is invalid\n", df->se_str);
> +				err = 0;
> +			}
> +			if (err)
> +				return err;
> +		}
> +	}
> +
> +	/* we can shallow copy the watch because we will not be freeing it 
> via
> +	   selinux_audit_rule_update (and we do nto modify it) */

On the flipside, when audit watches are updated, the se_str and
se_rule will need to be deep copied since they are freed in
audit_free_rule().  Is this what you intend?

> +	dest->watch = src->watch;
> +	dest->rlist = src->rlist;


> +
> +	return 0;
> +}
> +
> +/* This function will re-initialize the se_rule field of all applicable 
> rules.
> +   It will traverse the filter lists serarching for rules that contain 
> selinux
> +   specific filter fields.  When such a rule is found, it is copied, the
> +   selinux field is re-initialized, and the old rule is replaced with the
> +   updated rule. */
> +/* XXX: is the error handling below appropriate */
> +static int selinux_audit_rule_update(void)
> +{
> +	struct audit_entry *entry, *nentry;
> +	int i, err = 0, tmperr;
> +
> +	/* audit_netlink_sem synchronizes the writers */
> +	down(&audit_netlink_sem);
> +
> +	for (i = 0; i < AUDIT_NR_FILTERS; i++) {
> +		list_for_each_entry(entry, &audit_filter_list[i], list) {
> +			if (!audit_rule_has_selinux(&entry->rule))
> +				continue;
> +
> +			nentry = kmalloc(sizeof(*entry), GFP_ATOMIC);
> +			if (!nentry) {
> +				/* save the first error encountered for the
> +				   return value */
> +				if (!err)
> +					err = -ENOMEM;
> +				audit_panic("error updating selinux 
> filters");
> +				continue;
> +			}
> +
> +			tmperr = 
> selinux_audit_rule_update_helper(&nentry->rule,
> +			                                          
> &entry->rule);
> +			if (!nentry) {

How would we end up with !nentry here?  Maybe you mean if (temperr) ?

> +				/* save the first error encountered for the
> +				   return value */
> +				if (!err)
> +					err = -ENOMEM;

You don't want to hardcode the -ENOMEM as selinux_audit_rule_init()
can also return an error.

> +				audit_free_rule(nentry);
> +				audit_panic("error updating selinux 
> filters");
> +				continue;
> +			}
> +			list_replace_rcu(&entry->list, &nentry->list);
> +			call_rcu(&entry->rcu, audit_free_rule_rcu);
> +		}
> +	}
> +
> +	up(&audit_netlink_sem);
> +
> +	return err;
> +}
> +
> +/* Register the callback with selinux.  This callback will be invoked when 
> a
> +   new policy is loaded. */
> +static int __init register_selinux_update_callback(void)
> +{
> +	selinux_audit_set_callback(&selinux_audit_rule_update);
> +	return 0;
> +}
> +__initcall(register_selinux_update_callback);
> +

I don't know about anyone else, but I would prefer to keep all of the
initialization for the audit subsystem in audit.c:audit_init().  This
makes the audit initialization path more easily synchronized and
readable.

Regards,
Amy