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: <ZzPRYCEjL16p0vew@madcap2.tricolour.ca>
Date: Tue, 12 Nov 2024 17:06:24 -0500
From: Richard Guy Briggs <rgb@...hat.com>
To: Paul Moore <paul@...l-moore.com>
Cc: Ricardo Robaina <rrobaina@...hat.com>, audit@...r.kernel.org,
	linux-kernel@...r.kernel.org, eparis@...hat.com
Subject: Re: [PATCH v1] audit: fix suffixed '/' filename matching in
 __audit_inode_child()

On 2024-11-11 17:06, Paul Moore wrote:
> On Nov  5, 2024 Ricardo Robaina <rrobaina@...hat.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@...hat.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@...hat.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