kernel/auditsc.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
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@redhat.com>
---
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 "/" */
+ nlen = strlen(n->name->name);
+ if (dname->name[dlen - 1] != '/' && n->name->name[nlen - 1] == '/') {
+ fn = kmalloc(PATH_MAX, GFP_KERNEL);
+ if (!fn) {
+ audit_panic("out of memory in __audit_inode_child()");
+ return;
+ }
+ strscpy(fn, n->name->name, nlen);
+ }
+
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.0On Nov 5, 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 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@redhat.com>
> ---
> 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
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.
On November 13, 2024 6:04:27 PM Al Viro <viro@zeniv.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
On Wed, Nov 13, 2024 at 10:23:55PM -0500, Paul Moore wrote: > > 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. Do I ever... > 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. OK, my primary interest here is to separate struct filename from that stuff as much as possible. So we will end up stomping on the same ground for a while here. FWIW, my current filename-related pile is in #next.filename; there will be a lot more on the VFS side, but one of the obvious targets is ->aname, so __audit_inode() and its vicinity will get affected. We'll need to coordinate that stuff. Anyway, regarding audit_compare_dname_path(), handling just the last '/' is not enough - there might be any number of trailing slashes, not just one. Another fun issue with looking for matches is this: mkdir /tmp/foo mkdir /tmp/foo/bar mkdir /tmp/blah ln -s ../foo/bar/baz /tmp/blah/barf echo crap > /tmp/blah/barf The last one will create a regular file "baz" in /tmp/foo/bar and write "crap\n" into it. With the only pathname passed to open(2) being "/tmp/blah/barf". And there may be a longer chain of symlinks like that. What do you want to see reported in such case?
On 2024-11-11 17:06, Paul Moore wrote:
> On Nov 5, 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 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@redhat.com>
> > ---
> > 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.
I had similar concerns about the alloc/copy and using a fixed length
compare but hadn't thought of generalizing it.
> 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.
I agree this is a better approach.
> 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
- 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
© 2016 - 2024 Red Hat, Inc.