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(a)linux.vnet.ibm.com&gt; 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?!