From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Timothy R. Chavez" Subject: Re: [RFC][PATCH] (#2) file system auditing Date: Tue, 26 Apr 2005 15:23:28 -0500 Message-ID: <1114547008.8189.24.camel@localhost> References: <200504161006.52766.tinytim@us.ibm.com> <20050420070042.GB10147@in.ibm.com> Mime-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 7bit Cc: David Woodhouse , Chris Wright , Stephen Smalley , linux-fsdevel@vger.kernel.org Return-path: Received: from e31.co.us.ibm.com ([32.97.110.129]:54513 "EHLO e31.co.us.ibm.com") by vger.kernel.org with ESMTP id S261511AbVDZUWT (ORCPT ); Tue, 26 Apr 2005 16:22:19 -0400 Received: from westrelay02.boulder.ibm.com (westrelay02.boulder.ibm.com [9.17.195.11]) by e31.co.us.ibm.com (8.12.10/8.12.9) with ESMTP id j3QKMIua314828 for ; Tue, 26 Apr 2005 16:22:18 -0400 Received: from d03av01.boulder.ibm.com (d03av01.boulder.ibm.com [9.17.195.167]) by westrelay02.boulder.ibm.com (8.12.10/NCO/VER6.6) with ESMTP id j3QKMIP5296716 for ; Tue, 26 Apr 2005 14:22:18 -0600 Received: from d03av01.boulder.ibm.com (loopback [127.0.0.1]) by d03av01.boulder.ibm.com (8.12.11/8.13.3) with ESMTP id j3QKMHKK003766 for ; Tue, 26 Apr 2005 14:22:18 -0600 To: maneesh@in.ibm.com In-Reply-To: <20050420070042.GB10147@in.ibm.com> Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org 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 }, > > }; > > >