[PATCH] audit: fix a double fetch in audit_log_single_execve_arg()

[Paul Moore]

On Wed, Jul 20, 2016 at 2:43 PM, Paul Moore <pmoore at redhat.com> wrote:
> From: Paul Moore <paul at paul-moore.com>
>
> There is a double fetch problem in audit_log_single_execve_arg()
> where we first check the execve(2) argumnets for any "bad" characters
> which would require hex encoding and then re-fetch the arguments for
> logging in the audit record[1].  Of course this leaves a window of
> opportunity for an unsavory application to munge with the data.
>
> This patch reworks things by only fetching the argument data once[2]
> into a buffer where it is scanned and logged into the audit
> records(s).  In addition to fixing the double fetch, this patch
> improves on the original code in a few other ways: better handling
> of large arguments which require encoding, stricter record length
> checking, and some performance improvements (completely unverified,
> but we got rid of some strlen() calls, that's got to be a good
> thing).
>
> As part of the development of this patch, I've also created a basic
> regression test for the audit-testsuite, the test can be tracked on
> GitHub at the following link:
>
>  * https://github.com/linux-audit/audit-testsuite/issues/25
>
> [1] If you pay careful attention, there is actually a triple fetch
> problem due to a strnlen_user() call at the top of the function.
>
> [2] This is a tiny white lie, we do make a call to strnlen_user()
> prior to fetching the argument data.  I don't like it, but due to the
> way the audit record is structured we really have no choice unless we
> copy the entire argument at once (which would require a rather
> wasteful allocation).  The good news is that with this patch the
> kernel no longer relies on this strnlen_user() value for anything
> beyond recording it in the log, we also update it with a trustworthy
> value whenever possible.
>
> Reported-by: Pengfei Wang <wpengfeinudt at gmail.com>
> Cc: <stable at vger.kernel.org>
> Signed-off-by: Paul Moore <paul at paul-moore.com>
> ---
>  kernel/auditsc.c |  332 +++++++++++++++++++++++++++---------------------------
>  1 file changed, 164 insertions(+), 168 deletions(-)

Generally I would take a patch this late for the upcoming merge
window, but considering the nature of this patch I'm going to make an
exception.  I've added it to the audit#next branch and I've built a
temporary kernel for testing at the link below for anyone who may be
interested; I'm also building a new pcmoore/kernel-secnext COPR kernel
with the patch right now, it should be available in a few hours.

 * https://copr.fedorainfracloud.org/coprs/pcmoore/kernel-testing/build/394059

-- 
paul moore
www.paul-moore.com