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]

Message-Id: <85d2a40a-884c-c63d-50f6-024f7bbea4a8@linux.vnet.ibm.com>
Date:   Wed, 30 May 2018 17:49:35 -0400
From:   Stefan Berger <stefanb@...ux.vnet.ibm.com>
To:     Paul Moore <paul@...l-moore.com>
Cc:     Steve Grubb <sgrubb@...hat.com>, zohar@...ux.vnet.ibm.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 05/30/2018 05:24 PM, Paul Moore wrote:
> On Wed, May 30, 2018 at 3:54 PM, Stefan Berger
> <stefanb@...ux.vnet.ibm.com> wrote:
>> On 05/30/2018 12:27 PM, Steve Grubb wrote:
>>> On Wednesday, May 30, 2018 11:25:05 AM EDT Stefan Berger wrote:
>>>> On 05/30/2018 11:15 AM, Steve Grubb wrote:
>>>>> On Wednesday, May 30, 2018 9:54:00 AM EDT Stefan Berger wrote:
>>>>>> On 05/29/2018 05:30 PM, Steve Grubb wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> On Thursday, May 24, 2018 4:11:05 PM EDT 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
>>>>>>> Since this is a new event, do you mind moving the tty field to be
>>>>>>> between
>>>>>>> auid= and ses=  ?   That is the more natural place for it.
>>>>>> 6/8 refactors the code so that the integrity audit records produced by
>>>>>> IMA follow one format in terms of ordering of the fields, with fields
>>>>>> like inode optional, though, and AUDIT_INTEGRITY_RULE in the end being
>>>>>> the only one with a different format. Do we really want to change that
>>>>>> order just for 1806?
>>>>>>
>>>>>> 5/8 now produces the following:
>>>>>>
>>>>>> type=INTEGRITY_PCR msg=audit(1527685075.941:502): pid=2431 \
>>>>>> uid=0 auid=1000 ses=5 \
>>>>>> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 \
>>>>>> op=invalid_pcr cause=open_writers comm="grep" \
>>>>>> name="/var/log/audit/audit.log" dev="dm-0" ino=1962494 \
>>>>>> exe="/usr/bin/grep" tty=pts0 res=1
>>>>>>
>>>>>> Comparing the two:
>>>>>>
>>>>>> 1806:          action, fsmagic, pid, uid, auid, ses, subj, op, cause,
>>>>>> comm,    exe, tty, res
>>>>>> INTEGRITY_PCR:                  pid, uid, auid, ses, subj, op, cause,
>>>>>> comm, name, dev, ino, exe, tty, res
>>>>> OK. I guess go with it as is. It passes testing.
>>>> What about the position of 'res' field relative to the two new fields
>>>> 'exe' and 'tty'?
>>> res (results) is always the last field for every event. We have no events
>>> where it is not the last field. I'd prefer to go with it as is. The events
>>> pass my testing the way they are.
>>>
>>>> Do we want to keep them as shown or strictly append the
>>>> two new fields 'exe' and 'tty'?
>>> I'd prefer the first option to keep things as expected.
>>>
>>>> Paul seems to request that they appear after 'res'.
>>> I'd rather see them dropped, as useful as they could be, than to malform
>>> the
>>> events.
>>
>> Paul NACK'ed them since he wanted to have them added to the end. You seem to
>> say it's ok to add them before 'res'. Not sure whether to drop them now
>> since we are 'at it.'
> I talked about this in the other patch's thread, but the "new fields
> at the end of existing records" policy applies here too.

I am not sure what to post in v2. It looks like if I append it to the 
end after 'res' I could get the NACK'ed by Steve?!

So the other choice is to only keep patches 1,2, 6, and 7, so leave most 
of the integrity audit messages untouched. Then only create a different 
format for the new AUDIT_INTEGRITY_POLICY_RULE (current 8/8) that shares 
(for consistency reasons) the same format with the existing integrity 
audit messages but also misses tty= and exe= ?


>
> Also note Richard's earlier comment about "associating" the IMA
> records with all of the related audit records.
>