integrity

Quoted 2 lines

The integrity patches are in security-testing-2.6/#next and the auditd patch I just posted to linux-audit. How do you suggest we go forward?

We need to go over the event format and make sure its got everything we need in it. We also need to review the code that touches the audit system and make sure its using the audit API the way we intended. I'd like to do this on the linux-audit mail list so there is a record of it in the audit archives.

Thanks,
-Steve

(quoted diff) +0 -0

b/Documentation/kernel-parameters.txt index 7c67b94..31e0c2c 100644

Documentation/kernel-parameters.txt +0 -0

@@ -895,6 +895,15 @@ and is between 256 and 4096 characters. It is defined

in the file ihash_entries= [KNL]

Documentation/kernel-parameters.txt +9 -0

 			Set number of hash buckets for inode cache.
 
+	ima_audit=	[IMA]
+			Format: { "0" | "1" }
+			0 -- integrity auditing messages. (Default)
+			1 -- enable informational integrity auditing messages.
+
+	ima_hash=	[IMA]
+			Formt: { "sha1" | "md5" }
+			default: "sha1"
+
 	in2000=		[HW,SCSI]
 			See header of drivers/scsi/in2000.c.

include/linux/audit.h +0 -0

index 26c4f6f..8d1f677 100644
@@ -125,6 +125,11 @@
 #define AUDIT_LAST_KERN_ANOM_MSG    1799
 #define AUDIT_ANOM_PROMISCUOUS      1700 /* Device changed promiscuous

mode */
#define AUDIT_ANOM_ABEND 1701 /* Process ended abnormally */

#define AUDIT_KERNEL 2000 /* Asynchronous audit record. NOT A REQUEST. */

security/integrity/ima/ima.h new file +15 -0

new file mode 100644
index 0000000..bfa72ed
@@ -0,0 +1,135 @@
+/*
+ * Copyright (C) 2005,2006,2007,2008 IBM Corporation
+ *
+ * Authors:
+ * Reiner Sailer <[email protected]>
+ * Mimi Zohar <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2 of the
+ * License.
+ *
+ * File: ima.h
+ *	internal Integrity Measurement Architecture (IMA) definitions
+ */

(quoted diff) +0 -0

b/security/integrity/ima/ima_api.c new file mode 100644 index 0000000..a148a25

security/integrity/ima/ima_api.c new file +8 -0

@@ -0,0 +1,190 @@
+/*
+ * Copyright (C) 2008 IBM Corporation
+ *
+ * Author: Mimi Zohar <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2 of the

(quoted diff) +0 -0

b/security/integrity/ima/ima_audit.c new file mode 100644 index 0000000..8a0f1e2

security/integrity/ima/ima_audit.c new file +39 -0

@@ -0,0 +1,78 @@
+/*
+ * Copyright (C) 2008 IBM Corporation
+ * Author: Mimi Zohar <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, version 2 of the License.
+ *
+ * File: integrity_audit.c
+ * 	Audit calls for the integrity subsystem
+ */
+
+#include <linux/fs.h>
+#include <linux/audit.h>
+#include "ima.h"
+
+static int ima_audit;
+
+#ifdef CONFIG_IMA_AUDIT
+
+/* ima_audit_setup - enable informational auditing messages */
+static int __init ima_audit_setup(char *str)
+{
+	unsigned long audit;
+	int rc;
+	char *op;
+
+	rc = strict_strtoul(str, 0, &audit);
+	if (rc || audit > 1)
+		printk(KERN_INFO "ima: invalid ima_audit value\n");
+	else
+		ima_audit = audit;
+	op = ima_audit ? "ima_audit_enabled" : "ima_audit_not_enabled";
+	integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL, NULL, op, 0, 0);
+	return 1;
+}
+__setup("ima_audit=", ima_audit_setup);
+#endif
+

security/integrity/ima/ima_audit.c new file +14 -0

