From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ondrej Mosnacek Subject: Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths Date: Fri, 3 Aug 2018 09:08:26 +0200 Message-ID: References: <20180802114436.1209-1-omosnace@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx1.redhat.com (ext-mx14.extmail.prod.ext.phx2.redhat.com [10.5.110.43]) by smtp.corp.redhat.com (Postfix) with ESMTPS id B85AC173CC for ; Fri, 3 Aug 2018 07:08:38 +0000 (UTC) Received: from mail-oi0-f71.google.com (mail-oi0-f71.google.com [209.85.218.71]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9C3F0308FBA9 for ; Fri, 3 Aug 2018 07:08:38 +0000 (UTC) Received: by mail-oi0-f71.google.com with SMTP id u11-v6so3906840oif.22 for ; Fri, 03 Aug 2018 00:08:38 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: Paul Moore Cc: Richard Guy Briggs , Linux-Audit Mailing List List-Id: linux-audit@redhat.com On Fri, Aug 3, 2018 at 12:24 AM Paul Moore wrote: > On Thu, Aug 2, 2018 at 7:45 AM Ondrej Mosnacek 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 > > --- > > 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. > 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). -- Ondrej Mosnacek Associate Software Engineer, Security Technologies Red Hat, Inc.