Re: [RFC] [Patch 1/1] IBAC Patch

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

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 <zohar@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.
> > + */
> > +
> > +#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;
> > +				goto out;
> > +			}
> > +		}
> > +	}
> > +
> > +	rc = integrity_verify_data(dentry, &status);
> > +	if (rc < 0) {
> > +		printk(KERN_INFO "%s verify_data failed "
> > +		       "(rc: %d - status: %d)\n", bprm->filename, rc, status);
> > +		if (!integrity_enforce)
> > +			rc = 0;
> > +		goto out;
> > +	}
> > +	if (status != INTEGRITY_PASS) {
> > +		if (!is_kernel_thread(current)) {
> 
> Please remind me why kernel threads are exempt?

Same as above. 
> > +			printk(KERN_INFO "verify_data %s "
> > +			       "(Integrity status: FAIL)\n", bprm->filename);
> 
> Same question about FAIL vs NO_LABEL.. (Would NO_LABEL be caught by a
> failing verify_metadata above?)

For integrity_verify_data(), EVM verifies that the file hash matches the
one stored as an xattr.

> > +			if (integrity_enforce) {
> > +				rc = -EACCES;
> > +				goto out;
> > +			}
> > +		}
> > +	}
> > +
> > +	kfree(xattr_value);
> > +
> > +	/* measure all integrity level executables */
> > +	integrity_measure(dentry, bprm->filename, MAY_EXEC);
> > +	return 0;
> 
> If integrity_measure() fails (can it fail?) is allowing the exec still the
> right approach? (I seem to recall that "measuring integrity" is actually
> something more like "go off an compute the integrity, but don't compare
> it against anything" -- but even if it fails, is continuing correct?)

For integrity_measure(), EVM calls IMA, if enabled, to extend the
measurement list with the hash value it provides. In most cases, EVM
has already calculated the hash value, when it was called to verify the
data. integrity_measure() is not meant to be intrusive, so it is defined
as void.

> Rest elided :) Thanks Mimi

Thank you for your comments.

Mimi Zohar