Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <81d0122d14c4fbb3a2ad33d25fdf2dd001c7dcc7.1552737854.git.rgb@redhat.com>
 <CAHC9VhS61LTQMXiALRre+5_+wvwe6Wa9RN-WYNTjvz5LVsA5rw@mail.gmail.com> <20190321005008.wfz3bk7q262km5fz@madcap2.tricolour.ca>
In-Reply-To: <20190321005008.wfz3bk7q262km5fz@madcap2.tricolour.ca>
From:   Paul Moore <paul@paul-moore.com>
Date:   Wed, 20 Mar 2019 21:03:45 -0400
Message-ID: <CAHC9VhTPBF=8PNr4800yTWedrxxO91n8AQ8xmkBiNMK+cxGNGg@mail.gmail.com>
Subject: Re: [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record
 to syscall event
To:     Richard Guy Briggs <rgb@redhat.com>
Cc:     linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org,
        Linux-Audit Mailing List <linux-audit@redhat.com>,
        LKML <linux-kernel@vger.kernel.org>, sgrubb@redhat.com,
        omosnace@redhat.com, Eric Paris <eparis@parisplace.org>,
        Serge Hallyn <serge@hallyn.com>, zohar@linux.ibm.com,
        mjg59@google.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Mar 20, 2019 at 8:50 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-03-20 19:48, Paul Moore wrote:
> > On Sat, Mar 16, 2019 at 8:10 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > In commit fa516b66a1bf ("EVM: Allow runtime modification of the set of
> > > verified xattrs"), the call to audit_log_start() is missing a context to
> > > link it to an audit event. Since this event is in user context, add
> > > the process' syscall context to the record.
> > >
> > > In addition, the orphaned keyword "locked" appears in the record.
> > > Normalize this by changing it to "xattr=(locked)".
> > >
> > > Please see the github issue
> > > https://github.com/linux-audit/audit-kernel/issues/109
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > >  security/integrity/evm/evm_secfs.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)

...

> > > @@ -222,7 +223,7 @@ static ssize_t evm_write_xattrs(struct file *file, const char __user *buf,
> > >                 inode_lock(inode);
> > >                 err = simple_setattr(evm_xattrs, &newattrs);
> > >                 inode_unlock(inode);
> > > -               audit_log_format(ab, "locked");
> > > +               audit_log_format(ab, "xattr=(locked)");
> >
> > Two things come to mind:
> >
> > * While we can clearly trust the string above, should we be logging
> > the xattr field value as an untrusted string so it is consistent with
> > how we record other xattr names?
>
> That would be a question for Steve.

Yep, that's who I wanted to hear from, it's not really something I
expected you to answer Richard.  I vaguely remember something about
Steve's audit libs being able to handle both trusted and untrusted
value strings for a given field, but I could have confused "able to
handle" with "don't care".

-- 
paul moore
www.paul-moore.com