[PATCH 0/8] IMA: work on audit records produced by IMA

[PATCH 0/8] IMA: work on audit records produced by IMA

[Stefan Berger]

This series of patches cleans up some usages of the audit
subsystem's API by IMA and extends the audit subsystem's API
with API calls for adding new fields to the audit_buffer. Besides
that we extend the existing audit records created while parsing
IMA policy rules with fields that are common for audit records
produced by IMA. Besides that we introduce a new record type
that IMA creates while parsing policy rules.

   Stefan


Stefan Berger (8):
  ima: Call audit_log_string() rather than logging it untrusted
  ima: Use audit_log_format() rather than audit_log_string()
  audit: Implement audit_log_tty()
  audit: Allow others to call audit_log_d_path_exe()
  integrity: Add exe= and tty= before res= to integrity audits
  integrity: Factor out common part of integrity_audit_msg()
  ima: Do not audit if CONFIG_INTEGRITY_AUDIT is not set
  ima: Differentiate auditing policy rules from "audit" actions

 include/linux/audit.h                | 10 ++++++++++
 include/uapi/linux/audit.h           |  3 ++-
 kernel/audit.c                       |  8 ++++++++
 security/integrity/ima/Kconfig       |  1 +
 security/integrity/ima/ima_policy.c  | 12 ++++++++----
 security/integrity/integrity.h       | 26 ++++++++++++++++++++++++++
 security/integrity/integrity_audit.c | 32 +++++++++++++++++++-------------
 7 files changed, 74 insertions(+), 18 deletions(-)

-- 
2.13.6

[PATCH 1/8] ima: Call audit_log_string() rather than logging it untrusted

[Stefan Berger]

The parameters passed to this logging function are all provided by
a privileged user and therefore we can call audit_log_string()
rather than audit_log_untrustedstring().

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Suggested-by: Steve Grubb <sgrubb@redhat.com>
---
 security/integrity/ima/ima_policy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index d89bebf85421..a823f11a3e6b 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -615,7 +615,7 @@ static void ima_log_string_op(struct audit_buffer *ab, char *key, char *value,
 		audit_log_format(ab, "%s<", key);
 	else
 		audit_log_format(ab, "%s=", key);
-	audit_log_untrustedstring(ab, value);
+	audit_log_string(ab, value);
 	audit_log_format(ab, " ");
 }
 static void ima_log_string(struct audit_buffer *ab, char *key, char *value)
-- 
2.13.6

[PATCH 1/8] ima: Call audit_log_string() rather than logging it untrusted

[Stefan Berger]

The parameters passed to this logging function are all provided by
a privileged user and therefore we can call audit_log_string()
rather than audit_log_untrustedstring().

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Suggested-by: Steve Grubb <sgrubb@redhat.com>
---
 security/integrity/ima/ima_policy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index d89bebf85421..a823f11a3e6b 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -615,7 +615,7 @@ static void ima_log_string_op(struct audit_buffer *ab, char *key, char *value,
 		audit_log_format(ab, "%s<", key);
 	else
 		audit_log_format(ab, "%s=", key);
-	audit_log_untrustedstring(ab, value);
+	audit_log_string(ab, value);
 	audit_log_format(ab, " ");
 }
 static void ima_log_string(struct audit_buffer *ab, char *key, char *value)
-- 
2.13.6

[PATCH 2/8] ima: Use audit_log_format() rather than audit_log_string()

[Stefan Berger]

Remove the usage of audit_log_string() and replace it with
audit_log_format().

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Suggested-by: Steve Grubb <sgrubb@redhat.com>
Reviewed-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 security/integrity/ima/ima_policy.c  | 3 +--
 security/integrity/integrity_audit.c | 6 +-----
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index a823f11a3e6b..7297450b813c 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -615,8 +615,7 @@ static void ima_log_string_op(struct audit_buffer *ab, char *key, char *value,
 		audit_log_format(ab, "%s<", key);
 	else
 		audit_log_format(ab, "%s=", key);
-	audit_log_string(ab, value);
-	audit_log_format(ab, " ");
+	audit_log_format(ab, "%s ", value);
 }
 static void ima_log_string(struct audit_buffer *ab, char *key, char *value)
 {
diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c
index 90987d15b6fe..db30763d5525 100644
--- a/security/integrity/integrity_audit.c
+++ b/security/integrity/integrity_audit.c
@@ -45,11 +45,7 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode,
 			 from_kuid(&init_user_ns, audit_get_loginuid(current)),
 			 audit_get_sessionid(current));
 	audit_log_task_context(ab);
-	audit_log_format(ab, " op=");
-	audit_log_string(ab, op);
-	audit_log_format(ab, " cause=");
-	audit_log_string(ab, cause);
-	audit_log_format(ab, " comm=");
+	audit_log_format(ab, " op=%s cause=%s comm=", op, cause);
 	audit_log_untrustedstring(ab, get_task_comm(name, current));
 	if (fname) {
 		audit_log_format(ab, " name=");
-- 
2.13.6

[PATCH 3/8] audit: Implement audit_log_tty()

[Stefan Berger]

Implement audit_log_tty() so that IMA can add tty= to its audit records.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 include/linux/audit.h | 5 +++++
 kernel/audit.c        | 8 ++++++++
 2 files changed, 13 insertions(+)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 90aa63ddc9be..2deb76c74d10 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -154,6 +154,7 @@ extern void audit_log_task_info(struct audit_buffer *ab,
 				struct task_struct *tsk);
 
 extern int		    audit_update_lsm_rules(void);
+extern void audit_log_tty(struct audit_buffer *ab, struct task_struct *tsk);
 
 				/* Private API (for audit.c only) */
 extern int audit_rule_change(int type, int seq, void *data, size_t datasz);
@@ -202,6 +203,10 @@ static inline int audit_log_task_context(struct audit_buffer *ab)
 static inline void audit_log_task_info(struct audit_buffer *ab,
 				       struct task_struct *tsk)
 { }
+
+static inline void audit_log_tty(struct audit_buffer *ab,
+				 struct task_struct *tsk)
+{ }
 #define audit_enabled 0
 #endif /* CONFIG_AUDIT */
 
diff --git a/kernel/audit.c b/kernel/audit.c
index 670665c6e2a6..fa54695962b4 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -2305,6 +2305,14 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
 }
 EXPORT_SYMBOL(audit_log_task_info);
 
+void audit_log_tty(struct audit_buffer *ab, struct task_struct *tsk)
+{
+	struct tty_struct *tty = audit_get_tty(tsk);
+
+	audit_log_format(ab, " tty=%s", tty ? tty_name(tty) : "(none)");
+	audit_put_tty(tty);
+}
+
 /**
  * audit_log_link_denied - report a link restriction denial
  * @operation: specific link operation
-- 
2.13.6

[PATCH 4/8] audit: Allow others to call audit_log_d_path_exe()

[Stefan Berger]

Add the prototype for audit_log_d_path_exe() so that it can be
called by IMA later in this series.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Reviewed-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 include/linux/audit.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 2deb76c74d10..65eca0795308 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -144,6 +144,8 @@ extern void		    audit_log_untrustedstring(struct audit_buffer *ab,
 extern void		    audit_log_d_path(struct audit_buffer *ab,
 					     const char *prefix,
 					     const struct path *path);
+extern void		    audit_log_d_path_exe(struct audit_buffer *ab,
+						 struct mm_struct *mm);
 extern void		    audit_log_key(struct audit_buffer *ab,
 					  char *key);
 extern void		    audit_log_link_denied(const char *operation);
@@ -192,6 +194,9 @@ static inline void audit_log_d_path(struct audit_buffer *ab,
 				    const char *prefix,
 				    const struct path *path)
 { }
+static inline void audit_log_d_path_exe(struct audit_buffer *ab,
+					struct mm_struct *mm)
+{}
 static inline void audit_log_key(struct audit_buffer *ab, char *key)
 { }
 static inline void audit_log_link_denied(const char *string)
-- 
2.13.6

[PATCH 4/8] audit: Allow others to call audit_log_d_path_exe()

[Stefan Berger]

Add the prototype for audit_log_d_path_exe() so that it can be
called by IMA later in this series.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Reviewed-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 include/linux/audit.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 2deb76c74d10..65eca0795308 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -144,6 +144,8 @@ extern void		    audit_log_untrustedstring(struct audit_buffer *ab,
 extern void		    audit_log_d_path(struct audit_buffer *ab,
 					     const char *prefix,
 					     const struct path *path);
+extern void		    audit_log_d_path_exe(struct audit_buffer *ab,
+						 struct mm_struct *mm);
 extern void		    audit_log_key(struct audit_buffer *ab,
 					  char *key);
 extern void		    audit_log_link_denied(const char *operation);
@@ -192,6 +194,9 @@ static inline void audit_log_d_path(struct audit_buffer *ab,
 				    const char *prefix,
 				    const struct path *path)
 { }
+static inline void audit_log_d_path_exe(struct audit_buffer *ab,
+					struct mm_struct *mm)
+{}
 static inline void audit_log_key(struct audit_buffer *ab, char *key)
 { }
 static inline void audit_log_link_denied(const char *string)
-- 
2.13.6

[PATCH 5/8] integrity: Add exe= and tty= before res= to integrity audits

[Stefan Berger]

Use the new public audit functions to add the exe= and tty=
parts to the integrity audit records. We place them before
res=.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Suggested-by: Steve Grubb <sgrubb@redhat.com>
---
 security/integrity/integrity_audit.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c
index db30763d5525..8d25d3c4dcca 100644
--- a/security/integrity/integrity_audit.c
+++ b/security/integrity/integrity_audit.c
@@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode,
 		audit_log_untrustedstring(ab, inode->i_sb->s_id);
 		audit_log_format(ab, " ino=%lu", inode->i_ino);
 	}
+	audit_log_d_path_exe(ab, current->mm);
+	audit_log_tty(ab, current);
 	audit_log_format(ab, " res=%d", !result);
 	audit_log_end(ab);
 }
-- 
2.13.6

[PATCH 5/8] integrity: Add exe= and tty= before res= to integrity audits

[Stefan Berger]

Use the new public audit functions to add the exe= and tty=
parts to the integrity audit records. We place them before
res=.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Suggested-by: Steve Grubb <sgrubb@redhat.com>
---
 security/integrity/integrity_audit.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c
index db30763d5525..8d25d3c4dcca 100644
--- a/security/integrity/integrity_audit.c
+++ b/security/integrity/integrity_audit.c
@@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode,
 		audit_log_untrustedstring(ab, inode->i_sb->s_id);
 		audit_log_format(ab, " ino=%lu", inode->i_ino);
 	}
+	audit_log_d_path_exe(ab, current->mm);
+	audit_log_tty(ab, current);
 	audit_log_format(ab, " res=%d", !result);
 	audit_log_end(ab);
 }
-- 
2.13.6

[PATCH 6/8] integrity: Factor out common part of integrity_audit_msg()

[Stefan Berger]

Factor out a common part of integrity_audit_msg() that others
can also call.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 security/integrity/integrity.h       | 16 ++++++++++++++++
 security/integrity/integrity_audit.c | 24 ++++++++++++++++--------
 2 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 5e58e02ba8dc..9f2924cafa53 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -15,6 +15,7 @@
 #include <linux/integrity.h>
 #include <crypto/sha.h>
 #include <linux/key.h>
+#include <linux/audit.h>
 
 /* iint action cache flags */
 #define IMA_MEASURE		0x00000001
