Re: [RFC][Patch 1/1] IBAC Patch
On Tue, 2007-06-19 at 17:23 -0500, Serge E. Hallyn wrote:
+#define get_file_security(file) ((unsigned long)(file-f_security))
+#define set_file_security(file, val) (file-f_security = (void *)val)
+
+#define get_task_security(task) ((unsigned long)(task-security))
+#define set_task_security(task, val) (task-security = (void *)val)
I understand the above are likely remnants of stacker lsm support, and I
hate to say this, but not only is having those in there going to be
frowned upon, it then leads you later on to do things like
if (get_file_security(file)==0)
when using 0 for null upsets people in itself.
Instead of allocating memory for file-f_security, it uses
file-f_security itself, for storing 0 or 1. So it isn't
checking to see if it is NULL per se.
+
+#define XATTR_MEASURE_SUFFIX measure
+#define XATTR_MEASURE_SUFFIX_LEN (sizeof (XATTR_MEASURE_SUFFIX) -1)
+
+struct ibac_isec_data {
+ int mmapped;/* no. of times inode mmapped */
+ int measure;/* inode tagged to be measured */
+ spinlock_t lock;/* protect inode state */
+};
+
+static int ibac_inode_alloc_security(struct inode *inode)
+{
+ struct ibac_isec_data *isec;
+
+ isec = kzalloc(sizeof(struct ibac_isec_data), GFP_KERNEL);
+ if (!isec)
+ return -ENOMEM;
+
+ spin_lock_init(isec-lock);
+ inode-i_security = isec;
Heh, for file and task security you use the above helpers, but for inode
you do it like this? :) Please replace all x_security assignments and
checks with this format.
For file and task, the code uses the security field itself to store
information as opposed to allocating storage for it. In the case of
inode, it is using it for both the original digsig mmap tracking and
now to tag the inode that it needs to be measured. Tagging the inode
to be measured is based on the existence of the security.measure xattr,
which is controlled by a userspace application. The difference is
that storage is allocated for inode-i_security.
+/*
+ * For all inodes allocate inode-i_security(isec), before the security
+ * subsystem is enabled.
+ */
+static void ibac_fixup_inodes(void)
+{
+ struct super_block *sb;
+
+ spin_lock(sb_lock);
+ list_for_each_entry(sb, super_blocks, s_list) {
+ struct inode *inode;
+
+ spin_unlock(sb_lock);
+
+ spin_lock(inode_lock);
+ list_for_each_entry(inode, sb-s_inodes, i_sb_list) {
+ spin_unlock(inode_lock);
+
+ spin_lock(inode-i_lock);
+ if (!inode-i_security)
+ ibac_inode_alloc_security(inode);
since ibac_inode_alloc_security can return -ENOMEM, maybe you should at
least check for that condition and warn the user?
Yes, that definitely is a good idea. Will do.
+ spin_unlock(inode-i_lock);
+
+ spin_lock(inode_lock);
+ }
+ spin_unlock(inode_lock);
+
+ spin_lock(sb_lock);
+ }
+ spin_unlock(sb_lock);
+}
+
+static int ibac_inode_permission(struct inode *inode, int mask,
+struct nameidata *nd)
+{
+ struct ibac_isec_data *isec = inode-i_security;
+ struct dentry *dentry;
+ char *path = NULL;
+ char *fname = NULL;
+ int rc = 0;
+ int measure;
+
+ dentry = (!nd || !nd-dentry) ? d_find_alias(inode) : nd-dentry;
+ if (!dentry)
+ return 0;
+ if (nd) { /* preferably use fullname */
+ path = (char *)__get_free_page(GFP_KERNEL);
+ if (path)
+ fname = d_path(nd-dentry, nd-mnt, path, PAGE_SIZE);
+ }
+
+ if (!fname) /* no choice, use short name */
+ fname = (!dentry-d_name.name) ? (char *)dentry-d_iname :
+ (char *)dentry-d_name.name;
Please separate the above out into a helper function.
Ok.
This name is only ever used for debugging, right? I didn't miss any
place where the name is used for some security decision?
Correct, the filename is not used for any security decision, but it
is passed to integrity_measure(), which the IMA integrity provider
associates with the given hash value. To see the filename hints
'cat /sys/kernel/security/ima/ascii_runtime_measurements'.
+
+ /* Measure labeled files */
+ spin_lock(isec-lock);
+ measure = isec-measure;
+ spin_unlock(isec-lock);
+
+ if (S_ISREG(inode-i_mode) (measure == 1)
+(mask MAY_READ)) {
+ rc = verify_metadata_integrity(dentry);
+ if (rc == 0)
+ rc = verify_and_measure_integrity(dentry, NULL,
+ fname, mask);
+ }
+
+ /* Deny permission to write, if currently mmapped. */
+ if (inode mask MAY_WRITE) {
+ spin_lock(isec-lock);
+
Re: [RFC][Patch 1/1] IBAC Patch
Quoting Mimi Zohar ([EMAIL PROTECTED]): This is a re-release of Integrity Based Access Control(IBAC) LSM module which bases access control decisions on the new integrity framework services. IBAC is a sample LSM module to help clarify the interaction between LSM and Linux Integrity Modules(LIM). New to this release of IBAC is digsig's functionality of preventing files open for write to be mmapped, and files that are mmapped from being opened for write. IBAC originally verified/measured executables only in the bprm_check_security() hook. By only doing the verification/measurement in bprm_check_security(), libraries could be loaded without first being verified/measured. This release of IBAC, files are also verified/measured in the file_mmap() hook, which catches the libraries, and inode_permission() hook, which verifies/measures files tagged, by a userspace application, with the extended attribute 'security.measure'. IBAC can be included or excluded in the kernel configuration. If included in the kernel and IBAC_BOOTPARAM is enabled, IBAC can also be enabled/disabled on the kernel command line with 'ibac='. IBAC can be configured to either verify and enforce integrity or to just log integrity failures on the kernel command line with 'ibac_enforce='. When IBAC_BOOTPARAM is enabled, the default is only to log integrity failures. Signed-off-by: Mimi Zohar [EMAIL PROTECTED] --- Index: linux-2.6.22-rc4-mm2/security/ibac/Kconfig === --- /dev/null +++ linux-2.6.22-rc4-mm2/security/ibac/Kconfig @@ -0,0 +1,54 @@ +config SECURITY_IBAC + boolean IBAC support + depends on SECURITY SECURITY_NETWORK INTEGRITY + help + Integrity Based Access Control(IBAC) uses the Linux + Integrity Module(LIM) API calls to verify an executable's + metadata and data's integrity. Based on the results, + execution permission is permitted/denied. Integrity + providers may implement the LIM hooks differently. For + more information on integrity verification refer to the + specific integrity provider documentation. + +config SECURITY_IBAC_BOOTPARAM + bool IBAC boot parameter + depends on SECURITY_IBAC + default n + help + This option adds a kernel parameter 'ibac', which allows IBAC + to be disabled at boot. If this option is selected, IBAC + functionality can be disabled with ibac=0 on the kernel + command line. The purpose of this option is to allow a + single kernel image to be distributed with IBAC built in, + but not necessarily enabled. + + If you are unsure how to answer this question, answer N. + +config SECURITY_IBAC_BOOTPARAM_VALUE + int IBAC boot parameter default value + depends on SECURITY_IBAC_BOOTPARAM + range 0 1 + default 0 + help + This option sets the default value for the kernel parameter + 'ibac', which allows IBAC to be enabled at boot. If this + option is set to 1 (one), the IBAC kernel parameter will + default to 1, enabling IBAC at bootup. If this option is + set to 0 (zero), the IBAC kernel parameter will default to 0, + disabling IBAC at bootup. + + If you are unsure how to answer this question, answer 0. + +config SECURITY_IBAC_ENFORCE + bool IBAC integrity enforce boot parameter + depends on SECURITY_IBAC_BOOTPARAM + default y + help + This option adds a kernel parameter 'ibac_enforce', which + allows integrity enforcement to be enabled/disabled at boot. + If this option is selected, integrity enforcement can be + enabled with ibac_enforce=1 on the kernel command line. + The default is not to enforce integrity, but simply log + integrity verification errors. + + If you are unsure how to answer this question, answer Y. Index: linux-2.6.22-rc4-mm2/security/ibac/Makefile === --- /dev/null +++ linux-2.6.22-rc4-mm2/security/ibac/Makefile @@ -0,0 +1,6 @@ +# +# Makefile for building IBAC +# + +obj-$(CONFIG_SECURITY_IBAC) += ibac.o +ibac-y := ibac_main.o Index: linux-2.6.22-rc4-mm2/security/ibac/ibac_main.c === --- /dev/null +++ linux-2.6.22-rc4-mm2/security/ibac/ibac_main.c @@ -0,0 +1,434 @@ +/* + * Integrity Based Access Control(IBAC) sample LSM module calling LIM hooks. + * + * Copyright (C) 2007 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. + */ + +#include linux/module.h +#include linux/moduleparam.h +#include linux/kernel.h
Re: [RFC] [Patch 1/1] IBAC Patch
On Tue, 2007-03-13 at 19:27 -0700, Seth Arnold wrote:
On Thu, Mar 08, 2007 at 05:58:16PM -0500, Mimi Zohar wrote:
This is a request for comments for a new Integrity Based Access
Control(IBAC) LSM module which bases access control decisions
on the new integrity framework services.
Thanks Mimi, nice to see an example of how the integrity framework ought
to be used.
(Hopefully this will help clarify the interaction between an LSM
module and LIM module.)
Is this module intended to clarify an interface, or be useful in and of
itself?
It's a little bit of both. :-) Initially it was written to help me with
implementing and testing the integrity provider. But it could definitely stand
on it's own. As Serge Hallyn commented http://lkml.org/lkml/2007/3/13/220, by
adding the mmap hook, IBAC could replace the LSM aspect of digsig and a gpg
based integrity provider, could be written, instead of EVM, which is TPM based.
Index: linux-2.6.21-rc3-mm2/security/ibac/Makefile
===
--- /dev/null
+++ linux-2.6.21-rc3-mm2/security/ibac/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for building IBAC
+#
+
+obj-$(CONFIG_SECURITY_IBAC) += ibac.o
+ibac-y := ibac_main.o
Index: linux-2.6.21-rc3-mm2/security/ibac/ibac_main.c
===
--- /dev/null
+++ linux-2.6.21-rc3-mm2/security/ibac/ibac_main.c
@@ -0,0 +1,126 @@
+/*
+ * Integrity Based Access Control (IBAC)
+ *
+ * Copyright (C) 2007 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.
+ */
+
+#include linux/module.h
+#include linux/moduleparam.h
+#include linux/kernel.h
+#include linux/security.h
+#include linux/integrity.h
+
+#ifdef CONFIG_SECURITY_IBAC_BOOTPARAM
+int ibac_enabled = CONFIG_SECURITY_IBAC_BOOTPARAM_VALUE;
+
+static int __init ibac_enabled_setup(char *str)
+{
+ ibac_enabled = simple_strtol(str, NULL, 0);
+ return 1;
+}
+
+__setup(ibac=, ibac_enabled_setup);
+#else
+int ibac_enabled = 0;
+#endif
If the command line option isn't enabled, how will ibac_enabled ever be
set to '1'? Have I overlooked or forgotten some helper routine elsewhere?
I guess I was a bit over zealous in preventing IBAC from running
unintentionally. Will fix in the next IBAC patch release.
+static unsigned int integrity_enforce = 0;
+static int __init integrity_enforce_setup(char *str)
+{
+ integrity_enforce = simple_strtol(str, NULL, 0);
+ return 1;
+}
+
+__setup(ibac_enforce=, integrity_enforce_setup);
+
+#define XATTR_NAME security.evm.hash
Is this name unique to this IBAC module? Or should it be kept in sync
with the integrity framework?
In order to verify the metadata integrity, an xattr needs to be
specified on the integrity_verify_metadata() call. As IBAC does not
define an xattr of its own, I used this one, which at the time worked.
But as EVM is only one integrity provider, this probably is not a good
idea. To resolve this problem, I guess the integrity_verify_metadata()
API call should respond without requiring an actual xattr.
+static inline int is_kernel_thread(struct task_struct *tsk)
+{
+ return (!tsk-mm) ? 1 : 0;
+}
+
+static int ibac_bprm_check_security(struct linux_binprm *bprm)
+{
+ struct dentry *dentry = bprm-file-f_dentry;
+ int xattr_len;
+ char *xattr_value = NULL;
+ int rc, status;
+
+ rc = integrity_verify_metadata(dentry, XATTR_NAME,
+ xattr_value, xattr_len, status);
+ if (rc 0 rc == -EOPNOTSUPP) {
+ kfree(xattr_value);
+ return 0;
+ }
+
+ if (rc 0) {
+ printk(KERN_INFO verify_metadata %s failed
+ (rc: %d - status: %d)\n, bprm-filename, rc, status);
+ if (!integrity_enforce)
+ rc = 0;
+ goto out;
+ }
+ if (status != INTEGRITY_PASS) { /* FAIL | NO_LABEL */
+ if (!is_kernel_thread(current)) {
Please remind me why kernel threads are exempt?
You really don't want to prevent kernel threads from working. Nasty things
happen.
+ printk(KERN_INFO verify_metadata %s
+ (Integrity status: FAIL)\n, bprm-filename);
Integrity status may be FAIL or NO_LABEL at this point -- would it be
more useful to report the whole truth?
FAIL here indicates that the integrity of the metadata was bad, while
NOLABEL indicates, in the case of EVM, that there was no HMAC. I'll
update the error message to differentiate between the two.
+ if (integrity_enforce) {
+ rc = -EACCES;
+
