Audit link denied events were being unexpectedly produced in a disjointway when audit was disabled, and when they were expected, there wereduplicate PATH records. This patchset addresses both issues forsymlinks and hardlinks.

The remaining problem is how to address this when syscall logging isdisabled since it needs a parent path record and/or a CWD record tocomplete it. It could also use a proctitle record too. In fact, itlooks like we need a way to have multiple auxiliary records to supportan arbitrary record. Comments please.

Hmm, it's been a while since I've looked at the audit vfs/inode code,but isn't the audit_inode() call directly above effectively aduplicate of the audit_inode(nd->name, nd->stack[0].link.dentry, 0)call you added in patch 3/4?

Hmm, it's been a while since I've looked at the audit vfs/inode code,but isn't the audit_inode() call directly above effectively aduplicate of the audit_inode(nd->name, nd->stack[0].link.dentry, 0)call you added in patch 3/4?

It gets the full pathname string of the link, then converts it to astruct filename*, and then below, we feed it that struct filename* withthe parent's dentry and the LOOKUP_PARENT flag to drop the last part ofthe pathname and set the filetype to PARENT.

I didn't try, it, but I expect if I feed it parent's dentry above, I'dget only the parent pathname and when I feed it to audit_inode() belowit would again drop the last part of the pathname when the LOOKUP_PARENTflag is included, or not label it as a filetype PARENT if we don'tinclude the flag to get the full parent pathname.

Hmm, it's been a while since I've looked at the audit vfs/inode code,but isn't the audit_inode() call directly above effectively aduplicate of the audit_inode(nd->name, nd->stack[0].link.dentry, 0)call you added in patch 3/4?

It gets the full pathname string of the link, then converts it to astruct filename*, and then below, we feed it that struct filename* withthe parent's dentry and the LOOKUP_PARENT flag to drop the last part ofthe pathname and set the filetype to PARENT.

I think that last bit is what I was forgetting, thanks.

Post by Richard Guy BriggsI didn't try, it, but I expect if I feed it parent's dentry above, I'dget only the parent pathname and when I feed it to audit_inode() belowit would again drop the last part of the pathname when the LOOKUP_PARENTflag is included, or not label it as a filetype PARENT if we don'tinclude the flag to get the full parent pathname.

1. We need to allocate a buffer to feed to d_absolute_path(), andgetname_kernel() is just going to make another copy so we need to makesure we clean up after ourselves. Perhaps I missing it, but I'm notseeing where we free the kmalloc'd buffer or call putname(). Feelfree to correct me if I'm missing something obvious.

2. While the audit_* calls are going to bail early in the cases whereaudit is disabled, or not configured, we are going to pay the penaltyfor the kmalloc() call above, as well as the getname_kernel() andd_absolute_path() calls below. I think it might be beneficial tocreate a new function (audit_log_symlink_denied() perhaps?) whichencapsulates all the audit bits in may_follow_link() and exits earlywhen needed. What do you think?