@@ -197,6 +198,11 @@ static inline void evm_load_x509(void)
 void integrity_audit_msg(int audit_msgno, struct inode *inode,
 			 const unsigned char *fname, const char *op,
 			 const char *cause, int result, int info);
+
+void integrity_audit_msg_common(struct audit_buffer *ab, struct inode *inode,
+				const unsigned char *fname, const char *op,
+				const char *cause, int result);
+
 #else
 static inline void integrity_audit_msg(int audit_msgno, struct inode *inode,
 				       const unsigned char *fname,
@@ -204,4 +210,14 @@ static inline void integrity_audit_msg(int audit_msgno, struct inode *inode,
 				       int result, int info)
 {
 }
+
+static inline void integrity_audit_msg_common(struct audit_buffer *ab,
+					      struct inode *inode,
+					      const unsigned char *fname,
+					      const char *op,
+					      const char *cause,
+					      int result)
+{
+}
+
 #endif
diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c
index 8d25d3c4dcca..8f80b7c042a7 100644
--- a/security/integrity/integrity_audit.c
+++ b/security/integrity/integrity_audit.c
@@ -28,17 +28,12 @@ static int __init integrity_audit_setup(char *str)
 }
 __setup("integrity_audit=", integrity_audit_setup);
 
-void integrity_audit_msg(int audit_msgno, struct inode *inode,
-			 const unsigned char *fname, const char *op,
-			 const char *cause, int result, int audit_info)
+void integrity_audit_msg_common(struct audit_buffer *ab, struct inode *inode,
+				const unsigned char *fname, const char *op,
+				const char *cause, int result)
 {
-	struct audit_buffer *ab;
 	char name[TASK_COMM_LEN];
 
-	if (!integrity_audit_info && audit_info == 1)	/* Skip info messages */
-		return;
-
-	ab = audit_log_start(current->audit_context, GFP_KERNEL, audit_msgno);
 	audit_log_format(ab, "pid=%d uid=%u auid=%u ses=%u",
 			 task_pid_nr(current),
 			 from_kuid(&init_user_ns, current_cred()->uid),
@@ -59,5 +54,18 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode,
 	audit_log_d_path_exe(ab, current->mm);
 	audit_log_tty(ab, current);
 	audit_log_format(ab, " res=%d", !result);
+}
+
+void integrity_audit_msg(int audit_msgno, struct inode *inode,
+			 const unsigned char *fname, const char *op,
+			 const char *cause, int result, int audit_info)
+{
+	struct audit_buffer *ab;
+
+	if (!integrity_audit_info && audit_info == 1)	/* Skip info messages */
+		return;
+
+	ab = audit_log_start(current->audit_context, GFP_KERNEL, audit_msgno);
+	integrity_audit_msg_common(ab, inode, fname, op, cause, result);
 	audit_log_end(ab);
 }
-- 
2.13.6

[PATCH 6/8] integrity: Factor out common part of integrity_audit_msg()

[Stefan Berger]

Factor out a common part of integrity_audit_msg() that others
can also call.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 security/integrity/integrity.h       | 16 ++++++++++++++++
 security/integrity/integrity_audit.c | 24 ++++++++++++++++--------
 2 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 5e58e02ba8dc..9f2924cafa53 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -15,6 +15,7 @@
 #include <linux/integrity.h>
 #include <crypto/sha.h>
 #include <linux/key.h>
+#include <linux/audit.h>
 
 /* iint action cache flags */
 #define IMA_MEASURE		0x00000001
@@ -197,6 +198,11 @@ static inline void evm_load_x509(void)
 void integrity_audit_msg(int audit_msgno, struct inode *inode,
 			 const unsigned char *fname, const char *op,
 			 const char *cause, int result, int info);
+
+void integrity_audit_msg_common(struct audit_buffer *ab, struct inode *inode,
+				const unsigned char *fname, const char *op,
+				const char *cause, int result);
+
 #else
 static inline void integrity_audit_msg(int audit_msgno, struct inode *inode,
 				       const unsigned char *fname,
@@ -204,4 +210,14 @@ static inline void integrity_audit_msg(int audit_msgno, struct inode *inode,
 				       int result, int info)
 {
 }
+
+static inline void integrity_audit_msg_common(struct audit_buffer *ab,
+					      struct inode *inode,
+					      const unsigned char *fname,
+					      const char *op,
+					      const char *cause,
+					      int result)
+{
+}
+
 #endif
diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c
index 8d25d3c4dcca..8f80b7c042a7 100644
--- a/security/integrity/integrity_audit.c
+++ b/security/integrity/integrity_audit.c
@@ -28,17 +28,12 @@ static int __init integrity_audit_setup(char *str)
 }
 __setup("integrity_audit=", integrity_audit_setup);
 
-void integrity_audit_msg(int audit_msgno, struct inode *inode,
-			 const unsigned char *fname, const char *op,
-			 const char *cause, int result, int audit_info)
+void integrity_audit_msg_common(struct audit_buffer *ab, struct inode *inode,
+				const unsigned char *fname, const char *op,
+				const char *cause, int result)
 {
-	struct audit_buffer *ab;
 	char name[TASK_COMM_LEN];
 
-	if (!integrity_audit_info && audit_info == 1)	/* Skip info messages */
-		return;
-
-	ab = audit_log_start(current->audit_context, GFP_KERNEL, audit_msgno);
 	audit_log_format(ab, "pid=%d uid=%u auid=%u ses=%u",
 			 task_pid_nr(current),
 			 from_kuid(&init_user_ns, current_cred()->uid),
@@ -59,5 +54,18 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode,
 	audit_log_d_path_exe(ab, current->mm);
 	audit_log_tty(ab, current);
 	audit_log_format(ab, " res=%d", !result);
+}
+
+void integrity_audit_msg(int audit_msgno, struct inode *inode,
+			 const unsigned char *fname, const char *op,
+			 const char *cause, int result, int audit_info)
+{
+	struct audit_buffer *ab;
+
+	if (!integrity_audit_info && audit_info == 1)	/* Skip info messages */
+		return;
+
+	ab = audit_log_start(current->audit_context, GFP_KERNEL, audit_msgno);
+	integrity_audit_msg_common(ab, inode, fname, op, cause, result);
 	audit_log_end(ab);
 }
-- 
2.13.6

[PATCH 7/8] ima: Do not audit if CONFIG_INTEGRITY_AUDIT is not set

[Stefan Berger]

If Integrity is not auditing, IMA shouldn't audit, either.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 security/integrity/ima/Kconfig      |  1 +
 security/integrity/ima/ima_policy.c |  6 +++++-
 security/integrity/integrity.h      | 10 ++++++++++
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 6a8f67714c83..94c2151331aa 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -12,6 +12,7 @@ config IMA
 	select TCG_TIS if TCG_TPM && X86
 	select TCG_CRB if TCG_TPM && ACPI
 	select TCG_IBMVTPM if TCG_TPM && PPC_PSERIES
+	select INTEGRITY_AUDIT if AUDIT
 	help
 	  The Trusted Computing Group(TCG) runtime Integrity
 	  Measurement Architecture(IMA) maintains a list of hash
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 7297450b813c..3aed25a7178a 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -609,6 +609,9 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry,
 static void ima_log_string_op(struct audit_buffer *ab, char *key, char *value,
 			      bool (*rule_operator)(kuid_t, kuid_t))
 {
+	if (!ab)
+		return;
+
 	if (rule_operator == &uid_gt)
 		audit_log_format(ab, "%s>", key);
 	else if (rule_operator == &uid_lt)
@@ -630,7 +633,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 	bool uid_token;
 	int result = 0;
 
-	ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_RULE);
+	ab = integrity_audit_log_start(NULL, GFP_KERNEL,
+				       AUDIT_INTEGRITY_RULE);
 
 	entry->uid = INVALID_UID;
 	entry->fowner = INVALID_UID;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 9f2924cafa53..2afa266aea42 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -203,6 +203,11 @@ void integrity_audit_msg_common(struct audit_buffer *ab, struct inode *inode,
 				const unsigned char *fname, const char *op,
 				const char *cause, int result);
 
+static inline struct audit_buffer *
+integrity_audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, int type)
+{
+	return audit_log_start(ctx, gfp_mask, type);
+}
 #else
 static inline void integrity_audit_msg(int audit_msgno, struct inode *inode,
 				       const unsigned char *fname,
@@ -220,4 +225,9 @@ static inline void integrity_audit_msg_common(struct audit_buffer *ab,
 {
 }
 
+static inline struct audit_buffer *
+integrity_audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, int type)
+{
+	return NULL;
+}
 #endif
-- 
2.13.6

[PATCH 8/8] ima: Differentiate auditing policy rules from "audit" actions

[Stefan Berger]

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@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;
 }
-- 
2.13.6

Re: [PATCH 1/8] ima: Call audit_log_string() rather than logging it untrusted

[Paul Moore]

On Thu, May 24, 2018 at 4:10 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> The parameters passed to this logging function are all provided by
> a privileged user and therefore we can call audit_log_string()
> rather than audit_log_untrustedstring().
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Suggested-by: Steve Grubb <sgrubb@redhat.com>
> ---
>  security/integrity/ima/ima_policy.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Paul Moore <paul@paul-moore.com>

> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index d89bebf85421..a823f11a3e6b 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -615,7 +615,7 @@ static void ima_log_string_op(struct audit_buffer *ab, char *key, char *value,
>                 audit_log_format(ab, "%s<", key);
>         else
>                 audit_log_format(ab, "%s=", key);
> -       audit_log_untrustedstring(ab, value);
> +       audit_log_string(ab, value);
>         audit_log_format(ab, " ");
>  }
>  static void ima_log_string(struct audit_buffer *ab, char *key, char *value)
> --
> 2.13.6
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

-- 
paul moore
www.paul-moore.com

Re: [PATCH 2/8] ima: Use audit_log_format() rather than audit_log_string()

[Paul Moore]

On Thu, May 24, 2018 at 4:10 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> Remove the usage of audit_log_string() and replace it with
> audit_log_format().
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Suggested-by: Steve Grubb <sgrubb@redhat.com>
> Reviewed-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> ---
>  security/integrity/ima/ima_policy.c  | 3 +--
>  security/integrity/integrity_audit.c | 6 +-----
>  2 files changed, 2 insertions(+), 7 deletions(-)

Acked-by: Paul Moore <paul@paul-moore.com>

> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index a823f11a3e6b..7297450b813c 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -615,8 +615,7 @@ static void ima_log_string_op(struct audit_buffer *ab, char *key, char *value,
>                 audit_log_format(ab, "%s<", key);
>         else
>                 audit_log_format(ab, "%s=", key);
> -       audit_log_string(ab, value);
> -       audit_log_format(ab, " ");
> +       audit_log_format(ab, "%s ", value);
>  }
>  static void ima_log_string(struct audit_buffer *ab, char *key, char *value)
>  {
> diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c
> index 90987d15b6fe..db30763d5525 100644
> --- a/security/integrity/integrity_audit.c
> +++ b/security/integrity/integrity_audit.c
> @@ -45,11 +45,7 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode,
>                          from_kuid(&init_user_ns, audit_get_loginuid(current)),
>                          audit_get_sessionid(current));
>         audit_log_task_context(ab);
> -       audit_log_format(ab, " op=");
> -       audit_log_string(ab, op);
> -       audit_log_format(ab, " cause=");
> -       audit_log_string(ab, cause);
> -       audit_log_format(ab, " comm=");
> +       audit_log_format(ab, " op=%s cause=%s comm=", op, cause);
>         audit_log_untrustedstring(ab, get_task_comm(name, current));
>         if (fname) {
>                 audit_log_format(ab, " name=");
> --
> 2.13.6
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit



-- 
paul moore
www.paul-moore.com

Re: [PATCH 3/8] audit: Implement audit_log_tty()

[Paul Moore]

On Thu, May 24, 2018 at 4:11 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> Implement audit_log_tty() so that IMA can add tty= to its audit records.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>  include/linux/audit.h | 5 +++++
>  kernel/audit.c        | 8 ++++++++
>  2 files changed, 13 insertions(+)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 90aa63ddc9be..2deb76c74d10 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -154,6 +154,7 @@ extern void audit_log_task_info(struct audit_buffer *ab,
>                                 struct task_struct *tsk);
>
>  extern int                 audit_update_lsm_rules(void);
> +extern void audit_log_tty(struct audit_buffer *ab, struct task_struct *tsk);
>
>                                 /* Private API (for audit.c only) */
>  extern int audit_rule_change(int type, int seq, void *data, size_t datasz);
> @@ -202,6 +203,10 @@ static inline int audit_log_task_context(struct audit_buffer *ab)
>  static inline void audit_log_task_info(struct audit_buffer *ab,
>                                        struct task_struct *tsk)
>  { }
> +
> +static inline void audit_log_tty(struct audit_buffer *ab,
> +                                struct task_struct *tsk)
> +{ }
>  #define audit_enabled 0
>  #endif /* CONFIG_AUDIT */
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 670665c6e2a6..fa54695962b4 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2305,6 +2305,14 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
>  }
>  EXPORT_SYMBOL(audit_log_task_info);
>
> +void audit_log_tty(struct audit_buffer *ab, struct task_struct *tsk)
> +{
> +       struct tty_struct *tty = audit_get_tty(tsk);
> +
> +       audit_log_format(ab, " tty=%s", tty ? tty_name(tty) : "(none)");
> +       audit_put_tty(tty);
> +}

Perhaps I missed it, but your IMA patches only ever call this to log
current's tty, yes?  If so, I would prefer if we dropped the
task_struct argument and always had audit_log_tty() use current.

-- 
paul moore
www.paul-moore.com

Re: [PATCH 4/8] audit: Allow others to call audit_log_d_path_exe()

[Paul Moore]

On Thu, May 24, 2018 at 4:11 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> Add the prototype for audit_log_d_path_exe() so that it can be
> called by IMA later in this series.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Reviewed-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> ---
>  include/linux/audit.h | 5 +++++
>  1 file changed, 5 insertions(+)

Acked-by: Paul Moore <paul@paul-moore.com>

> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 2deb76c74d10..65eca0795308 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -144,6 +144,8 @@ extern void             audit_log_untrustedstring(struct audit_buffer *ab,
>  extern void                audit_log_d_path(struct audit_buffer *ab,
>                                              const char *prefix,
>                                              const struct path *path);
> +extern void                audit_log_d_path_exe(struct audit_buffer *ab,
> +                                                struct mm_struct *mm);
>  extern void                audit_log_key(struct audit_buffer *ab,
>                                           char *key);
>  extern void                audit_log_link_denied(const char *operation);
> @@ -192,6 +194,9 @@ static inline void audit_log_d_path(struct audit_buffer *ab,
>                                     const char *prefix,
>                                     const struct path *path)
>  { }
> +static inline void audit_log_d_path_exe(struct audit_buffer *ab,
> +                                       struct mm_struct *mm)
> +{}
>  static inline void audit_log_key(struct audit_buffer *ab, char *key)
>  { }
>  static inline void audit_log_link_denied(const char *string)
> --
> 2.13.6
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit



-- 
paul moore
www.paul-moore.com

Re: [PATCH 4/8] audit: Allow others to call audit_log_d_path_exe()

[Paul Moore]

On Thu, May 24, 2018 at 4:11 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> Add the prototype for audit_log_d_path_exe() so that it can be
> called by IMA later in this series.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Reviewed-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> ---
>  include/linux/audit.h | 5 +++++
>  1 file changed, 5 insertions(+)

Acked-by: Paul Moore <paul@paul-moore.com>

> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 2deb76c74d10..65eca0795308 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -144,6 +144,8 @@ extern void             audit_log_untrustedstring(struct audit_buffer *ab,
>  extern void                audit_log_d_path(struct audit_buffer *ab,
>                                              const char *prefix,
>                                              const struct path *path);
> +extern void                audit_log_d_path_exe(struct audit_buffer *ab,
> +                                                struct mm_struct *mm);
>  extern void                audit_log_key(struct audit_buffer *ab,
>                                           char *key);
>  extern void                audit_log_link_denied(const char *operation);
> @@ -192,6 +194,9 @@ static inline void audit_log_d_path(struct audit_buffer *ab,
>                                     const char *prefix,
>                                     const struct path *path)
>  { }
> +static inline void audit_log_d_path_exe(struct audit_buffer *ab,
> +                                       struct mm_struct *mm)
> +{}
>  static inline void audit_log_key(struct audit_buffer *ab, char *key)
>  { }
>  static inline void audit_log_link_denied(const char *string)
> --
> 2.13.6
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit



-- 
paul moore
www.paul-moore.com

Re: [PATCH 5/8] integrity: Add exe= and tty= before res= to integrity audits

[Paul Moore]

On Thu, May 24, 2018 at 4:11 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> Use the new public audit functions to add the exe= and tty=
> parts to the integrity audit records. We place them before
> res=.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Suggested-by: Steve Grubb <sgrubb@redhat.com>
> ---
>  security/integrity/integrity_audit.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c
> index db30763d5525..8d25d3c4dcca 100644
> --- a/security/integrity/integrity_audit.c
> +++ b/security/integrity/integrity_audit.c
> @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode,
>                 audit_log_untrustedstring(ab, inode->i_sb->s_id);
>                 audit_log_format(ab, " ino=%lu", inode->i_ino);
>         }
> +       audit_log_d_path_exe(ab, current->mm);
> +       audit_log_tty(ab, current);

NACK

Please add the new fields to the end of the audit record, thank you.

>         audit_log_format(ab, " res=%d", !result);
>         audit_log_end(ab);
>  }

-- 
paul moore
www.paul-moore.com

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

[Steve Grubb]

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. 

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.

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;
>  }

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

[Steve Grubb]

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. 

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.

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;
>  }

Re: [PATCH 6/8] integrity: Factor out common part of integrity_audit_msg()

[Steve Grubb]

On Thursday, May 24, 2018 4:11:03 PM EDT Stefan Berger wrote:
> Factor out a common part of integrity_audit_msg() that others
> can also call.

After all of these changes, do you mind sending an example event for testing/
review?

Thanks,
-Steve

> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>  security/integrity/integrity.h       | 16 ++++++++++++++++
>  security/integrity/integrity_audit.c | 24 ++++++++++++++++--------
>  2 files changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/security/integrity/integrity.h
> b/security/integrity/integrity.h index 5e58e02ba8dc..9f2924cafa53 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -15,6 +15,7 @@
>  #include <linux/integrity.h>
>  #include <crypto/sha.h>
>  #include <linux/key.h>
> +#include <linux/audit.h>
> 
>  /* iint action cache flags */
>  #define IMA_MEASURE		0x00000001
> @@ -197,6 +198,11 @@ static inline void evm_load_x509(void)
>  void integrity_audit_msg(int audit_msgno, struct inode *inode,
>  			 const unsigned char *fname, const char *op,
>  			 const char *cause, int result, int info);
> +
> +void integrity_audit_msg_common(struct audit_buffer *ab, struct inode
> *inode, +				const unsigned char *fname, const char *op,
> +				const char *cause, int result);
> +
>  #else
>  static inline void integrity_audit_msg(int audit_msgno, struct inode
> *inode, const unsigned char *fname,
> @@ -204,4 +210,14 @@ static inline void integrity_audit_msg(int
> audit_msgno, struct inode *inode, int result, int info)
>  {
>  }
> +
> +static inline void integrity_audit_msg_common(struct audit_buffer *ab,
> +					      struct inode *inode,
> +					      const unsigned char *fname,
> +					      const char *op,
> +					      const char *cause,
> +					      int result)
> +{
> +}
> +
>  #endif
> diff --git a/security/integrity/integrity_audit.c
> b/security/integrity/integrity_audit.c index 8d25d3c4dcca..8f80b7c042a7
> 100644
> --- a/security/integrity/integrity_audit.c
> +++ b/security/integrity/integrity_audit.c
> @@ -28,17 +28,12 @@ static int __init integrity_audit_setup(char *str)
>  }
>  __setup("integrity_audit=", integrity_audit_setup);
> 
> -void integrity_audit_msg(int audit_msgno, struct inode *inode,
> -			 const unsigned char *fname, const char *op,
> -			 const char *cause, int result, int audit_info)
> +void integrity_audit_msg_common(struct audit_buffer *ab, struct inode
> *inode, +				const unsigned char *fname, const char *op,
> +				const char *cause, int result)
>  {
> -	struct audit_buffer *ab;
>  	char name[TASK_COMM_LEN];
> 
> -	if (!integrity_audit_info && audit_info == 1)	/* Skip info messages */
> -		return;
> -
> -	ab = audit_log_start(current->audit_context, GFP_KERNEL, audit_msgno);
>  	audit_log_format(ab, "pid=%d uid=%u auid=%u ses=%u",
>  			 task_pid_nr(current),
>  			 from_kuid(&init_user_ns, current_cred()->uid),
> @@ -59,5 +54,18 @@ void integrity_audit_msg(int audit_msgno, struct inode
> *inode, audit_log_d_path_exe(ab, current->mm);
>  	audit_log_tty(ab, current);
>  	audit_log_format(ab, " res=%d", !result);
> +}
> +
> +void integrity_audit_msg(int audit_msgno, struct inode *inode,
> +			 const unsigned char *fname, const char *op,
> +			 const char *cause, int result, int audit_info)
> +{
> +	struct audit_buffer *ab;
> +
> +	if (!integrity_audit_info && audit_info == 1)	/* Skip info messages */
> +		return;
> +
> +	ab = audit_log_start(current->audit_context, GFP_KERNEL, audit_msgno);
> +	integrity_audit_msg_common(ab, inode, fname, op, cause, result);
>  	audit_log_end(ab);
>  }

Re: [PATCH 5/8] integrity: Add exe= and tty= before res= to integrity audits

[Steve Grubb]

On Tuesday, May 29, 2018 5:19:39 PM EDT Paul Moore wrote:
> On Thu, May 24, 2018 at 4:11 PM, Stefan Berger
> 
> <stefanb@linux.vnet.ibm.com> wrote:
> > Use the new public audit functions to add the exe= and tty=
> > parts to the integrity audit records. We place them before
> > res=.
> > 
> > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > Suggested-by: Steve Grubb <sgrubb@redhat.com>
> > ---
> > 
> >  security/integrity/integrity_audit.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/security/integrity/integrity_audit.c
> > b/security/integrity/integrity_audit.c index db30763d5525..8d25d3c4dcca
> > 100644
> > --- a/security/integrity/integrity_audit.c
> > +++ b/security/integrity/integrity_audit.c
> > @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode
> > *inode,> 
> >                 audit_log_untrustedstring(ab, inode->i_sb->s_id);
> >                 audit_log_format(ab, " ino=%lu", inode->i_ino);
> >         
> >         }
> > 
> > +       audit_log_d_path_exe(ab, current->mm);
> > +       audit_log_tty(ab, current);
> 
> NACK
> 
> Please add the new fields to the end of the audit record, thank you.

Let's see what an example event looks like before NACK'ing this. Way back in 
2013 the IMA events were good. I think this is repairing the event after some 
drift.

Thanks,
-Steve
 
> >         audit_log_format(ab, " res=%d", !result);
> >         audit_log_end(ab);
> >  
> >  }

