On Mon, Aug 27, 2018 at 3:00 PM Ondrej Mosnacek <[email protected]> wrote: > On Fri, Aug 24, 2018 at 4:09 PM Paul Moore <[email protected]> wrote: > > > > On Fri, Aug 3, 2018 at 3:08 AM Ondrej Mosnacek <[email protected]> wrote: > > > On Fri, Aug 3, 2018 at 12:24 AM Paul Moore <[email protected]> wrote: > > > > On Thu, Aug 2, 2018 at 7:45 AM Ondrej Mosnacek <[email protected]> > > > > 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 <[email protected]> > > > > > --- > > > > > 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, "\".\""); > > ?
Paul, could you please answer this question so I can move forward? :) > > > > > > > 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. Thanks, -- Ondrej Mosnacek <omosnace at redhat dot com> Associate Software Engineer, Security Technologies Red Hat, Inc. -- Linux-audit mailing list [email protected] https://www.redhat.com/mailman/listinfo/linux-audit
