linux-kernel - Re: [PATCH v1] audit: fix suffixed '/' filename matching in __audit_inode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241114040948.GK3387508@ZenIV>
Date: Thu, 14 Nov 2024 04:09:48 +0000
From: Al Viro <viro@...iv.linux.org.uk>
To: Paul Moore <paul@...l-moore.com>
Cc: Ricardo Robaina <rrobaina@...hat.com>, audit@...r.kernel.org,
	linux-kernel@...r.kernel.org, eparis@...hat.com, rgb@...hat.com
Subject: Re: [PATCH v1] audit: fix suffixed '/' filename matching in
 __audit_inode_child()

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.

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

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?