Re: [RFC][PATCH] (#2) file system auditing

["Timothy R. Chavez" <tinytim@us.ibm.com>]

On Wed, 2005-04-20 at 12:30 +0530, Maneesh Soni wrote: 
> On Sat, Apr 16, 2005 at 10:06:52AM +0000, Timothy R. Chavez wrote:
> > Hello,
> > 
> > The audit subsystem is currently incapable of auditing a file system object 
> > based on its location and name.  This is critical for auditing well-defined 
> > and security-relevant locations such as /etc/shadow, where the file is 
> > re-created on each transaction, and cannot rely on the (device, inode)-based 
> > filters to ensure persistence of auditing across transactions. This patch adds 
> > the necessary functionality to the audit subsystem and VFS to support file 
> > system auditing in which an object is audited based on its location and name.  
> > This work is being done to make the audit subsystem compliant with Common 
> > Criteria's Controlled Access Protection Profile (CAPP) specification.
> > 
> > This is the second (#2) RFC on linux-fsdevel.
> > 
> 
> Please see the comments interspread below.
> 

Hi Maneesh,

Thank you for the comments.  I apologize for the late response.  My
responses are embedded below.  Also, I've updated the patch to
incorporate much of the feedback I've received from you and others in
addition to some fixes for various bugs that I carelessly introduced
between the first and second patch I submitted to linux-fsdevel.  I'll
submit this patch for RFC (noting the changes) momentarily to
linux-fsdevel.

-tim

> Thanks
> Maneesh
> 
> 
> PS: Sorry to miss earlier versions.
> 
> 
> [..]
> > @@ -987,6 +991,7 @@ struct dentry *d_splice_alias(struct ino
> >  			/* d_instantiate takes dcache_lock, so we do it by hand */
> >  			list_add(&dentry->d_alias, &inode->i_dentry);
> >  			dentry->d_inode = inode;
> > +			audit_attach_watch(dentry, 0);
> >  			spin_unlock(&dcache_lock);
> >  			security_d_instantiate(dentry, inode);
> >  			d_rehash(dentry);
> > @@ -1090,6 +1095,7 @@ struct dentry * __d_lookup(struct dentry
> >  		if (!d_unhashed(dentry)) {
> >  			atomic_inc(&dentry->d_count);
> >  			found = dentry;
> > +			audit_attach_watch(found, 0);
> >  		}
> >  		spin_unlock(&dentry->d_lock);
> >  		break;
> 
> Are you sure about hooking __d_lookup()? I am not getting why would one want
> to watch every dcache lookup. IIUC, you probaly want to watch every fs object
> entering the dcache, so IMO, the right place is real_lookup() and not the 
> __d_lookup() which is performance sensitive code.

Well the purpose of using __d_lookup is to make updates to the inode's
audit data, not to watch every dcache lookup.  The __d_lookup hook is
strategic for getting us to a place where we can determine whether or
not the inode's audit data needs updating.  When we remove a watch from
the file system, all we're doing is unhashing it from the local watch
list and decrementing its reference count.  An object that's pointing at
an unhashed watchlist entry is no longer watched, so it has to drop its
reference to the watchlist entry, reset its pointer to NULL, or in the
case of a hard link, poll its current location for a watch it can attach
too (Note: A hard link can be implicitly watched by linking to a watched
file.  Should the hard link itself exist in a watched location, it
retains its original watch until the inode becomes unwatched.  The hard
link then has the oppurtunity to be watched at its own location).  

It doesn't really seem to me that real_lookup() is suited for the
purpose stated above.  However, I do realize that having to potentially
waiting for two locks in the __d_lookup routine is not a good thing.
Perhaps a better stradegy, in this regard, can be devised.

> 
> [..]
> > +
> > +static inline void audit_watch_free(struct audit_watch *watch)
> > +{
> > +	if (watch) {
> > +		kfree(watch->name);
> > +		kfree(watch->filterkey);
> > +		kmem_cache_free(audit_watch_cache, watch);
> > +	}
> > +}
> 
> I think you forgot, kfree(watch->path) here.

Yep.  Good eye, thank you.

> 
> [..]
> > +static inline int audit_insert_watch(char *path, char *filterkey, __u32 perms)
> > +{
> > +	int ret;
> > +	struct nameidata nd;
> > +
> > +	ret = path_lookup(path, LOOKUP_PARENT|LOOKUP_FOLLOW, &nd);

Note: I've discarded the LOOKUP_FOLLOWS

> > +	if (ret < 0)
> > +		goto audit_insert_watch_exit;
> > +
> > +	ret = -EPERM;
> > +	if (nd.last_type != LAST_NORM || !nd.last.name)
> > +		goto audit_insert_watch_release;
> > +
> > +	ret = audit_create_wentry(path, nd.last.name, filterkey, perms,
> > +				  nd.dentry->d_inode->i_sb->s_dev,
> > +				  nd.dentry->d_inode->i_audit);
> > +	/* __d_lookup will attach the audit data, if nd.last.name exists. */
> > +	d_lookup(nd.dentry, &nd.last);
> 
> Do you mean, if nd.last.name exists in dcache() or exists in the filesystem?

Dcache.  Is my thinking flawed?  I'm under the impression that all the
relevant dentries I need will be in the dcache.  The reason I called
d_lookup here is because d_lookup will call __d_lookup which has the
audit_attach_watch() hook.  I know this is not too obvious, but to call
the hook directly from here would require I do the d_lookup anyway (to
get the dentry).

> 
> [..]
> > +void audit_attach_watch(struct dentry *dentry, int remove)
> > +{
> > +	struct audit_wentry *wentry;
> > +	struct audit_data *data, *parent;
> > +
> > +	if (!dentry || !dentry->d_inode)
> > +		return;
> > +
> > +	if (!dentry->d_parent || !dentry->d_parent->d_inode)
> > +		return;
> > +
> > +	data = dentry->d_inode->i_audit;
> > +	parent = dentry->d_parent->d_inode->i_audit;
> > +
> > +	wentry = audit_wentry_fetch_lock(dentry->d_name.name, parent);
> > +
> > +	write_lock(&data->lock);
> > +	/* FIXME: long watchlist == too much spinning? */
> > +	if (remove) {
> > +		audit_drain_watchlist(data);
> > +		if (wentry && data->wentry) {
> > +			if (!strcmp(wentry->w_watch->name,
> > +				    data->wentry->w_watch->name)) {
> > +				audit_wentry_put(data->wentry);
> > +				dentry->d_inode->i_audit->wentry = NULL;
> > +			}
> > +		}
> > +	} else if (!data->wentry || hlist_unhashed(&data->wentry->w_node)) {
> > +		audit_wentry_put(data->wentry);
> > +		dentry->d_inode->i_audit->wentry = audit_wentry_get(wentry);
> 
> why not just, data->wentry = audit_wentry_get(wentry);

Oops this one slipped by me on this latest update, I'll add this to the
list of things I have to do..  Thanks

> 
> [..]
> > +int audit_list_watches(int pid, int seq)
> > +{
> > +	int ret = 0;
> > +	struct audit_wentry *wentry;
> > +	struct hlist_node *pos;
> > +
> > +	spin_lock(&audit_master_watchlist_lock);
> > +	hlist_for_each_entry(wentry, pos, &audit_master_watchlist, w_master) {
> > +		ret = audit_send_watch(pid, seq, wentry->w_watch);
> > +		if (ret < 0)
> > +			break;
> > +	}
> > +	spin_unlock(&audit_master_watchlist_lock);
> > +
> > +	audit_send_reply(pid, seq, AUDIT_WATCH_LIST, 1, 1, NULL, 0);
> > +
> > +	return ret;
> > +}
> > +
> 
> Looks like there is lock heirarachy issue here between 
> audit_master_watchlist_lock and data->lock(). 
> 
> audit_list_watches()---> audit_master_watchlist_lock
>   audit_send_watch()
>     audit_wentry_fetch_lock()---> data->lock
> 
> audit_insert_watch()
>   audit_create_wentry()---> data->lock
>     audit_master_insert()---> audit_master_watchlist_lock
> 
> OR
> 
> audit_attach_watch()---> data->lock
>   audit_drain_watchlist()
>     audit_destroy_watch()
>       audit_master_remove()---> audit_master_watchlist_lock
> 

Yes, this was definately an issue (and there was a problem with deadlock
on SMP).  I believe I resolved this by making the locking much tighter
(ie: audit_master_watchlist locking no longer spans function calls and
conditionals).

Thanks once again for your comments.

> > +int audit_receive_watch(int type, int pid, int uid, int seq,
> > +			struct audit_transport *req)
> > +{
> > +	int ret = 0;
> > +	char *path = NULL;
> > +	char *filterkey = NULL;
> > +
> > +	ret = -ENOMEM;
> > +	path = kmalloc(req->pathlen, GFP_KERNEL);
> > +	if (!path)
> > +		goto audit_receive_watch_exit;
> > +	strncpy(path, req->buf, req->pathlen);
> > +
> > +	if (req->fklen) {
> > +		filterkey = kmalloc(req->fklen, GFP_KERNEL);
> > +		if (!filterkey)
> > +			goto audit_receive_watch_exit;
> > +	}
> > +	strncpy(filterkey, req->buf+req->pathlen, req->fklen);
> > +
> > +	ret = -EINVAL;
> > +	if (!path || strlen(path) + 1 > PATH_MAX)
> > +		goto audit_receive_watch_exit;
> > +
> > +	/* Includes terminating '\0' */
> > +	if (req->fklen > AUDIT_FILTERKEY_MAX)
> > +		goto audit_receive_watch_exit;
> > +
> > +	if (req->perms > (MAY_READ|MAY_WRITE|MAY_EXEC|MAY_APPEND))
> > +		goto audit_receive_watch_exit;
> > +
> > +	switch (type) {
> > +	case AUDIT_WATCH_INS:
> > +		ret = audit_insert_watch(path, filterkey, req->perms);
> > +		break;
> > +	case AUDIT_WATCH_REM:
> > +		ret = audit_remove_watch(path);
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +	}
> > +
> > +audit_receive_watch_exit:
> > +	kfree(path);
> > +	kfree(filterkey);
> > +	return ret;
> > +}
> > +
> > +int audit_inode_alloc(struct inode *inode)
> > +{
> > +	if (inode) {
> > +		inode->i_audit = audit_data_alloc();
> > +		if (!inode->i_audit)
> > +			return ENOMEM;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +void audit_inode_free(struct inode *inode)
> > +{
> > +	if (inode)
> > +		audit_data_free(inode->i_audit);
> > +}
> > +
> > +int audit_filesystem_init()
> > +{
> > +	int ret = 0;
> > +
> > +	audit_watch_cache =
> > +	    kmem_cache_create("audit_watch_cache",
> > +			      sizeof(struct audit_watch), 0, 0, NULL, NULL);
> > +	if (!audit_watch_cache)
> > +		goto audit_filesystem_init_fail;
> > +
> > +	audit_wentry_cache =
> > +	    kmem_cache_create("audit_wentry_cache",
> > +			      sizeof(struct audit_wentry), 0, 0, NULL, NULL);
> > +	if (!audit_wentry_cache)
> > +		goto audit_filesystem_init_fail;
> > +
> > +	goto audit_filesystem_init_exit;
> > +
> > +audit_filesystem_init_fail:
> > +	ret = -ENOMEM;
> > +	kmem_cache_destroy(audit_watch_cache);
> > +	kmem_cache_destroy(audit_wentry_cache);
> > +audit_filesystem_init_exit:
> > +	return ret;
> > +
> > +}
> > diff -Nurp linux-2.6.12-rc2-mm1~orig/kernel/auditsc.c linux-2.6.12-rc2-mm1~audit/kernel/auditsc.c
> > --- linux-2.6.12-rc2-mm1~orig/kernel/auditsc.c	2005-04-11 14:15:36.000000000 +0000
> > +++ linux-2.6.12-rc2-mm1~audit/kernel/auditsc.c	2005-04-08 16:12:23.000000000 +0000
> > @@ -102,6 +102,7 @@ struct audit_aux_data {
> >  };
> >  
> >  #define AUDIT_AUX_IPCPERM	0
> > +#define AUDIT_AUX_WATCH		1
> >  
> >  struct audit_aux_data_ipcctl {
> >  	struct audit_aux_data	d;
> > @@ -112,6 +113,16 @@ struct audit_aux_data_ipcctl {
> >  	mode_t			mode;
> >  };
> >  
> > +struct audit_aux_data_watched {
> > +	struct audit_aux_data	link;
> > +	struct audit_wentry 	*wentry;
> > +	unsigned long		ino;
> > +	int			mask;
> > +	uid_t			uid;
> > +	gid_t			gid;
> > +	dev_t			dev;
> > +	dev_t			rdev;
> > +};
> >  
> >  /* The per-task audit context. */
> >  struct audit_context {
> > @@ -665,6 +676,24 @@ static void audit_log_exit(struct audit_
> >  			audit_log_format(ab, 
> >  					 " qbytes=%lx uid=%d gid=%d mode=%x",
> >  					 axi->qbytes, axi->uid, axi->gid, axi->mode);
> > +			break;
> > +			}
> > +
> > +		case AUDIT_AUX_WATCH: {
> > +			struct audit_aux_data_watched *axi = (void *)aux;
> > +			audit_log_format(ab, " name=");
> > +			audit_log_untrustedstring(ab, axi->wentry->w_watch->name);
> > +			audit_log_format(ab,
> > +				" filterkey=%s perm=%u perm_mask=%d"
> > +				" inode=%lu inode_uid=%d inode_gid=%d"
> > +				" inode_dev=%02x:%02x inode_rdev=%02x:%02x",
> > +				axi->wentry->w_watch->filterkey,
> > +				axi->wentry->w_watch->perms,
> > +				axi->mask, axi->ino, axi->uid, axi->gid,
> > +				MAJOR(axi->dev), MINOR(axi->dev),
> > +				MAJOR(axi->rdev), MINOR(axi->rdev));
> > +				audit_wentry_put(axi->wentry);
> > +			break;
> >  			}
> >  		}
> >  		audit_log_end(ab);
> > @@ -1024,3 +1053,52 @@ int audit_ipc_perms(unsigned long qbytes
> >  	context->aux = (void *)ax;
> >  	return 0;
> >  }
> > +
> > +int audit_notify_watch(struct inode *inode, int mask)
> > +{
> > +	int ret = 0;
> > +	struct audit_context *context = current->audit_context;
> > +	struct audit_aux_data_watched *ax;
> > +	struct audit_wentry *wentry = NULL;
> > +
> > +	if (likely(!context))
> > +		goto audit_notify_watch_fail;
> > +
> > +	if (!inode)
> > +		goto audit_notify_watch_fail;
> > +
> > +	wentry = audit_wentry_get(inode->i_audit->wentry);
> > +	if (!wentry)
> > +		goto audit_notify_watch_fail;
> > +
> > +	if (mask && (wentry->w_watch->perms && !(wentry->w_watch->perms&mask)))
> > +		goto audit_notify_watch_fail;
> > +
> > +	ret = -ENOMEM;
> > +	ax = kmalloc(sizeof(*ax), GFP_KERNEL);
> > +	if (!ax)
> > +		goto audit_notify_watch_fail;
> > +
> > +	ret = 0;
> > +	if (context->in_syscall && !context->auditable)
> > +		context->auditable = 1;
> > +
> > +	ax->wentry = wentry;
> > +	ax->mask = mask;
> > +	ax->ino = inode->i_ino;
> > +	ax->uid = inode->i_uid;
> > +	ax->gid = inode->i_gid;
> > +	ax->dev = inode->i_sb->s_dev;
> > +	ax->rdev = inode->i_rdev;
> > +
> > +	ax->link.type = AUDIT_AUX_WATCH;
> > +	ax->link.next = context->aux;
> > +	context->aux = (void *)ax;
> > +
> > +	goto audit_notify_watch_exit;
> > +
> > +audit_notify_watch_fail:
> > +	audit_wentry_put(wentry);
> > +audit_notify_watch_exit:
> > +	return ret;
> > +}
> > diff -Nurp linux-2.6.12-rc2-mm1~orig/security/selinux/nlmsgtab.c linux-2.6.12-rc2-mm1~audit/security/selinux/nlmsgtab.c
> > --- linux-2.6.12-rc2-mm1~orig/security/selinux/nlmsgtab.c	2005-04-11 14:15:36.000000000 +0000
> > +++ linux-2.6.12-rc2-mm1~audit/security/selinux/nlmsgtab.c	2005-04-05 22:58:51.000000000 +0000
> > @@ -98,6 +98,9 @@ static struct nlmsg_perm nlmsg_audit_per
> >  	{ AUDIT_DEL,		NETLINK_AUDIT_SOCKET__NLMSG_WRITE },
> >  	{ AUDIT_USER,		NETLINK_AUDIT_SOCKET__NLMSG_WRITE },
> >  	{ AUDIT_LOGIN,		NETLINK_AUDIT_SOCKET__NLMSG_WRITE },
> > +	{ AUDIT_WATCH_INS,	NETLINK_AUDIT_SOCKET__NLMSG_WRITE },
> > +	{ AUDIT_WATCH_REM,	NETLINK_AUDIT_SOCKET__NLMSG_WRITE },
> > +	{ AUDIT_WATCH_LIST,	NETLINK_AUDIT_SOCKET__NLMSG_WRITE },
> >  };
> >  
>