On Thursday 24 March 2005 09:26 am, Stephen Smalley wrote: This was sobering and reasonable. Thank you. I will definately work on this. No testing on SMP as of right now, I'll get right on this right now. I have a couple of minor fix-ups too (changed the description in init/Kconfig and repositioned the exec_permission_lite() hook because if exec_permission_lite() returns -EAGAIN, we'll get a record there and in permission()) <snip> Thanks again. -tim

On Thursday 24 March 2005 10:19 am, Stephen Smalley wrote: Funny you mentioned this. Right after you told me about the deadlock you were getting, I went and looked at this function, and noticed the same thing. I'm now testing for this case. -- -tim

On Wed, 2005-03-23 at 14:22 -0600, Timothy R. Chavez wrote: Other comments: - As it stands with your patch, alloc_inode() will leak memory if audit_inode_alloc() succeeds but security_inode_alloc() fails because nothing frees the audit data on the error handling path for that case. Also, I have to wonder whether it is truly necessary to make i_audit conditionally compiled; note that i_security is always present and initialized to NULL even if !CONFIG_SECURITY. If you can make it unconditional, then you can also initialize it to NULL prior to the audit_inode_alloc() call so that you have a known initial state, and then just call audit_inode_free() on the error handling path (which will either map to free'ing NULL, which is safe, or free'ing an actual audit data, avoiding the leak). Or you could just make setting i_audit to NULL the first step in audit_inode_alloc() prior to attempting the allocation to ensure a known initial state. - You've switched from storing the audit data in the current audit context for later processing by audit_log_exit() to immediately generation of partial audit data via audit_log* calls from audit_notify_watch(). That goes beyond what I suggested; all I suggested was setting the auditable flag in the current audit context (while still saving the file information in the current audit context) so that audit_syscall_exit will call audit_log_exit() and generate an audit record upon syscall exit whenever an audit_notify_watch() is triggered on an auditable inode during the syscall processing. Your new approach is more like what SELinux presently does, but seems counter to what others have been suggesting, i.e. batch everything up for processing on syscall exit. Both approaches ensure that an audit record is emitted whenever an auditable inode is encountered, but the present approach yields two separate audit records (one immediate from your hook and one upon syscall exit) vs. a single unified record. What do we want? What do others think? -- Stephen Smalley <sds(a)tycho.nsa.gov&gt; National Security Agency

On Thu, 2005-03-24 at 11:00 -0600, Timothy R. Chavez wrote: It has all of the desired information, but is being emitted as multiple audit records (which can be correlated using the timestamp/serial number), and the first line is being emitted immediately by your hook while the latter two are being emitted by the audit_log_exit upon audit_syscall_exit. The question is whether we want your older approach (save information in current audit context for later processing by audit_log_exit), with just the modification of setting the auditable flag to ensure generation, or the approach in your current patch, which is more like what SELinux does presently. -- Stephen Smalley <sds(a)tycho.nsa.gov&gt; National Security Agency

On Thu, 2005-03-24 at 14:13 -0500, Stephen Smalley wrote: Running strace on passwd, I see that the transaction consists of: 1) create /etc/nshadow 2) read old content from /etc/shadow 3) write new content to /etc/nshadow 4) rename /etc/nshadow to /etc/shadow So I would have expected to see an audit upon the rename from /etc/nshadow to /etc/shadow, no? I also tried adding a watch for nshadow, e.g. auditctl -w /etc/nshadow -k SHADOW -p w Still no audit messages upon using passwd to change my password. Now, if I change my watch to include read access as well, e.g. auditctl -W /etc/shadow auditctl -w /etc/shadow -k SHADOW -p rwa Then I start to see some audit messages during passwd, but I shouldn't have to request read access auditing in order to see modifications (especially as that will generate a lot more data, e.g. upon every authentication program's use of /etc/shadow). -- Stephen Smalley <sds(a)tycho.nsa.gov&gt; National Security Agency

On Thu, 2005-03-24 at 14:32 -0500, Stephen Smalley wrote: I suppose may_create() won't help you, as the child has a negative dentry at that point so you have no inode. You will have an inode upon the subsequent d_instantiate, but can't tell that you are dealing with a "just created" inode versus an already existing one, so you won't know that you need to notify of a create. So you are back to post-create style hooks for calling audit_notify_watch for file creations, right? -- Stephen Smalley <sds(a)tycho.nsa.gov&gt; National Security Agency

On Thu, 2005-03-24 at 11:52 -0800, Chris Wright wrote: I think that they are ok for notification (audit_notify_watch calls), but they aren't safe for attaching watches (audit_attach_watch calls), because the inode can be accessed via the dcache by another thread before the post-create hooks run. Hence, he still wants to keep hooks for attaching watches in places like d_instantiate, but the notification hooks can occur later in the processing, like the dnotify hooks. -- Stephen Smalley <sds(a)tycho.nsa.gov&gt; National Security Agency

On Thu, 2005-03-24 at 14:48 -0600, Timothy R. Chavez wrote: This brings up another issue I wanted to discuss - whether the watch mask should be more general than rwea, e.g. should it be possible to specify that I want to see "rename", "link", "unlink", etc. with specific mask values for each of these operations. At that point, you would be passing your own set of mask values to audit_notify_watch (with a translation table for the normal permission mask). Alternatively, you could just view "rename", "link", and "unlink" as another form of write, so you could pass MAY_WRITE here. With regard to additional hook placement for audit_notify_watch, I think you likely do want to mirror the security*_post* hooks for file creation (create, mkdir, mknod, symlink), rename, and link with audit_notify_watch calls to perform notifications of such events. Then you keep audit_attach_watch calls in the dcache routines to manage the i_audit fields and avoid races. However, I think you need to check whether you truly need all of the current hook placements in the dcache routines or whether some of them are duplicative on the same code path, e.g. do you need both __d_lookup and d_instantiate/d_splice_alias hooked? -- Stephen Smalley <sds(a)tycho.nsa.gov&gt; National Security Agency