Re: [PATCH 5/8] integrity: Add exe= and tty= before res= to integrity audits

[Steve Grubb]

On Tuesday, May 29, 2018 5:19:39 PM EDT Paul Moore wrote:
> On Thu, May 24, 2018 at 4:11 PM, Stefan Berger
> 
> <stefanb@linux.vnet.ibm.com> wrote:
> > Use the new public audit functions to add the exe= and tty=
> > parts to the integrity audit records. We place them before
> > res=.
> > 
> > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > Suggested-by: Steve Grubb <sgrubb@redhat.com>
> > ---
> > 
> >  security/integrity/integrity_audit.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/security/integrity/integrity_audit.c
> > b/security/integrity/integrity_audit.c index db30763d5525..8d25d3c4dcca
> > 100644
> > --- a/security/integrity/integrity_audit.c
> > +++ b/security/integrity/integrity_audit.c
> > @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode
> > *inode,> 
> >                 audit_log_untrustedstring(ab, inode->i_sb->s_id);
> >                 audit_log_format(ab, " ino=%lu", inode->i_ino);
> >         
> >         }
> > 
> > +       audit_log_d_path_exe(ab, current->mm);
> > +       audit_log_tty(ab, current);
> 
> NACK
> 
> Please add the new fields to the end of the audit record, thank you.

