linux-kernel - Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 30 May 2018 19:34:13 -0400
From:   Richard Guy Briggs <rgb@...hat.com>
To:     Stefan Berger <stefanb@...ux.vnet.ibm.com>
Cc:     Paul Moore <paul@...l-moore.com>, linux-integrity@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-audit@...hat.com
Subject: Re: [PATCH 8/8] ima: Differentiate auditing policy rules from
 "audit" actions

On 2018-05-30 17:38, Stefan Berger wrote:
> On 05/30/2018 05:22 PM, Paul Moore wrote:
> > On Wed, May 30, 2018 at 9:08 AM, Stefan Berger
> > <stefanb@...ux.vnet.ibm.com> wrote:
> > > On 05/30/2018 08:49 AM, Richard Guy Briggs wrote:
> > > > On 2018-05-24 16:11, Stefan Berger wrote:
> > > > > The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and
> > > > > the IMA "audit" policy action.  This patch defines
> > > > > AUDIT_INTEGRITY_POLICY_RULE to reflect the IMA policy rules.
> > > > > 
> > > > > With this change we now call integrity_audit_msg_common() to get
> > > > > common integrity auditing fields. This now produces the following
> > > > > record when parsing an IMA policy rule:
> > > > > 
> > > > > type=UNKNOWN[1806] msg=audit(1527004216.690:311): action=dont_measure \
> > > > >     fsmagic=0x9fa0 pid=1613 uid=0 auid=0 ses=2 \
> > > > >     subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \
> > > > >     op=policy_update cause=parse_rule comm="echo" exe="/usr/bin/echo" \
> > > > >     tty=tty2 res=1
> > > > > 
> > > > > Signed-off-by: Stefan Berger <stefanb@...ux.vnet.ibm.com>
> > > > > ---
> > > > >    include/uapi/linux/audit.h          | 3 ++-
> > > > >    security/integrity/ima/ima_policy.c | 5 +++--
> > > > >    2 files changed, 5 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > > > > index 4e61a9e05132..776e0abd35cf 100644
> > > > > --- a/include/uapi/linux/audit.h
> > > > > +++ b/include/uapi/linux/audit.h
> > > > > @@ -146,7 +146,8 @@
> > > > >    #define AUDIT_INTEGRITY_STATUS            1802 /* Integrity enable
> > > > > status */
> > > > >    #define AUDIT_INTEGRITY_HASH      1803 /* Integrity HASH type */
> > > > >    #define AUDIT_INTEGRITY_PCR       1804 /* PCR invalidation msgs */
> > > > > -#define AUDIT_INTEGRITY_RULE       1805 /* policy rule */
> > > > > +#define AUDIT_INTEGRITY_RULE       1805 /* IMA "audit" action policy
> > > > > msgs  */
> > > > > +#define AUDIT_INTEGRITY_POLICY_RULE 1806 /* IMA policy rules */
> > > > >      #define AUDIT_KERNEL                2000    /* Asynchronous audit
> > > > > record. NOT A REQUEST. */
> > > > >    diff --git a/security/integrity/ima/ima_policy.c
> > > > > b/security/integrity/ima/ima_policy.c
> > > > > index 3aed25a7178a..a8ae47a386b4 100644
> > > > > --- a/security/integrity/ima/ima_policy.c
> > > > > +++ b/security/integrity/ima/ima_policy.c
> > > > > @@ -634,7 +634,7 @@ static int ima_parse_rule(char *rule, struct
> > > > > ima_rule_entry *entry)
> > > > >          int result = 0;
> > > > >          ab = integrity_audit_log_start(NULL, GFP_KERNEL,
> > > > > -                                      AUDIT_INTEGRITY_RULE);
> > > > > +                                      AUDIT_INTEGRITY_POLICY_RULE);
> > > > Is it possible to connect this record to a syscall by replacing the
> > > > first parameter (NULL) by current->context?
> > We're likely going to need to "associate" this record (audit speak for
> > making the first parameter non-NULL) with others for the audit
> > container ID work.  If you do it now, Richard's patches will likely
> > get a few lines smaller and that will surely make him a bit happier :)
> 
> Richard is also introducing a local context that we can then create and use
> instead of the NULL. Can we not use that then?

That is for records for which there is no syscall or user associated.

In fact there is another recent change that would be better to use than
current->audit_context, which is the function audit_context().
See commit cdfb6b3 ("audit: use inline function to get audit context").

> Steven seems to say: "We don't want to add syscall records to everything.
> That messes up schemas and existing code. The integrity events are 1 record
> in size and should stay that way. This saves disk space and improves
> readability."
> 
> > > We would have to fix current->context in this case since it is NULL. We get
> > > to this location by root cat'ing a policy or writing a policy filename into
> > > /sys/kernel/security/ima/policy.
> > Perhaps I'm missing something, but current in this case should point
> > to the process which is writing to the policy file, yes?
> 
> Yes, but current->context is NULL for some reason.

Is it always this way?  If it isn't, which it should not be, we should
find out why.  Well, we should find out why this is NULL anyways, since
it shouldn't be.

- RGB

--
Richard Guy Briggs <rgb@...hat.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