Re: null pointer dereference regression in 5.7

From: Paul Moore <paul@paul-moore.com>
To: Dominick Grift <dominick.grift@defensec.nl>,
	bauen1 <j2468h@googlemail.com>,
	rgb@redhat.com
Cc: linux-audit@redhat.com
Subject: Re: null pointer dereference regression in 5.7
Date: Tue, 21 Jul 2020 18:45:58 -0400	[thread overview]
Message-ID: <CAHC9VhTbMB==FaxwUDviw9D5-M6wUGnYwwHz7wCDMYSOUw07dA@mail.gmail.com> (raw)
In-Reply-To: <CAHC9VhRGJ4EwFYpRtoHe=m_GNB6WAt3cDCf7pd4YbvydEPpy3w@mail.gmail.com>

On Tue, Jul 21, 2020 at 6:30 PM Paul Moore <paul@paul-moore.com> wrote:
> Richard, you broke it, you bought it :)  Did you want to take a closer
> look at this?  If you can't let me know.  Based on a quick look, my
> gut feeling is that either context->pwd is never set properly or it is
> getting free'd prematurely; I'm highly suspicious of the latter but
> the former seems like it might be a reasonable place to start.

Actually, yes, I'm pretty certain the problem is that context->pwd is
never set in this case.

Normally context->pwd would be set by a call to
audit_getname()/__audit_getname(), but if there audit context is a
dummy context, that is skipped and context->pwd is never set.
Normally that is fine, expect with Richard's patch if the kernel
explicitly calls audit_log_start() we mark the context as ... not a
dummy?  smart?  I'm not sure of the right term here ... which then
triggers all the usual logging one would expect.  In this particular
case, a SELinux AVC, the audit_log_start() happens *after* the
pathname has been resolved and the audit_getname() calls are made;
thus in this case context->pwd is not valid when the normal audit
logging takes place on exit and things explode in predictable fashion.

Unfortunately, it is beginning to look like 1320a4052ea1 ("audit:
trigger accompanying records when no rules present") may be more
dangerous than initially thought.  I'm borderline tempted to just
revert this patch, but I'll leave this open for discussion ...
Richard, I think you need to go through the code and audit all of the
functions that store data in an audit context that are skipped when
there is a dummy context to see which fields are potentially unset,
and then look at all the end of task/syscall code to make sure the
necessary set/unset checks are in place.

This should be a priority.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit