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: <19328b27f98.28a7.85c95baa4474aabc7814e68940a78392@paul-moore.com>
Date: Wed, 13 Nov 2024 22:23:55 -0500
From: Paul Moore <paul@...l-moore.com>
To: Al Viro <viro@...iv.linux.org.uk>
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 November 13, 2024 6:04:27 PM Al Viro <viro@...iv.linux.org.uk> wrote:
> 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?

[NOTE: network access is patchy right now so you're getting a phone reply 
without an opportunity to look more closely at the code]

To be fair, this was a quick example of "do something like this" to 
demonstrate a different idea than was proposed in the original patch.  The 
bit of code I shared was not a fully baked patch; I thought that would have 
been clear from the context, if not my comments.

Of course improvements are welcome, but in the future know that unless I'm 
submitting a proper patch, the code snippets I share during review are 
going to be *rough* and not fully developed.  Additional work by the 
original author is expected.

> 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. 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.

--
paul-moore.com