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

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

[Ricardo Robaina]

When the user specifies a directory to delete with the suffix '/',
the audit record fails to collect the filename, resulting in the
following logs:

 type=PATH msg=audit(10/30/2024 14:11:17.796:6304) : item=2 name=(null)
 type=PATH msg=audit(10/30/2024 14:11:17.796:6304) : item=1 name=(null)

It happens because the value of the variables dname, and n->name->name
in __audit_inode_child() differ only by the suffix '/'. This commit
treats this corner case by handling pathname's trailing slashes in
audit_compare_dname_path().

Steps to reproduce the issue:

 # auditctl -w /tmp
 $ mkdir /tmp/foo
 $ rm -r /tmp/foo/
 # ausearch -i | grep PATH | tail -3

The first version of this patch was based on a GitHub patch/PR by
user @hqh2010 [1].

Link: https://github.com/linux-audit/audit-kernel/pull/148 [1]

Suggested-by: Paul Moore <paul@paul-moore.com>
Reviewed-by: Richard Guy Briggs <rgb@redhat.com>
Reviewed-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Ricardo Robaina <rrobaina@redhat.com>

---
v2: handling pathname's trailing slashes in audit_compare_dname_path()
v1: https://lore.kernel.org/audit/20241114040948.GK3387508@ZenIV/T/#t
---
 kernel/auditfilter.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 470041c49a44..8ddccdb4a2a7 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -1319,13 +1319,20 @@ int audit_compare_dname_path(const struct qstr *dname, const char *path, int par
 	if (pathlen < dlen)
 		return 1;
 
-	parentlen = parentlen == AUDIT_NAME_FULL ? parent_len(path) : parentlen;
-	if (pathlen - parentlen != dlen)
-		return 1;
+	if (parentlen == AUDIT_NAME_FULL)
+		parentlen = parent_len(path);
 
 	p = path + parentlen;
 
-	return strncmp(p, dname->name, dlen);
+	/* handle trailing slashes */
+	pathlen -= parentlen;
+	while (p[pathlen - 1] == '/')
+		pathlen--;
+
+	if (pathlen != dlen)
+		return 1;
+
+	return memcmp(p, dname->name, dlen);
 }
 
 int audit_filter(int msgtype, unsigned int listtype)
-- 
2.47.0

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

[Paul Moore]

On Nov 22, 2024 Ricardo Robaina <rrobaina@redhat.com> wrote:
> 
> When the user specifies a directory to delete with the suffix '/',
> the audit record fails to collect the filename, resulting in the
> following logs:
> 
>  type=PATH msg=audit(10/30/2024 14:11:17.796:6304) : item=2 name=(null)
>  type=PATH msg=audit(10/30/2024 14:11:17.796:6304) : item=1 name=(null)
> 
> It happens because the value of the variables dname, and n->name->name
> in __audit_inode_child() differ only by the suffix '/'. This commit
> treats this corner case by handling pathname's trailing slashes in
> audit_compare_dname_path().
> 
> Steps to reproduce the issue:
> 
>  # auditctl -w /tmp
>  $ mkdir /tmp/foo
>  $ rm -r /tmp/foo/
>  # ausearch -i | grep PATH | tail -3
> 
> The first version of this patch was based on a GitHub patch/PR by
> user @hqh2010 [1].
> 
> Link: https://github.com/linux-audit/audit-kernel/pull/148 [1]
> 
> Suggested-by: Paul Moore <paul@paul-moore.com>
> Reviewed-by: Richard Guy Briggs <rgb@redhat.com>
> Reviewed-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Ricardo Robaina <rrobaina@redhat.com>
> ---
> v2: handling pathname's trailing slashes in audit_compare_dname_path()
> v1: https://lore.kernel.org/audit/20241114040948.GK3387508@ZenIV/T/#t
> ---
>  kernel/auditfilter.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)

Yes, Richard did provide a reviewed-by tag on the v1 patch, but v2 has
enough changes that I don't think we can reasonably carry that forward;
of course Richard re-review this iteration and provide a new tag.  I'm
going to remove it for now.

