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

[Maneesh Soni <maneesh@in.ibm.com>]

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.

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.

[..]
> +
> +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.

[..]
> +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);
> +	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?

[..]
> +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);

[..]
> +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

> +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 },
>  };
>  

-- 
Maneesh Soni
Linux Technology Center, 
IBM India Software Labs,
Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-25044990