8:42 p.m.
On Fri, 2009-02-06 at 17:04 -0500, Steve Grubb wrote:
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.
Definitely preferable.
On Friday 06 February 2009 02:52:07 pm Mimi Zohar wrote:
> diff --git a/Documentation/kernel-parameters.txt
> b/Documentation/kernel-parameters.txt index 7c67b94..31e0c2c 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -895,6 +895,15 @@ and is between 256 and 4096 characters. It is defined
> in the file ihash_entries= [KNL]
> 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.
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 26c4f6f..8d1f677 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -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_INTEGRITY_DATA 1800 /* Data integrity verification */
> +#define AUDIT_INTEGRITY_METADATA 1801 /* Metadata integrity verification
*/
> +#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
> #define AUDIT_KERNEL 2000 /* Asynchronous audit record. NOT A REQUEST. */
ok
> diff --git a/security/integrity/ima/ima.h
b/security/integrity/ima/ima.h
> new file mode 100644
> index 0000000..bfa72ed
> --- /dev/null
> +++ b/security/integrity/ima/ima.h
> @@ -0,0 +1,135 @@
> +/*
> + * Copyright (C) 2005,2006,2007,2008 IBM Corporation
> + *
> + * Authors:
> + * Reiner Sailer <sailer(a)watson.ibm.com>
> + * Mimi Zohar <zohar(a)us.ibm.com>
> + *
> + * 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>
> +#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.
IMA measures files based on policy, which can be described in terms of
LSM specific labels using security_audit_rule_init/match.
> diff --git a/security/integrity/ima/ima_api.c
> b/security/integrity/ima/ima_api.c new file mode 100644
> index 0000000..a148a25
> --- /dev/null
> +++ b/security/integrity/ima/ima_api.c
> @@ -0,0 +1,190 @@
> +/*
> + * Copyright (C) 2008 IBM Corporation
> + *
> + * Author: Mimi Zohar <zohar(a)us.ibm.com>
> + *
> + * 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,
> + * and store_template.
> + */
> +#include <linux/module.h>
> +
> +#include "ima.h"
> +static char *IMA_TEMPLATE_NAME = "ima";
Would this also be a const ?
yes
> diff --git a/security/integrity/ima/ima_audit.c
> b/security/integrity/ima/ima_audit.c new file mode 100644
> index 0000000..8a0f1e2
> --- /dev/null
> +++ b/security/integrity/ima/ima_audit.c
> @@ -0,0 +1,78 @@
> +/*
> + * Copyright (C) 2008 IBM Corporation
> + * Author: Mimi Zohar <zohar(a)us.ibm.com>
> + *
> + * 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;
> +
> + 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?
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.
> + 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.
> + 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.
ok
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().
> + 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);
> + }
> + 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.
> + }
> + 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.
ok.
> + audit_log_end(ab);
> +}
> diff --git a/security/integrity/ima/ima_init.c
> b/security/integrity/ima/ima_init.c new file mode 100644
> index 0000000..e0f02e3
> --- /dev/null
> +++ b/security/integrity/ima/ima_init.c
> @@ -0,0 +1,90 @@
> +/*
> + * Copyright (C) 2005,2006,2007,2008 IBM Corporation
> + *
> + * Authors:
> + * Reiner Sailer <sailer(a)watson.ibm.com>
> + * Leendert van Doorn <leendert(a)watson.ibm.com>
> + * Mimi Zohar <zohar(a)us.ibm.com>
> + *
> + * 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>
> +#include <linux/scatterlist.h>
> +#include <linux/err.h>
> +#include "ima.h"
> +
> +/* name for boot aggregate entry */
> +static char *boot_aggregate_name = "boot_aggregate";
const ?
yes
> +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.
> + */
> +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.
ok.
> + int violation = 1;
> +
> + entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> + if (!entry)
> + goto err_out;
> +
> + 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.
> + 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);
> +}
> +
> diff --git a/security/integrity/ima/ima_policy.c
> b/security/integrity/ima/ima_policy.c new file mode 100644
> index 0000000..7c3d1ff
> --- /dev/null
> +++ b/security/integrity/ima/ima_policy.c
> @@ -0,0 +1,126 @@
> +/*
> + * Copyright (C) 2008 IBM Corporation
> + * Author: Mimi Zohar <zohar(a)us.ibm.com>
> + *
> + * 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
> + * - initialize default measure policy rules
> + *
> + */
> +#include <linux/module.h>
> +#include <linux/list.h>
> +#include <linux/audit.h>
Is audit.h really needed here?
As it is already defined in ima.h, it isn't needed here.
I guess the main concern is the value of the result field.
-Steve
Ok.
Mimi