Skip to contentSkip to conversation

integrity

PATCH In review Mimi Zohar 4 participants 23 messages Nov 20, 2008 – Nov 21, 2008

Subsystems (10)
Memory management Documentation VFS Security (LSM) +6 more subsystems
lore
Share & tools
lore

Mimi Zohar

author
#0 lore raw

cover integrity

The first three patches address concerns raised on the mailing list. They apply cleanly to linux-2.6.28-rc5 and with minor offsets to security-testing-2.6/#next. The last patch is only for the security-testing-2.6/#next tree. It addresses credential merge issues.

- Merges the integrity radix tree patch into the existing integrity patches.
- Replaces integrity_nameidata_check() with integrity_path_check().
- Cleans up tget/tput callers and removed the null pointer test.
- Replaces remaining GFP_ATOMIC alloc with GFP_KERNEL.
- Replaces ima_iint_init() with RADIX_TREE macro.

Mimi Zohar (4):
integrity: TPM internel kernel interface
integrity: Linux Integrity Module(LIM)
integrity: IMA as an integrity service provider
integrity: replace task uid with cred uid

author
#1 lore raw

PATCH 1/4 integrity: TPM internel kernel interface

The internal TPM kernel interface did not protect itself from the removal of the TPM driver, while being used. We continue to protect the tpm_chip_list using the driver_lock as before, and are using an rcu lock to protect readers. The internal TPM kernel interface now protects itself from the driver being removed by incrementing the module reference count.

Resubmitting integrity-tpm-internal-kernel-interface.patch, which was previously Signed-off-by Kylene Hall.
Updated per feedback:

Adds the following support:
- make internal kernel interface to transmit TPM commands global
- adds reading a pcr value
- adds extending a pcr value
- adds lookup the tpm_chip for given chip number and type

Signed-off-by Mimi Zohar <[email protected]>
Signed-off-by Rajiv Andrade <[email protected]>

---

drivers/char/tpm/tpm.c +137 -3
index 9c47dc4..17d2849 100644
@@ -1,11 +1,12 @@
 /*
- * Copyright (C) 2004 IBM Corporation
+ * Copyright (C) 2004,2007,2008 IBM Corporation
  *
  * Authors:
  * Leendert van Doorn <[email protected]>
  * Dave Safford <[email protected]>
  * Reiner Sailer <[email protected]>
  * Kylene Hall <[email protected]>
+ * Debora Velarde <[email protected]>
  *
  * Maintained by: <[email protected]>
  *

@@ -28,6 +29,14 @@
 #include <linux/spinlock.h>
 #include <linux/smp_lock.h>
 
+#include <linux/mm.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/crypto.h>
+#include <linux/fs.h>
+#include <linux/scatterlist.h>
+#include <linux/rcupdate.h>
+#include <asm/unaligned.h>
 #include "tpm.h"
 
 enum tpm_const {

@@ -50,6 +59,8 @@ enum tpm_duration {
 static LIST_HEAD(tpm_chip_list);
 static DEFINE_SPINLOCK(driver_lock);
 static DECLARE_BITMAP(dev_mask, TPM_NUM_DEVICES);
+#define TPM_CHIP_NUM_MASK       0x0000ffff
+#define TPM_CHIP_TYPE_SHIFT     16
 
 /*
  * Array with one entry per ordinal defining the maximum amount

@@ -366,8 +377,7 @@ EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
 /*
  * Internal kernel interface to transmit TPM commands
  */
-static ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
-			    size_t bufsiz)
+ssize_t tpm_transmit(struct tpm_chip *chip, char *buf, size_t bufsiz)
 {
 	ssize_t rc;
 	u32 count, ordinal;

@@ -425,6 +435,7 @@ out:
 	mutex_unlock(&chip->tpm_mutex);
 	return rc;
 }
+EXPORT_SYMBOL_GPL(tpm_transmit);
 
 #define TPM_DIGEST_SIZE 20
 #define TPM_ERROR_SIZE 10

@@ -717,6 +728,7 @@ ssize_t tpm_show_temp_deactivated(struct device * dev,
 }
 EXPORT_SYMBOL_GPL(tpm_show_temp_deactivated);
 
+#define READ_PCR_RESULT_SIZE 30
 static const u8 pcrread[] = {
 	0, 193,			/* TPM_TAG_RQU_COMMAND */
 	0, 0, 0, 14,		/* length */

@@ -772,6 +784,128 @@ out:
 }
 EXPORT_SYMBOL_GPL(tpm_show_pcrs);
 