Let's see what an example event looks like before NACK'ing this. Way back in 
2013 the IMA events were good. I think this is repairing the event after some 
drift.

Thanks,
-Steve
 
> >         audit_log_format(ab, " res=%d", !result);
> >         audit_log_end(ab);
> >  
> >  }

Re: [PATCH 5/8] integrity: Add exe= and tty= before res= to integrity audits

[Paul Moore]

On Tue, May 29, 2018 at 5:35 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Tuesday, May 29, 2018 5:19:39 PM EDT Paul Moore wrote:
>> On Thu, May 24, 2018 at 4:11 PM, Stefan Berger
>>
>> <stefanb@linux.vnet.ibm.com> wrote:
>> > Use the new public audit functions to add the exe= and tty=
>> > parts to the integrity audit records. We place them before
>> > res=.
>> >
>> > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> > Suggested-by: Steve Grubb <sgrubb@redhat.com>
>> > ---
>> >
>> >  security/integrity/integrity_audit.c | 2 ++
>> >  1 file changed, 2 insertions(+)
>> >
>> > diff --git a/security/integrity/integrity_audit.c
>> > b/security/integrity/integrity_audit.c index db30763d5525..8d25d3c4dcca
>> > 100644
>> > --- a/security/integrity/integrity_audit.c
>> > +++ b/security/integrity/integrity_audit.c
>> > @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode
>> > *inode,>
>> >                 audit_log_untrustedstring(ab, inode->i_sb->s_id);
>> >                 audit_log_format(ab, " ino=%lu", inode->i_ino);
>> >
>> >         }
>> >
>> > +       audit_log_d_path_exe(ab, current->mm);
>> > +       audit_log_tty(ab, current);
>>
>> NACK
>>
>> Please add the new fields to the end of the audit record, thank you.
>
> Let's see what an example event looks like before NACK'ing this. Way back in
> 2013 the IMA events were good. I think this is repairing the event after some
> drift.

Can you reference a specific commit, or point in time during 2013?
Looking at the git log quickly, if I go back to commit d726d8d719b6
("integrity: move integrity_audit_msg()") from March 18, 2013 (the
commit that created integrity_audit.c) the field ordering appears to
be the same as it today.

My NACK still stands.

-- 
paul moore
www.paul-moore.com

Re: [PATCH 5/8] integrity: Add exe= and tty= before res= to integrity audits

[Mimi Zohar]

On Tue, 2018-05-29 at 17:47 -0400, Paul Moore wrote:
> On Tue, May 29, 2018 at 5:35 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > On Tuesday, May 29, 2018 5:19:39 PM EDT Paul Moore wrote:
> >> On Thu, May 24, 2018 at 4:11 PM, Stefan Berger
> >>
> >> <stefanb@linux.vnet.ibm.com> wrote:
> >> > Use the new public audit functions to add the exe= and tty=
> >> > parts to the integrity audit records. We place them before
> >> > res=.
> >> >
> >> > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> > Suggested-by: Steve Grubb <sgrubb@redhat.com>
> >> > ---
> >> >
> >> >  security/integrity/integrity_audit.c | 2 ++
> >> >  1 file changed, 2 insertions(+)
> >> >
> >> > diff --git a/security/integrity/integrity_audit.c
> >> > b/security/integrity/integrity_audit.c index db30763d5525..8d25d3c4dcca
> >> > 100644
> >> > --- a/security/integrity/integrity_audit.c
> >> > +++ b/security/integrity/integrity_audit.c
> >> > @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode
> >> > *inode,>
> >> >                 audit_log_untrustedstring(ab, inode->i_sb->s_id);
> >> >                 audit_log_format(ab, " ino=%lu", inode->i_ino);
> >> >
> >> >         }
> >> >
> >> > +       audit_log_d_path_exe(ab, current->mm);
> >> > +       audit_log_tty(ab, current);
> >>
> >> NACK
> >>
> >> Please add the new fields to the end of the audit record, thank you.
> >
> > Let's see what an example event looks like before NACK'ing this. Way back in
> > 2013 the IMA events were good. I think this is repairing the event after some
> > drift.
> 
> Can you reference a specific commit, or point in time during 2013?
> Looking at the git log quickly, if I go back to commit d726d8d719b6
> ("integrity: move integrity_audit_msg()") from March 18, 2013 (the
> commit that created integrity_audit.c) the field ordering appears to
> be the same as it today.
> 
> My NACK still stands.

There hasn't been any changes up to now.  This patch set refactors
integrity_audit_msg(), creating integrity_audit_msg_common(), which
will be called from both ima_audit_measurement() and
ima_parse_rule(). 

Previously the audit record generated by ima_parse_rule() did not
include this info.  The change in this patch will affect both the
existing and the new INTEGRITY_AUDIT_POLICY_RULE audit records.

Mimi

Re: [PATCH 5/8] integrity: Add exe= and tty= before res= to integrity audits

[Mimi Zohar]

On Tue, 2018-05-29 at 17:47 -0400, Paul Moore wrote:
> On Tue, May 29, 2018 at 5:35 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > On Tuesday, May 29, 2018 5:19:39 PM EDT Paul Moore wrote:
> >> On Thu, May 24, 2018 at 4:11 PM, Stefan Berger
> >>
> >> <stefanb@linux.vnet.ibm.com> wrote:
> >> > Use the new public audit functions to add the exe= and tty=
> >> > parts to the integrity audit records. We place them before
> >> > res=.
> >> >
> >> > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> >> > Suggested-by: Steve Grubb <sgrubb@redhat.com>
> >> > ---
> >> >
> >> >  security/integrity/integrity_audit.c | 2 ++
> >> >  1 file changed, 2 insertions(+)
> >> >
> >> > diff --git a/security/integrity/integrity_audit.c
> >> > b/security/integrity/integrity_audit.c index db30763d5525..8d25d3c4dcca
> >> > 100644
> >> > --- a/security/integrity/integrity_audit.c
> >> > +++ b/security/integrity/integrity_audit.c
> >> > @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode
> >> > *inode,>
> >> >                 audit_log_untrustedstring(ab, inode->i_sb->s_id);
> >> >                 audit_log_format(ab, " ino=%lu", inode->i_ino);
> >> >
> >> >         }
> >> >
> >> > +       audit_log_d_path_exe(ab, current->mm);
> >> > +       audit_log_tty(ab, current);
> >>
> >> NACK
> >>
> >> Please add the new fields to the end of the audit record, thank you.
> >
> > Let's see what an example event looks like before NACK'ing this. Way back in
> > 2013 the IMA events were good. I think this is repairing the event after some
> > drift.
> 
> Can you reference a specific commit, or point in time during 2013?
> Looking at the git log quickly, if I go back to commit d726d8d719b6
> ("integrity: move integrity_audit_msg()") from March 18, 2013 (the
> commit that created integrity_audit.c) the field ordering appears to
> be the same as it today.
> 
> My NACK still stands.

There hasn't been any changes up to now.  This patch set refactors
integrity_audit_msg(), creating integrity_audit_msg_common(), which
will be called from both ima_audit_measurement() and
ima_parse_rule(). 

Previously the audit record generated by ima_parse_rule() did not
include this info.  The change in this patch will affect both the
existing and the new INTEGRITY_AUDIT_POLICY_RULE audit records.

Mimi

Re: [PATCH 5/8] integrity: Add exe= and tty= before res= to integrity audits

[Stefan Berger]

On 05/29/2018 05:19 PM, Paul Moore wrote:
> On Thu, May 24, 2018 at 4:11 PM, Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
>> Use the new public audit functions to add the exe= and tty=
>> parts to the integrity audit records. We place them before
>> res=.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Suggested-by: Steve Grubb <sgrubb@redhat.com>
>> ---
>>   security/integrity/integrity_audit.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c
>> index db30763d5525..8d25d3c4dcca 100644
>> --- a/security/integrity/integrity_audit.c
>> +++ b/security/integrity/integrity_audit.c
>> @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode,
>>                  audit_log_untrustedstring(ab, inode->i_sb->s_id);
>>                  audit_log_format(ab, " ino=%lu", inode->i_ino);
>>          }
>> +       audit_log_d_path_exe(ab, current->mm);
>> +       audit_log_tty(ab, current);
> NACK
>
> Please add the new fields to the end of the audit record, thank you.

I put it there since Steve said '"res" is traditionally the last field 
in any event' (https://lkml.org/lkml/2018/5/22/539). I don't mind 
breaking with this tradition...

>
>>          audit_log_format(ab, " res=%d", !result);
>>          audit_log_end(ab);
>>   }

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

[Richard Guy Briggs]

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@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);

Is it possible to connect this record to a syscall by replacing the
first parameter (NULL) by current->context?

>  	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;
>  }

- 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

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

[Steve Grubb]

On Wednesday, May 30, 2018 8:49:20 AM EDT 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@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);
> 
> Is it possible to connect this record to a syscall by replacing the
> first parameter (NULL) by current->context?

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.

-Steve

> >  	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;
> >  
> >  }
> 
> - 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

