[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [RFC][PATCH] new audit rule interface



On Thu, 2005-12-15 at 10:40 -0500, Amy Griffis wrote: 
> 1) struct audit_rule_xprt
> 
>     Introducing a new data structure for specifying audit rules via
>     netlink provides a good opportunity to revisit the data structure
>     design and determine if we want to make any other changes, e.g.
>     adding a structure version field, reserving fields, etc.  At
>     present, I've only added the empty buf[] array.

nit: It also adds a buflen integer field.

You touch on this a bit later...  Ideally, I think it would be nice not
to bitmask into the upper bits of each field[] entry.  I would prefer
another integer array fieldflags[] where things such as the
audit_operator and whatever else might live.  That would give us a full
32 bits to mask against per field, and not cut into the total bits
available for field[] values.

> 2) struct audit_krule
> 
>     The kernel structure defining an audit rule isn't keeping enough
>     information about what it received from userspace.
> 
>     - the kernel currently converts the upper bits of a field[]
>       element to its own representation; when the rule is listed back
>       to userspace, the bits are not converted back to the way in
>       which they were originally specified

See my comment above.  I'd prefer field[] being just a field, and having
flags associated with each field in a separate array.  

>     - the difference between an inode-based and a path-based filter is
>       not always discernible, as it needs to be (e.g. kernel currently
>       won't allow a rule to be added for an inode # that can be
>       resolved from an existing path-based filter)

Might be a good thing to note by flipping a bit in the aforementioned
potentially new fieldflags[] location. 
    
>     - might not be an issue now, but should probably track which type
>       or version of structure was used to add a given rule

Hard to see, but I think a version field could be useful as the utility
of the audit system grows.

>     - the operator bits consistently need to be masked out; they
>       should just be a separate field in kernel

Agreed.  See comments above.

> 4) in general
> 
>     Perhaps most importantly, this patch mixes the interface changes
>     required to specify audit rules with string fields with some of
>     the initial filesystem audit functionality.  I think these pieces
>     should be split into separate patches to allow a more generalized
>     approach.

Yep, good call.  It would be nice if the new struct audit_rule_xprt plus
the functions required to receive the netlink messages and populate the
kernel structure were a patch of it's own.  I would absolutely begin
testing and developing on top of that as soon as such a patch is
available.

> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -182,7 +182,33 @@ struct audit_context {
>  #endif
>  };
>  
> -				/* Public API */
> +/* Audit Filters */
> +
> +/* kernel's representation of a watched filesystem location */
> +struct audit_watch {
> +	char			*path;	/* watch insertion path */
> +	struct list_head	mlist;	/* entry in master_watchlist */
> +	struct list_head	rules;	/* associated rules*/
> +};
> +
> +/* kernel's internal filter rule representation */
> +struct audit_krule {
> +	u32			flags;
> +	u32			action;
> +	u32			field_count;
> +	u32			mask[AUDIT_BITMASK_SIZE];
> +	u32			fields[AUDIT_MAX_FIELDS];
> +	u32			values[AUDIT_MAX_FIELDS];
> +	struct audit_watch	*watch; /* associated watch */
> +	struct list_head	rlist;	/* entry in audit_watch.rules list */
> +};
> +
> +struct audit_entry {
> +	struct list_head	list;	/* entry in audit_filter_list */
> +	struct rcu_head		rcu;
> +	struct audit_krule	rule;
> +};
> +
>  /* There are three lists of rules -- one to search at task creation
>   * time, one to search at syscall entry time, and another to search at
>   * syscall exit time. */

Not really your problem, but this documentation should probably be
updated, in that there are several more lists of rules now.

> @@ -198,101 +224,290 @@ static struct list_head audit_filter_lis
>  #endif
>  };
>  
> -struct audit_entry {
> -	struct list_head  list;
> -	struct rcu_head   rcu;
> -	struct audit_rule rule;
> -};
> +static LIST_HEAD(master_watchlist);
>  
>  extern int audit_pid;
>  
> -/* Copy rule from user-space to kernel-space.  Called from 
> - * audit_add_rule during AUDIT_ADD. */
> -static inline int audit_copy_rule(struct audit_rule *d, struct audit_rule *s)
> +/* Check to see if two rules are identical. */
> +static inline int audit_compare_rule(struct audit_krule *a, 
> +				     struct audit_krule *b,
> +				     unsigned skip_ino)
>  {
>  	int i;
>  
> -	if (s->action != AUDIT_NEVER
> -	    && s->action != AUDIT_POSSIBLE
> -	    && s->action != AUDIT_ALWAYS)
> -		return -1;
> -	if (s->field_count < 0 || s->field_count > AUDIT_MAX_FIELDS)
> -		return -1;
> -	if ((s->flags & ~AUDIT_FILTER_PREPEND) >= AUDIT_NR_FILTERS)
> -		return -1;
> -
> -	d->flags	= s->flags;
> -	d->action	= s->action;
> -	d->field_count	= s->field_count;
> -	for (i = 0; i < d->field_count; i++) {
> -		d->fields[i] = s->fields[i];
> -		d->values[i] = s->values[i];
> +	if (a->flags != b->flags || 
> +	    a->action != b->action || 
> +	    a->field_count != b->field_count)
> +		return 1;
> +
> +	/* rules must have same field ordering to match */
> +	for (i = 0; i < a->field_count; i++) {
> +		/* skip inode comparison with matching paths */
> +		if (skip_ino &&
> +		    (a->fields[i] & ~AUDIT_OPERATORS) == AUDIT_INODE &&
> +		    (b->fields[i] & ~AUDIT_OPERATORS) == AUDIT_INODE)
> +			continue;

I can see where this masking out of AUDIT_OPERATORS could get tedious.
These flags should be in it's own component of the struct.

As for the rest of the patch, it would be much easier to review in
separate pieces--ie, the (1) new rule structure handling code, then (2)
the new patch watching filter.

Thanks so much for posting, Amy!

:-Dustin

Attachment: signature.asc
Description: This is a digitally signed message part


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]