Message-ID: <20150807062411.GB12116@madcap2.tricolour.ca>
Date:	Fri, 7 Aug 2015 02:25:14 -0400
From:	Richard Guy Briggs <rgb@...hat.com>
To:	Paul Moore <pmoore@...hat.com>
Cc:	linux-audit@...hat.com, linux-kernel@...r.kernel.org,
	sgrubb@...hat.com, eparis@...hat.com, peter@...3.com
Subject: Re: [PATCH V9 2/3] audit: implement audit by executable

On 15/08/06, Paul Moore wrote:
> On Wednesday, August 05, 2015 04:29:37 PM Richard Guy Briggs wrote:
> > This adds the ability audit the actions of a not-yet-running process.
> > 
> > This patch implements the ability to filter on the executable path.  Instead
> > of just hard coding the ino and dev of the executable we care about at the
> > moment the rule is inserted into the kernel, use the new audit_fsnotify
> > infrastructure to manage this dynamically.  This means that if the filename
> > does not yet exist but the containing directory does, or if the inode in
> > question is unlinked and creat'd (aka updated) the rule will just continue
> > to work.  If the containing directory is moved or deleted or the filesystem
> > is unmounted, the rule is deleted automatically.  A future enhancement
> > would be to have the rule survive across directory disruptions.
> > 
> > This is a heavily modified version of a patch originally submitted by Eric
> > Paris with some ideas from Peter Moody.
> > 
> > Cc: Peter Moody <peter@...3.com>
> > Cc: Eric Paris <eparis@...hat.com>
> > Signed-off-by: Richard Guy Briggs <rgb@...hat.com>
> > ---
> >  include/linux/audit.h      |    1 +
> >  include/uapi/linux/audit.h |    5 +++-
> >  kernel/audit.h             |    4 +++
> >  kernel/audit_tree.c        |    2 +
> >  kernel/audit_watch.c       |   31 +++++++++++++++++++++++++
> >  kernel/auditfilter.c       |   53 ++++++++++++++++++++++++++++++++++++++++-
> >  kernel/auditsc.c           |    3 ++
> >  7 files changed, 97 insertions(+), 2 deletions(-)
> 
> Merged, although some more minor whitespace tweaks were necessary for 
> checkpatch.  On a related note, if you're not running ./scripts/checlpatch.pl 
> on your patches before sending them out, I would recommend it.  It isn't 
> perfect, but it can catch some silly things that we all do from time to time.

No excuses...  I have been running it pretty regularly and got lazy and
distracted with patch revisions.  I can't say I agree with the no space
before closing round parenthesis due to legibility, but will comply.

> Also, one last thing.  It is pretty late in the -rcX cycle to merge these two 
> patches, but considering that we've been talking about these for a while, I'm 
> reasonably okay merging them.  In the future, if it isn't in audit#next by the 
> time -rc5 is released, it isn't going to make the merge window.

I've been quite aware of that looming merge window...  This feature has
been iterating for a while, so there are no big surprises.  I was aiming
for earlier.  :)

> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index c2e7e3a..aee456f 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -59,6 +59,7 @@ struct audit_krule {
> >  	struct audit_field	*inode_f; /* quick access to an inode field */
> >  	struct audit_watch	*watch;	/* associated watch */
> >  	struct audit_tree	*tree;	/* associated watched tree */
> > +	struct audit_fsnotify_mark	*exe;
> >  	struct list_head	rlist;	/* entry in audit_{watch,tree}.rules list */
> >  	struct list_head	list;	/* for AUDIT_LIST* purposes only */
> >  	u64			prio;
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 971df22..e2ca600 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -266,6 +266,7 @@
> >  #define AUDIT_OBJ_UID	109
> >  #define AUDIT_OBJ_GID	110
> >  #define AUDIT_FIELD_COMPARE	111
> > +#define AUDIT_EXE	112
> > 
> >  #define AUDIT_ARG0      200
> >  #define AUDIT_ARG1      (AUDIT_ARG0+1)
> > @@ -324,8 +325,10 @@ enum {
> > 
> >  #define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT	0x00000001
> >  #define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME	0x00000002
> > +#define AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH	0x00000004
> >  #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
> > -				  AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME)
> > +				  AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \
> > +				  AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH )
> > 
> >  /* deprecated: AUDIT_VERSION_* */
> >  #define AUDIT_VERSION_LATEST 		AUDIT_FEATURE_BITMAP_ALL
> > diff --git a/kernel/audit.h b/kernel/audit.h
> > index 46d10dd..dadf86a 100644
> > --- a/kernel/audit.h
> > +++ b/kernel/audit.h
> > @@ -277,6 +277,8 @@ extern char *audit_mark_path(struct audit_fsnotify_mark
> > *mark); extern void audit_remove_mark(struct audit_fsnotify_mark
> > *audit_mark); extern void audit_remove_mark_rule(struct audit_krule
> > *krule);
> >  extern int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned
> > long ino, dev_t dev); +extern int audit_dupe_exe(struct audit_krule *new,
> > struct audit_krule *old); +extern int audit_exe_compare(struct task_struct
> > *tsk, struct audit_fsnotify_mark *mark);
> > 
> >  #else
> >  #define audit_put_watch(w) {}
> > @@ -292,6 +294,8 @@ extern int audit_mark_compare(struct audit_fsnotify_mark
> > *mark, unsigned long in #define audit_remove_mark(m)
> >  #define audit_remove_mark_rule(k)
> >  #define audit_mark_compare(m, i, d) 0
> > +#define audit_exe_compare(t, m) (-EINVAL)
> > +#define audit_dupe_exe(n, o) (-EINVAL)
> >  #endif /* CONFIG_AUDIT_WATCH */
> > 
> >  #ifdef CONFIG_AUDIT_TREE
> > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> > index b0f9877..94ecdab 100644
> > --- a/kernel/audit_tree.c
> > +++ b/kernel/audit_tree.c
> > @@ -479,6 +479,8 @@ static void kill_rules(struct audit_tree *tree)
> >  		if (rule->tree) {
> >  			/* not a half-baked one */
> >  			audit_tree_log_remove_rule(rule);
> > +			if (entry->rule.exe)
> > +				audit_remove_mark(entry->rule.exe);
> >  			rule->tree = NULL;
> >  			list_del_rcu(&entry->list);
> >  			list_del(&entry->rule.list);
> > diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> > index c668bfc..1255dbf 100644
> > --- a/kernel/audit_watch.c
> > +++ b/kernel/audit_watch.c
> > @@ -312,6 +312,8 @@ static void audit_update_watch(struct audit_parent
> > *parent, list_replace(&oentry->rule.list,
> >  					     &nentry->rule.list);
> >  			}
> > +			if (oentry->rule.exe)
> > +				audit_remove_mark(oentry->rule.exe);
> > 
> >  			audit_watch_log_rule_change(r, owatch, "updated_rules");
> > 
> > @@ -342,6 +344,8 @@ static void audit_remove_parent_watches(struct
> > audit_parent *parent) list_for_each_entry_safe(r, nextr, &w->rules, rlist)
> > {
> >  			e = container_of(r, struct audit_entry, rule);
> >  			audit_watch_log_rule_change(r, w, "remove_rule");
> > +			if (e->rule.exe)
> > +				audit_remove_mark(e->rule.exe);
> >  			list_del(&r->rlist);
> >  			list_del(&r->list);
> >  			list_del_rcu(&e->list);
> > @@ -514,3 +518,30 @@ static int __init audit_watch_init(void)
> >  	return 0;
> >  }
> >  device_initcall(audit_watch_init);
> > +
> > +int audit_dupe_exe(struct audit_krule *new, struct audit_krule *old)
> > +{
> > +	struct audit_fsnotify_mark *audit_mark;
> > +	char *pathname;
> > +
> > +	pathname = kstrdup(audit_mark_path(old->exe), GFP_KERNEL);
> > +	if (!pathname)
> > +		return -ENOMEM;
> > +
> > +	audit_mark = audit_alloc_mark(new, pathname, strlen(pathname));
> > +	if (IS_ERR(audit_mark)) {
> > +		kfree(pathname);
> > +		return PTR_ERR(audit_mark);
> > +	}
> > +	new->exe = audit_mark;
> > +
> > +	return 0;
> > +}
> > +
> > +int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark
> > *mark) +{
> > +	unsigned long ino = tsk->mm->exe_file->f_inode->i_ino;
> > +	dev_t dev = tsk->mm->exe_file->f_inode->i_sb->s_dev;
> > +
> > +	return audit_mark_compare(mark, ino, dev);
> > +}
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 3d99196..c662638 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -405,6 +405,12 @@ static int audit_field_valid(struct audit_entry *entry,
> > struct audit_field *f) if (f->val > AUDIT_MAX_FIELD_COMPARE)
> >  			return -EINVAL;
> >  		break;
> > +	case AUDIT_EXE:
> > +		if (f->op != Audit_equal)
> > +			return -EINVAL;
> > +		if (entry->rule.listnr != AUDIT_FILTER_EXIT)
> > +			return -EINVAL;
> > +		break;
> >  	};
> >  	return 0;
> >  }
> > @@ -419,6 +425,7 @@ static struct audit_entry *audit_data_to_entry(struct
> > audit_rule_data *data, size_t remain = datasz - sizeof(struct
> > audit_rule_data);
> >  	int i;
> >  	char *str;
> > +	struct audit_fsnotify_mark *audit_mark;
> > 
> >  	entry = audit_to_entry_common(data);
> >  	if (IS_ERR(entry))
> > @@ -539,6 +546,24 @@ static struct audit_entry *audit_data_to_entry(struct
> > audit_rule_data *data, entry->rule.buflen += f->val;
> >  			entry->rule.filterkey = str;
> >  			break;
> > +		case AUDIT_EXE:
> > +			if (entry->rule.exe || f->val > PATH_MAX)
> > +				goto exit_free;
> > +			str = audit_unpack_string(&bufp, &remain, f->val);
> > +			if (IS_ERR(str)) {
> > +				err = PTR_ERR(str);
> > +				goto exit_free;
> > +			}
> > +			entry->rule.buflen += f->val;
> > +
> > +			audit_mark = audit_alloc_mark(&entry->rule, str, f->val);
> > +			if (IS_ERR(audit_mark)) {
> > +				kfree(str);
> > +				err = PTR_ERR(audit_mark);
> > +				goto exit_free;
> > +			}
> > +			entry->rule.exe = audit_mark;
> > +			break;
> >  		}
> >  	}
> > 
> > @@ -551,6 +576,8 @@ exit_nofree:
> >  exit_free:
> >  	if (entry->rule.tree)
> >  		audit_put_tree(entry->rule.tree); /* that's the temporary one */
> > +	if (entry->rule.exe)
> > +		audit_remove_mark(entry->rule.exe); /* that's the template one */
> >  	audit_free_rule(entry);
> >  	return ERR_PTR(err);
> >  }
> > @@ -615,6 +642,10 @@ static struct audit_rule_data
> > *audit_krule_to_data(struct audit_krule *krule) data->buflen +=
> > data->values[i] =
> >  				audit_pack_string(&bufp, krule->filterkey);
> >  			break;
> > +		case AUDIT_EXE:
> > +			data->buflen += data->values[i] =
> > +				audit_pack_string(&bufp, audit_mark_path(krule->exe));
> > +			break;
> >  		case AUDIT_LOGINUID_SET:
> >  			if (krule->pflags & AUDIT_LOGINUID_LEGACY && !f->val) {
> >  				data->fields[i] = AUDIT_LOGINUID;
> > @@ -678,6 +709,12 @@ static int audit_compare_rule(struct audit_krule *a,
> > struct audit_krule *b) if (strcmp(a->filterkey, b->filterkey))
> >  				return 1;
> >  			break;
> > +		case AUDIT_EXE:
> > +			/* both paths exist based on above type compare */
> > +			if (strcmp(audit_mark_path(a->exe),
> > +				   audit_mark_path(b->exe)))
> > +				return 1;
> > +			break;
> >  		case AUDIT_UID:
> >  		case AUDIT_EUID:
> >  		case AUDIT_SUID:
> > @@ -799,8 +836,14 @@ struct audit_entry *audit_dupe_rule(struct audit_krule
> > *old) err = -ENOMEM;
> >  			else
> >  				new->filterkey = fk;
> > +			break;
> > +		case AUDIT_EXE:
> > +			err = audit_dupe_exe(new, old);
> > +			break;
> >  		}
> >  		if (err) {
> > +			if (new->exe)
> > +				audit_remove_mark(new->exe);
> >  			audit_free_rule(entry);
> >  			return ERR_PTR(err);
> >  		}
> > @@ -963,6 +1006,9 @@ int audit_del_rule(struct audit_entry *entry)
> >  	if (e->rule.tree)
> >  		audit_remove_tree_rule(&e->rule);
> > 
> > +	if (e->rule.exe)
> > +		audit_remove_mark_rule(&e->rule);
> > +
> >  #ifdef CONFIG_AUDITSYSCALL
> >  	if (!dont_count)
> >  		audit_n_rules--;
> > @@ -1067,8 +1113,11 @@ int audit_rule_change(int type, __u32 portid, int
> > seq, void *data, WARN_ON(1);
> >  	}
> > 
> > -	if (err || type == AUDIT_DEL_RULE)
> > +	if (err || type == AUDIT_DEL_RULE) {
> > +		if (entry->rule.exe)
> > +			audit_remove_mark(entry->rule.exe);
> >  		audit_free_rule(entry);
> > +	}
> > 
> >  	return err;
> >  }
> > @@ -1360,6 +1409,8 @@ static int update_lsm_rule(struct audit_krule *r)
> >  		return 0;
> > 
> >  	nentry = audit_dupe_rule(r);
> > +	if (entry->rule.exe)
> > +		audit_remove_mark(entry->rule.exe);
> >  	if (IS_ERR(nentry)) {
> >  		/* save the first error encountered for the
> >  		 * return value */
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 701ea5c..e9bac2b 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -466,6 +466,9 @@ static int audit_filter_rules(struct task_struct *tsk,
> >  				result = audit_comparator(ctx->ppid, f->op, f->val);
> >  			}
> >  			break;
> > +		case AUDIT_EXE:
> > +			result = audit_exe_compare(tsk, rule->exe);
> > +			break;
> >  		case AUDIT_UID:
> >  			result = audit_uid_comparator(cred->uid, f->op, f->uid);
> >  			break;
> 
> -- 
> paul moore
> security @ redhat
> 

- RGB

--
Richard Guy Briggs <rbriggs@...hat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists