From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Berger Subject: Re: [PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions Date: Wed, 30 May 2018 09:54:00 -0400 Message-ID: <2d5baf73-755b-dc82-a778-25a3cd22989a@linux.vnet.ibm.com> References: <20180524201105.3179904-1-stefanb@linux.vnet.ibm.com> <20180524201105.3179904-9-stefanb@linux.vnet.ibm.com> <1569841.KfYyxMilWs@x2> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1601396324766382425==" Return-path: Received: from mx1.redhat.com (ext-mx20.extmail.prod.ext.phx2.redhat.com [10.5.110.49]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 261E25E9C6 for ; Wed, 30 May 2018 13:54:13 +0000 (UTC) Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 996C0316B651 for ; Wed, 30 May 2018 13:54:07 +0000 (UTC) Received: from pps.filterd (m0098399.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id w4UDnYmF007192 for ; Wed, 30 May 2018 09:54:07 -0400 Received: from e31.co.us.ibm.com (e31.co.us.ibm.com [32.97.110.149]) by mx0a-001b2d01.pphosted.com with ESMTP id 2j9w6a06wg-1 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=NOT) for ; Wed, 30 May 2018 09:54:06 -0400 Received: from localhost by e31.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 30 May 2018 07:54:05 -0600 In-Reply-To: <1569841.KfYyxMilWs@x2> Content-Language: en-MW List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: Steve Grubb Cc: linux-integrity@vger.kernel.org, linux-audit@redhat.com, linux-kernel@vger.kernel.org List-Id: linux-audit@redhat.com This is a multi-part message in MIME format. --===============1601396324766382425== Content-Type: multipart/alternative; boundary="------------AEB539D2A0BE90E0B0402886" Content-Language: en-MW This is a multi-part message in MIME format. --------------AEB539D2A0BE90E0B0402886 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit 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 > Also, it might be more natural for the op= and cause= fields to be before the > pid= portion. This doesn't matter as much to me because those are not > searchable fields and they are skipped right over. But moving the tty field > is the main comment from me. With the refactoring in 6/8 we at least have consistency among the INTEGRITY_* records, with the only exception being AUDIT_INTEGRITY_RULE that has its own format: https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_api.c#L324 The other ones currently all format using integrity_audit_msg(). > > Thanks, > -Steve > >> Signed-off-by: Stefan Berger >> --- >> 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); >> >> entry->uid = INVALID_UID; >> entry->fowner = INVALID_UID; >> @@ -926,7 +926,8 @@ static int ima_parse_rule(char *rule, struct >> ima_rule_entry *entry) temp_ima_appraise |= IMA_APPRAISE_FIRMWARE; >> else if (entry->func == POLICY_CHECK) >> temp_ima_appraise |= IMA_APPRAISE_POLICY; >> - audit_log_format(ab, "res=%d", !result); >> + integrity_audit_msg_common(ab, NULL, NULL, >> + "policy_update", "parse_rule", result); >> audit_log_end(ab); >> return result; >> } > > --------------AEB539D2A0BE90E0B0402886 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit
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

Also, it might be more natural for the op= and cause= fields to be before the 
pid= portion. This doesn't matter as much to me because those are not 
searchable fields and they are skipped right over. But moving the tty field 
is the main comment from me.

With the refactoring in 6/8 we at least have consistency among the INTEGRITY_* records, with the only exception being AUDIT_INTEGRITY_RULE that has its own format:

https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_api.c#L324

The other ones currently all format using integrity_audit_msg().


Thanks,
-Steve


Signed-off-by: Stefan Berger <stefanb@linux.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);

 	entry->uid = INVALID_UID;
 	entry->fowner = INVALID_UID;
@@ -926,7 +926,8 @@ static int ima_parse_rule(char *rule, struct
ima_rule_entry *entry) temp_ima_appraise |= IMA_APPRAISE_FIRMWARE;
 	else if (entry->func == POLICY_CHECK)
 		temp_ima_appraise |= IMA_APPRAISE_POLICY;
-	audit_log_format(ab, "res=%d", !result);
+	integrity_audit_msg_common(ab, NULL, NULL,
+				   "policy_update", "parse_rule", result);
 	audit_log_end(ab);
 	return result;
 }


--------------AEB539D2A0BE90E0B0402886-- --===============1601396324766382425== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline --===============1601396324766382425==--