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

On Fri, Aug 3, 2018 at 3:08 AM Ondrej Mosnacek <omosnace(a)redhat.com&gt; wrote: > On Fri, Aug 3, 2018 at 12:24 AM Paul Moore <paul(a)paul-moore.com&gt; wrote: > > On Thu, Aug 2, 2018 at 7:45 AM Ondrej Mosnacek <omosnace(a)redhat.com&gt; 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(a)redhat.com&gt; > > > --- > > > 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.

> > 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.