[PATCH 2/8] integrity: IMA as an integrity service provider
Steve Grubb
sgrubb at redhat.com
Fri Feb 6 22:04:02 UTC 2009
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.
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. */
> 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 at watson.ibm.com>
> + * Mimi Zohar <zohar at 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.
> 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 at 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 ?
> 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 at 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?
> + 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.
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.
> + 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);
> + }
> + 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.
> + 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 at watson.ibm.com>
> + * Leendert van Doorn <leendert at watson.ibm.com>
> + * Mimi Zohar <zohar at 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 ?
> +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.
> + 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 at 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?
I guess the main concern is the value of the result field.
-Steve
More information about the Linux-audit
mailing list