+	audit_log_task_context(ab);
+	switch (audit_msgno) {
+	case AUDIT_INTEGRITY_DATA:
+	case AUDIT_INTEGRITY_METADATA:
+	case AUDIT_INTEGRITY_PCR:
+		audit_log_format(ab, " op=%s cause=%s", op, cause);
+		break;
+	case AUDIT_INTEGRITY_HASH:
+		audit_log_format(ab, " op=%s hash=%s", op, cause);
+		break;
+	case AUDIT_INTEGRITY_STATUS:
+	default:
+		audit_log_format(ab, " op=%s", op);
+	}

security/integrity/ima/ima_audit.c new file +2 -0

+	audit_log_end(ab);
+}

(quoted diff) +0 -0

b/security/integrity/ima/ima_init.c new file mode 100644 index 0000000..e0f02e3

security/integrity/ima/ima_init.c new file +13 -0

@@ -0,0 +1,90 @@
+/*
+ * Copyright (C) 2005,2006,2007,2008 IBM Corporation
+ *
+ * Authors:
+ * Reiner Sailer      <[email protected]>
+ * Leendert van Doorn <[email protected]>
+ * Mimi Zohar         <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2 of the
+ * License.
+ *

security/integrity/ima/ima_init.c new file +13 -0

+int ima_used_chip;
+
+/* Add the boot aggregate to the IMA measurement list and extend
+ * the PCR register.
+ *
+ * Calculate the boot aggregate, a SHA1 over tpm registers 0-7,
+ * assuming a TPM chip exists, and zeroes if the TPM chip does not
+ * exist.  Add the boot aggregate measurement to the measurement
+ * list and extend the PCR register.
+ *
+ * If a tpm chip does not exist, indicate the core root of trust is
+ * not hardware based by invalidating the aggregate PCR value.
+ * (The aggregate PCR value is invalidated by adding one value to

security/integrity/ima/ima_init.c new file +5 -0

+	int violation = 1;
+
+	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		goto err_out;

security/integrity/ima/ima_init.c new file +12 -0

+			goto err_out;
+		}
+	}
+	result = ima_store_template(entry, violation, NULL);
+	if (result < 0)
+		kfree(entry);
+	return;
+err_out:
+	integrity_audit_msg(AUDIT_INTEGRITY_PCR, NULL, boot_aggregate_name, op,
+			    audit_cause, result, 0);
+}
+

(quoted diff) +0 -0

b/security/integrity/ima/ima_policy.c new file mode 100644 index 0000000..7c3d1ff

security/integrity/ima/ima_policy.c new file +5 -0

@@ -0,0 +1,126 @@
+/*
+ * Copyright (C) 2008 IBM Corporation
+ * Author: Mimi Zohar <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify

Hi,

Thanks for sending the audit piece to the mail list so we could go over the details without bothering the whole lkml. I have some comments in line below.

Quoted 29 lines

(quoted diff) +0 -0

b/Documentation/kernel-parameters.txt index 7c67b94..31e0c2c 100644

Documentation/kernel-parameters.txt +0 -0

@@ -895,6 +895,15 @@ and is between 256 and 4096 characters. It is defined

in the file ihash_entries= [KNL]

Documentation/kernel-parameters.txt +9 -0

 			Set number of hash buckets for inode cache.
 
+	ima_audit=	[IMA]
+			Format: { "0" | "1" }
+			0 -- integrity auditing messages. (Default)
+			1 -- enable informational integrity auditing messages.
+
+	ima_hash=	[IMA]
+			Formt: { "sha1" | "md5" }
+			default: "sha1"
+
 	in2000=		[HW,SCSI]
 			See header of drivers/scsi/in2000.c.

include/linux/audit.h +0 -0

index 26c4f6f..8d1f677 100644
@@ -125,6 +125,11 @@
 #define AUDIT_LAST_KERN_ANOM_MSG    1799
 #define AUDIT_ANOM_PROMISCUOUS      1700 /* Device changed promiscuous

mode */
#define AUDIT_ANOM_ABEND 1701 /* Process ended abnormally */

include/linux/audit.h

Quoted 2 lines

include/linux/audit.h +2 -0

+#define AUDIT_INTEGRITY_DATA	    1800 /* Data integrity verification */ 
+#define AUDIT_INTEGRITY_METADATA    1801 /* Metadata integrity verification 

*/

include/linux/audit.h line 132

Quoted 3 lines

include/linux/audit.h +3 -0

+#define AUDIT_INTEGRITY_STATUS	    1802 /* Integrity enable status */
+#define AUDIT_INTEGRITY_HASH	    1803 /* Integrity HASH type */
+#define AUDIT_INTEGRITY_PCR	    1804 /* PCR invalidation msgs */

If you are taking this block, I'd like to have the netlink block map around line 39 updated too. Sb something like:

* 1800 - 1899 integrity labels and related events
* 1900 - 1999 future kernel use

Quoted 1 line

#define AUDIT_KERNEL 2000 /* Asynchronous audit record. NOT A REQUEST. */

Quoted 26 lines

security/integrity/ima/ima.h new file +20 -0

new file mode 100644
index 0000000..bfa72ed
@@ -0,0 +1,135 @@
+/*
+ * Copyright (C) 2005,2006,2007,2008 IBM Corporation
+ *
+ * Authors:
+ * Reiner Sailer <[email protected]>
+ * Mimi Zohar <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2 of the
+ * License.
+ *
+ * File: ima.h
+ *	internal Integrity Measurement Architecture (IMA) definitions
+ */
+
+#ifndef __LINUX_IMA_H
+#define __LINUX_IMA_H
+
+#include <linux/types.h>

security/integrity/ima/ima.h line 25

Quoted 5 lines

security/integrity/ima/ima.h new file +5 -0

+#include <linux/crypto.h>
+#include <linux/security.h>
+#include <linux/hash.h>
+#include <linux/tpm.h>
+#include <linux/audit.h>

So why did you include the audit.h file in this header? I don't see where its used for any function prototype.

Quoted 18 lines

(quoted diff) +0 -0

b/security/integrity/ima/ima_api.c new file mode 100644 index 0000000..a148a25

security/integrity/ima/ima_api.c new file +12 -0

@@ -0,0 +1,190 @@
+/*
+ * Copyright (C) 2008 IBM Corporation
+ *
+ * Author: Mimi Zohar <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2 of the
+ * License.
+ *
+ * File: ima_api.c
+ *	Implements must_measure, collect_measurement, store_measurement,

security/integrity/ima/ima_api.c line 18

Quoted 6 lines

security/integrity/ima/ima_api.c new file +6 -0

+ *	and store_template.
+ */
+#include <linux/module.h>
+
+#include "ima.h"
+static char *IMA_TEMPLATE_NAME = "ima";

Would this also be a const ?

Quoted 51 lines

(quoted diff) +0 -0

b/security/integrity/ima/ima_audit.c new file mode 100644 index 0000000..8a0f1e2

security/integrity/ima/ima_audit.c new file +45 -0

@@ -0,0 +1,78 @@
+/*
+ * Copyright (C) 2008 IBM Corporation
+ * Author: Mimi Zohar <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, version 2 of the License.
+ *
+ * File: integrity_audit.c
+ * 	Audit calls for the integrity subsystem
+ */
+
+#include <linux/fs.h>
+#include <linux/audit.h>
+#include "ima.h"
+
+static int ima_audit;
+
+#ifdef CONFIG_IMA_AUDIT
+
+/* ima_audit_setup - enable informational auditing messages */
+static int __init ima_audit_setup(char *str)
+{
+	unsigned long audit;
+	int rc;
+	char *op;
+
+	rc = strict_strtoul(str, 0, &audit);
+	if (rc || audit > 1)
+		printk(KERN_INFO "ima: invalid ima_audit value\n");
+	else
+		ima_audit = audit;
+	op = ima_audit ? "ima_audit_enabled" : "ima_audit_not_enabled";
+	integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL, NULL, op, 0, 0);
+	return 1;
+}
+__setup("ima_audit=", ima_audit_setup);
+#endif
+
+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;
+

security/integrity/ima/ima_audit.c line 49

Quoted 4 lines

security/integrity/ima/ima_audit.c new file +4 -0

+	if (!ima_audit && audit_info == 1) /* Skip informational messages */
+		return;
+
+	ab = audit_log_start(current->audit_context, GFP_KERNEL, audit_msgno);

Is this a standalone event or would it be an auxiliary record added to a syscall record? IOW, if I have a watch on the same file as was being measured, would the event have the same serial number?

security/integrity/ima/ima_audit.c line 50

Quoted 1 line

security/integrity/ima/ima_audit.c new file +1 -0

+	audit_log_format(ab, "integrity: pid=%d uid=%u auid=%u",

The word "integrity:" can be deleted. All the events will have it in the event type.

security/integrity/ima/ima_audit.c line 52

Quoted 2 lines

security/integrity/ima/ima_audit.c new file +2 -0

+			 current->pid, current->cred->uid,
+			 audit_get_loginuid(current));

After logging the auid, we now require logging ses=%u for sessionid, use audit_get_sessionid() for the value. This is something that has come up in the last year, but we have it in all events now so that if someone logs in multiple times, we know which session it originated in.

If you are depending on a syscall record, it will record much of what's recorded here for itself and you won't need this much. If you wanted this to be standalone, I think the call to audit_ log_start has NULL as the first param so that it cannot be associated with a syscall.

Quoted 11 lines

security/integrity/ima/ima_audit.c new file +11 -0

+	audit_log_task_context(ab);
+	switch (audit_msgno) {
+	case AUDIT_INTEGRITY_DATA:
+	case AUDIT_INTEGRITY_METADATA:
+	case AUDIT_INTEGRITY_PCR:
+		audit_log_format(ab, " op=%s cause=%s", op, cause);
+		break;
+	case AUDIT_INTEGRITY_HASH:
+		audit_log_format(ab, " op=%s hash=%s", op, cause);
+		break;
+	case AUDIT_INTEGRITY_STATUS:

security/integrity/ima/ima_audit.c line 71

Quoted 8 lines

security/integrity/ima/ima_audit.c new file +8 -0

+	default:
+		audit_log_format(ab, " op=%s", op);
+	}
+	audit_log_format(ab, " comm=");
+	audit_log_untrustedstring(ab, current->comm);
+	if (fname) {
+		audit_log_format(ab, " name=");
+		audit_log_untrustedstring(ab, fname);

security/integrity/ima/ima_audit.c line 76

Quoted 5 lines

security/integrity/ima/ima_audit.c new file +5 -0

+	}
+	if (inode)
+		audit_log_format(ab, " dev=%s ino=%lu",
+				 inode->i_sb->s_id, inode->i_ino);
+	audit_log_format(ab, " res=%d", result);

Note that result should only be 0 (success) or 1 (failure). I saw earlier that a result was getting set to -ENOMEM.

Quoted 27 lines

security/integrity/ima/ima_audit.c new file +2 -0

+	audit_log_end(ab);
+}

(quoted diff) +0 -0

b/security/integrity/ima/ima_init.c new file mode 100644 index 0000000..e0f02e3

security/integrity/ima/ima_init.c new file +17 -0

@@ -0,0 +1,90 @@
+/*
+ * Copyright (C) 2005,2006,2007,2008 IBM Corporation
+ *
+ * Authors:
+ * Reiner Sailer      <[email protected]>
+ * Leendert van Doorn <[email protected]>
+ * Mimi Zohar         <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2 of the
+ * License.
+ *
+ * File: ima_init.c
+ *             initialization and cleanup functions
+ */
+#include <linux/module.h>

security/integrity/ima/ima_init.c line 23

Quoted 6 lines

security/integrity/ima/ima_init.c new file +6 -0

+#include <linux/scatterlist.h>
+#include <linux/err.h>
+#include "ima.h"
+
+/* name for boot aggregate entry */
+static char *boot_aggregate_name = "boot_aggregate";

const ?

Quoted 17 lines

security/integrity/ima/ima_init.c new file +17 -0

