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: <20241113230425.GJ3387508@ZenIV>
Date: Wed, 13 Nov 2024 23:04:25 +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 Mon, Nov 11, 2024 at 05:06:43PM -0500, Paul Moore wrote:

> This is completely untested, I didn't even compile it, but what about
> something like the following?  We do add an extra strlen(), but that is
> going to be faster than the alloc/copy.
> 
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 470041c49a44..c30c2ee9fb77 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -1320,10 +1320,13 @@ int audit_compare_dname_path(const struct qstr *dname, const char *path, int par
>                 return 1;
>  
>         parentlen = parentlen == AUDIT_NAME_FULL ? parent_len(path) : parentlen;
> -       if (pathlen - parentlen != dlen)
> -               return 1;
> -
>         p = path + parentlen;
> +       pathlen = strlen(p);

Huh?  We have
        pathlen = strlen(path);
several lines prior to this.  So unless parentlen + path manages to exceed
strlen(path) (in which case your strlen() is really asking for trouble),
this is simply
	pathlen -= parentlen;

What am I missing here?  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()?


> +       if (p[pathlen - 1] == '/')
> +               pathlen--;
> +
> +       if (pathlen != dlen)
> +               return 1;
>  
>         return strncmp(p, dname->name, dlen);

... which really should've been memcmp(), at that.