Re: [PATCH v1] audit: fix suffixed '/' filename matching in __audit_inode_child()
From: Paul Moore
Date: Mon Nov 11 2024 - 17:19:54 EST
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