[PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Ondrej Mosnacek]

On Fri, Aug 24, 2018 at 4:09 PM Paul Moore <paul at paul-moore.com> wrote:
>
> On Fri, Aug 3, 2018 at 3:08 AM Ondrej Mosnacek <omosnace at redhat.com> wrote:
> > On Fri, Aug 3, 2018 at 12:24 AM Paul Moore <paul at paul-moore.com> wrote:
> > > On Thu, Aug 2, 2018 at 7:45 AM Ondrej Mosnacek <omosnace at redhat.com> wrote:
> > > >
> > > > When a relative path has just a single component and we want to emit a
> > > > nametype=PARENT record, the current implementation just reports the full
> > > > CWD path (which is alrady available in the audit context).
> > > >
> > > > This is wrong for three reasons:
> > > > 1. Wasting log space for redundant data (CWD path is already in the CWD
> > > >    record).
> > > > 2. Inconsistency with other PATH records (if a relative PARENT directory
> > > >    path contains at least one component, only the verbatim relative path
> > > >    is logged).
> > > > 3. In some syscalls (e.g. openat(2)) the relative path may not even be
> > > >    relative to the CWD, but to another directory specified as a file
> > > >    descriptor. In that case the logged path is simply plain wrong.
> > > >
> > > > This patch modifies this behavior to simply report "." in the
> > > > aforementioned case, which is equivalent to an "empty" directory path
> > > > and can be concatenated with the actual base directory path (CWD or
> > > > dirfd from openat(2)-like syscall) once support for its logging is added
> > > > later. In the meantime, defaulting to CWD as base directory on relative
> > > > paths (as already done by the userspace tools) will be enough to achieve
> > > > results equivalent to the current behavior.
> > > >
> > > > See: https://github.com/linux-audit/audit-kernel/issues/95
> > > >
> > > > Fixes: 9c937dcc7102 ("[PATCH] log more info for directory entry change events")
> > > > Signed-off-by: Ondrej Mosnacek <omosnace at redhat.com>
> > > > ---
> > > >  kernel/audit.c | 9 ++++-----
> > > >  1 file changed, 4 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > index 2a8058764aa6..4f18bd48eb4b 100644
> > > > --- a/kernel/audit.c
> > > > +++ b/kernel/audit.c
> > > > @@ -2127,28 +2127,27 @@ void audit_log_name(struct audit_context *context, struct audit_names *n,
> > > >
> > > >         audit_log_format(ab, "item=%d", record_num);
> > > >
> > > > +       audit_log_format(ab, " name=");
> > > >         if (path)
> > > > -               audit_log_d_path(ab, " name=", path);
> > > > +               audit_log_d_path(ab, NULL, path);
> > > >         else if (n->name) {
> > > >                 switch (n->name_len) {
> > > >                 case AUDIT_NAME_FULL:
> > > >                         /* log the full path */
> > > > -                       audit_log_format(ab, " name=");
> > > >                         audit_log_untrustedstring(ab, n->name->name);
> > > >                         break;
> > > >                 case 0:
> > > >                         /* name was specified as a relative path and the
> > > >                          * directory component is the cwd */
> > > > -                       audit_log_d_path(ab, " name=", &context->pwd);
> > > > +                       audit_log_untrustedstring(ab, ".");
> > >
> > > This isn't a comprehensive review, I only gave this a quick look, but
> > > considering you are only logging "." above we can safely use
> > > audit_log_string() and safe a few cycles.
> >
> > I used audit_log_untrustedstring() to maintain the current norm that
> > the name= field would always contain a quoted string (either in
> > double-quotes or hex-escaped). I don't know if such consistency is
> > important for parsing in userspace (it should probably be robust
> > enough to handle any format), but I wanted to be on the safe side.
>
> In this particular case there should be no visible difference in the
> resulting record and audit_log_string() is going to have less overhead
> in the kernel.

OK, so should I just replace it with:

    audit_log_string(ab, ".");

or should I still escape the path manually:

    audit_log_string(ab, "\".\"");

?

>
> > > Honestly, looking at the rest of this function, why are we using
> > > audit_log_format() in the case where we are simply writing a string
> > > literal?  While I haven't tested it, simple code inspection would seem
> > > to indicate that audit_log_string() should be much quicker than
> > > audit_log_format()?  I realize this isn't strictly a problem from this
> > > patch, but we probably shouldn't be propagating this mistake any
> > > further.
> >
> > Good point. If I happen to be sending a v2 of the patch, I will switch
> > to audit_log_string() where possible. Otherwise, I'll leave it to you
> > to fix up when applying (it will probably clash with your patch
> > anyway).
>
> Please fit it yourself and resubmit.
>
> As a general rule I shouldn't need to fix things like this when
> merging.  While things like this sometimes happen, they are special
> cases and are usually the result of short deadlines, missing devs,
> etc.  In general I try to limit my modifications to merge fuzz and
> minor code changes like whitespace fixes, silly checkpatch warnings,
> etc.

Understood, will do.

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.