+int ima_used_chip;
+
+/* Add the boot aggregate to the IMA measurement list and extend
+ * the PCR register.
+ *
+ * Calculate the boot aggregate, a SHA1 over tpm registers 0-7,
+ * assuming a TPM chip exists, and zeroes if the TPM chip does not
+ * exist.  Add the boot aggregate measurement to the measurement
+ * list and extend the PCR register.
+ *
+ * If a tpm chip does not exist, indicate the core root of trust is
+ * not hardware based by invalidating the aggregate PCR value.
+ * (The aggregate PCR value is invalidated by adding one value to
+ * the measurement list and extending the aggregate PCR value with
+ * a different value.) Violations add a zero entry to the measurement
+ * list and extend the aggregate PCR value with ff...ff's.
+ */

security/integrity/ima/ima_init.c line 46

Quoted 6 lines

security/integrity/ima/ima_init.c new file +6 -0

+static void ima_add_boot_aggregate(void)
+{
+	struct ima_template_entry *entry;
+	const char *op = "add_boot_aggregate";
+	const char *audit_cause = "ENOMEM";
+	int result = -ENOMEM;

You can't send the audit system this value.

Quoted 6 lines

security/integrity/ima/ima_init.c new file +5 -0

+	int violation = 1;
+
+	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		goto err_out;

security/integrity/ima/ima_init.c line 61

Quoted 10 lines

security/integrity/ima/ima_init.c new file +10 -0

+
+	memset(&entry->template, 0, sizeof(entry->template));
+	strncpy(entry->template.file_name, boot_aggregate_name,
+		IMA_EVENT_NAME_LEN_MAX);
+	if (ima_used_chip) {
+		violation = 0;
+		result = ima_calc_boot_aggregate(entry->template.digest);
+		if (result < 0) {
+			audit_cause = "hashing_error";
+			kfree(entry);

You may want "!!result" so that its changed to a 1 for failure. Or maybe you want to do that in integrity_audit_msg so you only have 1 place to change it.

Quoted 28 lines

security/integrity/ima/ima_init.c new file +12 -0

+			goto err_out;
+		}
+	}
+	result = ima_store_template(entry, violation, NULL);
+	if (result < 0)
+		kfree(entry);
+	return;
+err_out:
+	integrity_audit_msg(AUDIT_INTEGRITY_PCR, NULL, boot_aggregate_name, op,
+			    audit_cause, result, 0);
+}
+

(quoted diff) +0 -0

b/security/integrity/ima/ima_policy.c new file mode 100644 index 0000000..7c3d1ff

security/integrity/ima/ima_policy.c new file +9 -0

@@ -0,0 +1,126 @@
+/*
+ * Copyright (C) 2008 IBM Corporation
+ * Author: Mimi Zohar <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, version 2 of the License.
+ *
+ * ima_policy.c

security/integrity/ima/ima_policy.c line 15

Quoted 6 lines

security/integrity/ima/ima_policy.c new file +6 -0

+ * 	- initialize default measure policy rules
+ *
+ */
+#include <linux/module.h>
+#include <linux/list.h>
+#include <linux/audit.h>

Is audit.h really needed here?

I guess the main concern is the value of the result field.

-Steve

Quoted 12 lines

Quoted 12 lines

(quoted diff) +0 -0

b/security/integrity/ima/ima_audit.c new file mode 100644 index 0000000..8a0f1e2

security/integrity/ima/ima_audit.c new file +7 -0

+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 (!ima_audit && audit_info == 1) /* Skip informational messages */

Quoted 8 lines

Quoted 4 lines

security/integrity/ima/ima_audit.c new file +3 -0

+		return;
+
+	ab = audit_log_start(current->audit_context, GFP_KERNEL,

audit_msgno);

Is this a standalone event or would it be an auxiliary record added to a syscall record? IOW, if I have a watch on the same file as was being measured, would the event have the same serial number?

Having it as an auxiliary record to syscall seems appropriate.

There seems to be different behavior if you stop auditd and if it isn't enabled at boot. In the former case the syscall message still goes to /var/log/messages, while in the latter case it doesn't.

Quoted 4 lines

If you are depending on a syscall record, it will record much of what's recorded here for itself and you won't need this much. If you wanted this to be standalone, I think the call to audit_ log_start has NULL as the first param so that it cannot be associated with a syscall.

yes that works. Some of the messages don't need the syscall info, like in ima_parse_rules().

Quoted 13 lines