On Friday 25 March 2005 07:28 am, Stephen Smalley wrote: Hello, I've kind of struggled with this one and am was a bit reluctant to add it. Perhaps my logic is right, bu there's a better placement. The reason why the hook was placed in __d_lookup() was to auto-update a hardlink with the correct watch. The only way a hardlink will generate audit records is if it's inode is being watched and the only way the inode can be watched is if one of it's dentry's is at a watch point. So, take this scenario for example -- this is how we should currently perform: watch /tmp/foo # We get a record for "foo" cat /tmp/foo ln /tmp/foo /tmp/bar watch /tmp/bar # We get a record for "foo" cat /tmp/bar rm /tmp/foo # __d_lookup() does its magic and we get a record for "bar" cat /tmp/bar This might be a completely assanine case now.... If /tmp/bar has a "natural" watch (ie: there's a watch on /tmp for "bar"), and /tmp/bar is a hardlink to /tmp/foo, then if /tmp/foo left (it was the dentry for the inode that was fufilling the watch criteria), then the inode would lose its "foo" watch and /tmp/bar would be watchless. But /tmp/bar is being watched! So we use __d_lookup() to automatically detect whether or not there is a "natural" watch in its current path that it can attach too. This could get a little more interesting if there are N hardlinks to /tmp/foo which are all being watched. Then, any of the /tmp/XN's could claim the inode and attach their watch -- it depends on what /tmp/XN calls the audiit_attach_watch() first. Comments? Opinions? -tim

On Fri, 2005-03-25 at 12:25 -0500, Stephen Smalley wrote: Ok, I guess not, as the inode will eventually become "unwatched" anyway if it is evicted and then re-looked up as /tmp/bar or if the system reboots. The particular scenario seems a bit contrived, but the more general case you mention later (file has multiple hard links, all with watches defined a priori, the inode picks up the watch for whatever name is used first to access it, and then that link is deleted) does seem like a legitimate concern, as you don't want to lose auditing for accessing the inode via the other links at that point. -- Stephen Smalley <sds(a)tycho.nsa.gov&gt; National Security Agency

On Fri, 2005-03-25 at 10:46 -0600, Timothy R. Chavez wrote: Are you also relying on the __d_lookup() hook to properly update/clear i_audit->wentry fields for inodes already in the dcache for removed watches (i.e. after an auditctl -W /tmp/foo, the subsequent audit_attach_watch call by __d_lookup is what will reset the i_audit field for /tmp/foo)? -- Stephen Smalley <sds(a)tycho.nsa.gov&gt; National Security Agency

On Friday 25 March 2005 07:04 am, Stephen Smalley wrote: I think we should keep it simple for the time being and go with this. I'm not entirely sure we should hook mknod or symlink. We're not making any claims about the watchability of a device, or symlink with this code. Do you agree? -tim

On Fri, 2005-03-25 at 12:05 -0500, Stephen Smalley wrote: As a trivial test of the ability to audit reads and writes to device nodes already, I did: auditctl -w /dev/null and then did: echo hello > /dev/null As expected, this generated an audit record. Hence, while it may be fine to omit symlinks, I see no reason to not include an audit_notify_watch call at the end of vfs_mknod that allows you to generate an audit record for device creations based on name, as you can already attach watches to device nodes and generate audit for opens on them. -- Stephen Smalley <sds(a)tycho.nsa.gov&gt; National Security Agency

On Thursday 24 March 2005 10:28 am, Stephen Smalley wrote: Is this too bold of a statement: fs/inode.c: inode_alloc if (audit_inode_alloc(inode) && security_inode_alloc(inode)) { .... } I figure, that if either is unsuccessful, we're probably not in any shape to audit anyway... and visa versa. Is this a reasonable assumption? Or is it too assertive? -tim

On Thursday 24 March 2005 10:28 am, Stephen Smalley wrote: I took your advice on just initializing inode->i_audit to NULL like the security field. So would this be reasonable Stephen? if (audit_inode_free(inode) || security_inode_alloc(inode)) { audit_inode_free(inode); security_inode_free(inode); if (inode->i_sb->s_op->destroy_inode) inode->i_sb->s_op->destroy_inode(inode); else kmem_cache_free(inode_cachep, (inode)); return NULL; } This way, in either case where there could be leakage, we clean up the memory before the inode goes bye-bye. -tim

On Thu, 2005-03-24 at 16:02 -0600, Timothy R. Chavez wrote: 1,1 s/audit_inode_free/audit_inode_alloc/ You don't need the security_inode_free() call on the error handling path. Consider the cases: 1) audit_inode_alloc() fails, so we don't call security_inode_alloc() and both i_audit and i_security are NULL. The audit_inode_free() call on the error path does no harm. 2) audit_inode_alloc() succeeds and security_inode_alloc() fails, so i_audit is non-NULL and i_security is NULL. In this case, we need the audit_inode_free() call on the error handling path. In neither case do you need a security_inode_free() call on the error handling path. -- Stephen Smalley <sds(a)tycho.nsa.gov&gt; National Security Agency

On Fri, 2005-03-25 at 14:54 +0000, David Woodhouse wrote: I don't think so; I think all callers of audit_notify_watch() can sleep at the point of the call (unlike callers of audit_attach_watch, which must not sleep, but that only attaches watches; it doesn't do any audit generation). Now for SELinux avc_audit, that would be an issue, because it cannot perform blocking allocation or otherwise deal with failures. -- Stephen Smalley <sds(a)tycho.nsa.gov&gt; National Security Agency