From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maneesh Soni Subject: Re: [RFC][PATCH] (#2) file system auditing Date: Wed, 20 Apr 2005 12:30:42 +0530 Message-ID: <20050420070042.GB10147@in.ibm.com> References: <200504161006.52766.tinytim@us.ibm.com> Reply-To: maneesh@in.ibm.com Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-fsdevel@vger.kernel.org, Stephen Smalley , Chris Wright , David Woodhouse Return-path: Received: from e35.co.us.ibm.com ([32.97.110.133]:20166 "EHLO e35.co.us.ibm.com") by vger.kernel.org with ESMTP id S261386AbVDTHBK (ORCPT ); Wed, 20 Apr 2005 03:01:10 -0400 Received: from d03relay04.boulder.ibm.com (d03relay04.boulder.ibm.com [9.17.195.106]) by e35.co.us.ibm.com (8.12.10/8.12.9) with ESMTP id j3K717Lg130162 for ; Wed, 20 Apr 2005 03:01:08 -0400 Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d03relay04.boulder.ibm.com (8.12.10/NCO/VER6.6) with ESMTP id j3K717wU061030 for ; Wed, 20 Apr 2005 01:01:07 -0600 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.12.11/8.12.11) with ESMTP id j3K716Pb011246 for ; Wed, 20 Apr 2005 01:01:07 -0600 To: "Timothy R. Chavez" Content-Disposition: inline In-Reply-To: <200504161006.52766.tinytim@us.ibm.com> Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org 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