+/*
+ * tpm_chip_lookup - return tpm_chip for given chip number and type
+ *
+ * Must be called with rcu_read_lock.
+ */
+static struct tpm_chip *tpm_chip_lookup(int chip_num, int chip_typ)
+{
+	struct tpm_chip *pos;
+	int rc;
+
+	list_for_each_entry_rcu(pos, &tpm_chip_list, list) {
+		rc = (chip_num == TPM_ANY_NUM || pos->dev_num == chip_num)
+		    && (chip_typ == TPM_ANY_TYPE);
+		if (rc)
+			return pos;
+	}
+	return NULL;
+}
+
+/**
+ * tpm_pcr_read - read a pcr value
+ * @chip_id: 	tpm chip identifier
+ * 		Upper 2 bytes: ANY, HW_ONLY or SW_ONLY
+ * 		Lower 2 bytes: tpm idx # or AN&
+ * @pcr_idx:	pcr idx to retrieve
+ * @res_buf: 	TPM_PCR value
+ * 		size of res_buf is 20 bytes (or NULL if you don't care)
+ *
+ * The TPM driver should be built-in, but for whatever reason it
+ * isn't, protect against the chip disappearing, by incrementing
+ * the module usage count.
+ */
+int tpm_pcr_read(u32 chip_id, int pcr_idx, u8 *res_buf)
+{
+	u8 data[READ_PCR_RESULT_SIZE];
+	int rc;
+	__be32 index;
+	int chip_num = chip_id & TPM_CHIP_NUM_MASK;
+	struct tpm_chip *chip;
+
+	rcu_read_lock();
+	chip = tpm_chip_lookup(chip_num, chip_id >> TPM_CHIP_TYPE_SHIFT);
+	if (chip == NULL) {
+		rcu_read_unlock();
+		return -ENODEV;
+	}
+	if (!try_module_get(chip->dev->driver->owner)) {
+		rcu_read_unlock();
+		return -ENODEV;
+	}
+	rcu_read_unlock();
+
+	BUILD_BUG_ON(sizeof(pcrread) > READ_PCR_RESULT_SIZE);
+	memcpy(data, pcrread, sizeof(pcrread));
+	index = cpu_to_be32(pcr_idx);
+	memcpy(data + 10, &index, 4);
+	rc = tpm_transmit(chip, data, sizeof(data));
+	if (rc > 0)
+		rc = get_unaligned_be32((__be32 *) (data + 6));
+
+	if (rc == 0 && res_buf)
+		memcpy(res_buf, data + 10, TPM_DIGEST_SIZE);
+
+	module_put(chip->dev->driver->owner);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(tpm_pcr_read);
+
+#define EXTEND_PCR_SIZE 34
+static const u8 pcrextend[] = {
+	0, 193,			/* TPM_TAG_RQU_COMMAND */
+	0, 0, 0, 34,		/* length */
+	0, 0, 0, 20,		/* TPM_ORD_Extend */
+	0, 0, 0, 0		/* PCR index */
+};
+
+/**
+ * tpm_pcr_extend - extend pcr value with hash
+ * @chip_id: 	tpm chip identifier
+ * 		Upper 2 bytes: ANY, HW_ONLY or SW_ONLY
+ * 		Lower 2 bytes: tpm idx # or AN&
+ * @pcr_idx:	pcr idx to extend
+ * @hash: 	hash value used to extend pcr value
+ *
+ * The TPM driver should be built-in, but for whatever reason it
+ * isn't, protect against the chip disappearing, by incrementing
+ * the module usage count.
+ */
+int tpm_pcr_extend(u32 chip_id, int pcr_idx, const u8 *hash)
+{
+	u8 data[EXTEND_PCR_SIZE];
+	int rc;
+	__be32 index;
+	int chip_num = chip_id & TPM_CHIP_NUM_MASK;
+	struct tpm_chip *chip;
+
+	rcu_read_lock();
+	chip = tpm_chip_lookup(chip_num, chip_id >> TPM_CHIP_TYPE_SHIFT);
+	if (chip == NULL) {
+		rcu_read_unlock();
+		return -ENODEV;
+	}
+	if (!try_module_get(chip->dev->driver->owner)) {
+		rcu_read_unlock();
+		return -ENODEV;
+	}
+	rcu_read_unlock();
+
+	BUILD_BUG_ON(sizeof(pcrextend) > EXTEND_PCR_SIZE);
+	memcpy(data, pcrextend, sizeof(pcrextend));
+	index = cpu_to_be32(pcr_idx);
+	memcpy(data + 10, &index, 4);
+	memcpy(data + 14, hash, TPM_DIGEST_SIZE);
+	rc = tpm_transmit(chip, data, sizeof(data));
+	if (rc > 0)
+		rc = get_unaligned_be32((__be32 *) (data + 6));
+
+	module_put(chip->dev->driver->owner);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(tpm_pcr_extend);
+
 #define  READ_PUBEK_RESULT_SIZE 314
 static const u8 readpubek[] = {
 	0, 193,			/* TPM_TAG_RQU_COMMAND */
drivers/char/tpm/tpm.h +3 -1
index 8e30df4..e0ffddb 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2004 IBM Corporation
+ * Copyright (C) 2004, 2007, 2008 IBM Corporation
  *
  * Authors:
  * Leendert van Doorn <[email protected]>

@@ -26,6 +26,7 @@
 #include <linux/miscdevice.h>
 #include <linux/platform_device.h>
 #include <linux/io.h>
+#include <linux/tpm.h>
 
 enum tpm_timeout {
 	TPM_TIMEOUT = 5,	/* msecs */

@@ -128,6 +129,7 @@ extern void tpm_get_timeouts(struct tpm_chip *);
 extern void tpm_gen_interrupt(struct tpm_chip *);
 extern void tpm_continue_selftest(struct tpm_chip *);
 extern unsigned long tpm_calc_ordinal_duration(struct tpm_chip *, u32);
+ssize_t tpm_transmit(struct tpm_chip *chip, char *buf, size_t bufsiz);
 extern struct tpm_chip* tpm_register_hardware(struct device *,
 				 const struct tpm_vendor_specific *);
 extern int tpm_open(struct inode *, struct file *);
drivers/char/tpm/tpm_tis.c +3 -0
index 717af7a..bc26116 100644
@@ -642,6 +642,9 @@ static __devexit void tpm_tis_pnp_remove(struct pnp_dev *dev)
 
 static struct pnp_driver tis_pnp_driver = {
 	.name = "tpm_tis",
+	.driver = {
+		.owner = THIS_MODULE,
+	},
 	.id_table = tpm_pnp_tbl,
 	.probe = tpm_tis_pnp_init,
 	.suspend = tpm_tis_pnp_suspend,
include/linux/tpm.h new file +49 -0

49 lines not on this page · raw message

Signature · 1 line
1.5.6.5
author
#2 lore raw

PATCH 2/4 integrity: Linux Integrity Module(LIM)

In reply to Mimi Zohar

This version resolves the merge issues resulting from the removal of the nameidata parameter to inode_permission(), by moving the integrity_inode_permission() call from inode_permission() to may_open(), and renaming the hook to integrity_path_check(). The path is needed in order to open and read the file, so that the file can be hashed(a cryptographically strong checksum.)

This patch is a redesign of the integrity framework, which address a number of issues, including
- generalizing the measurement API beyond just inode measurements.
- separation of the measurement into distinct collection, appraisal, and commitment phases, for greater flexibility.

Extended Verification Module(EVM) and the Integrity Measurement Architecture(IMA) were originally implemented as an LSM module. Based on discussions on the LSM mailing list, a decision was made that the LSM hooks should only be used to enforce mandatory access control decisions and a new set of hooks should be defined specifically for integrity.

EVM/IMA was limited to verifying and measuring a file's (i.e. an inode) integrity and the metadata associated with it. Current research is looking into other types of integrity measurements. (i.e. "Linux kernel integrity measurement using contextual inspection", by Peter A. Loscocco, Perry W. Wilson, J. Aaron Pendergrass, C. Durward McDonell,
http://doi.acm.org/10.1145/1314354.1314362). As a result, a requirement of the new integrity framework is support for different types of integrity measurements.

This patch provides an integrity framework(api and hooks) and placement of the integrity hooks in the appropriate places in the fs directory. Collecting, appraising, and storing of file and other types of integrity data is supported. Multiple integrity templates, which implement the integrity API, may register themselves. For now, only a single integrity provider can register itself for the integrity hooks. (Support for multiple providers registering themselves for the integrity hooks would require some form of stacking.)

The six integrity hooks are:
path_check_integrity, inode_alloc_integrity, inode_free_integrity, bprm_check_integrity, file_free_integrity, file_mmap

The five integrity API calls provided are:
integrity_must_measure, integrity_collect_measurement, integrity_appraise_measurement, integrity_store_measurement, and integrity_display_template.

The type of integrity data being collected, appraised, stored, or displayed is template dependent.

(Details on the calls and their exact arguments are in linux/integrity.h, included in the patch.)

Signed-off-by Mimi Zohar <[email protected]>

---

Documentation/kernel-parameters.txt +6 -0
index 9fa6508..9789805 100644
@@ -44,6 +44,7 @@ parameter is applicable:
 	FB	The frame buffer device is enabled.
 	HW	Appropriate hardware is enabled.
 	IA-64	IA-64 architecture is enabled.
+	INTEGRITY Integrity support is enabled.
 	IOSCHED	More than one I/O scheduler is enabled.
 	IP_PNP	IP DHCP, BOOTP, or RARP is enabled.
 	ISAPNP	ISA PnP code is enabled.

@@ -894,6 +895,11 @@ and is between 256 and 4096 characters. It is defined in the file
 	inport.irq=	[HW] Inport (ATI XL and Microsoft) busmouse driver
 			Format: <irq>
 
+	integrity_audit= [INTEGRITY]
+                        Format: { "0" | "1" }
+                        0 -- integrity auditing messages. (Default)
+                        1 -- enable informational integrity auditing messages.
+
 	inttest=	[IA64]
 
 	iommu=		[x86]
fs/exec.c +7 -0
index 4e834f1..3494c4c 100644
@@ -45,6 +45,7 @@
 #include <linux/proc_fs.h>
 #include <linux/mount.h>
 #include <linux/security.h>
+#include <linux/integrity.h>
 #include <linux/syscalls.h>
 #include <linux/tsacct_kern.h>
 #include <linux/cn_proc.h>

@@ -129,6 +130,9 @@ asmlinkage long sys_uselib(const char __user * library)
 	error = vfs_permission(&nd, MAY_READ | MAY_EXEC | MAY_OPEN);
 	if (error)
 		goto exit;
+	error = integrity_path_check(&nd.path, MAY_READ | MAY_EXEC | MAY_OPEN);
+	if (error)
+		goto exit;
 
 	file = nameidata_to_filp(&nd, O_RDONLY|O_LARGEFILE);
 	error = PTR_ERR(file);

@@ -1199,6 +1203,9 @@ int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
 	retval = security_bprm_check(bprm);
 	if (retval)
 		return retval;
+	retval = integrity_bprm_check(bprm);
+	if (retval)
+		return retval;
 
 	/* kernel module loader fixup */
 	/* so we don't try to load run modprobe in kernel space. */
fs/file_table.c +3 -0
index 5ad0eca..74b30ba 100644
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/fs.h>
 #include <linux/security.h>
+#include <linux/integrity.h>
 #include <linux/eventpoll.h>
 #include <linux/rcupdate.h>
 #include <linux/mount.h>

@@ -276,6 +277,7 @@ void __fput(struct file *file)
 	if (file->f_op && file->f_op->release)
 		file->f_op->release(inode, file);
 	security_file_free(file);
+	integrity_file_free(file);
 	if (unlikely(S_ISCHR(inode->i_mode) && inode->i_cdev != NULL))
 		cdev_put(inode->i_cdev);
 	fops_put(file->f_op);

@@ -347,6 +349,7 @@ void put_filp(struct file *file)
 {
 	if (atomic_long_dec_and_test(&file->f_count)) {
 		security_file_free(file);
+		integrity_file_free(file);
 		file_kill(file);
 		file_free(file);
 	}
fs/inode.c +16 -6
index 0487ddb..bc2c3a5 100644
@@ -17,6 +17,7 @@
 #include <linux/hash.h>
 #include <linux/swap.h>
 #include <linux/security.h>
+#include <linux/integrity.h>
 #include <linux/pagemap.h>
 #include <linux/cdev.h>
 #include <linux/bootmem.h>

@@ -143,12 +144,13 @@ static struct inode *alloc_inode(struct super_block *sb)
 		inode->i_cdev = NULL;
 		inode->i_rdev = 0;
 		inode->dirtied_when = 0;
-		if (security_inode_alloc(inode)) {
-			if (inode->i_sb->s_op->destroy_inode)
-				inode->i_sb->s_op->destroy_inode(inode);
-			else
-				kmem_cache_free(inode_cachep, (inode));
-			return NULL;
+		if (security_inode_alloc(inode))
+			goto out_free_inode;
+
+		/* allocate, attach and initialize an i_integrity */
+		if (integrity_inode_alloc(inode)) {
+			security_inode_free(inode);
+			goto out_free_inode;
 		}
 
 		spin_lock_init(&inode->i_lock);

@@ -185,12 +187,20 @@ static struct inode *alloc_inode(struct super_block *sb)
 		inode->i_mapping = mapping;
 	}
 	return inode;
+
+out_free_inode:
+	if (inode->i_sb->s_op->destroy_inode)
+		inode->i_sb->s_op->destroy_inode(inode);
+	else
+		kmem_cache_free(inode_cachep, (inode));
+	return NULL;
 }
 
 void destroy_inode(struct inode *inode) 
 {
 	BUG_ON(inode_has_buffers(inode));
 	security_inode_free(inode);
+	integrity_inode_free(inode);
 	if (inode->i_sb->s_op->destroy_inode)
 		inode->i_sb->s_op->destroy_inode(inode);
 	else
fs/namei.c +6 -0
index 09ce58e..d7d4e59 100644
@@ -24,6 +24,7 @@
 #include <linux/fsnotify.h>
 #include <linux/personality.h>
 #include <linux/security.h>
+#include <linux/integrity.h>
 #include <linux/syscalls.h>
 #include <linux/mount.h>
 #include <linux/audit.h>

@@ -1523,6 +1524,11 @@ int may_open(struct nameidata *nd, int acc_mode, int flag)
 	error = vfs_permission(nd, acc_mode);
 	if (error)
 		return error;
+
+	error = integrity_path_check(&nd->path, acc_mode
+				     & (MAY_READ | MAY_WRITE | MAY_EXEC));
+	if (error)
+		return error;
 	/*
 	 * An append-only file must be opened in append mode for writing.
 	 */
include/linux/audit.h +5 -0
index 6272a39..63bb3ec 100644
@@ -123,6 +123,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 */
 
 #define AUDIT_KERNEL		2000	/* Asynchronous audit record. NOT A REQUEST. */
include/linux/integrity.h new file +166 -0

166 lines not on this page · raw message

mm/mmap.c +4 -0
index d4855a6..7cdfd3a 100644
@@ -20,6 +20,7 @@
 #include <linux/fs.h>
 #include <linux/personality.h>
 #include <linux/security.h>
+#include <linux/integrity.h>
 #include <linux/hugetlb.h>
 #include <linux/profile.h>
 #include <linux/module.h>

@@ -1050,6 +1051,9 @@ unsigned long do_mmap_pgoff(struct file * file, unsigned long addr,
 	error = security_file_mmap(file, reqprot, prot, flags, addr, 0);
 	if (error)
 		return error;
+	error = integrity_file_mmap(file, prot);
+	if (error)
+		return error;
 
 	return mmap_region(file, addr, len, flags, vm_flags, pgoff,
 			   accountable);
security/Kconfig +2 -0
index d9f47ce..4c602be 100644
@@ -126,5 +126,7 @@ config SECURITY_DEFAULT_MMAP_MIN_ADDR
 source security/selinux/Kconfig
 source security/smack/Kconfig
 
+source security/integrity/Kconfig
+
 endmenu
security/Makefile +4 -0
index c05c127..db9efb5 100644
@@ -17,3 +17,7 @@ obj-$(CONFIG_SECURITY_SELINUX)		+= selinux/built-in.o
 obj-$(CONFIG_SECURITY_SMACK)		+= smack/built-in.o
 obj-$(CONFIG_SECURITY_ROOTPLUG)		+= root_plug.o
 obj-$(CONFIG_CGROUP_DEVICE)		+= device_cgroup.o
+
+# Object integrity file lists
+subdir-$(CONFIG_INTEGRITY)		+= integrity
+obj-$(CONFIG_INTEGRITY)			+= integrity/built-in.o
security/integrity/Kconfig new file +24 -0
new file mode 100644
index 0000000..2123dfc
@@ -0,0 +1,24 @@
+#
+# Integrity configuration
+#
+
+config INTEGRITY
+	bool "Enable different integrity models"
+	help
+	  This allows you to choose different integrity modules to be
+	  configured into your kernel. Integrity models can collect,
+	  verify, and record measurements, such as of a file's data
+	  and metadata before it is read or executed.
+
+	  If you are unsure how to answer this question, answer N.
+
+config INTEGRITY_AUDIT
+	bool "Integrity audit boot parameter"
+	depends on INTEGRITY
+	default n
+	help
+	  This option adds a kernel parameter 'integrity_audit', which
+	  allows informational integrity auditing messages to be enabled
+	  at boot.  If this option is selected, informational integrity
+	  auditing messages can be enabled with 'integrity_audit=1' on
+	  the kernel command line.
security/integrity/Makefile new file +6 -0
new file mode 100644
index 0000000..a6e1405
@@ -0,0 +1,6 @@
+#
+# Makefile for the kernel integrity code
+#
+
+# Object file lists
+obj-$(CONFIG_INTEGRITY) += integrity.o integrity_audit.o
security/integrity/integrity.c new file +313 -0

313 lines not on this page · raw message

security/integrity/integrity.h new file +30 -0
new file mode 100644
index 0000000..c57fa2d
@@ -0,0 +1,30 @@
+/*
+ * 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.
+ */
+
+#define TEMPLATE_NAME_LEN_MAX 12
+struct template_list_entry {
+	struct list_head template;
+	char template_name[TEMPLATE_NAME_LEN_MAX + 1];
+	const struct template_operations *template_ops;
+	atomic_t refcount;
+};
+
+static inline void tget(struct template_list_entry *entry)
+{
+	atomic_inc(&entry->refcount);
+}
+
+static inline void tput(struct template_list_entry *entry)
+{
+	if (atomic_dec_and_test(&entry->refcount))
+		kfree(entry);
+}
+
+extern int integrity_find_template(const char *,
+				   struct template_list_entry **entry);
security/integrity/integrity_audit.c new file +79 -0

79 lines not on this page · raw message

Signature · 1 line
1.5.6.5
#3 lore raw

Re: [PATCH 2/4] integrity: Linux Integrity Module(LIM)

Ok, the API looks sane to me. But one big question: any reason you don't just directly call into your implementation instead of all these odd hooks? This seems to be a lot of overhead just for making the code less readable..

#4 lore raw

Re: [PATCH 2/4] integrity: Linux Integrity Module(LIM)

Quoted 5 lines · Christoph Hellwig

Ok, the API looks sane to me. But one big question: any reason you don't just directly call into your implementation instead of all these odd hooks? This seems to be a lot of overhead just for making the code less readable..

The consensus in the (insane) security community was to have an interface with selectable modules similar to LSM and its modules, so that users could easily choose among a set of integrity providers.

dave

#5 lore raw

Re: [PATCH 2/4] integrity: Linux Integrity Module(LIM)

Quoted 3 lines · david safford

The consensus in the (insane) security community was to have an interface with selectable modules similar to LSM and its modules, so that users could easily choose among a set of integrity providers.

So what other integrity provider is there waiting to be merged? Unless there is a realistic mid-term candidate it's just pure bloat, and we can introduce an abstraction once it's actually needed.

#6 lore raw

Re: [PATCH 2/4] integrity: Linux Integrity Module(LIM)

Quoted 9 lines · Christoph Hellwig
Quoted 3 lines · david safford

The consensus in the (insane) security community was to have an interface with selectable modules similar to LSM and its modules, so that users could easily choose among a set of integrity providers.

So what other integrity provider is there waiting to be merged? Unless there is a realistic mid-term candidate it's just pure bloat, and we can introduce an abstraction once it's actually needed.

IMA tries to be generic, but it is still oriented around the Trusted Computing Group concept of hardware anchored lists of file measurements. We know of other projects looking at measurements of things that are not files, such as introspection of process memory invariants, and other integrity models not anchored in TCG hardware, such as public key signed files. I don't really know how close these other projects are to submission, but when this was reviewed on the LSM mailing list, everyone agreed with the abstraction. Hopefully some of the other interested people will chime in here.

dave

#7 lore raw

Re: [PATCH 2/4] integrity: Linux Integrity Module(LIM)

In reply to Mimi Zohar
Quoted 8 lines · Mimi Zohar
security/integrity/integrity.c new file +6 -0
+int integrity_find_template(const char *template_name,
+                           struct template_list_entry **template_entry)
+{
+       struct template_list_entry *entry;
+
+       list_for_each_entry_rcu(entry, &integrity_templates, template)

{

security/integrity/integrity.c line 147

Quoted 9 lines
security/integrity/integrity.c new file +9 -0
+               if (strncmp(entry->template_name, template_name,
+                           strlen(entry->template_name)) == 0) {
+                       *template_entry = entry;
+                       return 0;
+               }
+       }
+       return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(integrity_find_template);

Is this an appropriate place for an ERR_PTR() to get used?

-- Dave

#8 lore raw

Re: [PATCH 2/4] integrity: Linux Integrity Module(LIM)

In reply to Mimi Zohar

security/integrity/integrity.c line 167

Quoted 6 lines · Mimi Zohar
security/integrity/integrity.c new file +5 -0
+       rcu_read_lock();
+       rc = integrity_find_template(template_name, &template_entry);
+       if (rc == 0)
+               tget(template_entry);
+       rcu_read_unlock();

This little nugget is repeated over and over. How about an integrity_find_get_template()?

-- Dave

author
#9 lore raw

Re: [PATCH 2/4] integrity: Linux Integrity Module(LIM)

Quoted 8 lines · Dave Hansen

security/integrity/integrity.c line 167

Quoted 6 lines · Mimi Zohar
security/integrity/integrity.c new file +5 -0
+       rcu_read_lock();
+       rc = integrity_find_template(template_name, &template_entry);
+       if (rc == 0)
+               tget(template_entry);
+       rcu_read_unlock();
Quoted 4 lines

This little nugget is repeated over and over. How about an integrity_find_get_template()?

-- Dave

Sounds good.

Mimi

#10 lore raw

Re: [PATCH 2/4] integrity: Linux Integrity Module(LIM)

In reply to Mimi Zohar

security/integrity/integrity.h line 27

Quoted 11 lines · Mimi Zohar
security/integrity/integrity.h new file +10 -0
+static inline void tget(struct template_list_entry *entry)
+{
+       atomic_inc(&entry->refcount);
+}
+
+static inline void tput(struct template_list_entry *entry)
+{
+       if (atomic_dec_and_test(&entry->refcount))
+               kfree(entry);
+}

Seems like a good place for a kref.

-- Dave

author
#11 lore raw

Re: [PATCH 2/4] integrity: Linux Integrity Module(LIM)

Quoted 15 lines · Dave Hansen

security/integrity/integrity.h line 27

Quoted 11 lines · Mimi Zohar
security/integrity/integrity.h new file +10 -0
+static inline void tget(struct template_list_entry *entry)
+{
+       atomic_inc(&entry->refcount);
+}
+
+static inline void tput(struct template_list_entry *entry)
+{
+       if (atomic_dec_and_test(&entry->refcount))
+               kfree(entry);
+}

Seems like a good place for a kref.

Quoted 1 line

-- Dave

Thanks!

Mimi

#12 lore raw

Re: [PATCH 2/4] integrity: Linux Integrity Module(LIM)

In reply to Mimi Zohar

security/integrity/integrity_audit.c line 26

Quoted 11 lines · Mimi Zohar
security/integrity/integrity_audit.c new file +10 -0
+static int integrity_audit;
+
+#ifdef CONFIG_INTEGRITY_AUDIT
+static int __init integrity_audit_setup(char *str)
+{
+       ulong audit;
+       int rc;
+       char *op;
+
+       rc = strict_strtoul(str, 0, &audit);

I think we usually write out 'unsigned long'.

-- Dave

author
#13 lore raw

Re: [PATCH 2/4] integrity: Linux Integrity Module(LIM)

Quoted 15 lines · Dave Hansen

security/integrity/integrity_audit.c line 26

Quoted 11 lines · Mimi Zohar
security/integrity/integrity_audit.c new file +10 -0
+static int integrity_audit;
+
+#ifdef CONFIG_INTEGRITY_AUDIT
+static int __init integrity_audit_setup(char *str)
+{
+       ulong audit;
+       int rc;
+       char *op;
+
+       rc = strict_strtoul(str, 0, &audit);

I think we usually write out 'unsigned long'.

Quoted 1 line

-- Dave

Ok

Mimi

author
#14 lore raw

PATCH 3/4 integrity: IMA as an integrity service provider

In reply to Mimi Zohar

This is a re-release of Integrity Measurement Architecture(IMA) as an independent Linunx Integrity Module(LIM) service provider.

As a LIM integrity provider, IMA implements the new LIM must_measure(), collect_measurement(), store_measurement(), and display_template() API calls. The store_measurement() call supports two types of data, IMA (i.e. file data) and generic template data.

IMA provides hardware (TPM) based measurement and attestation for both files and other types of template measurements. As the Trusted Computing (TPM) model requires, IMA measures all files before they are accessed in any way (on the bprm_check_integrity, nameidata_check_integrity, file_mmap hooks), and commits the measurements to the TPM. In addition, IMA maintains a list of these hash values, which can be used to validate the aggregate PCR value. The TPM can sign these measurements, and thus the system can prove to itself and to a third party these measurements in a way that cannot be circumvented by malicious or compromised software.

When store_measurement() is called for the IMA type of data, the file measurement and the file name hint are used to form an IMA template. IMA then calculates the IMA template measurement(hash) and submits it to the TPM chip for inclusion in one of the chip's Platform Configuration Registers (PCR).

When store_measurement() is called for generic template data, IMA calculates the measurement(hash) of the template data, and submits the template measurement to the TPM chip for inclusion in one of the chip's Platform Configuration Registers(PCR).

In order to view the contents of template data through securityfs, the template_display() function must be defined in the registered template_operations. In the case of the IMA template, the list of file names and files hashes submitted can be viewed through securityfs.

As mentioned above, IMA maintains a list of hash values of executables and other sensitive system files loaded into the run-time of the system. Our work has shown that requests for integrity appraisal and measurement need to be based on knowledge of the filesystem, requiring the system to either be labeled with integrity data or depend on the existent LSM security labels. The previous set of integrity patches modified the LSM modules to be integrity context aware, meaning that the LSM modules made integrity data/metadata appraisal and measurement API calls based on an understanding of the LSM security labels. Both of the LSM maintainers felt that the changes were too intrusive and that integrity enforcement should be made by the integrity provider, not the LSM module.

To address these concerns, Stephen Smalley suggested using the security_audit_rule_match(), renamed to security_filter_rule_match(), to define LSM specific integrity measurement policy rules, in lieu of modifying the LSM modules. In the current set of patches, the integrity API calls can be made either by IMA, based on an LSM specific integrity policy, or by an integrity context aware LSM.

Signed-off-by Mimi Zohar <[email protected]>

---

Documentation/ABI/testing/ima_policy new file +61 -0
new file mode 100644
index 0000000..6434f0d
@@ -0,0 +1,61 @@
+What:		security/ima/policy
+Date:		May 2008
+Contact:	Mimi Zohar <[email protected]>
+Description:
+		The Trusted Computing Group(TCG) runtime Integrity
+		Measurement Architecture(IMA) maintains a list of hash
+		values of executables and other sensitive system files
+		loaded into the run-time of this system.  At runtime,
+		the policy can be constrained based on LSM specific data.
+		Policies are loaded into the securityfs file ima/policy
+		by opening the file, writing the rules one at a time and
+		then closing the file.  The new policy takes effect after
+		the file ima/policy is closed.
+
+		rule format: action [condition ...]
+
+		action: measure | dont_measure
+		condition:= base | lsm
+			base:	[[func=] [mask=] [fsmagic=] [uid=]]
+			lsm:	[[subj_user=] [subj_role=] [subj_type=]
+				 [obj_user=] [obj_role=] [obj_type=]]
+
+		base: 	func:= [BPRM_CHECK][FILE_MMAP][INODE_PERMISSION]
+			mask:= [MAY_READ] [MAY_WRITE] [MAY_APPEND] [MAY_EXEC]
+			fsmagic:= hex value
+			uid:= decimal value
+		lsm:  	are LSM specific
+
+		default policy:
+			# PROC_SUPER_MAGIC
+			dont_measure fsmagic=0x9fa0
+			# SYSFS_MAGIC
+			dont_measure fsmagic=0x62656572
+			# DEBUGFS_MAGIC
+			dont_measure fsmagic=0x64626720
+			# TMPFS_MAGIC
+			dont_measure fsmagic=0x01021994
+			# SECURITYFS_MAGIC
+			dont_measure fsmagic=0x73636673
+
+			measure func=BPRM_CHECK
+			measure func=FILE_MMAP mask=MAY_EXEC
+			measure func=INODE_PERM mask=MAY_READ uid=0
+
+		The default policy measures all executables in bprm_check,
+		all files mmapped executable in file_mmap, and all files
+		open for read by root in inode_permission.
+
+		Examples of LSM specific definitions:
+
+		SELinux:
+			# SELINUX_MAGIC
+			dont_measure fsmagic=0xF97CFF8C
+
+			dont_measure obj_type=var_log_t
+			dont_measure obj_type=auditd_log_t
+			measure subj_user=system_u func=INODE_PERM mask=MAY_READ
+			measure subj_role=system_r func=INODE_PERM mask=MAY_READ
+
+		Smack:
+			measure subj_user=_ func=INODE_PERM mask=MAY_READ
Documentation/kernel-parameters.txt +5 -0
index 9789805..10c3b87 100644
@@ -44,6 +44,7 @@ parameter is applicable:
 	FB	The frame buffer device is enabled.
 	HW	Appropriate hardware is enabled.
 	IA-64	IA-64 architecture is enabled.
+	IMA     Integrity measurement architecture is enabled.
 	INTEGRITY Integrity support is enabled.
 	IOSCHED	More than one I/O scheduler is enabled.
 	IP_PNP	IP DHCP, BOOTP, or RARP is enabled.

@@ -878,6 +879,10 @@ 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_hash=	[IMA] runtime ability to define hash crypto algorithm.
+			Format: { "MD5" | "SHA1" }
+			Default is "SHA1".
+
 	in2000=		[HW,SCSI]
 			See header of drivers/scsi/in2000.c.
include/linux/ima.h new file +46 -0

46 lines not on this page · raw message

security/Kconfig +2 -1
index 4c602be..665a1b7 100644
@@ -55,7 +55,8 @@ config SECURITYFS
 	bool "Enable the securityfs filesystem"
 	help
 	  This will build the securityfs filesystem.  It is currently used by
-	  the TPM bios character driver.  It is not used by SELinux or SMACK.
+	  the TPM bios character driver and IMA, an integrity provider.  It
+	  is not used by SELinux or SMACK.
 
 	  If you are unsure how to answer this question, answer N.
security/integrity/Kconfig +2 -0
index 2123dfc..57213d6 100644
@@ -22,3 +22,5 @@ config INTEGRITY_AUDIT
 	  at boot.  If this option is selected, informational integrity
 	  auditing messages can be enabled with 'integrity_audit=1' on
 	  the kernel command line.
+
+source security/integrity/ima/Kconfig
security/integrity/Makefile +1 -0
index a6e1405..107dafd 100644
@@ -4,3 +4,4 @@
 
 # Object file lists
 obj-$(CONFIG_INTEGRITY) += integrity.o integrity_audit.o
+obj-$(CONFIG_IMA) += ima/
security/integrity/ima/Kconfig new file +48 -0

48 lines not on this page · raw message

security/integrity/ima/Makefile new file +9 -0
new file mode 100644
index 0000000..959ae66
@@ -0,0 +1,9 @@
+#
+# Makefile for building Trusted Computing Group's(TCG) runtime Integrity
+# Measurement Architecture(IMA).
+#
+
+obj-$(CONFIG_IMA) += ima.o
+
+ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
+	 ima_policy.o ima_iint.o
security/integrity/ima/ima.h new file +198 -0

198 lines not on this page · raw message

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

420 lines not on this page · raw message

security/integrity/ima/ima_crypto.c new file +147 -0

147 lines not on this page · raw message

security/integrity/ima/ima_fs.c new file +488 -0

488 lines not on this page · raw message

security/integrity/ima/ima_iint.c new file +63 -0

63 lines not on this page · raw message

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

105 lines not on this page · raw message

security/integrity/ima/ima_main.c new file +331 -0

331 lines not on this page · raw message

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

334 lines not on this page · raw message

security/integrity/ima/ima_queue.c new file +124 -0

124 lines not on this page · raw message

Signature · 1 line
1.5.6.5
#15 lore raw

Re: [PATCH 3/4] integrity: IMA as an integrity service provider

Ok, after the API is sorted out I had a quick looks at this patch.

The first very odd thing is the data strucutures:

include/linux/ima.h line 27

Quoted 10 lines
include/linux/ima.h new file +10 -0
+struct ima_args_data {
+	const char *filename;
+	struct file *file;
+	struct path *path;
+	struct dentry *dentry;
+	struct inode *inode;
+	enum lim_hooks function;
+	u32 osid;
+	int mask;
+};

You can always get from a file to a path, from a path to a dentry, from a dentry to and inode and from a path to some defintion of a filename. So a lot of things here seems very redundant.

When looking at how it's used it's acually even worse. AFAICS the code would be a lot cleaner if you'd just kill struct ima_args_data and the odd pass arguments as void pointers obsfucations and just pass the file/path + mask directly to the lower level functions.

That's also help killing things like ima_store_measurement which do entirely different things depending on idata->type.

security/integrity/ima/ima_api.c line 239

Quoted 12 lines
security/integrity/ima/ima_api.c new file +12 -0
+static int skip_measurement(struct inode *inode, int mask)
+{
+	if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode))
+		return 1;	/* can't measure */
+
+	if (special_file(inode->i_mode) || S_ISLNK(inode->i_mode))
+		return 1;	/* don't measure */
+
+	if (S_ISREG(inode->i_mode))
+		return 0;	/* measure */
+	return 1;		/* don't measure */
+}

This could just be an

	if (!S_ISREG(inode->i_mode))

in the caller..

security/integrity/ima/ima_crypto.c line 31

Quoted 2 lines
security/integrity/ima/ima_crypto.c new file +2 -0
+static int update_file_hash(struct file *f, struct path *path,
+			    struct hash_desc *desc)

Please split this into a update_file_hash that always operates on a struct file, and a wrapper around it that creates the struct file for the cases that needs it.

Quoted 12 lines
security/integrity/ima/ima_main.c new file +12 -0
+void ima_fixup_argsdata(struct ima_args_data *data, struct file *file,
+			struct path *path, int mask, int function)
+{
+	struct dentry *dentry = NULL;
+
+	data->file = file;
+	data->path = path;
+	data->mask = mask;
+	data->function = function;
+
+	if (file)
+		data->dentry = dentry = file->f_dentry;

security/integrity/ima/ima_main.c line 82

Quoted 10 lines
security/integrity/ima/ima_main.c new file +10 -0
+
+	if (path) {
+		if (!dentry)
+			data->dentry = dentry = path->dentry;
+	}
+	if (dentry)
+		data->inode = dentry->d_inode;
+
+	return;
+}

You have two different callers for this, either file NULL or path NULL but never neither or both. So just do the setup in the callers and do the right thing there. (and please kill the inode member, it's entirely superflous)

security/integrity/ima/ima_main.c line 96

Quoted 7 lines
security/integrity/ima/ima_main.c new file +7 -0
+static void ima_file_free(struct file *file)
+{
+	struct inode *inode = NULL;
+	struct ima_iint_cache *iint;
+
+	if (!file->f_dentry)	/* can be NULL */
+		return;

No, it can't.

Quoted 12 lines
security/integrity/ima/ima_main.c new file +11 -0
+
+	inode = file->f_dentry->d_inode;
+	if (S_ISDIR(inode->i_mode))
+		return;
+	if ((file->f_mode & FMODE_WRITE) &&
+	    (atomic_read(&inode->i_writecount) == 1)) {
 
+ * Returns 0 on success, -ENOMEM on failure
+ */
+static int ima_inode_alloc_integrity(struct inode *inode)
+{
+	return ima_iint_insert(inode);

security/integrity/ima/ima_main.c line 138

Quoted 10 lines
security/integrity/ima/ima_main.c new file +10 -0
+}
+
+/**
+ * ima_inode_free_integrity - free the integrity structure
+ * @inode: the inode structure
+ */
+static void ima_inode_free_integrity(struct inode *inode)
+{
+	ima_iint_delete(inode);
+}

Why these wrappers?

security/integrity/ima/ima_main.c line 171

Quoted 4 lines
security/integrity/ima/ima_main.c new file +4 -0
+	/* The file name is only a hint. */
+	dentry = path->dentry;
+	data->filename = (!dentry->d_name.name) ? (char *)dentry->d_iname :
+	    (char *)dentry->d_name.name;

d_iname is the internal storage for d_name, always use d_name only.

security/integrity/ima/ima_main.c line 174

Quoted 2 lines
security/integrity/ima/ima_main.c new file +2 -0
+	/* Invalidate PCR, if a measured file is already open for read */
+	if ((mask == MAY_WRITE) || (mask == MAY_APPEND)) {

MAY_APPEND is ORed into a mask, but never used standalone.

security/integrity/ima/ima_main.c line 202

Quoted 11 lines
security/integrity/ima/ima_main.c new file +10 -0
+		if (!rc) {
+			if (atomic_read(&(data->dentry->d_count)) - 1 >
+			    atomic_read(&(inode->i_writecount)))
+				ima_add_violation(inode, data->filename,
+						  "invalid_pcr", "ToMToU");
+		}
 
+			if (atomic_read(&(inode->i_writecount)) > 0)
+				ima_add_violation(inode, data->filename,
+						  "invalid_pcr",
+						  "open_writers");

All these d_count and i_writecount access needs a lot of explanation, they certainly don't make sense as-is, but I'd like to find out what you're actually trying to do here.

Review

Quoted 2 lines
(quoted diff) +2 -0
+	if (!file || !file->f_dentry)
+		return rc;

Can't happen.

security/integrity/ima/ima_policy.c line 29

Quoted 2 lines
security/integrity/ima/ima_policy.c new file +2 -0
+#define audit_type(type) AUDIT_ ##type
+#define lsm_type(type) LSM_ ##type

Just spelling out the constants would be a lot more readable..

author
#16 lore raw

Re: [PATCH 3/4] integrity: IMA as an integrity service provider

Quoted 7 lines · Christoph Hellwig
Quoted 6 lines
security/integrity/ima/ima_main.c new file +6 -0
+		if (!rc) {
+			if (atomic_read(&(data->dentry->d_count)) - 1 >
+			    atomic_read(&(inode->i_writecount)))
+				ima_add_violation(inode, data->filename,
+						  "invalid_pcr", "ToMToU");
+		}
Quoted 8 lines

security/integrity/ima/ima_main.c line 202

Quoted 4 lines
security/integrity/ima/ima_main.c new file +4 -0
+			if (atomic_read(&(inode->i_writecount)) > 0)
+				ima_add_violation(inode, data->filename,
+						  "invalid_pcr",
+						  "open_writers");

All these d_count and i_writecount access needs a lot of explanation, they certainly don't make sense as-is, but I'd like to find out what you're actually trying to do here.

The OS protects against an executable file, already open for write, from being executed; and an executable file, open for execute, from being modified. In the same vein, we want to know that the file measured is the same file read, that it hasn't been modified. So, if a file already open for read is then opened for write, we log it with a "Time of Measure, Time of Use" error (ToMToU) and invalidate the PCR. Or, if a file already open for write is then opened for read, we log the fact that the file could change while being read and invalidate the PCR.

This is important when measuring configuration files, shell scripts (not measured in brpm_check_integrity which are protected by the OS), and files imported/included by scripts.

Mimi

author
#17 lore raw

Re: [PATCH 3/4] integrity: IMA as an integrity service provider

In reply to Christoph Hellwig
Quoted 19 lines · Christoph Hellwig

Ok, after the API is sorted out I had a quick looks at this patch.

The first very odd thing is the data strucutures:

include/linux/ima.h line 27

Quoted 10 lines
include/linux/ima.h new file +10 -0
+struct ima_args_data {
+	const char *filename;
+	struct file *file;
+	struct path *path;
+	struct dentry *dentry;
+	struct inode *inode;
+	enum lim_hooks function;
+	u32 osid;
+	int mask;
+};

You can always get from a file to a path, from a path to a dentry, from a dentry to and inode and from a path to some defintion of a filename. So a lot of things here seems very redundant.

Quoted 7 lines

When looking at how it's used it's acually even worse. AFAICS the code would be a lot cleaner if you'd just kill struct ima_args_data and the odd pass arguments as void pointers obsfucations and just pass the file/path + mask directly to the lower level functions.

That's also help killing things like ima_store_measurement which do entirely different things depending on idata->type.

hm, looking into it.

Quoted 15 lines

security/integrity/ima/ima_api.c line 239

Quoted 12 lines
security/integrity/ima/ima_api.c new file +12 -0
+static int skip_measurement(struct inode *inode, int mask)
+{
+	if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode))
+		return 1;	/* can't measure */
+
+	if (special_file(inode->i_mode) || S_ISLNK(inode->i_mode))
+		return 1;	/* don't measure */
+
+	if (S_ISREG(inode->i_mode))
+		return 0;	/* measure */
+	return 1;		/* don't measure */
+}

This could just be an

Quoted 3 lines
	if (!S_ISREG(inode->i_mode))

in the caller..

Done.

Quoted 6 lines

security/integrity/ima/ima_crypto.c line 31

Quoted 2 lines
security/integrity/ima/ima_crypto.c new file +2 -0
+static int update_file_hash(struct file *f, struct path *path,
+			    struct hash_desc *desc)

Please split this into a update_file_hash that always operates on a struct file, and a wrapper around it that creates the struct file for the cases that needs it.

Ok.

Quoted 54 lines
Quoted 12 lines
security/integrity/ima/ima_main.c new file +12 -0
+void ima_fixup_argsdata(struct ima_args_data *data, struct file *file,
+			struct path *path, int mask, int function)
+{
+	struct dentry *dentry = NULL;
+
+	data->file = file;
+	data->path = path;
+	data->mask = mask;
+	data->function = function;
+
+	if (file)
+		data->dentry = dentry = file->f_dentry;

security/integrity/ima/ima_main.c line 82

Quoted 10 lines
security/integrity/ima/ima_main.c new file +10 -0
+
+	if (path) {
+		if (!dentry)
+			data->dentry = dentry = path->dentry;
+	}
+	if (dentry)
+		data->inode = dentry->d_inode;
+
+	return;
+}

You have two different callers for this, either file NULL or path NULL but never neither or both. So just do the setup in the callers and do the right thing there. (and please kill the inode member, it's entirely superflous)

security/integrity/ima/ima_main.c line 96

Quoted 7 lines
security/integrity/ima/ima_main.c new file +7 -0
+static void ima_file_free(struct file *file)
+{
+	struct inode *inode = NULL;
+	struct ima_iint_cache *iint;
+
+	if (!file->f_dentry)	/* can be NULL */
+		return;

No, it can't.

Quoted 16 lines
security/integrity/ima/ima_main.c new file +15 -0
+
+	inode = file->f_dentry->d_inode;
+	if (S_ISDIR(inode->i_mode))
+		return;
+	if ((file->f_mode & FMODE_WRITE) &&
+	    (atomic_read(&inode->i_writecount) == 1)) {
 
+ * Returns 0 on success, -ENOMEM on failure
+ */
+static int ima_inode_alloc_integrity(struct inode *inode)
+{
+	return ima_iint_insert(inode);
+}
+
+/**
+ * ima_inode_free_integrity - free the integrity structure
Quoted 8 lines

security/integrity/ima/ima_main.c line 138

Quoted 6 lines

+ * @inode: the inode structure
+ */

security/integrity/ima/ima_main.c new file +4 -0
+static void ima_inode_free_integrity(struct inode *inode)
+{
+	ima_iint_delete(inode);
+}

Why these wrappers?

You're right. I'll remove these routines and assign ima_iint_insert()/delete() directly in ima_integrity_ops.

Quoted 6 lines

security/integrity/ima/ima_main.c line 171

Quoted 4 lines
security/integrity/ima/ima_main.c new file +4 -0
+	/* The file name is only a hint. */
+	dentry = path->dentry;
+	data->filename = (!dentry->d_name.name) ? (char *)dentry->d_iname :
+	    (char *)dentry->d_name.name;

d_iname is the internal storage for d_name, always use d_name only.

ok

Quoted 5 lines

Review

Quoted 3 lines
(quoted diff) +2 -0
+	if (!file || !file->f_dentry)
+		return rc;

Can't happen.

Will remove test.

Quoted 4 lines

security/integrity/ima/ima_policy.c line 29

Quoted 2 lines
security/integrity/ima/ima_policy.c new file +2 -0
+#define audit_type(type) AUDIT_ ##type
+#define lsm_type(type) LSM_ ##type

Just spelling out the constants would be a lot more readable..

ok

Mimi

#18 lore raw

Re: [PATCH 3/4] integrity: IMA as an integrity service provider

In reply to Mimi Zohar
Quoted 8 lines · Mimi Zohar

+ /* Invalidate PCR, if a measured file is already open for read
*/

security/integrity/ima/ima_main.c new file +5 -0
+       if ((mask == MAY_WRITE) || (mask == MAY_APPEND)) {
+               int mask_sav = data->mask;
+               int rc;
+
+               data->mask = MAY_READ;

security/integrity/ima/ima_main.c line 188

Quoted 10 lines
security/integrity/ima/ima_main.c new file +10 -0
+               rc = ima_must_measure(&idata);
+               if (!rc) {
+                       if (atomic_read(&(data->dentry->d_count)) - 1 >
+                           atomic_read(&(inode->i_writecount)))
+                               ima_add_violation(inode, data->filename,
+                                                 "invalid_pcr", "ToMToU");
+               }
+               data->mask = mask_sav;
+               goto out;
+       }

Following up on Christoph's comment...

I'm worried that this calculation isn't very precise. The calculation that you're trying to come up with here is the number of opens (d_count) vs. the number of writers (i_writecount). When they don't match, you know that the new open is the first write, and you must 'invalidate the PCR'?

There are a number of things that elevate d_count, and it is a lot more than just an open() that can do it. Is that OK?

-- Dave

author
#19 lore raw

Re: [PATCH 3/4] integrity: IMA as an integrity service provider

Quoted 28 lines · Dave Hansen
Quoted 8 lines · Mimi Zohar

+ /* Invalidate PCR, if a measured file is already open for read
*/

security/integrity/ima/ima_main.c new file +5 -0
+       if ((mask == MAY_WRITE) || (mask == MAY_APPEND)) {
+               int mask_sav = data->mask;
+               int rc;
+
+               data->mask = MAY_READ;

security/integrity/ima/ima_main.c line 188

Quoted 10 lines
security/integrity/ima/ima_main.c new file +10 -0
+               rc = ima_must_measure(&idata);
+               if (!rc) {
+                       if (atomic_read(&(data->dentry->d_count)) - 1 >
+                           atomic_read(&(inode->i_writecount)))
+                               ima_add_violation(inode, data->filename,
+                                                 "invalid_pcr", "ToMToU");
+               }
+               data->mask = mask_sav;
+               goto out;
+       }

Following up on Christoph's comment...

I'm worried that this calculation isn't very precise. The calculation that you're trying to come up with here is the number of opens (d_count) vs. the number of writers (i_writecount). When they don't match, you know that the new open is the first write, and you must 'invalidate the PCR'?

Quoted 6 lines

There are a number of things that elevate d_count, and it is a lot more than just an open() that can do it. Is that OK?

-- Dave

From an integrity perspective, a file measurement might be invalidated

unnecessarily, but it is safe. For any file when opened for write, while having an existing reader, will cause the file measurement to be invalidated. Can you give examples of things, other than open(), that elevate d_count?

Is there a different, better way to determine if there are any readers?

Thanks!

Mimi

#20 lore raw

Re: [PATCH 3/4] integrity: IMA as an integrity service provider

Quoted 29 lines · Mimi Zohar
Quoted 28 lines · Dave Hansen
Quoted 8 lines · Mimi Zohar

+ /* Invalidate PCR, if a measured file is already open for read
*/

security/integrity/ima/ima_main.c new file +5 -0
+       if ((mask == MAY_WRITE) || (mask == MAY_APPEND)) {
+               int mask_sav = data->mask;
+               int rc;
+
+               data->mask = MAY_READ;

security/integrity/ima/ima_main.c line 188

Quoted 10 lines
security/integrity/ima/ima_main.c new file +10 -0
+               rc = ima_must_measure(&idata);
+               if (!rc) {
+                       if (atomic_read(&(data->dentry->d_count)) - 1 >
+                           atomic_read(&(inode->i_writecount)))
+                               ima_add_violation(inode, data->filename,
+                                                 "invalid_pcr", "ToMToU");
+               }
+               data->mask = mask_sav;
+               goto out;
+       }

Following up on Christoph's comment...

I'm worried that this calculation isn't very precise. The calculation that you're trying to come up with here is the number of opens (d_count) vs. the number of writers (i_writecount). When they don't match, you know that the new open is the first write, and you must 'invalidate the PCR'?

Quoted 8 lines
Quoted 4 lines

There are a number of things that elevate d_count, and it is a lot more than just an open() that can do it. Is that OK?

From an integrity perspective, a file measurement might be invalidated

unnecessarily, but it is safe. For any file when opened for write, while having an existing reader, will cause the file measurement to be invalidated. Can you give examples of things, other than open(), that elevate d_count?

Just do a little search for dget(). There are plenty of places that we'll do a lookup (and a dget), realize that we can't complete an operation (say O_RDWR on a 400 file), then dput() the dentry. If you look at dput at the wrong time, you could mistake that for an *actual* open.

Are you holding any locks here? If not, it is completely conceivable that you do the atomic_read(d_count)=100, then by the time you do the aomtic_read(i_writecount), 90 of those references have been dput()'d.

Quoted 1 line

Is there a different, better way to determine if there are any readers?

I think you're looking at it from the wrong angle. If you have a writer, does it matter whether there are any readers? You should just unconditionally invalidate the integrity measurement.

-- Dave

author
#21 lore raw

PATCH 4/4 integrity: replace task uid with cred uid

In reply to Mimi Zohar

This patch addresses the credential merge changes in the security-testing-2.6/next tree.

Signed-off-by Mimi Zohar <[email protected]>

---

security/integrity/ima/ima_crypto.c +1 -1
index f62f2b7..4d59a86 100644
@@ -44,7 +44,7 @@ static int update_file_hash(struct file *f, struct path *path,
 			rc = -EINVAL;
 			goto out;
 		}
-		file = dentry_open(de, mnt, O_RDONLY);
+		file = dentry_open(de, mnt, O_RDONLY, current->cred);
 		if (IS_ERR(file)) {
 			ima_info("%s dentry_open failed\n", de->d_name.name);
 			rc = PTR_ERR(file);
security/integrity/ima/ima_policy.c +1 -1
index 2e95c35..867bfaf 100644
@@ -95,7 +95,7 @@ static bool ima_match_rules(struct ima_measure_rule_entry *rule,
 	if ((rule->flags & IMA_FSMAGIC)
 	    && rule->fsmagic != inode->i_sb->s_magic)
 		return false;
-	if ((rule->flags & IMA_UID) && rule->uid != tsk->uid)
+	if ((rule->flags & IMA_UID) && rule->uid != tsk->cred->uid)
 		return false;
 	for (i = 0; i < MAX_LSM_RULES; i++) {
 		int rc;
security/integrity/integrity_audit.c +1 -1
index 758e5dd..8e32d84 100644
@@ -49,7 +49,7 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode,
 
 	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->uid,
+			 current->pid, current->cred->uid,
 			 audit_get_loginuid(current));
 	audit_log_task_context(ab);
 	switch (audit_msgno) {
Signature · 1 line
1.5.6.5
#22 lore raw
In reply to Mimi Zohar

My worry:

	36 files changed, 3250 insertions(+), 14 deletions(-)

Can you find any better way to split this up? 79k is still pretty huge for one patch. Were you just trying to get under the LKML limit? ;)

 Documentation/kernel-parameters.txt    |    5 
 b/Documentation/ABI/testing/ima_policy |   61 ++++
 b/Documentation/kernel-parameters.txt  |    6 
 b/drivers/char/tpm/tpm.c               |  140 +++++++++
 b/drivers/char/tpm/tpm.h               |    4 
 b/drivers/char/tpm/tpm_tis.c           |    3 
 b/fs/exec.c                            |    7 
 b/fs/file_table.c                      |    3 
 b/fs/inode.c                           |   22 +
 b/fs/namei.c                           |    6 
 b/include/linux/audit.h                |    5 
 b/include/linux/ima.h                  |   46 +++
 b/include/linux/integrity.h            |  166 +++++++++++
 b/include/linux/tpm.h                  |   49 +++
 b/mm/mmap.c                            |    4 
 b/security/Kconfig                     |    2 
 b/security/Makefile                    |    4 
 b/security/integrity/Kconfig           |   24 +
 b/security/integrity/Makefile          |    6 
 b/security/integrity/ima/Kconfig       |   48 +++
 b/security/integrity/ima/Makefile      |    9 
 b/security/integrity/ima/ima.h         |  198 +++++++++++++
 b/security/integrity/ima/ima_api.c     |  420 ++++++++++++++++++++++++++++
 b/security/integrity/ima/ima_crypto.c  |  149 ++++++++++
 b/security/integrity/ima/ima_fs.c      |  488 +++++++++++++++++++++++++++++++++
 b/security/integrity/ima/ima_iint.c    |   63 ++++
 b/security/integrity/ima/ima_init.c    |  105 +++++++
 b/security/integrity/ima/ima_main.c    |  331 ++++++++++++++++++++++
 b/security/integrity/ima/ima_policy.c  |  336 ++++++++++++++++++++++
 b/security/integrity/ima/ima_queue.c   |  124 ++++++++
 b/security/integrity/integrity.c       |  313 +++++++++++++++++++++
 b/security/integrity/integrity.h       |   30 ++
 b/security/integrity/integrity_audit.c |   81 +++++
 security/Kconfig                       |    3 
 security/integrity/Kconfig             |    2 
 security/integrity/Makefile            |    1 
 36 files changed, 3250 insertions(+), 14 deletions(-)

-- Dave