Al never provided an explicit reviewed-by tag; simply commenting on a
patch is not the same as providing a 'Reviewed-by', the reviewer will
provide an explicit 'Reviewed-by' tag in their email.  I'm going to
remove Al's tag too.

Other than those issues, I think this looks much better than v1, I'm
going to merge this into audit/dev now, thanks!

> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 470041c49a44..8ddccdb4a2a7 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -1319,13 +1319,20 @@ int audit_compare_dname_path(const struct qstr *dname, const char *path, int par
>  	if (pathlen < dlen)
>  		return 1;
>  
> -	parentlen = parentlen == AUDIT_NAME_FULL ? parent_len(path) : parentlen;
> -	if (pathlen - parentlen != dlen)
> -		return 1;
> +	if (parentlen == AUDIT_NAME_FULL)
> +		parentlen = parent_len(path);
>  
>  	p = path + parentlen;
>  
> -	return strncmp(p, dname->name, dlen);
> +	/* handle trailing slashes */
> +	pathlen -= parentlen;
> +	while (p[pathlen - 1] == '/')
> +		pathlen--;
> +
> +	if (pathlen != dlen)
> +		return 1;
> +
> +	return memcmp(p, dname->name, dlen);
>  }
>  
>  int audit_filter(int msgtype, unsigned int listtype)
> -- 
> 2.47.0

--
paul-moore.com

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

[Ricardo Robaina]

On Thu, Dec 5, 2024 at 9:22 PM Paul Moore <paul@paul-moore.com> wrote:
>
> Yes, Richard did provide a reviewed-by tag on the v1 patch, but v2 has
> enough changes that I don't think we can reasonably carry that forward;
> of course Richard re-review this iteration and provide a new tag.  I'm
> going to remove it for now.
>
> Al never provided an explicit reviewed-by tag; simply commenting on a
> patch is not the same as providing a 'Reviewed-by', the reviewer will
> provide an explicit 'Reviewed-by' tag in their email.  I'm going to
> remove Al's tag too.

I'm sorry about that, I was just trying to give credit to the ones who
helped. Thanks for clarifying the expected review process, I
appreciate it!

> Other than those issues, I think this looks much better than v1, I'm
> going to merge this into audit/dev now, thanks!

Thanks, Paul!

- Ricardo

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

[Richard Guy Briggs]

On 2024-12-06 10:00, Ricardo Robaina wrote:
> On Thu, Dec 5, 2024 at 9:22 PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > Yes, Richard did provide a reviewed-by tag on the v1 patch, but v2 has
> > enough changes that I don't think we can reasonably carry that forward;
> > of course Richard re-review this iteration and provide a new tag.  I'm
> > going to remove it for now.
> >
> > Al never provided an explicit reviewed-by tag; simply commenting on a
> > patch is not the same as providing a 'Reviewed-by', the reviewer will
> > provide an explicit 'Reviewed-by' tag in their email.  I'm going to
> > remove Al's tag too.
> 
> I'm sorry about that, I was just trying to give credit to the ones who
> helped. Thanks for clarifying the expected review process, I
> appreciate it!

Please re-add my reviewed-by tag.

> > Other than those issues, I think this looks much better than v1, I'm
> > going to merge this into audit/dev now, thanks!
> 
> Thanks, Paul!
> 
> - Ricardo

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
Upstream IRC: SunRaycer
Voice: +1.613.860 2354 SMS: +1.613.518.6570

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

[Paul Moore]

On Fri, Dec 6, 2024 at 10:18 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2024-12-06 10:00, Ricardo Robaina wrote:
> > On Thu, Dec 5, 2024 at 9:22 PM Paul Moore <paul@paul-moore.com> wrote:
> > >
> > > Yes, Richard did provide a reviewed-by tag on the v1 patch, but v2 has
> > > enough changes that I don't think we can reasonably carry that forward;
> > > of course Richard re-review this iteration and provide a new tag.  I'm
> > > going to remove it for now.

...

> Please re-add my reviewed-by tag.

Done, it will be pushed back up shortly.

-- 
paul-moore.com