Re: [PATCH ghak21 V2 3/4] audit: add refused symlink to audit_names

On 2018-03-13 11:38, Steve Grubb wrote: > On Tue, 13 Mar 2018 06:11:08 -0400 > Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > > > On 2018-03-13 09:35, Steve Grubb wrote: > > > On Mon, 12 Mar 2018 11:52:56 -0400 > > > Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > > > > > > > On 2018-03-12 11:53, Paul Moore wrote: > > > > > On Mon, Mar 12, 2018 at 11:26 AM, Richard Guy Briggs > > > > > <rgb(a)redhat.com&gt; wrote: > > > > > > On 2018-03-12 11:12, Paul Moore wrote: > > > > > >> On Mon, Mar 12, 2018 at 2:31 AM, Richard Guy Briggs > > > > > >> <rgb(a)redhat.com&gt; wrote: > > > > > >> > Audit link denied events for symlinks had duplicate PATH > > > > > >> > records rather than just updating the existing PATH record. > > > > > >> > Update the symlink's PATH record with the current dentry > > > > > >> > and inode information. > > > > > >> > > > > > > >> > See: https://github.com/linux-audit/audit-kernel/issues/21 > > > > > >> > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; > > > > > >> > --- > > > > > >> > fs/namei.c | 1 + > > > > > >> > 1 file changed, 1 insertion(+) > > > > > >> > > > > > >> Why didn't you include this in patch 4/4 like I asked during > > > > > >> the previous review? > > > > > > > > > > > > Please see the last comment of: > > > > > > https://www.redhat.com/archives/linux-audit/2018-March/msg00070.html > > > > > > > > > > Yes, I just saw that ... I hadn't seen your replies on the v1 > > > > > patches until I had finished reviewing v2. I just replied to > > > > > that mail in the v1 thread, but basically you need to figure > > > > > out what is necessary here and let us know. If I have to > > > > > figure it out it likely isn't going to get done with enough > > > > > soak time prior to the upcoming merge window. > > > > > > > > Steve? I was hoping you could chime in here. > > > > > > If the CWD record will always be the same as the PARENT record, > > > then we do not need the parent record. Duplicate information is > > > bad. Like all the duplicate SYSCALL information. > > > > The CWD record could be different from the PARENT record, since I > > could have SYMLINK=/tmp/test/symlink, CWD=/tmp, PARENT=/tmp/test. > > Does the parent record even matter since it might not be a directory > > operation like creat, unlink or rename? > > There's 2 issues. One is creating the path if what we have is relative. > In this situation CWD should be enough. But if the question is whether > the PARENT directory should be included...what if the PARENT > permissions do not allow the successful name resolution? In that case > we might only get a PARENT record no? In that case we would need it. I think in the case of symlink creation, normal file create code path would be in effect, and would properly log parent and symlink source file paths (if a rule to log it was in effect) which is not something that would trigger a symlink link denied error. Symlink link denied happens only when trying to actually follow the link before resolving the target path of a read/write/exec of the symlink target. If the parent permissions of the link's target don't allow successful name resolution then the symlink link denied condition isn't met, but rather any other rule that applies to the target path.