Re: [PATCH 5/8] integrity: Add exe= and tty= before res= to integrity audits

[Mimi Zohar]

On Tue, 2018-05-29 at 18:58 -0400, Mimi Zohar wrote:
> On Tue, 2018-05-29 at 17:47 -0400, Paul Moore wrote:
> > On Tue, May 29, 2018 at 5:35 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > > On Tuesday, May 29, 2018 5:19:39 PM EDT Paul Moore wrote:
> > >> On Thu, May 24, 2018 at 4:11 PM, Stefan Berger
> > >>
> > >> <stefanb@linux.vnet.ibm.com> wrote:
> > >> > Use the new public audit functions to add the exe= and tty=
> > >> > parts to the integrity audit records. We place them before
> > >> > res=.
> > >> >
> > >> > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > >> > Suggested-by: Steve Grubb <sgrubb@redhat.com>
> > >> > ---
> > >> >
> > >> >  security/integrity/integrity_audit.c | 2 ++
> > >> >  1 file changed, 2 insertions(+)
> > >> >
> > >> > diff --git a/security/integrity/integrity_audit.c
> > >> > b/security/integrity/integrity_audit.c index db30763d5525..8d25d3c4dcca
> > >> > 100644
> > >> > --- a/security/integrity/integrity_audit.c
> > >> > +++ b/security/integrity/integrity_audit.c
> > >> > @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode
> > >> > *inode,>
> > >> >                 audit_log_untrustedstring(ab, inode->i_sb->s_id);
> > >> >                 audit_log_format(ab, " ino=%lu", inode->i_ino);
> > >> >
> > >> >         }
> > >> >
> > >> > +       audit_log_d_path_exe(ab, current->mm);
> > >> > +       audit_log_tty(ab, current);
> > >>
> > >> NACK
> > >>
> > >> Please add the new fields to the end of the audit record, thank you.
> > >
> > > Let's see what an example event looks like before NACK'ing this. Way back in
> > > 2013 the IMA events were good. I think this is repairing the event after some
> > > drift.
> > 
> > Can you reference a specific commit, or point in time during 2013?
> > Looking at the git log quickly, if I go back to commit d726d8d719b6
> > ("integrity: move integrity_audit_msg()") from March 18, 2013 (the
> > commit that created integrity_audit.c) the field ordering appears to
> > be the same as it today.
> > 
> > My NACK still stands.
> 
> There hasn't been any changes up to now.  This patch set refactors
> integrity_audit_msg(), creating integrity_audit_msg_common(), which
> will be called from both ima_audit_measurement() and
> ima_parse_rule().

That should have been "from integrity_audit_msg() and
ima_parse_rule()", not ima_audit_measurement().

> Previously the audit record generated by ima_parse_rule() did not
> include this info.  The change in this patch will affect both the
> existing and the new INTEGRITY_AUDIT_POLICY_RULE audit records.

Re: [PATCH 5/8] integrity: Add exe= and tty= before res= to integrity audits

[Mimi Zohar]

On Tue, 2018-05-29 at 18:58 -0400, Mimi Zohar wrote:
> On Tue, 2018-05-29 at 17:47 -0400, Paul Moore wrote:
> > On Tue, May 29, 2018 at 5:35 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > > On Tuesday, May 29, 2018 5:19:39 PM EDT Paul Moore wrote:
> > >> On Thu, May 24, 2018 at 4:11 PM, Stefan Berger
> > >>
> > >> <stefanb@linux.vnet.ibm.com> wrote:
> > >> > Use the new public audit functions to add the exe= and tty=
> > >> > parts to the integrity audit records. We place them before
> > >> > res=.
> > >> >
> > >> > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > >> > Suggested-by: Steve Grubb <sgrubb@redhat.com>
> > >> > ---
> > >> >
> > >> >  security/integrity/integrity_audit.c | 2 ++
> > >> >  1 file changed, 2 insertions(+)
> > >> >
> > >> > diff --git a/security/integrity/integrity_audit.c
> > >> > b/security/integrity/integrity_audit.c index db30763d5525..8d25d3c4dcca
> > >> > 100644
> > >> > --- a/security/integrity/integrity_audit.c
> > >> > +++ b/security/integrity/integrity_audit.c
> > >> > @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode
> > >> > *inode,>
> > >> >                 audit_log_untrustedstring(ab, inode->i_sb->s_id);
> > >> >                 audit_log_format(ab, " ino=%lu", inode->i_ino);
> > >> >
> > >> >         }
> > >> >
> > >> > +       audit_log_d_path_exe(ab, current->mm);
> > >> > +       audit_log_tty(ab, current);
> > >>
> > >> NACK
> > >>
> > >> Please add the new fields to the end of the audit record, thank you.
> > >
> > > Let's see what an example event looks like before NACK'ing this. Way back in
> > > 2013 the IMA events were good. I think this is repairing the event after some
> > > drift.
> > 
> > Can you reference a specific commit, or point in time during 2013?
> > Looking at the git log quickly, if I go back to commit d726d8d719b6
> > ("integrity: move integrity_audit_msg()") from March 18, 2013 (the
> > commit that created integrity_audit.c) the field ordering appears to
> > be the same as it today.
> > 
> > My NACK still stands.
> 
> There hasn't been any changes up to now.  This patch set refactors
> integrity_audit_msg(), creating integrity_audit_msg_common(), which
> will be called from both ima_audit_measurement() and
> ima_parse_rule().

That should have been "from integrity_audit_msg() and
ima_parse_rule()", not ima_audit_measurement().

> Previously the audit record generated by ima_parse_rule() did not
> include this info.  The change in this patch will affect both the
> existing and the new INTEGRITY_AUDIT_POLICY_RULE audit records.

Re: [PATCH 6/8] integrity: Factor out common part of integrity_audit_msg()

[Stefan Berger]

On 05/29/2018 05:32 PM, Steve Grubb wrote:
> On Thursday, May 24, 2018 4:11:03 PM EDT Stefan Berger wrote:
>> Factor out a common part of integrity_audit_msg() that others
>> can also call.
> After all of these changes, do you mind sending an example event for testing/
> review?

Adding example to 5/8 since this patch here doesn't change any records.

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

[Stefan Berger]

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@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);
> Is it possible to connect this record to a syscall by replacing the
> first parameter (NULL) by current->context?

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.




>
>>   	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;
>>   }
> - 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
>

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

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 4344 bytes --]

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;
>>   }
>
>


[-- Attachment #1.2: Type: text/html, Size: 5524 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Steve Grubb]

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.

-Steve
 
> > 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_a
> pi.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;
> >>   
> >>   }

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

[Steve Grubb]

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.

-Steve
 
> > 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_a
> pi.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;
> >>   
> >>   }

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

[Stefan Berger]

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'? Do we want to keep them as shown or strictly append the 
two new fields 'exe' and 'tty'? Paul seems to request that they appear 
after 'res'.

     Stefan

>
> -Steve
>
>>> 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_a
>> pi.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;
>>>>    
>>>>    }
>
>
>

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

[Stefan Berger]

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'? Do we want to keep them as shown or strictly append the 
two new fields 'exe' and 'tty'? Paul seems to request that they appear 
after 'res'.

     Stefan

>
> -Steve
>
>>> 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_a
>> pi.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;
>>>>    
>>>>    }
>
>
>

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

[Steve Grubb]

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.

-Steve

Re: [PATCH 3/8] audit: Implement audit_log_tty()

[Stefan Berger]

On 05/29/2018 05:07 PM, Paul Moore wrote:
> On Thu, May 24, 2018 at 4:11 PM, Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
>>
>> +void audit_log_tty(struct audit_buffer *ab, struct task_struct *tsk)
>> +{
>> +       struct tty_struct *tty = audit_get_tty(tsk);
>> +
>> +       audit_log_format(ab, " tty=%s", tty ? tty_name(tty) : "(none)");
>> +       audit_put_tty(tty);
>> +}
> Perhaps I missed it, but your IMA patches only ever call this to log
> current's tty, yes?  If so, I would prefer if we dropped the
> task_struct argument and always had audit_log_tty() use current.

Done.


>

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

[Stefan Berger]

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.'

    Stefan

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

[Stefan Berger]

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.'

    Stefan

Re: [PATCH 5/8] integrity: Add exe= and tty= before res= to integrity audits

[Paul Moore]

On Wed, May 30, 2018 at 8:17 AM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> On 05/29/2018 05:19 PM, Paul Moore wrote:
>>
>> On Thu, May 24, 2018 at 4:11 PM, Stefan Berger
>> <stefanb@linux.vnet.ibm.com> wrote:
>>>
>>> Use the new public audit functions to add the exe= and tty=
>>> parts to the integrity audit records. We place them before
>>> res=.
>>>
>>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>> Suggested-by: Steve Grubb <sgrubb@redhat.com>
>>> ---
>>>   security/integrity/integrity_audit.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/security/integrity/integrity_audit.c
>>> b/security/integrity/integrity_audit.c
>>> index db30763d5525..8d25d3c4dcca 100644
>>> --- a/security/integrity/integrity_audit.c
>>> +++ b/security/integrity/integrity_audit.c
>>> @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode
>>> *inode,
>>>                  audit_log_untrustedstring(ab, inode->i_sb->s_id);
>>>                  audit_log_format(ab, " ino=%lu", inode->i_ino);
>>>          }
>>> +       audit_log_d_path_exe(ab, current->mm);
>>> +       audit_log_tty(ab, current);
>>
>> NACK
>>
>> Please add the new fields to the end of the audit record, thank you.
>
> I put it there since Steve said '"res" is traditionally the last field in
> any event' (https://lkml.org/lkml/2018/5/22/539). I don't mind breaking with
> this tradition...