Quoted 13 lines

security/integrity/ima/ima_audit.c new file +13 -0

+	audit_log_task_context(ab);
+	switch (audit_msgno) {
+	case AUDIT_INTEGRITY_DATA:
+	case AUDIT_INTEGRITY_METADATA:
+	case AUDIT_INTEGRITY_PCR:
+		audit_log_format(ab, " op=%s cause=%s", op, cause);
+		break;
+	case AUDIT_INTEGRITY_HASH:
+		audit_log_format(ab, " op=%s hash=%s", op, cause);
+		break;
+	case AUDIT_INTEGRITY_STATUS:
+	default:
+		audit_log_format(ab, " op=%s", op);

Quoted 6 lines

security/integrity/ima/ima_audit.c line 71

Quoted 6 lines

security/integrity/ima/ima_audit.c new file +6 -0

+	}
+	audit_log_format(ab, " comm=");
+	audit_log_untrustedstring(ab, current->comm);
+	if (fname) {
+		audit_log_format(ab, " name=");
+		audit_log_untrustedstring(ab, fname);

Sure would be nice to be able to print the full pathname.

Quoted 21 lines · Mimi Zohar

Quoted 12 lines

Quoted 12 lines

(quoted diff) +0 -0

b/security/integrity/ima/ima_audit.c new file mode 100644 index 0000000..8a0f1e2

security/integrity/ima/ima_audit.c new file +7 -0

+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 (!ima_audit && audit_info == 1) /* Skip informational messages */

Quoted 8 lines

Quoted 4 lines

security/integrity/ima/ima_audit.c new file +3 -0

+		return;
+
+	ab = audit_log_start(current->audit_context, GFP_KERNEL,

audit_msgno);

Is this a standalone event or would it be an auxiliary record added to a syscall record? IOW, if I have a watch on the same file as was being measured, would the event have the same serial number?

Quoted 1 line

Having it as an auxiliary record to syscall seems appropriate.

OK. There are are three options then.

1) The first is to just supplement the syscall record whether its there or not. This is kind of what AVCs do. In that case, everything you are logging is fine.
2) Change the audit event really be an auxiliary record that is output at syscall exit only if an audit event has triggered the event. This is more like what the PATH, CWD, or EXECVE records do.
3) The last approach is to cause a syscall audit record to be emitted. This is what the MAC_STATUS does. For this case and #2 above, we could shorten the data collected since the syscall record duplicates many fields.

Quoted 3 lines

There seems to be different behavior if you stop auditd and if it isn't enabled at boot. In the former case the syscall message still goes to /var/log/messages, while in the latter case it doesn't.

It all depends on if the audit system is enabled. If its enabled, events go to either the daemon or syslog. If the audit system is not enabled, then the admin didn't want any notification.

Quoted 5 lines

Quoted 4 lines

If you are depending on a syscall record, it will record much of what's recorded here for itself and you won't need this much. If you wanted this to be standalone, I think the call to audit_ log_start has NULL as the first param so that it cannot be associated with a syscall.

Quoted 2 lines

yes that works. Some of the messages don't need the syscall info, like in ima_parse_rules().

The perhaps you want 2 logging functions? Or maybe pass something to the function that determines if it should be related to an existing event or its entirely async.

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

security/integrity/ima/ima_audit.c new file +13 -0

