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

[Richard Guy Briggs]

On 2018-03-13 16:24, Paul Moore wrote:
> On Tue, Mar 13, 2018 at 6:52 AM, Richard Guy Briggs <rgb@xxxxxxxxxx> wrote:
> > On 2018-03-13 11:38, Steve Grubb wrote:
> >> On Tue, 13 Mar 2018 06:11:08 -0400
> >> Richard Guy Briggs <rgb@xxxxxxxxxx> wrote:
> >>
> >> > On 2018-03-13 09:35, Steve Grubb wrote:
> >> > > On Mon, 12 Mar 2018 11:52:56 -0400
> >> > > Richard Guy Briggs <rgb@xxxxxxxxxx> wrote:
> >> > >
> >> > > > On 2018-03-12 11:53, Paul Moore wrote:
> >> > > > > On Mon, Mar 12, 2018 at 11:26 AM, Richard Guy Briggs
> >> > > > > <rgb@xxxxxxxxxx> wrote:
> >> > > > > > On 2018-03-12 11:12, Paul Moore wrote:
> >> > > > > >> On Mon, Mar 12, 2018 at 2:31 AM, Richard Guy Briggs
> >> > > > > >> <rgb@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> > > > > >> > ---
> >> > > > > >> >  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.
> 
> I'm guessing you are in the process of tracking all this down, but if
> not, lets get to a point where we can answer this definitively and not
> guess :)

I was fairly certain but being polite, expecting confirmation or
possibly correction if I've overlooked something.

Additionally, this denial message only happens in certain parts of the
permission check for symlinks:
	/proc/sys/fs/protected_symlinks == 1
	and follower and link owner don't match
	and parent sticky and world-writable
	and link parent and link owner don't match

If you want other symlink denials logged, you need to set a rule for the
target filtering on operation failure such as unix file permissions.

The similar situation exists for hardlinks.

> paul moore

- RGB

--
Richard Guy Briggs <rgb@xxxxxxxxxx>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635