(Point #2 is why I didn't merge patch 3/4, just include it in thisrevised patch)

1. We need to allocate a buffer to feed to d_absolute_path(), andgetname_kernel() is just going to make another copy so we need to makesure we clean up after ourselves. Perhaps I missing it, but I'm notseeing where we free the kmalloc'd buffer or call putname(). Feelfree to correct me if I'm missing something obvious.

That is put very diplomatically! Both fixed, with audit_panic() asneeded.

Post by Paul Moore2. While the audit_* calls are going to bail early in the cases whereaudit is disabled, or not configured, we are going to pay the penaltyfor the kmalloc() call above, as well as the getname_kernel() andd_absolute_path() calls below. I think it might be beneficial tocreate a new function (audit_log_symlink_denied() perhaps?) whichencapsulates all the audit bits in may_follow_link() and exits earlywhen needed. What do you think?

I agree a seperate function is appropriate. There was some back andforth between namei.c and audit.c due to struct nameidata andaudit_panic() not able to co-exist due to them both being local to theirrespective sub-systems.

Post by Paul Moore(Point #2 is why I didn't merge patch 3/4, just include it in thisrevised patch)

On reviewing this, I'm not totally convinced the parent record isnecessary to fully understand what happenned since there is a CWDrecord. I left 3 and 4 as seperate patches in case it is found thatonly 3 is necessary.

Post by Paul Moore(Point #2 is why I didn't merge patch 3/4, just include it in thisrevised patch)

On reviewing this, I'm not totally convinced the parent record isnecessary to fully understand what happenned since there is a CWDrecord. I left 3 and 4 as seperate patches in case it is found thatonly 3 is necessary.

Doesn't this means errors here would be silent if audit isn't enabled?I don't that; sysadmins should see this notification regardless of theaudit state...

This is a user error and not a system error, so I would think if systemauditing is disabled, they don't care about this kind of error.

It could indicate an attack attempt...

We get beat up by several folks when we emit audit records with auditdisabled, and they have a very valid point.I'm not arguing that the information isn't useful, I'm arguing that ifyou are interested in the sort of information that audit provides youshould enable audit. :)

Post by Richard Guy BriggsAudit link denied events were being unexpectedly produced in a disjointway when audit was disabled, and when they were expected, there wereduplicate PATH records. This patchset addresses both issues forsymlinks and hardlinks.This was introduced withcommit b24a30a7305418ff138ff51776fc555ec57c011a("audit: fix event coverage of AUDIT_ANOM_LINK")commit a51d9eaa41866ab6b4b6ecad7b621f8b66ece0dc("fs: add link restriction audit reporting")

Perhaps this can only be emitted correctly with SYSCALL auditing enabled.Otherwise, the event should stand completely on its own without syscall andpath records. The information from them can be added, but it risks hittingthe record size limit.

Post by Richard Guy BriggsAudit link denied events were being unexpectedly produced in a disjointway when audit was disabled, and when they were expected, there wereduplicate PATH records. This patchset addresses both issues forsymlinks and hardlinks.This was introduced withcommit b24a30a7305418ff138ff51776fc555ec57c011a("audit: fix event coverage of AUDIT_ANOM_LINK")commit a51d9eaa41866ab6b4b6ecad7b621f8b66ece0dc("fs: add link restriction audit reporting")

Perhaps this can only be emitted correctly with SYSCALL auditing enabled.Otherwise, the event should stand completely on its own without syscall andpath records. The information from them can be added, but it risks hittingthe record size limit.

As Paul just pointed out (which rang a bell...) in:https://github.com/linux-audit/audit-kernel/issues/51#issuecomment-365759325CONFIG_AUDITSYSCALL is now forced on and if you sabbotage youraudit.rules with -a task,never, your warranty is void.

So now, the lurking questions in the back of my head about theavailability of syscall records has been alleviated and we should alwayssee a syscall record available unless an audit rule says we are notinterested.

Post by Richard Guy BriggsAudit link denied events were being unexpectedly produced in a disjointway when audit was disabled, and when they were expected, there wereduplicate PATH records. This patchset addresses both issues forsymlinks and hardlinks.This was introduced withcommit b24a30a7305418ff138ff51776fc555ec57c011a("audit: fix event coverage of AUDIT_ANOM_LINK")commit a51d9eaa41866ab6b4b6ecad7b621f8b66ece0dc("fs: add link restriction audit reporting")

Have these been tested with ausearch-test?

Not yet.

I should have reported that a day or two later I ran the ausearch-testwhich passed.

Perhaps this can only be emitted correctly with SYSCALL auditing enabled.Otherwise, the event should stand completely on its own without syscall andpath records. The information from them can be added, but it risks hittingthe record size limit.

https://github.com/linux-audit/audit-kernel/issues/51#issuecomment-365759325CONFIG_AUDITSYSCALL is now forced on and if you sabbotage youraudit.rules with -a task,never, your warranty is void.So now, the lurking questions in the back of my head about theavailability of syscall records has been alleviated and we should alwayssee a syscall record available unless an audit rule says we are notinterested.