Unfortunately Steve and I don't see eye-to-eye on everything, and this
is perhaps one of the more prominent issues.

I'll save you several years of arguments, on and off-list, and simply
say that the "safe" option, and the only option I'm likely to ACK,
would be to add new fields at the end of existing records.  We have
made exceptions in the past, but those were pretty extreme cases.

-- 
paul moore
www.paul-moore.com

Re: [PATCH 5/8] integrity: Add exe= and tty= before res= to integrity audits

[Paul Moore]

On Wed, May 30, 2018 at 9:04 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Tue, 2018-05-29 at 18:58 -0400, Mimi Zohar wrote:
>> On Tue, 2018-05-29 at 17:47 -0400, Paul Moore wrote:
>> > On Tue, May 29, 2018 at 5:35 PM, Steve Grubb <sgrubb@redhat.com> wrote:
>> > > On Tuesday, May 29, 2018 5:19:39 PM EDT Paul Moore wrote:
>> > >> On Thu, May 24, 2018 at 4:11 PM, Stefan Berger
>> > >>
>> > >> <stefanb@linux.vnet.ibm.com> wrote:
>> > >> > Use the new public audit functions to add the exe= and tty=
>> > >> > parts to the integrity audit records. We place them before
>> > >> > res=.
>> > >> >
>> > >> > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> > >> > Suggested-by: Steve Grubb <sgrubb@redhat.com>
>> > >> > ---
>> > >> >
>> > >> >  security/integrity/integrity_audit.c | 2 ++
>> > >> >  1 file changed, 2 insertions(+)
>> > >> >
>> > >> > diff --git a/security/integrity/integrity_audit.c
>> > >> > b/security/integrity/integrity_audit.c index db30763d5525..8d25d3c4dcca
>> > >> > 100644
>> > >> > --- a/security/integrity/integrity_audit.c
>> > >> > +++ b/security/integrity/integrity_audit.c
>> > >> > @@ -56,6 +56,8 @@ void integrity_audit_msg(int audit_msgno, struct inode
>> > >> > *inode,>
>> > >> >                 audit_log_untrustedstring(ab, inode->i_sb->s_id);
>> > >> >                 audit_log_format(ab, " ino=%lu", inode->i_ino);
>> > >> >
>> > >> >         }
>> > >> >
>> > >> > +       audit_log_d_path_exe(ab, current->mm);
>> > >> > +       audit_log_tty(ab, current);
>> > >>
>> > >> NACK
>> > >>
>> > >> Please add the new fields to the end of the audit record, thank you.
>> > >
>> > > Let's see what an example event looks like before NACK'ing this. Way back in
>> > > 2013 the IMA events were good. I think this is repairing the event after some
>> > > drift.
>> >
>> > Can you reference a specific commit, or point in time during 2013?
>> > Looking at the git log quickly, if I go back to commit d726d8d719b6
>> > ("integrity: move integrity_audit_msg()") from March 18, 2013 (the
>> > commit that created integrity_audit.c) the field ordering appears to
>> > be the same as it today.
>> >
>> > My NACK still stands.
>>
>> There hasn't been any changes up to now.  This patch set refactors
>> integrity_audit_msg(), creating integrity_audit_msg_common(), which
>> will be called from both ima_audit_measurement() and
>> ima_parse_rule().
>
> That should have been "from integrity_audit_msg() and
> ima_parse_rule()", not ima_audit_measurement().

No worries, the important part is that the record format really hasn't
changed from 2013 as far as I can tell.

>> Previously the audit record generated by ima_parse_rule() did not
>> include this info.  The change in this patch will affect both the
>> existing and the new INTEGRITY_AUDIT_POLICY_RULE audit records.

-- 
paul moore
www.paul-moore.com

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

[Paul Moore]

On Wed, May 30, 2018 at 9:08 AM, Stefan Berger
<stefanb@linux.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@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);
>>
>> 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 :)

> 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?

-- 
paul moore
www.paul-moore.com

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

[Paul Moore]

On Wed, May 30, 2018 at 3:54 PM, Stefan Berger
<stefanb@linux.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.

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

-- 
paul moore
www.paul-moore.com

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

[Stefan Berger]

On 05/30/2018 05:22 PM, Paul Moore wrote:
> On Wed, May 30, 2018 at 9:08 AM, Stefan Berger
> <stefanb@linux.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@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);
>>> 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?

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.

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

[Stefan Berger]

On 05/30/2018 05:24 PM, Paul Moore wrote:
> On Wed, May 30, 2018 at 3:54 PM, Stefan Berger
> <stefanb@linux.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.
>

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

[Mimi Zohar]

On Wed, 2018-05-30 at 17:49 -0400, Stefan Berger wrote:
> 
> 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= ?

Another option would be for the new AUDIT_INTEGRITY_POLICY_RULE to
call audit_log_task_info() similar to what ima_audit_measurement()
does.

Mimi

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

[Stefan Berger]

On 05/30/2018 06:00 PM, Mimi Zohar wrote:
> On Wed, 2018-05-30 at 17:49 -0400, Stefan Berger wrote:
>> 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= ?
> Another option would be for the new AUDIT_INTEGRITY_POLICY_RULE to
> call audit_log_task_info() similar to what ima_audit_measurement()
> does.

Right. [That would mean keep 1,2, 7 and modify 8.] Is that the best 
solution?

>
> Mimi

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

[Mimi Zohar]

On Wed, 2018-05-30 at 18:15 -0400, Stefan Berger wrote:
> On 05/30/2018 06:00 PM, Mimi Zohar wrote:
> > On Wed, 2018-05-30 at 17:49 -0400, Stefan Berger wrote:
> >> 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= ?
> > Another option would be for the new AUDIT_INTEGRITY_POLICY_RULE to
> > call audit_log_task_info() similar to what ima_audit_measurement()
> > does.
> 
> Right. [That would mean keep 1,2, 7 and modify 8.] Is that the best 
> solution?

Yes, I think so.  Calling audit_log_task_info() will only add the
"exe=" and "tty=" to the new AUDIT_INTEGRITY_POLICY_RULE.

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

[Mimi Zohar]

On Wed, 2018-05-30 at 18:15 -0400, Stefan Berger wrote:
> On 05/30/2018 06:00 PM, Mimi Zohar wrote:
> > On Wed, 2018-05-30 at 17:49 -0400, Stefan Berger wrote:
> >> 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= ?
> > Another option would be for the new AUDIT_INTEGRITY_POLICY_RULE to
> > call audit_log_task_info() similar to what ima_audit_measurement()
> > does.
> 
> Right. [That would mean keep 1,2, 7 and modify 8.] Is that the best 
> solution?

Yes, I think so.  Calling audit_log_task_info() will only add the
"exe=" and "tty=" to the new AUDIT_INTEGRITY_POLICY_RULE.

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

[Richard Guy Briggs]

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@linux.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@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);
> > > > 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@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

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

[Paul Moore]

On Wed, May 30, 2018 at 5:49 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> On 05/30/2018 05:24 PM, Paul Moore wrote:
>>
>> On Wed, May 30, 2018 at 3:54 PM, Stefan Berger
>> <stefanb@linux.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?!

My apologies, you are getting caught in the middle of this and that
isn't fair to you.

Steve maintains an audit userspace package, I maintain the audit
subsystem in the kernel.  We always try to come to a consensus, but we
sometime reach a "agree to disagree" point and in those cases the
maintainer breaks the tie.  For the purposes of the audit kernel code,
I'm the tie breaker.

There have been a lot of messages spread across the threads, but I
believe Steve's position was that he would prefer to not include new
fields if that meant placing them after the "res" field; combined with
my "new fields at the end of existing records" rule that would mean
the "best" solution would be to simply not add the new fields to the
existing record.  New records do not have these restrictions.

The good news is that Richard's suggestion of associating the record
with other related records should provide most (all?) of the
information that you were trying to log in the first place.

Finally, since you probably haven't followed all of the discussion
around associating records into a single event, I wanted to give you
my side of the story (if you don't care, you can simply skip the rest
of this email).  Currently an audit "event" can consist of multiple
audit "records"; these records can contain PATH information, SYSCALL
information, SELinux AVC information, as well as INTEGRITY_PCR
information.  The current kernel code does a poor job of associating
some of these record types, which can make a single user action look
like multiple audit events.  For example, a single user action/event
(writing a new IMA policy) could result in multiple audit "events"
because the SYSCALL record for the write() syscall is not associated
with the INTEGRITY_POLICY_RULE record; this is bogus because the write
syscall is inherently linked with the IMA policy update.  Keeping the
records as separate audit "events" is not only conceptually odd, it is
misleading.

