linux-kernel - Re: [RFC] [Patch 1/1] IBAC Patch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20070314022713.GI27643@suse.de>
Date:	Tue, 13 Mar 2007 19:27:13 -0700
From:	Seth Arnold <seth.arnold@...e.de>
To:	Mimi Zohar <zohar@...ux.vnet.ibm.com>
Cc:	linux-security-module@...r.kernel.org, safford@...son.ibm.com,
	serue@...ux.vnet.ibm.com, kjhall@...ux.vnet.ibm.com,
	zohar@...ibm.com, linux-kernel@...r.kernel.org
Subject: Re: [RFC] [Patch 1/1] IBAC Patch

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?

> 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@...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?

> +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?

> +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?

> +			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?

> +			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?

> +			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?)

> +			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?)

Rest elided :) Thanks Mimi

Content of type "application/pgp-signature" skipped