From: Steve Grubb <sgrubb@redhat.com>
To: Richard Guy Briggs <rgb@redhat.com>
Cc: Paul Moore <paul@paul-moore.com>,
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>,
omosnace@redhat.com, Eric Paris <eparis@parisplace.org>,
Serge Hallyn <serge@hallyn.com>,
zohar@linux.ibm.com, mjg59@google.com
Subject: Re: [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event
Date: Tue, 26 Mar 2019 11:22:07 -0400 [thread overview]
Message-ID: <2006016.NXIvICiRTL@x2> (raw)
In-Reply-To: <20190321005008.wfz3bk7q262km5fz@madcap2.tricolour.ca>
On Wednesday, March 20, 2019 8:50:08 PM EDT Richard Guy Briggs 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(-)
> > >
> > > diff --git a/security/integrity/evm/evm_secfs.c
> > > b/security/integrity/evm/evm_secfs.c index 015aea8fdf1e..4171d174e9da
> > > 100644
> > > --- a/security/integrity/evm/evm_secfs.c
> > > +++ b/security/integrity/evm/evm_secfs.c
> > > @@ -192,7 +192,8 @@ static ssize_t evm_write_xattrs(struct file *file,
> > > const char __user *buf,> >
> > > if (count > XATTR_NAME_MAX)
> > >
> > > return -E2BIG;
> > >
> > > - ab = audit_log_start(NULL, GFP_KERNEL,
> > > AUDIT_INTEGRITY_EVM_XATTR);
> > > + ab = audit_log_start(audit_context(), GFP_KERNEL,
> > > + AUDIT_INTEGRITY_EVM_XATTR);
> >
> > This part is fine.
> >
> > > if (!ab)
> > >
> > > return -ENOMEM;
> > >
> > > @@ -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.
All fields with the same name must be represented the same way. If one
instance is untrusted, every instance of the same keyword must be untrusted.
-Steve
> > * I'm not sure you can ever have parens in a xattr (I would hope not),
> > but if we are going to use the xattr field, perhaps we should simply
> > stick with the name as provided (".") so we don't ever run afoul of
> > xattr names? I'm curious to hear what the IMA/EVM folks think of
> > this.
>
> The legal xaddr names start with XATTR_SECURITY_PREFIX which is
> "security." so there is no danger of collision with legal names, but I
> suppose someone could try to use "(locked)" as a name which would look
> identical but fail with a different res= number. I think I prefer your
> idea of printing the given value verbatim.
>
> > paul moore
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635
next prev parent reply other threads:[~2019-03-26 15:22 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-16 12:10 [PATCH ghak109 V1] audit: link integrity evm_write_xattrs record to syscall event Richard Guy Briggs
2019-03-20 23:48 ` Paul Moore
2019-03-21 0:50 ` Richard Guy Briggs
2019-03-21 1:03 ` Paul Moore
2019-03-26 15:11 ` Mimi Zohar
2019-03-26 15:22 ` Steve Grubb [this message]
2019-03-26 15:29 ` Mimi Zohar
2019-03-26 16:14 ` Richard Guy Briggs
2019-03-26 17:42 ` Richard Guy Briggs
2019-03-26 17:55 ` Matthew Garrett
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2006016.NXIvICiRTL@x2 \
--to=sgrubb@redhat.com \
--cc=eparis@parisplace.org \
--cc=linux-audit@redhat.com \
--cc=linux-integrity@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=mjg59@google.com \
--cc=omosnace@redhat.com \
--cc=paul@paul-moore.com \
--cc=rgb@redhat.com \
--cc=serge@hallyn.com \
--cc=zohar@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).