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

[Paul Moore]

On Nov  5, 2024 Ricardo Robaina <rrobaina@xxxxxxxxxx> 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 cleaning the input and passing the correct
> filename to audit_compare_dname_path().
> 
> Steps to reproduce the issue:
> 
>  # auditctl -w /tmp
>  $ mkdir /tmp/foo
>  $ rm -r /tmp/foo/ or rmdir /tmp/foo/
>  # ausearch -i | grep PATH | tail -3
> 
> This patch is based on a GitHub patch/PR by user @hqh2010.
> https://github.com/linux-audit/audit-kernel/pull/148
> 
> Signed-off-by: Ricardo Robaina <rrobaina@xxxxxxxxxx>
> ---
>  kernel/auditsc.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 6f0d6fb6523f..d4fbac6b71a8 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2419,7 +2419,8 @@ void __audit_inode_child(struct inode *parent,
>  	struct audit_names *n, *found_parent = NULL, *found_child = NULL;
>  	struct audit_entry *e;
>  	struct list_head *list = &audit_filter_list[AUDIT_FILTER_FS];
> -	int i;
> +	int i, dlen, nlen;
> +	char *fn = NULL;
>  
>  	if (context->context == AUDIT_CTX_UNUSED)
>  		return;
> @@ -2443,6 +2444,7 @@ void __audit_inode_child(struct inode *parent,
>  	if (inode)
>  		handle_one(inode);
>  
> +	dlen = strlen(dname->name);
>  	/* look for a parent entry first */
>  	list_for_each_entry(n, &context->names_list, list) {
>  		if (!n->name ||
> @@ -2450,15 +2452,27 @@ void __audit_inode_child(struct inode *parent,
>  		     n->type != AUDIT_TYPE_UNKNOWN))
>  			continue;
>  
> +		/* special case, entry name has the sufix "/" */

/sufix/suffix/

> +		nlen = strlen(n->name->name);
> +		if (dname->name[dlen - 1] != '/' && n->name->name[nlen - 1] == '/') {

I'm guessing @dname is never going to have a trailing slash so we don't
care about @n missing the trailing slash?

> +			fn = kmalloc(PATH_MAX, GFP_KERNEL);
> +			if (!fn) {
> +				audit_panic("out of memory in __audit_inode_child()");
> +				return;
> +			}
> +			strscpy(fn, n->name->name, nlen);
> +		}

I'm looking at the extra work involved above with the alloc/copy and I'm
wondering if we can't solve this a bit more generically (I suspect all
the audit_compare_dname_path() callers may have similar issues) and with
out the additional alloc/copy.

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);
+       if (p[pathlen - 1] == '/')
+               pathlen--;
+
+       if (pathlen != dlen)
+               return 1;
 
        return strncmp(p, dname->name, dlen);
 }

>  		if (n->ino == parent->i_ino && n->dev == parent->i_sb->s_dev &&
>  		    !audit_compare_dname_path(dname,
> -					      n->name->name, n->name_len)) {
> +					      fn ? fn : n->name->name, n->name_len)) {
>  			if (n->type == AUDIT_TYPE_UNKNOWN)
>  				n->type = AUDIT_TYPE_PARENT;
>  			found_parent = n;
>  			break;
>  		}
>  	}
> +	kfree(fn);
>  
>  	cond_resched();
>  
> -- 
> 2.47.0

--
paul-moore.com