-- 
paul moore
www.paul-moore.com

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

[Lenny Bruzenak]

On 05/30/2018 06:54 PM, Paul Moore wrote:

...

> Finally, since you probably haven't followed all of the discussion
> around associating records into a single event, I wanted to give you
> my side of the story (if you don't care, you can simply skip the rest
> of this email).  Currently an audit "event" can consist of multiple
> audit "records"; these records can contain PATH information, SYSCALL
> information, SELinux AVC information, as well as INTEGRITY_PCR
> information.  The current kernel code does a poor job of associating
> some of these record types, which can make a single user action look
> like multiple audit events.  For example, a single user action/event
> (writing a new IMA policy) could result in multiple audit "events"
> because the SYSCALL record for the write() syscall is not associated
> with the INTEGRITY_POLICY_RULE record; this is bogus because the write
> syscall is inherently linked with the IMA policy update.  Keeping the
> records as separate audit "events" is not only conceptually odd, it is
> misleading.
A vote of support from the user side; thanks Paul for this. The
misleading part of having multiple events for what is conceptually a
unitary event makes life really hard on guys like me trying to explain
to security people what happened when. So while I understand it can be
challenging to associate those events in the kernel, particularly when
they occur at spaced times, trying to patch those together on the
analysis side is problematic too (which I know you well understand). So
- your effort to keep these these records tied to one event is very much
appreciated!

LCB

-- 
Lenny Bruzenak
MagitekLTD

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

[Paul Moore]

On Wed, May 30, 2018 at 8:46 PM, Lenny Bruzenak <lenny@magitekltd.com> wrote:
> On 05/30/2018 06:54 PM, Paul Moore wrote:
>
> ...
>
>> Finally, since you probably haven't followed all of the discussion
>> around associating records into a single event, I wanted to give you
>> my side of the story (if you don't care, you can simply skip the rest
>> of this email).  Currently an audit "event" can consist of multiple
>> audit "records"; these records can contain PATH information, SYSCALL
>> information, SELinux AVC information, as well as INTEGRITY_PCR
>> information.  The current kernel code does a poor job of associating
>> some of these record types, which can make a single user action look
>> like multiple audit events.  For example, a single user action/event
>> (writing a new IMA policy) could result in multiple audit "events"
>> because the SYSCALL record for the write() syscall is not associated
>> with the INTEGRITY_POLICY_RULE record; this is bogus because the write
>> syscall is inherently linked with the IMA policy update.  Keeping the
>> records as separate audit "events" is not only conceptually odd, it is
>> misleading.
>
> A vote of support from the user side; thanks Paul for this. The
> misleading part of having multiple events for what is conceptually a
> unitary event makes life really hard on guys like me trying to explain
> to security people what happened when. So while I understand it can be
> challenging to associate those events in the kernel, particularly when
> they occur at spaced times, trying to patch those together on the
> analysis side is problematic too (which I know you well understand). So
> - your effort to keep these these records tied to one event is very much
> appreciated!

Thanks Lenny, I appreciate the feedback.  We should have better
connection between records when we make the audit container ID
changes, if not sooner.

<fingers crossed>

-- 
paul moore
www.paul-moore.com

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

[Stefan Berger]

On 05/30/2018 07:34 PM, Richard Guy Briggs wrote:
> 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@linux.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@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);
>>>>> 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.

When someone writes a policy for IMA into securityfs, it's always NULL. 
There's another location where IMA uses the current->audit_context, and 
that's here:

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

At this location we sometimes see a (background) process with an 
audit_context but in the majority of cases it's current->audit_context 
is NULL. Starting a process as root or also non-root user, with the 
appropriate IMA audit policy rules set, we always see a NULL 
audit_context here.


>
> - 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
>

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

[Paul Moore]

On Fri, Jun 1, 2018 at 4:00 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> On 05/30/2018 07:34 PM, Richard Guy Briggs wrote:
>>
>> 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@linux.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@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);
>>>>>>
>>>>>> 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.
>
>
> When someone writes a policy for IMA into securityfs, it's always NULL.
> There's another location where IMA uses the current->audit_context, and
> that's here:
>
> https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_api.c#L323
>
> At this location we sometimes see a (background) process with an
> audit_context but in the majority of cases it's current->audit_context is
> NULL. Starting a process as root or also non-root user, with the appropriate
> IMA audit policy rules set, we always see a NULL audit_context here.

What does your audit configuration look like?

Depending on your configuration a NULL audit_context can be expected,
see audit_dummy_context().  I believe the default Fedora audit config
will leave you with a NULL audit_context for all processes.  I also
believe that unless you explicitly set "audit=1" on the kernel command
line the init/systemd process will have a NULL audit_context (there
was actually a range of kernels where even setting "audit=1" wouldn't
be sufficient due to a bug we fixed a little while ago).

Look at the audit_alloc() function, it is called when a new process is
fork'd and is responsible for allocating a new audit_context.  If the
currently loaded audit config dictates that auditing is to be disabled
for this new process (state == AUDIT_DISABLED) then an audit_context
is not allocated and current->context remains NULL.

-- 
paul moore
www.paul-moore.com

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

[Paul Moore]

On Fri, Jun 1, 2018 at 4:13 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Fri, Jun 1, 2018 at 4:00 PM, Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
>> On 05/30/2018 07:34 PM, Richard Guy Briggs wrote:
>>>
>>> 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@linux.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@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);
>>>>>>>
>>>>>>> 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.
>>
>>
>> When someone writes a policy for IMA into securityfs, it's always NULL.
>> There's another location where IMA uses the current->audit_context, and
>> that's here:
>>
>> https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_api.c#L323
>>
>> At this location we sometimes see a (background) process with an
>> audit_context but in the majority of cases it's current->audit_context is
>> NULL. Starting a process as root or also non-root user, with the appropriate
>> IMA audit policy rules set, we always see a NULL audit_context here.
>
> What does your audit configuration look like?
>
> Depending on your configuration a NULL audit_context can be expected,
> see audit_dummy_context().  I believe the default Fedora audit config
> will leave you with a NULL audit_context for all processes.  I also
> believe that unless you explicitly set "audit=1" on the kernel command
> line the init/systemd process will have a NULL audit_context (there
> was actually a range of kernels where even setting "audit=1" wouldn't
> be sufficient due to a bug we fixed a little while ago).
>
> Look at the audit_alloc() function, it is called when a new process is
> fork'd and is responsible for allocating a new audit_context.  If the
> currently loaded audit config dictates that auditing is to be disabled
> for this new process (state == AUDIT_DISABLED) then an audit_context
> is not allocated and current->context remains NULL.

I should also add that a NULL current->context is not necessarily a
problem, assuming that it is the proper result of the loaded audit
configuration.  If current->context is NULL then the audit records
that are generated by that process will not be accompanied/associated
with a matching SYSCALL record ... which is okay since the
configuration explicitly blocked the creation of the SYSCALL record.
If current->context is non-NULL, then the audit records will be
associated with the matching SYSCALL record because that is the Right
Thing To Do.

While the exact details are still TBD, I expect there to be slight
changes to how this is all implemented in the upcoming audit container
ID work.  The impact on the IMA code should be minimal/nothing if you
are already passing current->context back into the audit subsystem.

-- 
paul moore
www.paul-moore.com

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

[Stefan Berger]

On 06/01/2018 04:13 PM, Paul Moore wrote:
> On Fri, Jun 1, 2018 at 4:00 PM, Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
>> On 05/30/2018 07:34 PM, Richard Guy Briggs wrote:
>>> 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@linux.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@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);
>>>>>>> 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.
>>
>> When someone writes a policy for IMA into securityfs, it's always NULL.
>> There's another location where IMA uses the current->audit_context, and
>> that's here:
>>
>> https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_api.c#L323
>>
>> At this location we sometimes see a (background) process with an
>> audit_context but in the majority of cases it's current->audit_context is
>> NULL. Starting a process as root or also non-root user, with the appropriate
>> IMA audit policy rules set, we always see a NULL audit_context here.
> What does your audit configuration look like?
>
> Depending on your configuration a NULL audit_context can be expected,
> see audit_dummy_context().  I believe the default Fedora audit config
> will leave you with a NULL audit_context for all processes.  I also
> believe that unless you explicitly set "audit=1" on the kernel command
> line the init/systemd process will have a NULL audit_context (there
> was actually a range of kernels where even setting "audit=1" wouldn't
> be sufficient due to a bug we fixed a little while ago).
>
> Look at the audit_alloc() function, it is called when a new process is
> fork'd and is responsible for allocating a new audit_context.  If the
> currently loaded audit config dictates that auditing is to be disabled
> for this new process (state == AUDIT_DISABLED) then an audit_context
> is not allocated and current->context remains NULL.

I found that out also. The background process that had the audit context 
was created when a different audit policy was active and therefore still 
has the audit_context and creates the associated syscall messages. The 
new processes don't get it because of -a task,never rule.