+	audit_log_task_context(ab);
+	switch (audit_msgno) {
+	case AUDIT_INTEGRITY_DATA:
+	case AUDIT_INTEGRITY_METADATA:
+	case AUDIT_INTEGRITY_PCR:
+		audit_log_format(ab, " op=%s cause=%s", op, cause);
+		break;
+	case AUDIT_INTEGRITY_HASH:
+		audit_log_format(ab, " op=%s hash=%s", op, cause);
+		break;
+	case AUDIT_INTEGRITY_STATUS:
+	default:
+		audit_log_format(ab, " op=%s", op);

Quoted 8 lines

Quoted 6 lines

security/integrity/ima/ima_audit.c line 71

Quoted 6 lines

security/integrity/ima/ima_audit.c new file +6 -0

+	}
+	audit_log_format(ab, " comm=");
+	audit_log_untrustedstring(ab, current->comm);
+	if (fname) {
+		audit_log_format(ab, " name=");
+		audit_log_untrustedstring(ab, fname);

Sure would be nice to be able to print the full pathname.

There is an AUDIT_PATH record that should have the whole path. Its an auxiliary record emitted with the syscall.

Maybe we should talk a little about when the different events would be emitted. Would it be just on creation of the hash? Would it be just on a miscompare? IOW, how many records are likely to be created during a typical session?

Thanks,
-Steve

security/integrity/ima/ima_audit.c new file +9 -0

+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 (!ima_audit && audit_info == 1) /* Skip informational messages */
+               return;
+

security/integrity/ima/ima_audit.c new file +19 -0

+               break;
+       case AUDIT_INTEGRITY_HASH:
+               audit_log_format(ab, " op=%s hash=%s", op, cause);
+               break;
+       case AUDIT_INTEGRITY_STATUS:
+       default:
+               audit_log_format(ab, " op=%s", op);
+       }
+       audit_log_format(ab, " comm=");
+       audit_log_untrustedstring(ab, current->comm);
+       if (fname) {
+               audit_log_format(ab, " name=");
+               audit_log_untrustedstring(ab, fname);
+       }
+       if (inode)
+               audit_log_format(ab, " dev=%s ino=%lu",
+                                inode->i_sb->s_id, inode->i_ino);
+       audit_log_format(ab, " res=%d", result);
+       audit_log_end(ab);

I'm very slow to the game, I know, but today was the first kernel that I built from linux-next with IMA on. I have a comment, and hopefully more to come....

Quoted 9 lines

security/integrity/ima/ima_audit.c new file +9 -0

+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 (!ima_audit && audit_info == 1) /* Skip informational messages */
+               return;
+

security/integrity/ima/ima_audit.c line 58

Quoted 10 lines

security/integrity/ima/ima_audit.c new file +10 -0

+       ab = audit_log_start(current->audit_context, GFP_KERNEL, audit_msgno);
+       audit_log_format(ab, "integrity: pid=%d uid=%u auid=%u",
+                        current->pid, current->cred->uid,
+                        audit_get_loginuid(current));
+       audit_log_task_context(ab);
+       switch (audit_msgno) {
+       case AUDIT_INTEGRITY_DATA:
+       case AUDIT_INTEGRITY_METADATA:
+       case AUDIT_INTEGRITY_PCR:
+               audit_log_format(ab, " op=%s cause=%s", op, cause);

There are cases where (at least) cause can be a multiword string. These should be using audit_log_string() instead of %s. I'm starting to frown on %s in audit log more and more heavily these days, actually I don't think I should let any more users of %s in at all. If you want I'll go through and change all of these, but I want to make sure that you will be ok with multiword string being either "" or translated. An example of the problem I see would be in say ima_update_policy() where you have const char *cause = "already exists"; Currently that record will get emitted like:

blah cause=already exists blah

which screws up parsers. It should be:

blah cause="already exists" blah

make sense?

Quoted 3 lines

security/integrity/ima/ima_audit.c new file +3 -0

+               break;
+       case AUDIT_INTEGRITY_HASH:
+               audit_log_format(ab, " op=%s hash=%s", op, cause);

Quoted 6 lines

security/integrity/ima/ima_audit.c new file +6 -0

+               break;
+       case AUDIT_INTEGRITY_STATUS:
+       default:
+               audit_log_format(ab, " op=%s", op);
+       }
+       audit_log_format(ab, " comm=");

security/integrity/ima/ima_audit.c line 77

Quoted 10 lines

security/integrity/ima/ima_audit.c new file +10 -0

+       audit_log_untrustedstring(ab, current->comm);
+       if (fname) {
+               audit_log_format(ab, " name=");
+               audit_log_untrustedstring(ab, fname);
+       }
+       if (inode)
+               audit_log_format(ab, " dev=%s ino=%lu",
+                                inode->i_sb->s_id, inode->i_ino);
+       audit_log_format(ab, " res=%d", result);
+       audit_log_end(ab);