Re: [PATCH v1] audit: fix suffixed '/' filename matching in __audit_inode_child()

[Paul Moore]

On Wed, Nov 13, 2024 at 11:09 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> On Wed, Nov 13, 2024 at 10:23:55PM -0500, Paul Moore wrote:
> > > And while we are at it,
> > > parentlen = parentlen == AUDIT_NAME_FULL ? parent_len(path) : parentlen;
> > > is a bloody awful way to spell
> > > if (parentlen == AUDIT_NAME_FULL)
> > >  parentlen = parent_len(path);
> > > What's more, parent_len(path) starts with *yet* *another* strlen(path),
> > > followed by really awful crap - we trim the trailing slashes (if any),
> > > then search for the last slash before that...  is that really worth
> > > the chance to skip that strncmp()?
> >
> > Pretty much all of the audit code is awkward at best Al, you should know
> > that.
>
> Do I ever...
>
> > We're not going to fix it all in one patch, and considering the nature
> > of this patch effort, I think there is going to be a lot of value in keeping
> > the initial fix patch to a minimum to ease backporting.  We can improve on
> > some of those other issues in a second patch or at a later time.
> >
> > As a reminder to everyone, patches are always welcome.  Fixing things is a
> > great way to channel disgust into something much more useful.
>
> > >
> > > > +       if (p[pathlen - 1] == '/')
> > > > +               pathlen--;
> > > > +
> > > > +       if (pathlen != dlen)
> > > > +               return 1;
> > > >
> > > > return strncmp(p, dname->name, dlen);
> > >
> > > ... which really should've been memcmp(), at that.
> >
> > Agreed. See above.
>
> OK, my primary interest here is to separate struct filename from that stuff
> as much as possible.  So we will end up stomping on the same ground for
> a while here.  FWIW, my current filename-related pile is in #next.filename;
> there will be a lot more on the VFS side, but one of the obvious targets is
> ->aname, so __audit_inode() and its vicinity will get affected.  We'll need
> to coordinate that stuff.

[Sorry for the delay, it took me a bit longer than expected to get
through my inbox upon returning home, but I should be mostly caught up
now.]

We've talked a bit in other threads about the struct filename changes
you're working on, and while I haven't looked at your next.filename
branch, the stuff you were talking about doing made sense.  As far as
cross-tree conflicts go, I don't think we'll have too many problems
there as I don't foresee any major audit work happening in the
immediate future; sure we'll likely have a small number of fixes but
those should be easy enough to manage with your changes.  I'm assuming
your next.filename finds its way into linux-next?  If so, that should
give us a heads-up regarding conflicts.  If there is a nasty
collision, I'm confident we can figure something out.

> Anyway, regarding audit_compare_dname_path(), handling just the last '/' is
> not enough - there might be any number of trailing slashes, not just one.

Good reminder, thanks.  I see that Ricardo has sent an updated patch,
I haven't looked at it yet (waiting for the merge window to close),
but hopefully he has taken that into account (hint: now would be a
good time to verify that Ricardo <g>).

> Another fun issue with looking for matches is this:
>
> mkdir /tmp/foo
> mkdir /tmp/foo/bar
> mkdir /tmp/blah
> ln -s ../foo/bar/baz /tmp/blah/barf
> echo crap > /tmp/blah/barf
>
> The last one will create a regular file "baz" in /tmp/foo/bar and write
> "crap\n" into it.  With the only pathname passed to open(2) being
> "/tmp/blah/barf".  And there may be a longer chain of symlinks like that.
>
> What do you want to see reported in such case?

In the absence of anyone pushing third party requirements on the audit
subsystem to meet various security certification goals, my guiding
light has been to try and do the simplest thing that makes sense.
Looking at this quickly, there are three different types of events:
mkdir, symlink, and a file lookup/write.

The mkdir events in the example above are trivial, you log the usual
info when walking the pathname and call it good.

The symlink is mostly the same as mkdir, but you've got the relative
path to contend with so the current working directory is more
important and should be logged as well.

The file lookup/write can be a bit more complicated if we have all of
the symlink information as part of the pathname lookup/walk, but if we
don't have that I think we can manage with just the "/tmp/blah/barf".
Users/admins that really care about links will likely be logging the
various link*/symlink*/unlink* syscalls for the directories/files they
care about.

Of course if anyone has any requirements or reasoned opinions about
what we should log here, I'd love to hear it.  This includes you Al.

-- 
paul-moore.com