* Re: [RFC]integrity: SELinux patch
2007-07-16 13:57 [RFC]integrity: SELinux patch Mimi Zohar
@ 2007-07-16 18:40 ` Joshua Brindle
2007-07-16 23:13 ` Mimi Zohar
2007-07-16 19:23 ` Paul Moore
` (3 subsequent siblings)
4 siblings, 1 reply; 23+ messages in thread
From: Joshua Brindle @ 2007-07-16 18:40 UTC (permalink / raw)
To: Mimi Zohar; +Cc: selinux, zohar, safford
Mimi Zohar wrote:
> This is a first attempt to verify and measure file integrity, by
> adding the new Linux Integrity Modules(LIM) API calls to SElinux.
> We are planning on posting the corresponding LIM and IMA patches to
> LKML, but would like comments/suggestions here first, particularly
> in regards to the policy checking code in selinux_measure() called
> from selinux_inode_permission().
>
> SELINUX_ENFORCE_INTEGRITY can be configured to either verify and
> enforce integrity or to just log integrity failures. The default
> is to just log integrity failures.
>
>
I haven't reviewed the patch yet but reference to the above comments.
This should be controlled with policy I think, its tricky though and I
haven't thought about all the ramifications yet, I'll get back to this
after I've thought about it.
> The integrity of the SELinux metadata is verified when the xattr
> is initially retrieved. On an integrity failure, assuming that
> integrity verification is enforced, normal error processing occurs.
>
> By default, all executables and all files that are mmap'ed executable
> are measured. This patch extends the file class with 'measure'.
> Additional files can be measured in selinux_inode_permission()
> based on a FILE__MEASURE policy. (As the policy call is causing too
> many files to be measured, it is commented out.) For example, to
> measure kernel modules add:
>
I'd like to say that if SELinux is controlling whether measurements
happen or not I don't think there should be a 'default' policy in the
module. Being able to avoid the measurement overhead for domains that we
don't care about would be a win, fine grained as well as course grained
measurement selections can be done with SELinux.
> module ima 1.0;
>
> require {
> type insmod_t;
> type modules_object_t;
> class file measure;
> }
>
> #============= insmod_t ==============
> allow insmod_t modules_object_t:file measure;
>
> Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
> ---
> Index: linux-2.6.22-rc6-mm1/security/selinux/hooks.c
> ===================================================================
> --- linux-2.6.22-rc6-mm1.orig/security/selinux/hooks.c
> +++ linux-2.6.22-rc6-mm1/security/selinux/hooks.c
> @@ -70,6 +70,7 @@
> #include <linux/audit.h>
> #include <linux/string.h>
> #include <linux/selinux.h>
> +#include <linux/integrity.h>
> #include <linux/mutex.h>
>
> #include "avc.h"
> @@ -109,6 +110,12 @@ __setup("selinux=", selinux_enabled_setu
> int selinux_enabled = 1;
> #endif
>
> +#ifdef CONFIG_SECURITY_SELINUX_ENFORCE_INTEGRITY
> +static int integrity_enforce = 1;
> +#else
> +static int integrity_enforce;
> +#endif
> +
> /* Original (dummy) security module. */
> static struct security_operations *original_ops = NULL;
>
> @@ -847,6 +854,7 @@ static int inode_doinit_with_dentry(stru
> char *context = NULL;
> unsigned len = 0;
> int rc = 0;
> + int status;
>
> if (isec->initialized)
> goto out;
> @@ -890,35 +898,12 @@ static int inode_doinit_with_dentry(stru
> goto out_unlock;
> }
>
> - len = INITCONTEXTLEN;
> - context = kmalloc(len, GFP_KERNEL);
> - if (!context) {
> - rc = -ENOMEM;
> + rc = integrity_verify_metadata(dentry, XATTR_NAME_SELINUX,
> + &context, &len, &status);
> + if (rc == -ENOMEM) {
> dput(dentry);
> goto out_unlock;
> }
> - rc = inode->i_op->getxattr(dentry, XATTR_NAME_SELINUX,
> - context, len);
> - if (rc == -ERANGE) {
> - /* Need a larger buffer. Query for the right size. */
> - rc = inode->i_op->getxattr(dentry, XATTR_NAME_SELINUX,
> - NULL, 0);
> - if (rc < 0) {
> - dput(dentry);
> - goto out_unlock;
> - }
> - kfree(context);
> - len = rc;
> - context = kmalloc(len, GFP_KERNEL);
> - if (!context) {
> - rc = -ENOMEM;
> - dput(dentry);
> - goto out_unlock;
> - }
> - rc = inode->i_op->getxattr(dentry,
> - XATTR_NAME_SELINUX,
> - context, len);
> - }
> dput(dentry);
> if (rc < 0) {
> if (rc != -ENODATA) {
> @@ -932,6 +917,19 @@ static int inode_doinit_with_dentry(stru
> sid = sbsec->def_sid;
> rc = 0;
> } else {
> + /* Log integrity failures, if integrity enforced
> + * behave like for any other failure.
> + */
> + if (status == INTEGRITY_FAIL) {
> + printk(KERN_WARNING "%s: verify_metadata "
> + "failed for dev=%s ino=%ld\n",
> + __FUNCTION__,
> + inode->i_sb->s_id, inode->i_ino);
> + if (integrity_enforce) {
> + kfree(context);
> + goto out_unlock;
> + }
> + }
> rc = security_context_to_sid_default(context, rc, &sid,
> sbsec->def_sid);
> if (rc) {
> @@ -1701,9 +1699,109 @@ static int selinux_bprm_set_security(str
> return 0;
> }
>
> -static int selinux_bprm_check_security (struct linux_binprm *bprm)
> +static inline int is_kernel_thread(struct task_struct *tsk)
> +{
> + return (!tsk->mm) ? 1 : 0;
> +}
> +
> +static int selinux_verify_metadata(struct dentry *dentry)
> +{
> + int rc, status;
> +
> + if (!dentry)
> + return 0;
> +
> + rc = integrity_verify_metadata(dentry, NULL, NULL, NULL, &status);
> + if (rc == -EOPNOTSUPP)
> + return 0;
> +
> + if (rc < 0) {
> + printk(KERN_INFO "%s: verify_metadata %s failed"
> + "(rc: %d - status: %d)\n", __FUNCTION__,
> + dentry->d_name.name, rc, status);
> + if (!integrity_enforce)
> + rc = 0;
> + goto out;
> + }
> + if (status != INTEGRITY_PASS) { /* FAIL | NO_LABEL */
> + if (!is_kernel_thread(current)) {
> + printk(KERN_INFO "%s: verify_metadata %s "
> + "(Integrity status: %s)\n", __FUNCTION__,
> + dentry->d_name.name,
> + status == INTEGRITY_FAIL ? "FAIL" : "NOLABEL");
> + if (integrity_enforce) {
> + rc = -EACCES;
> + goto out;
> + }
> + }
> + }
> + return 0;
> +out:
> + return rc;
> +}
> +
> +static int selinux_verify_and_measure(struct dentry *dentry,
> + struct file *file,
> + char *filename, int mask)
> +{
> + int rc, status;
> +
> + if (!dentry && !file)
> + return 0;
> +
> + rc = integrity_verify_data(dentry, file, &status);
> + if (rc < 0) {
> + printk(KERN_INFO "%s: %s verify_data failed "
> + "(rc: %d - status: %d)\n", __FUNCTION__,
> + filename, rc, status);
> + if (!integrity_enforce)
> + rc = 0;
> + goto out;
> + }
> +
> + if (status != INTEGRITY_PASS) {
> + if (!is_kernel_thread(current)) {
> + printk(KERN_INFO "%s: verify_data %s "
> + "(Integrity status: FAIL)\n",
> + __FUNCTION__, filename);
> + if (integrity_enforce) {
> + rc = -EACCES;
> + goto out;
> + }
> + }
> + }
> +
> + /* Only measure files that passed integrity verification. */
> + integrity_measure(dentry, file, filename, mask);
> + return 0;
> +out:
> + /*
> + * Verification failed, but as integrity is not being enforced,
> + * we still need to measure.
> + */
> + if (rc == 0)
> + integrity_measure(dentry, file, filename, mask);
> + return rc;
> +}
> +
> +static int selinux_bprm_check_security(struct linux_binprm *bprm)
> {
> - return secondary_ops->bprm_check_security(bprm);
> + struct dentry *dentry = bprm->file->f_dentry;
> + int rc;
> +
> + rc = secondary_ops->bprm_check_security(bprm);
> + if (rc != 0)
> + return rc;
> +
> + /* We probably want to re-verify the metadata, verify
> + * the data, and measure the integrity of all executables.
> + */
> + rc = selinux_verify_metadata(dentry);
> + if (rc == 0)
> + rc = selinux_verify_and_measure(dentry,
> + bprm->file, bprm->filename, MAY_EXEC);
> +
> + return rc;
> }
>
>
> @@ -2252,6 +2350,30 @@ static int selinux_inode_follow_link(str
> return dentry_has_perm(current, NULL, dentry, FILE__READ);
> }
>
> +/*
> + * Measure based on a policy
> + */
> +static int selinux_measure(struct inode *inode, struct dentry *dentry)
> +{
> + int rc = 1;
> +#if 0
> + struct inode_security_struct *isec = inode->i_security;
> + struct task_security_struct *tsec = current->security;
> + struct av_decision avd;
> + struct avc_audit_data ad;
> +
> + /* This initial attempt to base on policy measures too much. */
> + rc = avc_has_perm_noaudit(tsec->sid, isec->sid, SECCLASS_FILE,
> + FILE__MEASURE, AVC_STRICT, &avd);
> + AVC_AUDIT_DATA_INIT(&ad,FS);
> + ad.u.fs.mnt = NULL;
> + ad.u.fs.dentry = dentry;
> + avc_audit(tsec->sid, isec->sid, SECCLASS_FILE, FILE__MEASURE,
> + &avd, rc, &ad);
> +#endif
> + return rc;
> +}
> +
> static int selinux_inode_permission(struct inode *inode, int mask,
> struct nameidata *nd)
> {
> @@ -2265,9 +2387,26 @@ static int selinux_inode_permission(stru
> /* No permission to check. Existence test. */
> return 0;
> }
> -
> - return inode_has_perm(current, inode,
> + rc = inode_has_perm(current, inode,
> file_mask_to_av(inode->i_mode, mask), NULL);
> + if (rc != 0)
> + return rc;
> +
> + if (S_ISREG(inode->i_mode) && (mask & MAY_READ)) {
> + struct dentry *dentry;
> +
> + dentry = (!nd || !nd->dentry) ?
> + d_find_alias(inode) : nd->dentry;
> +
> + if (dentry) {
> + if (selinux_measure(inode, dentry) == 0)
> + integrity_measure(dentry, NULL,
> + (char *)dentry->d_name.name, mask);
> + if (!nd || !nd->dentry)
> + dput(dentry);
> + }
> + }
> + return rc;
> }
>
> static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
> @@ -2630,8 +2769,20 @@ static int selinux_file_mprotect(struct
> return rc;
> }
> #endif
> + rc = file_map_prot_check(vma->vm_file, prot, vma->vm_flags&VM_SHARED);
> + if (rc != 0)
> + return rc;
>
> - return file_map_prot_check(vma->vm_file, prot, vma->vm_flags&VM_SHARED);
> + if (vma->vm_file && vma->vm_file->f_dentry) {
> + rc = selinux_verify_metadata(vma->vm_file->f_dentry);
> + if (rc == 0)
> + rc = selinux_verify_and_measure(NULL, vma->vm_file,
> + (char *)vma->vm_file->f_dentry->d_name.name,
> + MAY_EXEC);
> + if (rc != 0)
> + return rc;
> + }
> + return rc;
> }
>
> static int selinux_file_lock(struct file *file, unsigned int cmd)
> Index: linux-2.6.22-rc6-mm1/security/selinux/Kconfig
> ===================================================================
> --- linux-2.6.22-rc6-mm1.orig/security/selinux/Kconfig
> +++ linux-2.6.22-rc6-mm1/security/selinux/Kconfig
> @@ -161,3 +161,15 @@ config SECURITY_SELINUX_POLICYDB_VERSION
> installed under /etc/selinux/$SELINUXTYPE/policy, where
> SELINUXTYPE is defined in your /etc/selinux/config.
>
> +config SECURITY_SELINUX_ENFORCE_INTEGRITY
> + bool "SELinux integrity "
> + depends on SECURITY_SELINUX && INTEGRITY
> + default n
> + help
> + This option enables SELinux, using the Linux Integrity
> + Module (LIM) API, to verify metadata stored as extended
> + attributes, verify the file data and extend the TPM PCR
> + measurements.
> +
> + If you are unsure how to answer this question, answer N.
> +
> Index: linux-2.6.22-rc6-mm1/security/selinux/include/av_permissions.h
> ===================================================================
> --- linux-2.6.22-rc6-mm1.orig/security/selinux/include/av_permissions.h
> +++ linux-2.6.22-rc6-mm1/security/selinux/include/av_permissions.h
> @@ -99,6 +99,7 @@
> #define FILE__EXECUTE_NO_TRANS 0x00020000UL
> #define FILE__ENTRYPOINT 0x00040000UL
> #define FILE__EXECMOD 0x00080000UL
> +#define FILE__MEASURE 0x00100000UL
> #define LNK_FILE__IOCTL 0x00000001UL
> #define LNK_FILE__READ 0x00000002UL
> #define LNK_FILE__WRITE 0x00000004UL
> Index: linux-2.6.22-rc6-mm1/security/selinux/include/av_perm_to_string.h
> ===================================================================
> --- linux-2.6.22-rc6-mm1.orig/security/selinux/include/av_perm_to_string.h
> +++ linux-2.6.22-rc6-mm1/security/selinux/include/av_perm_to_string.h
> @@ -17,6 +17,7 @@
> S_(SECCLASS_FILE, FILE__EXECUTE_NO_TRANS, "execute_no_trans")
> S_(SECCLASS_FILE, FILE__ENTRYPOINT, "entrypoint")
> S_(SECCLASS_FILE, FILE__EXECMOD, "execmod")
> + S_(SECCLASS_FILE, FILE__MEASURE, "measure")
> S_(SECCLASS_CHR_FILE, CHR_FILE__EXECUTE_NO_TRANS, "execute_no_trans")
> S_(SECCLASS_CHR_FILE, CHR_FILE__ENTRYPOINT, "entrypoint")
> S_(SECCLASS_CHR_FILE, CHR_FILE__EXECMOD, "execmod")
>
>
> --
> This message was distributed to subscribers of the selinux mailing list.
> If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
> the words "unsubscribe selinux" without quotes as the message.
>
>
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [RFC]integrity: SELinux patch
2007-07-16 18:40 ` Joshua Brindle
@ 2007-07-16 23:13 ` Mimi Zohar
0 siblings, 0 replies; 23+ messages in thread
From: Mimi Zohar @ 2007-07-16 23:13 UTC (permalink / raw)
To: Joshua Brindle; +Cc: selinux, zohar, safford
On Mon, 2007-07-16 at 14:40 -0400, Joshua Brindle wrote:
> Mimi Zohar wrote:
> > This is a first attempt to verify and measure file integrity, by
> > adding the new Linux Integrity Modules(LIM) API calls to SElinux.
> > We are planning on posting the corresponding LIM and IMA patches to
> > LKML, but would like comments/suggestions here first, particularly
> > in regards to the policy checking code in selinux_measure() called
> > from selinux_inode_permission().
> >
> > SELINUX_ENFORCE_INTEGRITY can be configured to either verify and
> > enforce integrity or to just log integrity failures. The default
> > is to just log integrity failures.
> >
> >
> I haven't reviewed the patch yet but reference to the above comments.
> This should be controlled with policy I think, its tricky though and I
> haven't thought about all the ramifications yet, I'll get back to this
> after I've thought about it.
Without the SELINUX_INTEGRITY_ENFORCE option enabled, everything is
logged, nothing is denied. This is meant to be similar to the selinux
'enforcing' option. Currently it is only a compile option, perhaps
just as enforcing is a runtime option, it should be a runtime option as
well.
> > The integrity of the SELinux metadata is verified when the xattr
> > is initially retrieved. On an integrity failure, assuming that
> > integrity verification is enforced, normal error processing occurs.
> >
> > By default, all executables and all files that are mmap'ed executable
> > are measured. This patch extends the file class with 'measure'.
> > Additional files can be measured in selinux_inode_permission()
> > based on a FILE__MEASURE policy. (As the policy call is causing too
> > many files to be measured, it is commented out.) For example, to
> > measure kernel modules add:
> >
> I'd like to say that if SELinux is controlling whether measurements
> happen or not I don't think there should be a 'default' policy in the
> module. Being able to avoid the measurement overhead for domains that we
> don't care about would be a win, fine grained as well as course grained
> measurement selections can be done with SELinux.
Ok. I can understand that as long as SELinux is making some measurement
decisions based on policy, it could just as easily make all of the
measurement decisions. At the moment though, I am having problems with
the avc_has_perm_noaudit() call in inode_permission. Without defining a
'measure' policy, it measures a lot of files. Is this because the system
is running with a targeted policy?
Are you suggesting that there is no need for the measurement calls in
file_mmap()/bprm_check_security() and that there should only be the one
measurement call in inode_permission(), which is based on policy, or
are you suggesting that the measurement calls in bprm_check_security()
and file_mmap() be based on policy?
Thanks!
Mimi Zohar
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC]integrity: SELinux patch
2007-07-16 13:57 [RFC]integrity: SELinux patch Mimi Zohar
2007-07-16 18:40 ` Joshua Brindle
@ 2007-07-16 19:23 ` Paul Moore
2007-07-17 14:30 ` Mimi Zohar
2007-07-17 0:20 ` [RFC]integrity: SELinux patch James Morris
` (2 subsequent siblings)
4 siblings, 1 reply; 23+ messages in thread
From: Paul Moore @ 2007-07-16 19:23 UTC (permalink / raw)
To: Mimi Zohar; +Cc: selinux, zohar, safford
On Monday, July 16 2007 9:57:20 am Mimi Zohar wrote:
> Index: linux-2.6.22-rc6-mm1/security/selinux/hooks.c
> ===================================================================
> --- linux-2.6.22-rc6-mm1.orig/security/selinux/hooks.c
> +++ linux-2.6.22-rc6-mm1/security/selinux/hooks.c
> @@ -932,6 +917,19 @@ static int inode_doinit_with_dentry(stru
> sid = sbsec->def_sid;
> rc = 0;
> } else {
> + /* Log integrity failures, if integrity enforced
> + * behave like for any other failure.
> + */
> + if (status == INTEGRITY_FAIL) {
> + printk(KERN_WARNING "%s: verify_metadata "
> + "failed for dev=%s ino=%ld\n",
> + __FUNCTION__,
> + inode->i_sb->s_id, inode->i_ino);
Should this event be audited via the audit subsystem? Or is it audited
elsewhere and I'm just missing it (I only saw a disabled block w/audit code).
> @@ -1701,9 +1699,109 @@ static int selinux_bprm_set_security(str
> return 0;
> }
>
> -static int selinux_bprm_check_security (struct linux_binprm *bprm)
> +static inline int is_kernel_thread(struct task_struct *tsk)
> +{
> + return (!tsk->mm) ? 1 : 0;
> +}
> +
> +static int selinux_verify_metadata(struct dentry *dentry)
> +{
> + int rc, status;
> +
> + if (!dentry)
> + return 0;
> +
> + rc = integrity_verify_metadata(dentry, NULL, NULL, NULL, &status);
> + if (rc == -EOPNOTSUPP)
> + return 0;
> +
> + if (rc < 0) {
> + printk(KERN_INFO "%s: verify_metadata %s failed"
> + "(rc: %d - status: %d)\n", __FUNCTION__,
> + dentry->d_name.name, rc, status);
Same comment about audit here ...
> +static int selinux_verify_and_measure(struct dentry *dentry,
> + struct file *file,
> + char *filename, int mask)
> +{
> + int rc, status;
> +
> + if (!dentry && !file)
> + return 0;
> +
> + rc = integrity_verify_data(dentry, file, &status);
> + if (rc < 0) {
> + printk(KERN_INFO "%s: %s verify_data failed "
> + "(rc: %d - status: %d)\n", __FUNCTION__,
> + filename, rc, status);
... and here ...
--
paul moore
linux security @ hp
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [RFC]integrity: SELinux patch
2007-07-16 19:23 ` Paul Moore
@ 2007-07-17 14:30 ` Mimi Zohar
2007-07-17 14:32 ` Paul Moore
2007-07-17 14:54 ` James Morris
0 siblings, 2 replies; 23+ messages in thread
From: Mimi Zohar @ 2007-07-17 14:30 UTC (permalink / raw)
To: Paul Moore; +Cc: selinux, zohar, safford
On Mon, 2007-07-16 at 15:23 -0400, Paul Moore wrote:
> On Monday, July 16 2007 9:57:20 am Mimi Zohar wrote:
> > Index: linux-2.6.22-rc6-mm1/security/selinux/hooks.c
> > ===================================================================
> > --- linux-2.6.22-rc6-mm1.orig/security/selinux/hooks.c
> > +++ linux-2.6.22-rc6-mm1/security/selinux/hooks.c
> > @@ -932,6 +917,19 @@ static int inode_doinit_with_dentry(stru
> > sid = sbsec->def_sid;
> > rc = 0;
> > } else {
> > + /* Log integrity failures, if integrity enforced
> > + * behave like for any other failure.
> > + */
> > + if (status == INTEGRITY_FAIL) {
> > + printk(KERN_WARNING "%s: verify_metadata "
> > + "failed for dev=%s ino=%ld\n",
> > + __FUNCTION__,
> > + inode->i_sb->s_id, inode->i_ino);
>
> Should this event be audited via the audit subsystem? Or is it audited
> elsewhere and I'm just missing it (I only saw a disabled block w/audit code).
No, it isn't being audited, but should be. The question is what type of audit
message would be appropriate here. It could be the normal denied/granted
message, but that would be confusing as this isn't based on a permission or
capability check, but an integrity error. Any suggestions how to handle this
here and in the other places?
Thanks!
Mimi Zohar
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [RFC]integrity: SELinux patch
2007-07-17 14:30 ` Mimi Zohar
@ 2007-07-17 14:32 ` Paul Moore
2007-07-17 14:54 ` James Morris
1 sibling, 0 replies; 23+ messages in thread
From: Paul Moore @ 2007-07-17 14:32 UTC (permalink / raw)
To: Mimi Zohar; +Cc: selinux, zohar, safford
On Tuesday, July 17 2007 10:30:13 am Mimi Zohar wrote:
> On Mon, 2007-07-16 at 15:23 -0400, Paul Moore wrote:
> > On Monday, July 16 2007 9:57:20 am Mimi Zohar wrote:
> > > Index: linux-2.6.22-rc6-mm1/security/selinux/hooks.c
> > > ===================================================================
> > > --- linux-2.6.22-rc6-mm1.orig/security/selinux/hooks.c
> > > +++ linux-2.6.22-rc6-mm1/security/selinux/hooks.c
> > > @@ -932,6 +917,19 @@ static int inode_doinit_with_dentry(stru
> > > sid = sbsec->def_sid;
> > > rc = 0;
> > > } else {
> > > + /* Log integrity failures, if integrity enforced
> > > + * behave like for any other failure.
> > > + */
> > > + if (status == INTEGRITY_FAIL) {
> > > + printk(KERN_WARNING "%s: verify_metadata "
> > > + "failed for dev=%s ino=%ld\n",
> > > + __FUNCTION__,
> > > + inode->i_sb->s_id, inode->i_ino);
> >
> > Should this event be audited via the audit subsystem? Or is it audited
> > elsewhere and I'm just missing it (I only saw a disabled block w/audit
> > code).
>
> No, it isn't being audited, but should be. The question is what type of
> audit message would be appropriate here. It could be the normal
> denied/granted message, but that would be confusing as this isn't based on
> a permission or capability check, but an integrity error. Any suggestions
> how to handle this here and in the other places?
I would suggest asking some of the folks on the audit mailing list,
linux-audit@redhat.com. It doesn't have to be a deny/grant message like
SELinux AVC messages to be "auditable". Look at some of the other audit
messages to get an idea. The NetLabel code, for example, emits several audit
messages which I would consider configuration notifications and not access
control results.
--
paul moore
linux security @ hp
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [RFC]integrity: SELinux patch
2007-07-17 14:30 ` Mimi Zohar
2007-07-17 14:32 ` Paul Moore
@ 2007-07-17 14:54 ` James Morris
2007-07-18 15:05 ` Steve G
1 sibling, 1 reply; 23+ messages in thread
From: James Morris @ 2007-07-17 14:54 UTC (permalink / raw)
To: Mimi Zohar; +Cc: Paul Moore, selinux, zohar, safford
On Tue, 17 Jul 2007, Mimi Zohar wrote:
> No, it isn't being audited, but should be. The question is what type of audit
> message would be appropriate here. It could be the normal denied/granted
> message, but that would be confusing as this isn't based on a permission or
> capability check, but an integrity error. Any suggestions how to handle this
> here and in the other places?
If integrity is being enforced, then the final AVC denial should include
information that it was because of an integrity failure.
I'm not sure that it's useful to have non-enforced integrity, aside from
debugging/development use.
For general use, it should either be enabled or disabled.
- James
--
James Morris
<jmorris@namei.org>
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC]integrity: SELinux patch
2007-07-17 14:54 ` James Morris
@ 2007-07-18 15:05 ` Steve G
2007-09-04 20:46 ` Mimi Zohar
[not found] ` <1188999967.5563.2.camel@localhost.localdomain>
0 siblings, 2 replies; 23+ messages in thread
From: Steve G @ 2007-07-18 15:05 UTC (permalink / raw)
To: James Morris, Mimi Zohar, sgrubb; +Cc: Paul Moore, selinux, zohar, safford
>> No, it isn't being audited, but should be. The question is what type of audit
>> message would be appropriate here. It could be the normal denied/granted
>> message, but that would be confusing as this isn't based on a permission or
>> capability check, but an integrity error. Any suggestions how to handle this
>> here and in the other places?
MRPP places some requirements on intergrity checking. Maybe it tells you more
information about what's required. More info:
http://www.niap-ccevs.org/cc-scheme/pp/pp.cfm?id=PP_OS_ML_MR2.0_V1.91
>If integrity is being enforced, then the final AVC denial should include
>information that it was because of an integrity failure.
Might ought to be an integrity audit record type rather than avc. This way
aureport can separate it out for its summary report. In
/usr/include/linux/audit.h is this note:
* 1800 - 1999 future kernel use (maybe integrity labels and related events)
So, we could assign the 1800 block to kernel integrity checking. I think we'd
need information access decision, creation, modification, and deletion of
integrity information/labels. We also probably need the ability to audit by
integrity, too. For a detailed audit discussion, I'd recommend linux-audit mail
list or at least cc'ing it
>I'm not sure that it's useful to have non-enforced integrity, aside from
>debugging/development use.
Hard to say. You may want to upgrade a machine and not have anything fail due to
integrity checking. I could see it being useful in much the same way that
permissive is for selinux.
>For general use, it should either be enabled or disabled.
What about immutable, too? Changing between enabled/disabled is an auditable
event, too.
-Steve
____________________________________________________________________________________
We won't tell. Get more on shows you hate to love
(and love to hate): Yahoo! TV's Guilty Pleasures list.
http://tv.yahoo.com/collections/265
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [RFC]integrity: SELinux patch
2007-07-18 15:05 ` Steve G
@ 2007-09-04 20:46 ` Mimi Zohar
2007-09-04 21:08 ` Steve Grubb
[not found] ` <1188999967.5563.2.camel@localhost.localdomain>
1 sibling, 1 reply; 23+ messages in thread
From: Mimi Zohar @ 2007-09-04 20:46 UTC (permalink / raw)
To: Steve G; +Cc: James Morris, sgrubb, Paul Moore, selinux, zohar, safford
On Wed, 2007-07-18 at 08:05 -0700, Steve G wrote:
>
> >> No, it isn't being audited, but should be. The question is what type of audit
>
> >> message would be appropriate here. It could be the normal denied/granted
> >> message, but that would be confusing as this isn't based on a permission or
> >> capability check, but an integrity error. Any suggestions how to handle this
> >> here and in the other places?
>
> MRPP places some requirements on intergrity checking. Maybe it tells you more
> information about what's required. More info:
>
> http://www.niap-ccevs.org/cc-scheme/pp/pp.cfm?id=PP_OS_ML_MR2.0_V1.91
>
> >If integrity is being enforced, then the final AVC denial should include
> >information that it was because of an integrity failure.
>
> Might ought to be an integrity audit record type rather than avc. This way
> aureport can separate it out for its summary report. In
> /usr/include/linux/audit.h is this note:
>
> * 1800 - 1999 future kernel use (maybe integrity labels and related events)
>
> So, we could assign the 1800 block to kernel integrity checking. I think we'd
> need information access decision, creation, modification, and deletion of
> integrity information/labels. We also probably need the ability to audit by
> integrity, too. For a detailed audit discussion, I'd recommend linux-audit mail
> list or at least cc'ing it
As a first take, the "integrity: API, hooks, placement and dummy provider" patch
posted on Aug. 28th, adds the following auditing integrity support.
Added to audit.h:
#define AUDIT_INTEGRITY 1800 /* Integrity verify success/failure */
#define AUDIT_INTEGRITY_ERR 1801 /* Internal integrity errors */
#define AUDIT_INTEGRITY_PCR 1802 /* PCR invalidation errors */
Added to integrity.h:
void integrity_audit(char *function, const unsigned char *fname, char *cause);
void integrity_audit_pcr(const unsigned char *fname, char *cause);
void integrity_audit_err(char *cause);
These functions are defined in security/integrity_audit.c.
Based on Stephen's request, I've removed the kernel integrity logging in
the selinux integrity patch. I've added audit calls to IMA, which log PCR
violations. The integrity_audit() calls would be made from a LIM provider
that implements integrity_verify_metadata() and integrity_verify_data().
Mimi Zohar
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [RFC]integrity: SELinux patch
2007-09-04 20:46 ` Mimi Zohar
@ 2007-09-04 21:08 ` Steve Grubb
0 siblings, 0 replies; 23+ messages in thread
From: Steve Grubb @ 2007-09-04 21:08 UTC (permalink / raw)
To: Mimi Zohar; +Cc: Steve G, James Morris, Paul Moore, selinux, zohar, safford
On Tuesday 04 September 2007 16:46:50 Mimi Zohar wrote:
> Based on Stephen's request, I've removed the kernel integrity logging in
> the selinux integrity patch. I've added audit calls to IMA, which log PCR
> violations. The integrity_audit() calls would be made from a LIM provider
> that implements integrity_verify_metadata() and integrity_verify_data().
And these were sent to linux-audit mail list for review?
-Steve
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 23+ messages in thread
[parent not found: <1188999967.5563.2.camel@localhost.localdomain>]
* Re: Integrity auditing
[not found] ` <1188999967.5563.2.camel@localhost.localdomain>
@ 2007-09-05 15:11 ` Steve Grubb
2007-09-05 19:26 ` Mimi Zohar
0 siblings, 1 reply; 23+ messages in thread
From: Steve Grubb @ 2007-09-05 15:11 UTC (permalink / raw)
To: Mimi Zohar; +Cc: zohar, safford, James Morris, Steve G, linux-audit
On Wednesday 05 September 2007 09:46:06 Mimi Zohar wrote:
> On Wed, 2007-07-18 at 08:05 -0700, Steve G wrote:
> > MRPP places some requirements on intergrity checking. Maybe it tells you
> > more information about what's required. More info:
> >
> > http://www.niap-ccevs.org/cc-scheme/pp/pp.cfm?id=PP_OS_ML_MR2.0_V1.91
This ^^^ spells out some requirements for INTEGRITY checks.
> > Might ought to be an integrity audit record type rather than avc. This
> > way aureport can separate it out for its summary report. In
> > /usr/include/linux/audit.h is this note:
> >
> > * 1800 - 1999 future kernel use (maybe integrity labels and related
> > events)
> >
> > So, we could assign the 1800 block to kernel integrity checking. I think
> > we'd need information access decision, creation, modification, and
> > deletion of integrity information/labels. We also probably need the
> > ability to audit by integrity, too. For a detailed audit discussion, I'd
> > recommend linux-audit mail list or at least cc'ing it
>
> I would assume that the integrity label would be managed by the LIM
> provider itself. In which case, does it make sense to audit the LIM
> provider's creation, modification or deletion of the integrity label stored
> as an xattr?
Yes. That is required per section FMT_MSA.1(4), assuming this hardware
assisted integrity checking code needs to go through any kind of
certification.
> IMA, a LIM provider, implements integrity_measure, which does not require
> an integrity label. It is, however, important to log/audit PCR invalidation
> errors. I propose adding the following audit numbers for integrity.
>
> Add to audit.h:
> #define AUDIT_INTEGRITY 1800 /* Integrity verify success/failure
> */ #define AUDIT_INTEGRITY_ERR 1801 /* Internal integrity errors */
> #define AUDIT_INTEGRITY_PCR 1802 /* PCR invalidation errors */
What about configuration changes to it? Can you select the hash algorithm
used? What about enable/disable of checking? Does this integrity scheme cover
only objects or does it also cover subjects? What does a typical integrity
label look like? Is there anything like a mass relabel after installation?
Are there any self-tests for the hardware or keys stored within it?
> Add to integrity.h:
> void integrity_audit(char *function, const unsigned char *fname, char
> *cause); void integrity_audit_pcr(const unsigned char *fname, char *cause);
> void integrity_audit_err(char *cause);
Actually, it would be nice to see the messages being generated to see if they
have everything needed and that they conform to audit system specs.
Thanks,
-Steve
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: Integrity auditing
2007-09-05 15:11 ` Integrity auditing Steve Grubb
@ 2007-09-05 19:26 ` Mimi Zohar
2007-09-06 17:07 ` Steve Grubb
0 siblings, 1 reply; 23+ messages in thread
From: Mimi Zohar @ 2007-09-05 19:26 UTC (permalink / raw)
To: Steve Grubb; +Cc: zohar, safford, James Morris, Steve G, linux-audit
On Wed, 2007-09-05 at 11:11 -0400, Steve Grubb wrote:
> On Wednesday 05 September 2007 09:46:06 Mimi Zohar wrote:
> > On Wed, 2007-07-18 at 08:05 -0700, Steve G wrote:
> > > MRPP places some requirements on intergrity checking. Maybe it tells you
> > > more information about what's required. More info:
> > >
> > > http://www.niap-ccevs.org/cc-scheme/pp/pp.cfm?id=PP_OS_ML_MR2.0_V1.91
>
> This ^^^ spells out some requirements for INTEGRITY checks.
>
>
> > > Might ought to be an integrity audit record type rather than avc. This
> > > way aureport can separate it out for its summary report. In
> > > /usr/include/linux/audit.h is this note:
> > >
> > > * 1800 - 1999 future kernel use (maybe integrity labels and related
> > > events)
> > >
> > > So, we could assign the 1800 block to kernel integrity checking. I think
> > > we'd need information access decision, creation, modification, and
> > > deletion of integrity information/labels. We also probably need the
> > > ability to audit by integrity, too. For a detailed audit discussion, I'd
> > > recommend linux-audit mail list or at least cc'ing it
> >
> > I would assume that the integrity label would be managed by the LIM
> > provider itself. In which case, does it make sense to audit the LIM
> > provider's creation, modification or deletion of the integrity label stored
> > as an xattr?
>
> Yes. That is required per section FMT_MSA.1(4), assuming this hardware
> assisted integrity checking code needs to go through any kind of
> certification.
Ok. For now, we are not releasing a LIM provider, which implements either
integrity_verify_metadata()/data(). Would it be better to define the
auditing msgs for monitoring the creation, deletion, modification of
integrity information/labels now or when we submit a LIM provider, which
implements these functions?
> > IMA, a LIM provider, implements integrity_measure, which does not require
> > an integrity label. It is, however, important to log/audit PCR invalidation
> > errors. I propose adding the following audit numbers for integrity.
> >
> > Add to audit.h:
> > #define AUDIT_INTEGRITY 1800 /* Integrity verify success/failure
> > */ #define AUDIT_INTEGRITY_ERR 1801 /* Internal integrity errors */
> > #define AUDIT_INTEGRITY_PCR 1802 /* PCR invalidation errors */
>
> What about configuration changes to it? Can you select the hash algorithm
> used? What about enable/disable of checking? Does this integrity scheme cover
> only objects or does it also cover subjects? What does a typical integrity
> label look like? Is there anything like a mass relabel after installation?
> Are there any self-tests for the hardware or keys stored within it?
There are two IMA boot parameters. The option "ima=" permits IMA to be
enabled/disabled. The option "ima_hash=" is used to change the default
hash method from SHA1 to MD5. There is no runtime mechanism to disable
integrity_measurements or to change the type of hash value used to extend
the PCR register. An LSM module determines which files to measure. For
SElinux, this determination is based on policy. The LSM module calls
integrity_measure() to extend the PCR value with the hash of the file.
As previously mentioned, IMA does not have an integrity label. EVM, which
was previously in the -mm tree, did implement integrity_verify_metadata/data.
EVM stored the HMAC, the integrity label, as an extended attribute called
security.evm.hmac. The HMAC was on the file's metadata, which based on
configuration, included the file's hash, the LSM xattr data, etc. There
were two ways of labeling the system, during installation or post install
using a labeling fixup program.
> > Add to integrity.h:
> > void integrity_audit(char *function, const unsigned char *fname, char
> > *cause); void integrity_audit_pcr(const unsigned char *fname, char *cause);
> > void integrity_audit_err(char *cause);
>
> Actually, it would be nice to see the messages being generated to see if they
> have everything needed and that they conform to audit system specs.
The first msg was generated by a test program attempting to read a file that
was already open for write. The second msg was generated by a test program
attempting to write a file that was already open for read, thus previously
measured.
integrity_audit_pcr() msgs:
Aug 23 17:29:02 L3X098X kernel: audit(1187904542.211:2): integrity:
invalidate pcr(measured file has writers) for pid=5864 comm="tt_read1"
name="/tmp/hello"
Aug 23 17:29:14 L3X098X kernel: audit(1187904554.964:3): integrity:
invalidate pcr(ToMToU violation) for pid=5870 comm="tt_write1"
name="/tmp/hello"
Mimi Zohar
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Integrity auditing
2007-09-05 19:26 ` Mimi Zohar
@ 2007-09-06 17:07 ` Steve Grubb
2007-09-10 20:16 ` Mimi Zohar
0 siblings, 1 reply; 23+ messages in thread
From: Steve Grubb @ 2007-09-06 17:07 UTC (permalink / raw)
To: Mimi Zohar; +Cc: zohar, safford, James Morris, Steve G, linux-audit
On Wednesday 05 September 2007 15:26:22 Mimi Zohar wrote:
> On Wed, 2007-09-05 at 11:11 -0400, Steve Grubb wrote:
> > On Wednesday 05 September 2007 09:46:06 Mimi Zohar wrote:
> Ok. For now, we are not releasing a LIM provider, which implements either
> integrity_verify_metadata()/data(). Would it be better to define the
> auditing msgs for monitoring the creation, deletion, modification of
> integrity information/labels now or when we submit a LIM provider, which
> implements these functions?
I suspect later.
> > > Add to audit.h:
> > > #define AUDIT_INTEGRITY 1800 /* Integrity verify
> > > success/failure */ #define AUDIT_INTEGRITY_ERR 1801 /* Internal
> > > integrity errors */ #define AUDIT_INTEGRITY_PCR 1802 /* PCR
> > > invalidation errors */
> >
> > What about configuration changes to it? Can you select the hash algorithm
> > used? What about enable/disable of checking? Does this integrity scheme
> > cover only objects or does it also cover subjects? What does a typical
> > integrity label look like? Is there anything like a mass relabel after
> > installation? Are there any self-tests for the hardware or keys stored
> > within it?
>
> There are two IMA boot parameters. The option "ima=" permits IMA to be
> enabled/disabled. The option "ima_hash=" is used to change the default
> hash method from SHA1 to MD5. There is no runtime mechanism to disable
> integrity_measurements or to change the type of hash value used to extend
> the PCR register.
I wonder if these should be emitted as an audit event?
> An LSM module determines which files to measure. For SElinux, this
> determination is based on policy. The LSM module calls integrity_measure()
> to extend the PCR value with the hash of the file.
>
> As previously mentioned, IMA does not have an integrity label. EVM, which
> was previously in the -mm tree, did implement
> integrity_verify_metadata/data. EVM stored the HMAC, the integrity label,
> as an extended attribute called security.evm.hmac. The HMAC was on the
> file's metadata, which based on configuration, included the file's hash,
> the LSM xattr data, etc.
If that is being accepted into mainline, I think it needs some auditing work,
too.
> There were two ways of labeling the system, during installation or post
> install using a labeling fixup program.
Shouldn't that be an auditable event?
> The first msg was generated by a test program attempting to read a file
> that was already open for write. The second msg was generated by a test
> program attempting to write a file that was already open for read, thus
> previously measured.
>
> integrity_audit_pcr() msgs:
> Aug 23 17:29:02 L3X098X kernel: audit(1187904542.211:2): integrity:
> invalidate pcr(measured file has writers) for pid=5864 comm="tt_read1"
> name="/tmp/hello"
The linux audit system has to fulfill some basic audit requirements. Its
required for each event to answer some basic questions like: who initiated
the event, what were they doing, what resource was involved, and were they
successful? Because we store millions of records, it has to as short as
possible.
Typically, we know what is happening by the record type. Where that need
further explaining, we usually have "op=something". But its kept to 1 or two
word separated by a hyphen if its important for the parsers to be able to
associate the words with an op.
The above message needs to have auid so that we know who initiated the action.
It also should have res=0 or res=1 at the end to indicate success or failure.
0 being failure and 1 being success. Also, this audit record has only a file
name in it. Because mount points can change the names of things, you should
also probably include the device and inode to help identify what is really
being reported. If selinux is enabled, this event should also have obj= and
the selinux context. The subj should be likewise recorded.
> Aug 23 17:29:14 L3X098X kernel: audit(1187904554.964:3): integrity:
> invalidate pcr(ToMToU violation) for pid=5870 comm="tt_write1"
> name="/tmp/hello"
What about examples of the other message types? This looks like its the same
kind as the above.
Thanks,
-Steve
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Integrity auditing
2007-09-06 17:07 ` Steve Grubb
@ 2007-09-10 20:16 ` Mimi Zohar
2007-09-12 13:25 ` Steve Grubb
0 siblings, 1 reply; 23+ messages in thread
From: Mimi Zohar @ 2007-09-10 20:16 UTC (permalink / raw)
To: Steve Grubb; +Cc: zohar, safford, James Morris, Steve G, linux-audit
On Thu, 2007-09-06 at 13:07 -0400, Steve Grubb wrote:
> On Wednesday 05 September 2007 15:26:22 Mimi Zohar wrote:
> > On Wed, 2007-09-05 at 11:11 -0400, Steve Grubb wrote:
> > > On Wednesday 05 September 2007 09:46:06 Mimi Zohar wrote:
> > Ok. For now, we are not releasing a LIM provider, which implements either
> > integrity_verify_metadata()/data(). Would it be better to define the
> > auditing msgs for monitoring the creation, deletion, modification of
> > integrity information/labels now or when we submit a LIM provider, which
> > implements these functions?
>
> I suspect later.
agreed.
> > > > Add to audit.h:
> > > > #define AUDIT_INTEGRITY 1800 /* Integrity verify
> > > > success/failure */ #define AUDIT_INTEGRITY_ERR 1801 /* Internal
> > > > integrity errors */ #define AUDIT_INTEGRITY_PCR 1802 /* PCR
> > > > invalidation errors */
> > >
> > > What about configuration changes to it? Can you select the hash algorithm
> > > used? What about enable/disable of checking? Does this integrity scheme
> > > cover only objects or does it also cover subjects? What does a typical
> > > integrity label look like? Is there anything like a mass relabel after
> > > installation? Are there any self-tests for the hardware or keys stored
> > > within it?
> >
> > There are two IMA boot parameters. The option "ima=" permits IMA to be
> > enabled/disabled. The option "ima_hash=" is used to change the default
> > hash method from SHA1 to MD5. There is no runtime mechanism to disable
> > integrity_measurements or to change the type of hash value used to extend
> > the PCR register.
>
> I wonder if these should be emitted as an audit event?
Ok. Instead of only logging the boot parameter errors, we will audit them.
The format will be: integrity: pid= auid= comm= op=
where op is one of:
op=ima_enabled
op=ima_not_enabled
op=hash_setup(sha1)
op=hash_setup(md5)
op=hash_setup(invalid_hash_type)
> > An LSM module determines which files to measure. For SElinux, this
> > determination is based on policy. The LSM module calls integrity_measure()
> > to extend the PCR value with the hash of the file.
> >
> > As previously mentioned, IMA does not have an integrity label. EVM, which
> > was previously in the -mm tree, did implement
> > integrity_verify_metadata/data. EVM stored the HMAC, the integrity label,
> > as an extended attribute called security.evm.hmac. The HMAC was on the
> > file's metadata, which based on configuration, included the file's hash,
> > the LSM xattr data, etc.
>
> If that is being accepted into mainline, I think it needs some auditing work,
> too.
>
> > There were two ways of labeling the system, during installation or post
> > install using a labeling fixup program.
>
> Shouldn't that be an auditable event?
Ok. For now, we decided to first work on an IMA-only solution, which does not
require labeling. Once IMA is accepted and upstreamed, we will go back and add
the EVM functionality and the additional auditing msgs.
> > The first msg was generated by a test program attempting to read a file
> > that was already open for write. The second msg was generated by a test
> > program attempting to write a file that was already open for read, thus
> > previously measured.
> >
> > integrity_audit_pcr() msgs:
> > Aug 23 17:29:02 L3X098X kernel: audit(1187904542.211:2): integrity:
> > invalidate pcr(measured file has writers) for pid=5864 comm="tt_read1"
> > name="/tmp/hello"
>
> The linux audit system has to fulfill some basic audit requirements. Its
> required for each event to answer some basic questions like: who initiated
> the event, what were they doing, what resource was involved, and were they
> successful? Because we store millions of records, it has to as short as
> possible.
>
> Typically, we know what is happening by the record type. Where that need
> further explaining, we usually have "op=something". But its kept to 1 or two
> word separated by a hyphen if its important for the parsers to be able to
> associate the words with an op.
>
> The above message needs to have auid so that we know who initiated the action.
> It also should have res=0 or res=1 at the end to indicate success or failure.
> 0 being failure and 1 being success. Also, this audit record has only a file
> name in it. Because mount points can change the names of things, you should
> also probably include the device and inode to help identify what is really
> being reported.
Ok. Based on your comments, would the following format be acceptable?
integrity: pid= auid= comm= name= dev= inode= op= res=
Although both integrity_audit() and integrity_audit_pcr() would have
the same format, integrity_audit() would be used to report the results
of integrity_verify_metadata()/data(), while integrity_audit_pcr()
would report the results of extending the PCR register.
For integrity_audit_pcr, the op would be:
op=invalidate_pcr(ToMToU)
op=invalidate_pcr(open_writers)
op=add_measure(ENOMEM)
op=add_measure(null_hash)
op=add_measure(long_digest)
op=add_measure
For integrity_audit(), the op would probably be something similar to:
op=verify_metadata
op=verify_data
> If selinux is enabled, this event should also have obj= and
> the selinux context. The subj should be likewise recorded.
This brings us back to the original question as to who should be emitting
the integrity audit msgs. If the LIM provider is emitting the audit msg,
then shouldn't it be LSM agnostic?
> > Aug 23 17:29:14 L3X098X kernel: audit(1187904554.964:3): integrity:
> > invalidate pcr(ToMToU violation) for pid=5870 comm="tt_write1"
> > name="/tmp/hello"
>
> What about examples of the other message types? This looks like its the same
> kind as the above.
There are now two message types as described above.
Mimi Zohar
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Integrity auditing
2007-09-10 20:16 ` Mimi Zohar
@ 2007-09-12 13:25 ` Steve Grubb
0 siblings, 0 replies; 23+ messages in thread
From: Steve Grubb @ 2007-09-12 13:25 UTC (permalink / raw)
To: Mimi Zohar; +Cc: zohar, safford, James Morris, Steve G, linux-audit
On Monday 10 September 2007 16:16:23 Mimi Zohar wrote:
> > I wonder if these should be emitted as an audit event?
>
> Ok. Instead of only logging the boot parameter errors, we will audit them.
> The format will be: integrity: pid= auid= comm= op=
How about:
pid= uid= auid= subj= op= comm= res=
pid, uid, comm are not really required, but they are nice to have.
> where op is one of:
> op=ima_enabled
> op=ima_not_enabled
>
> op=hash_setup(sha1)
> op=hash_setup(md5)
> op=hash_setup(invalid_hash_type)
For the last 3 messages op=setup hash=
But in a way, this really sounds like you need 2 event types. One for IMA and
one for hash selection. This is because they are doing fundamentally
different things.
> > The above message needs to have auid so that we know who initiated the
> > action. It also should have res=0 or res=1 at the end to indicate
> > success or failure. 0 being failure and 1 being success. Also, this audit
> > record has only a file name in it. Because mount points can change the
> > names of things, you should also probably include the device and inode to
> > help identify what is really being reported.
>
> Ok. Based on your comments, would the following format be acceptable?
>
> integrity: pid= auid= comm= name= dev= inode= op= res=
Sure. optionally, you could put uid= right before auid=. The selinux labels
are missing. So, I'd suggest an order like this:
pid= uid= auid= subj= op= comm= name= dev= inode= obj= res=
> Although both integrity_audit() and integrity_audit_pcr() would have
> the same format, integrity_audit() would be used to report the results
> of integrity_verify_metadata()/data(), while integrity_audit_pcr()
> would report the results of extending the PCR register.
OK
> For integrity_audit_pcr, the op would be:
> op=invalidate_pcr(ToMToU)
> op=invalidate_pcr(open_writers)
what you have in parenthesis should probably be name=value.
> op=add_measure(ENOMEM)
> op=add_measure(null_hash)
> op=add_measure(long_digest)
> op=add_measure
>
> For integrity_audit(), the op would probably be something similar to:
> op=verify_metadata
> op=verify_data
Again, this sounds like you need 3 audit event types since the messages are
trying to express something different.
> > If selinux is enabled, this event should also have obj= and
> > the selinux context. The subj should be likewise recorded.
>
> This brings us back to the original question as to who should be emitting
> the integrity audit msgs. If the LIM provider is emitting the audit msg,
> then shouldn't it be LSM agnostic?
We just went through an LSPP certification of the Linux kernel. We have labels
on everything. We really need to keep the standard since its a lot of trouble
to hunt these all down and fix next time the kernel goes through a common
criteria certification. I don't know enough about your subsystem to say who
should do it. But we should have the labels in a portable fashion. What we
were doing is something like this:
struct audit_aux_data_ipcctl *axi = (void *)aux;
audit_log_format(ab,
"ouid=%u ogid=%u mode=%x",
axi->uid, axi->gid, axi->mode);
^^^ This would be the normal part that is for CAPP. The next part optionally
does the obj if selinux is enabled:
if (axi->osid != 0) {
char *ctx = NULL;
u32 len;
if (selinux_ctxid_to_string(
axi->osid, &ctx, &len)) {
audit_log_format(ab, " osid=%u",
axi->osid);
call_panic = 1;
} else
audit_log_format(ab, " obj=%s", ctx);
kfree(ctx);
}
By doing this, people that have selinux get the labels, the people without
selinux do not have wasted space in the audit logs.
Thanks,
-Steve
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC]integrity: SELinux patch
2007-07-16 13:57 [RFC]integrity: SELinux patch Mimi Zohar
2007-07-16 18:40 ` Joshua Brindle
2007-07-16 19:23 ` Paul Moore
@ 2007-07-17 0:20 ` James Morris
2007-07-17 13:20 ` Joshua Brindle
2007-07-17 14:44 ` James Morris
2007-07-17 14:52 ` Stephen Smalley
4 siblings, 1 reply; 23+ messages in thread
From: James Morris @ 2007-07-17 0:20 UTC (permalink / raw)
To: Mimi Zohar; +Cc: selinux, Eric Paris, Stephen Smalley, zohar, safford
On Mon, 16 Jul 2007, Mimi Zohar wrote:
> +#define FILE__MEASURE 0x00100000UL
Just a logistical note, we need Eric's patch to handle undefined
permissions, so we don't break userspace with changes such as this.
- James
--
James Morris
<jmorris@namei.org>
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC]integrity: SELinux patch
2007-07-17 0:20 ` [RFC]integrity: SELinux patch James Morris
@ 2007-07-17 13:20 ` Joshua Brindle
0 siblings, 0 replies; 23+ messages in thread
From: Joshua Brindle @ 2007-07-17 13:20 UTC (permalink / raw)
To: James Morris
Cc: Mimi Zohar, selinux, Eric Paris, Stephen Smalley, zohar, safford
James Morris wrote:
> On Mon, 16 Jul 2007, Mimi Zohar wrote:
>
>
>> +#define FILE__MEASURE 0x00100000UL
>>
>
> Just a logistical note, we need Eric's patch to handle undefined
> permissions, so we don't break userspace with changes such as this.
>
I don't think this will break anything. A denial (from the policy not
having this perm) should result in no measurement. It won't stop
operations like other permission denials will.
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC]integrity: SELinux patch
2007-07-16 13:57 [RFC]integrity: SELinux patch Mimi Zohar
` (2 preceding siblings ...)
2007-07-17 0:20 ` [RFC]integrity: SELinux patch James Morris
@ 2007-07-17 14:44 ` James Morris
2007-07-18 21:33 ` Mimi Zohar
2007-07-17 14:52 ` Stephen Smalley
4 siblings, 1 reply; 23+ messages in thread
From: James Morris @ 2007-07-17 14:44 UTC (permalink / raw)
To: Mimi Zohar; +Cc: selinux, zohar, safford
On Mon, 16 Jul 2007, Mimi Zohar wrote:
> This is a first attempt to verify and measure file integrity, by
> adding the new Linux Integrity Modules(LIM) API calls to SElinux.
> We are planning on posting the corresponding LIM and IMA patches to
> LKML, but would like comments/suggestions here first, particularly
> in regards to the policy checking code in selinux_measure() called
> from selinux_inode_permission().
Can you please post the integrity code being called? (Perhaps I missed it,
or should we look at your older patches?)
One general issue is that I think there's a related need to be able to
cryptographically bind security labels to objects to protect against
inappropriate offline manipulation with or without the availability of
TPM. This is a potentially complex area, and I would not expect that you
address it directly, although I think it should be taken into
consideration, and that the integrity framework be capable of greater
generalization.
There are also potentially more (or at least, extended) models for
integrity measurement & verification, e.g. verifying code at any point
during execution.
Also, how do you see this being used in a practical, general purpose
sense? How would we convince, e.g. the Fedora project to adopt and
support this ?
Example use-cases for general corporate or personal users would certainly
be useful.
- James
--
James Morris
<jmorris@namei.org>
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [RFC]integrity: SELinux patch
2007-07-17 14:44 ` James Morris
@ 2007-07-18 21:33 ` Mimi Zohar
0 siblings, 0 replies; 23+ messages in thread
From: Mimi Zohar @ 2007-07-18 21:33 UTC (permalink / raw)
To: James Morris; +Cc: selinux, zohar, safford, sailer
On Tue, 2007-07-17 at 10:44 -0400, James Morris wrote:
> On Mon, 16 Jul 2007, Mimi Zohar wrote:
>
> > This is a first attempt to verify and measure file integrity, by
> > adding the new Linux Integrity Modules(LIM) API calls to SElinux.
> > We are planning on posting the corresponding LIM and IMA patches to
> > LKML, but would like comments/suggestions here first, particularly
> > in regards to the policy checking code in selinux_measure() called
> > from selinux_inode_permission().
>
> Can you please post the integrity code being called? (Perhaps I missed it,
> or should we look at your older patches?)
Based on comments here on the selinux mailing list, I need to make some
additional changes to the LIM patches (i.e use audit). So I'll post the
current set of LIM patches here.
Mimi
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC]integrity: SELinux patch
2007-07-16 13:57 [RFC]integrity: SELinux patch Mimi Zohar
` (3 preceding siblings ...)
2007-07-17 14:44 ` James Morris
@ 2007-07-17 14:52 ` Stephen Smalley
2007-07-18 21:43 ` Mimi Zohar
4 siblings, 1 reply; 23+ messages in thread
From: Stephen Smalley @ 2007-07-17 14:52 UTC (permalink / raw)
To: Mimi Zohar; +Cc: selinux, zohar, safford, Joshua Brindle
On Mon, 2007-07-16 at 09:57 -0400, Mimi Zohar wrote:
> This is a first attempt to verify and measure file integrity, by
> adding the new Linux Integrity Modules(LIM) API calls to SElinux.
> We are planning on posting the corresponding LIM and IMA patches to
> LKML, but would like comments/suggestions here first, particularly
> in regards to the policy checking code in selinux_measure() called
> from selinux_inode_permission().
>
> SELINUX_ENFORCE_INTEGRITY can be configured to either verify and
> enforce integrity or to just log integrity failures. The default
> is to just log integrity failures.
Wrong place, if you want permissive vs. enforcing modes for your
integrity checks, put them into your integrity module and let that
determine the result it returns to the caller.
> The integrity of the SELinux metadata is verified when the xattr
> is initially retrieved. On an integrity failure, assuming that
> integrity verification is enforced, normal error processing occurs.
>
> By default, all executables and all files that are mmap'ed executable
> are measured. This patch extends the file class with 'measure'.
As others have said, it would be better to let policy fully control what
is measured to avoid unnecessary measurements.
> Additional files can be measured in selinux_inode_permission()
> based on a FILE__MEASURE policy. (As the policy call is causing too
> many files to be measured, it is commented out.) For example, to
> measure kernel modules add:
> module ima 1.0;
>
> require {
> type insmod_t;
> type modules_object_t;
> class file measure;
> }
>
> #============= insmod_t ==============
> allow insmod_t modules_object_t:file measure;
Seems like the wrong granularity.
You just want a selector based on file type, right?
>
> Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
> ---
> Index: linux-2.6.22-rc6-mm1/security/selinux/hooks.c
> ===================================================================
> --- linux-2.6.22-rc6-mm1.orig/security/selinux/hooks.c
> +++ linux-2.6.22-rc6-mm1/security/selinux/hooks.c
> @@ -70,6 +70,7 @@
> #include <linux/audit.h>
> #include <linux/string.h>
> #include <linux/selinux.h>
> +#include <linux/integrity.h>
> #include <linux/mutex.h>
>
> #include "avc.h"
> @@ -109,6 +110,12 @@ __setup("selinux=", selinux_enabled_setu
> int selinux_enabled = 1;
> #endif
>
> +#ifdef CONFIG_SECURITY_SELINUX_ENFORCE_INTEGRITY
> +static int integrity_enforce = 1;
> +#else
> +static int integrity_enforce;
> +#endif
Belongs as part of your integrity module, not here.
> /* Original (dummy) security module. */
> static struct security_operations *original_ops = NULL;
>
> @@ -847,6 +854,7 @@ static int inode_doinit_with_dentry(stru
> char *context = NULL;
> unsigned len = 0;
> int rc = 0;
> + int status;
>
> if (isec->initialized)
> goto out;
> @@ -890,35 +898,12 @@ static int inode_doinit_with_dentry(stru
> goto out_unlock;
> }
>
> - len = INITCONTEXTLEN;
> - context = kmalloc(len, GFP_KERNEL);
> - if (!context) {
> - rc = -ENOMEM;
> + rc = integrity_verify_metadata(dentry, XATTR_NAME_SELINUX,
> + &context, &len, &status);
> + if (rc == -ENOMEM) {
Digging out your other integrity patches, I see that
dummy_verify_metadata is not equivalent to our code in a couple of
important respects:
- you always call getxattr twice to probe for the length, whereas we
only do it twice if our default length isn't sufficient. That will have
a performance impact as well as make the system more prone to error
(example: interleaving of setxattr with those two getxattr calls may
cause the length to change between them, causing the latter call to
still fail).
- you call vfs_getxattr rather than directly calling i_op->getxattr, but
the VFS code has additional processing that we specifically do not want
here (e.g. permission checking, fallback to the security module to
supply the canonical value). We only want to fetch the value from the
filesystem here for SELinux-internal use, not apply permission checking
based on the current process or loop back into SELinux again to ask for
the in-core value.
> @@ -932,6 +917,19 @@ static int inode_doinit_with_dentry(stru
> sid = sbsec->def_sid;
> rc = 0;
> } else {
> + /* Log integrity failures, if integrity enforced
> + * behave like for any other failure.
> + */
> + if (status == INTEGRITY_FAIL) {
> + printk(KERN_WARNING "%s: verify_metadata "
> + "failed for dev=%s ino=%ld\n",
> + __FUNCTION__,
> + inode->i_sb->s_id, inode->i_ino);
> + if (integrity_enforce) {
> + kfree(context);
> + goto out_unlock;
> + }
> + }
Belongs in your integrity module, and should use audit rather than
printk.
> @@ -1701,9 +1699,109 @@ static int selinux_bprm_set_security(str
> return 0;
> }
>
> -static int selinux_bprm_check_security (struct linux_binprm *bprm)
> +static inline int is_kernel_thread(struct task_struct *tsk)
> +{
> + return (!tsk->mm) ? 1 : 0;
> +}
Rationale for this special case, and argument that this test is a sound
way for catching all such cases?
> +
> +static int selinux_verify_metadata(struct dentry *dentry)
> +{
> + int rc, status;
> +
> + if (!dentry)
> + return 0;
Under what conditions can dentry be NULL here, and if it can be, why it
is "safe" to fail open by returning 0?
> +
> + rc = integrity_verify_metadata(dentry, NULL, NULL, NULL, &status);
> + if (rc == -EOPNOTSUPP)
> + return 0;
What are you verifying here?
> +static int selinux_verify_and_measure(struct dentry *dentry,
> + struct file *file,
> + char *filename, int mask)
> +{
> + int rc, status;
> +
> + if (!dentry && !file)
> + return 0;
> +
> + rc = integrity_verify_data(dentry, file, &status);
What are we verifying here?
> + if (rc < 0) {
> + printk(KERN_INFO "%s: %s verify_data failed "
> + "(rc: %d - status: %d)\n", __FUNCTION__,
> + filename, rc, status);
> + if (!integrity_enforce)
> + rc = 0;
> + goto out;
> + }
> +
> + if (status != INTEGRITY_PASS) {
> + if (!is_kernel_thread(current)) {
> + printk(KERN_INFO "%s: verify_data %s "
> + "(Integrity status: FAIL)\n",
> + __FUNCTION__, filename);
> + if (integrity_enforce) {
> + rc = -EACCES;
> + goto out;
> + }
> + }
> + }
> +
> + /* Only measure files that passed integrity verification. */
> + integrity_measure(dentry, file, filename, mask);
Are we measuring the same thing we verified? Can it change in between?
Can it change after being verified and measured before use? What good
is it?
> @@ -2252,6 +2350,30 @@ static int selinux_inode_follow_link(str
> return dentry_has_perm(current, NULL, dentry, FILE__READ);
> }
>
> +/*
> + * Measure based on a policy
> + */
> +static int selinux_measure(struct inode *inode, struct dentry *dentry)
> +{
> + int rc = 1;
> +#if 0
> + struct inode_security_struct *isec = inode->i_security;
> + struct task_security_struct *tsec = current->security;
> + struct av_decision avd;
> + struct avc_audit_data ad;
> +
> + /* This initial attempt to base on policy measures too much. */
> + rc = avc_has_perm_noaudit(tsec->sid, isec->sid, SECCLASS_FILE,
> + FILE__MEASURE, AVC_STRICT, &avd);
> + AVC_AUDIT_DATA_INIT(&ad,FS);
> + ad.u.fs.mnt = NULL;
> + ad.u.fs.dentry = dentry;
> + avc_audit(tsec->sid, isec->sid, SECCLASS_FILE, FILE__MEASURE,
> + &avd, rc, &ad);
You certainly don't want to audit these denials, since you don't want
audit2allow to ever generate them. So no call to avc_audit() at all.
Unfortunately for you, policy uses "*" in allow rules for unconfined
domains, and this means that your new permission is actually allowed by
existing policies (because the policy compiler is just turning "*" into
~0UL and likewise turning "~{ a b c}" into the complement of that set,
so the access vectors can have the bits turned on even if the permission
wasn't defined yet.
Function name is confusing as it suggests that it actually does the
measurement, not just deciding whether or not to do it.
> @@ -2265,9 +2387,26 @@ static int selinux_inode_permission(stru
> /* No permission to check. Existence test. */
> return 0;
> }
> -
> - return inode_has_perm(current, inode,
> + rc = inode_has_perm(current, inode,
> file_mask_to_av(inode->i_mode, mask), NULL);
> + if (rc != 0)
> + return rc;
> +
> + if (S_ISREG(inode->i_mode) && (mask & MAY_READ)) {
> + struct dentry *dentry;
> +
> + dentry = (!nd || !nd->dentry) ?
> + d_find_alias(inode) : nd->dentry;
NAK. Either you truly need the dentry (in which case this is the wrong
place for it) or you don't (in which case you shouldn't depend on it).
> +
> + if (dentry) {
> + if (selinux_measure(inode, dentry) == 0)
> + integrity_measure(dentry, NULL,
> + (char *)dentry->d_name.name, mask);
> + if (!nd || !nd->dentry)
> + dput(dentry);
> + }
So we measure the data at permission check time, but it changes before
it gets read by the actual data consumer (kernel or userland). What do
we gain?
--
Stephen Smalley
National Security Agency
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [RFC]integrity: SELinux patch
2007-07-17 14:52 ` Stephen Smalley
@ 2007-07-18 21:43 ` Mimi Zohar
2007-07-19 13:08 ` Stephen Smalley
0 siblings, 1 reply; 23+ messages in thread
From: Mimi Zohar @ 2007-07-18 21:43 UTC (permalink / raw)
To: Stephen Smalley; +Cc: selinux, zohar, safford, Joshua Brindle, sailer
On Tue, 2007-07-17 at 10:52 -0400, Stephen Smalley wrote:
> On Mon, 2007-07-16 at 09:57 -0400, Mimi Zohar wrote:
> > This is a first attempt to verify and measure file integrity, by
> > adding the new Linux Integrity Modules(LIM) API calls to SElinux.
> > We are planning on posting the corresponding LIM and IMA patches to
> > LKML, but would like comments/suggestions here first, particularly
> > in regards to the policy checking code in selinux_measure() called
> > from selinux_inode_permission().
> >
> > SELINUX_ENFORCE_INTEGRITY can be configured to either verify and
> > enforce integrity or to just log integrity failures. The default
> > is to just log integrity failures.
>
> Wrong place, if you want permissive vs. enforcing modes for your
> integrity checks, put them into your integrity module and let that
> determine the result it returns to the caller.
The LIM provider returns the integrity status of file metadata/data.
What is done with this information should be left up to the LSM
module as it is responsible for all access control decisions,
including how to handle integrity errors.
The SELINUX_ENFORCE_INTEGRITY option is meant for testing and
development of basing access control decisions on a new criteria
integrity.
> > The integrity of the SELinux metadata is verified when the xattr
> > is initially retrieved. On an integrity failure, assuming that
> > integrity verification is enforced, normal error processing occurs.
> >
> > By default, all executables and all files that are mmap'ed executable
> > are measured. This patch extends the file class with 'measure'.
>
> As others have said, it would be better to let policy fully control what
> is measured to avoid unnecessary measurements.
Ok.
> > Additional files can be measured in selinux_inode_permission()
> > based on a FILE__MEASURE policy. (As the policy call is causing too
> > many files to be measured, it is commented out.) For example, to
> > measure kernel modules add:
> > module ima 1.0;
> >
> > require {
> > type insmod_t;
> > type modules_object_t;
> > class file measure;
> > }
> >
> > #============= insmod_t ==============
> > allow insmod_t modules_object_t:file measure;
>
> Seems like the wrong granularity.
> You just want a selector based on file type, right?
Initially I thought that was the case, but perhaps the
only times kernel modules should be measured is when they're
actually being insmod/modprobe.
> > Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
> > ---
> > Index: linux-2.6.22-rc6-mm1/security/selinux/hooks.c
> > ===================================================================
> > --- linux-2.6.22-rc6-mm1.orig/security/selinux/hooks.c
> > +++ linux-2.6.22-rc6-mm1/security/selinux/hooks.c
> > @@ -70,6 +70,7 @@
> > #include <linux/audit.h>
> > #include <linux/string.h>
> > #include <linux/selinux.h>
> > +#include <linux/integrity.h>
> > #include <linux/mutex.h>
> >
> > #include "avc.h"
> > @@ -109,6 +110,12 @@ __setup("selinux=", selinux_enabled_setu
> > int selinux_enabled = 1;
> > #endif
> >
> > +#ifdef CONFIG_SECURITY_SELINUX_ENFORCE_INTEGRITY
> > +static int integrity_enforce = 1;
> > +#else
> > +static int integrity_enforce;
> > +#endif
>
> Belongs as part of your integrity module, not here.
Discussed above.
> > /* Original (dummy) security module. */
> > static struct security_operations *original_ops = NULL;
> >
> > @@ -847,6 +854,7 @@ static int inode_doinit_with_dentry(stru
> > char *context = NULL;
> > unsigned len = 0;
> > int rc = 0;
> > + int status;
> >
> > if (isec->initialized)
> > goto out;
> > @@ -890,35 +898,12 @@ static int inode_doinit_with_dentry(stru
> > goto out_unlock;
> > }
> >
> > - len = INITCONTEXTLEN;
> > - context = kmalloc(len, GFP_KERNEL);
> > - if (!context) {
> > - rc = -ENOMEM;
> > + rc = integrity_verify_metadata(dentry, XATTR_NAME_SELINUX,
> > + &context, &len, &status);
> > + if (rc == -ENOMEM) {
>
> Digging out your other integrity patches, I see that
> dummy_verify_metadata is not equivalent to our code in a couple of
> important respects:
> - you always call getxattr twice to probe for the length, whereas we
> only do it twice if our default length isn't sufficient. That will have
> a performance impact as well as make the system more prone to error
> (example: interleaving of setxattr with those two getxattr calls may
> cause the length to change between them, causing the latter call to
> still fail).
Ok, I will look into changing this behavior.
> - you call vfs_getxattr rather than directly calling i_op->getxattr, but
> the VFS code has additional processing that we specifically do not want
> here (e.g. permission checking, fallback to the security module to
> supply the canonical value). We only want to fetch the value from the
> filesystem here for SELinux-internal use, not apply permission checking
> based on the current process or loop back into SELinux again to ask for
> the in-core value.
The current version of the dummy integrity provider, which has not been
posted, does not use vfs_getxattr. Will post the current version here.
> > @@ -932,6 +917,19 @@ static int inode_doinit_with_dentry(stru
> > sid = sbsec->def_sid;
> > rc = 0;
> > } else {
> > + /* Log integrity failures, if integrity enforced
> > + * behave like for any other failure.
> > + */
> > + if (status == INTEGRITY_FAIL) {
> > + printk(KERN_WARNING "%s: verify_metadata "
> > + "failed for dev=%s ino=%ld\n",
> > + __FUNCTION__,
> > + inode->i_sb->s_id, inode->i_ino);
> > + if (integrity_enforce) {
> > + kfree(context);
> > + goto out_unlock;
> > + }
> > + }
>
> Belongs in your integrity module, and should use audit rather than
> printk.
Discussed above. Will look into using the audit commands as Paul Moore
and Steve Grubb suggested.
> > @@ -1701,9 +1699,109 @@ static int selinux_bprm_set_security(str
> > return 0;
> > }
> >
> > -static int selinux_bprm_check_security (struct linux_binprm *bprm)
> > +static inline int is_kernel_thread(struct task_struct *tsk)
> > +{
> > + return (!tsk->mm) ? 1 : 0;
> > +}
>
> Rationale for this special case, and argument that this test is a sound
> way for catching all such cases?
When based on a policy, this and other code, will be unnecessary.
> > +static int selinux_verify_metadata(struct dentry *dentry)
> > +{
> > + int rc, status;
> > +
> > + if (!dentry)
> > + return 0;
>
> Under what conditions can dentry be NULL here, and if it can be, why it
> is "safe" to fail open by returning 0?
>
> > +
> > + rc = integrity_verify_metadata(dentry, NULL, NULL, NULL, &status);
> > + if (rc == -EOPNOTSUPP)
> > + return 0;
>
> What are you verifying here?
The EOPNOTSUPP is for those filesystems that do not support extended
attributes. The call verifies the integrity of the metadata in general,
not a specific extended xattr.
> > +static int selinux_verify_and_measure(struct dentry *dentry,
> > + struct file *file,
> > + char *filename, int mask)
> > +{
> > + int rc, status;
> > +
> > + if (!dentry && !file)
> > + return 0;
> > +
> > + rc = integrity_verify_data(dentry, file, &status);
>
> What are we verifying here?
The actual file data.
> > + if (rc < 0) {
> > + printk(KERN_INFO "%s: %s verify_data failed "
> > + "(rc: %d - status: %d)\n", __FUNCTION__,
> > + filename, rc, status);
> > + if (!integrity_enforce)
> > + rc = 0;
> > + goto out;
> > + }
> > +
> > + if (status != INTEGRITY_PASS) {
> > + if (!is_kernel_thread(current)) {
> > + printk(KERN_INFO "%s: verify_data %s "
> > + "(Integrity status: FAIL)\n",
> > + __FUNCTION__, filename);
> > + if (integrity_enforce) {
> > + rc = -EACCES;
> > + goto out;
> > + }
> > + }
> > + }
> > +
> > + /* Only measure files that passed integrity verification. */
> > + integrity_measure(dentry, file, filename, mask);
>
> Are we measuring the same thing we verified? Can it change in between?
> Can it change after being verified and measured before use? What good
> is it?
The OS itself protects against an executable file open for execute from
being modified. As for other files, you are correct, the file could be
modified before being read. The current source forge version of IMA
tracks possible violations. This is on my todo list.
> > @@ -2252,6 +2350,30 @@ static int selinux_inode_follow_link(str
> > return dentry_has_perm(current, NULL, dentry, FILE__READ);
> > }
> >
> > +/*
> > + * Measure based on a policy
> > + */
> > +static int selinux_measure(struct inode *inode, struct dentry *dentry)
> > +{
> > + int rc = 1;
> > +#if 0
> > + struct inode_security_struct *isec = inode->i_security;
> > + struct task_security_struct *tsec = current->security;
> > + struct av_decision avd;
> > + struct avc_audit_data ad;
> > +
> > + /* This initial attempt to base on policy measures too much. */
> > + rc = avc_has_perm_noaudit(tsec->sid, isec->sid, SECCLASS_FILE,
> > + FILE__MEASURE, AVC_STRICT, &avd);
> > + AVC_AUDIT_DATA_INIT(&ad,FS);
> > + ad.u.fs.mnt = NULL;
> > + ad.u.fs.dentry = dentry;
> > + avc_audit(tsec->sid, isec->sid, SECCLASS_FILE, FILE__MEASURE,
> > + &avd, rc, &ad);
>
> You certainly don't want to audit these denials, since you don't want
> audit2allow to ever generate them. So no call to avc_audit() at all.
Correct, I certainly don't want to audit the denials. If I did I would
have used avc_has_perm(). The audit call is only here to help
understand what is being measured.
> Unfortunately for you, policy uses "*" in allow rules for unconfined
> domains, and this means that your new permission is actually allowed by
> existing policies (because the policy compiler is just turning "*" into
> ~0UL and likewise turning "~{ a b c}" into the complement of that set,
> so the access vectors can have the bits turned on even if the permission
> wasn't defined yet.
Thank you for the explanation. At OLS, there was a suggestion to use
avc_has_perm_noaudit(). As this is not measuring the appropriate files,
do you, or anyone else, have any other recommendations?
> Function name is confusing as it suggests that it actually does the
> measurement, not just deciding whether or not to do it.
Ok.
> > @@ -2265,9 +2387,26 @@ static int selinux_inode_permission(stru
> > /* No permission to check. Existence test. */
> > return 0;
> > }
> > -
> > - return inode_has_perm(current, inode,
> > + rc = inode_has_perm(current, inode,
> > file_mask_to_av(inode->i_mode, mask), NULL);
> > + if (rc != 0)
> > + return rc;
> > +
> > + if (S_ISREG(inode->i_mode) && (mask & MAY_READ)) {
> > + struct dentry *dentry;
> > +
> > + dentry = (!nd || !nd->dentry) ?
> > + d_find_alias(inode) : nd->dentry;
>
> NAK. Either you truly need the dentry (in which case this is the wrong
> place for it) or you don't (in which case you shouldn't depend on it).
Wouldn't it depend on how you are using the dentry? In this case,
the dentry is used to get a file handle in order to measure the file.
> > +
> > + if (dentry) {
> > + if (selinux_measure(inode, dentry) == 0)
> > + integrity_measure(dentry, NULL,
> > + (char *)dentry->d_name.name, mask);
> > + if (!nd || !nd->dentry)
> > + dput(dentry);
> > + }
>
> So we measure the data at permission check time, but it changes before
> it gets read by the actual data consumer (kernel or userland). What do
> we gain?
Discussed above.
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [RFC]integrity: SELinux patch
2007-07-18 21:43 ` Mimi Zohar
@ 2007-07-19 13:08 ` Stephen Smalley
2007-07-20 18:57 ` Mimi Zohar
0 siblings, 1 reply; 23+ messages in thread
From: Stephen Smalley @ 2007-07-19 13:08 UTC (permalink / raw)
To: Mimi Zohar; +Cc: selinux, zohar, safford, Joshua Brindle, sailer, James Morris
On Wed, 2007-07-18 at 17:43 -0400, Mimi Zohar wrote:
> On Tue, 2007-07-17 at 10:52 -0400, Stephen Smalley wrote:
> > On Mon, 2007-07-16 at 09:57 -0400, Mimi Zohar wrote:
> > > This is a first attempt to verify and measure file integrity, by
> > > adding the new Linux Integrity Modules(LIM) API calls to SElinux.
> > > We are planning on posting the corresponding LIM and IMA patches to
> > > LKML, but would like comments/suggestions here first, particularly
> > > in regards to the policy checking code in selinux_measure() called
> > > from selinux_inode_permission().
> > >
> > > SELINUX_ENFORCE_INTEGRITY can be configured to either verify and
> > > enforce integrity or to just log integrity failures. The default
> > > is to just log integrity failures.
> >
> > Wrong place, if you want permissive vs. enforcing modes for your
> > integrity checks, put them into your integrity module and let that
> > determine the result it returns to the caller.
>
> The LIM provider returns the integrity status of file metadata/data.
> What is done with this information should be left up to the LSM
> module as it is responsible for all access control decisions,
> including how to handle integrity errors.
>
> The SELINUX_ENFORCE_INTEGRITY option is meant for testing and
> development of basing access control decisions on a new criteria
> integrity.
There is nothing you are doing with it that can't be done transparently
by the integrity module itself, thereby simplifying the callers. It is
a global permissive vs. enforcing mode for integrity, nothing
selinux-specific.
> > > Additional files can be measured in selinux_inode_permission()
> > > based on a FILE__MEASURE policy. (As the policy call is causing too
> > > many files to be measured, it is commented out.) For example, to
> > > measure kernel modules add:
> > > module ima 1.0;
> > >
> > > require {
> > > type insmod_t;
> > > type modules_object_t;
> > > class file measure;
> > > }
> > >
> > > #============= insmod_t ==============
> > > allow insmod_t modules_object_t:file measure;
> >
> > Seems like the wrong granularity.
> > You just want a selector based on file type, right?
>
> Initially I thought that was the case, but perhaps the
> only times kernel modules should be measured is when they're
> actually being insmod/modprobe.
For that particular case, I think you'd do it via a new hook in the
kernel module loading code, making sure that you are verifying/measuring
the same data that gets used. More along the lines of the MODSIGN
patches posted a while ago on lkml and used in Red Hat kernels.
> > > Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
> > > ---
> > > Index: linux-2.6.22-rc6-mm1/security/selinux/hooks.c
> > > ===================================================================
> > > --- linux-2.6.22-rc6-mm1.orig/security/selinux/hooks.c
> > > +++ linux-2.6.22-rc6-mm1/security/selinux/hooks.c
> > > @@ -70,6 +70,7 @@
> > > #include <linux/audit.h>
> > > #include <linux/string.h>
> > > #include <linux/selinux.h>
> > > +#include <linux/integrity.h>
> > > #include <linux/mutex.h>
> > >
> > > #include "avc.h"
> > > @@ -109,6 +110,12 @@ __setup("selinux=", selinux_enabled_setu
> > > int selinux_enabled = 1;
> > > #endif
> > >
> > > +#ifdef CONFIG_SECURITY_SELINUX_ENFORCE_INTEGRITY
> > > +static int integrity_enforce = 1;
> > > +#else
> > > +static int integrity_enforce;
> > > +#endif
> >
> > Belongs as part of your integrity module, not here.
>
> Discussed above.
But I still don't agree. I don't want multiple permissive vs. enforcing
modes defined by selinux itself, particularly for something that it
isn't computing anyway.
> > - you call vfs_getxattr rather than directly calling i_op->getxattr, but
> > the VFS code has additional processing that we specifically do not want
> > here (e.g. permission checking, fallback to the security module to
> > supply the canonical value). We only want to fetch the value from the
> > filesystem here for SELinux-internal use, not apply permission checking
> > based on the current process or loop back into SELinux again to ask for
> > the in-core value.
>
> The current version of the dummy integrity provider, which has not been
> posted, does not use vfs_getxattr. Will post the current version here.
Ok, but you should update the comment to note that it isn't about the
vfs layer being available (not sure what you mean there), but rather
about avoiding extraneous processing that should only be applied upon
userspace access to the xattr (like permission checking based on the
current process' credentials and giving the security module a chance to
provide or canonicalize the result) and not upon a kernel-internal
request for it.
> > Are we measuring the same thing we verified? Can it change in between?
> > Can it change after being verified and measured before use? What good
> > is it?
>
> The OS itself protects against an executable file open for execute from
> being modified. As for other files, you are correct, the file could be
> modified before being read. The current source forge version of IMA
> tracks possible violations. This is on my todo list.
So you are assuming that your measurement and verification calls are
happening after deny_write_access() has occurred; should be noted.
For other files, should be handled by putting the verify+measure at the
same point where the data is being loaded, acting on the same copy of
the data in memory rather than done just upon open-time permission
checking.
> > You certainly don't want to audit these denials, since you don't want
> > audit2allow to ever generate them. So no call to avc_audit() at all.
>
> Correct, I certainly don't want to audit the denials. If I did I would
> have used avc_has_perm(). The audit call is only here to help
> understand what is being measured.
There is no difference between calling avc_has_perm() vs.
avc_has_perm_noaudit()+avc_audit(), so by doing both, you are
effectively doing the former. I guess you mean you'll drop the
avc_audit() call for real use.
> > Unfortunately for you, policy uses "*" in allow rules for unconfined
> > domains, and this means that your new permission is actually allowed by
> > existing policies (because the policy compiler is just turning "*" into
> > ~0UL and likewise turning "~{ a b c}" into the complement of that set,
> > so the access vectors can have the bits turned on even if the permission
> > wasn't defined yet.
>
> Thank you for the explanation. At OLS, there was a suggestion to use
> avc_has_perm_noaudit(). As this is not measuring the appropriate files,
> do you, or anyone else, have any other recommendations?
Defining a new class for this purpose will free you from having any
legacy policies implicitly granting the permission.
> > > + rc = inode_has_perm(current, inode,
> > > file_mask_to_av(inode->i_mode, mask), NULL);
> > > + if (rc != 0)
> > > + return rc;
> > > +
> > > + if (S_ISREG(inode->i_mode) && (mask & MAY_READ)) {
> > > + struct dentry *dentry;
> > > +
> > > + dentry = (!nd || !nd->dentry) ?
> > > + d_find_alias(inode) : nd->dentry;
> >
> > NAK. Either you truly need the dentry (in which case this is the wrong
> > place for it) or you don't (in which case you shouldn't depend on it).
>
> Wouldn't it depend on how you are using the dentry? In this case,
> the dentry is used to get a file handle in order to measure the file.
You don't want your code to sometimes succeed in measuring and sometimes
not depending on whether you happen to have a dentry available for the
inode.
> > > +
> > > + if (dentry) {
> > > + if (selinux_measure(inode, dentry) == 0)
> > > + integrity_measure(dentry, NULL,
> > > + (char *)dentry->d_name.name, mask);
> > > + if (!nd || !nd->dentry)
> > > + dput(dentry);
> > > + }
> >
> > So we measure the data at permission check time, but it changes before
> > it gets read by the actual data consumer (kernel or userland). What do
> > we gain?
>
> Discussed above.
In general, I'm not sure that you are doing measurement and verification
at the right points, or that the LSM hooks fit well for placement.
--
Stephen Smalley
National Security Agency
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [RFC]integrity: SELinux patch
2007-07-19 13:08 ` Stephen Smalley
@ 2007-07-20 18:57 ` Mimi Zohar
0 siblings, 0 replies; 23+ messages in thread
From: Mimi Zohar @ 2007-07-20 18:57 UTC (permalink / raw)
To: Stephen Smalley
Cc: selinux, zohar, safford, Joshua Brindle, sailer, James Morris
On Thu, 2007-07-19 at 09:08 -0400, Stephen Smalley wrote:
> On Wed, 2007-07-18 at 17:43 -0400, Mimi Zohar wrote:
> > On Tue, 2007-07-17 at 10:52 -0400, Stephen Smalley wrote:
> > > On Mon, 2007-07-16 at 09:57 -0400, Mimi Zohar wrote:
> > > > This is a first attempt to verify and measure file integrity, by
> > > > adding the new Linux Integrity Modules(LIM) API calls to SElinux.
> > > > We are planning on posting the corresponding LIM and IMA patches to
> > > > LKML, but would like comments/suggestions here first, particularly
> > > > in regards to the policy checking code in selinux_measure() called
> > > > from selinux_inode_permission().
> > > >
> > > > SELINUX_ENFORCE_INTEGRITY can be configured to either verify and
> > > > enforce integrity or to just log integrity failures. The default
> > > > is to just log integrity failures.
> > >
> > > Wrong place, if you want permissive vs. enforcing modes for your
> > > integrity checks, put them into your integrity module and let that
> > > determine the result it returns to the caller.
> >
> > The LIM provider returns the integrity status of file metadata/data.
> > What is done with this information should be left up to the LSM
> > module as it is responsible for all access control decisions,
> > including how to handle integrity errors.
> >
> > The SELINUX_ENFORCE_INTEGRITY option is meant for testing and
> > development of basing access control decisions on a new criteria
> > integrity.
>
> There is nothing you are doing with it that can't be done transparently
> by the integrity module itself, thereby simplifying the callers. It is
> a global permissive vs. enforcing mode for integrity, nothing
> selinux-specific.
The difference between having the option in the LSM module vs. the
integrity provider is the completeness of the error message reported.
I have no problem moving the option into the integrity module, but as
the integrity provider doesn't implement access control, the name
enforce_integrity obviously would be inappropriate, perhaps
integrity_logmode. Other LSM modules would still have the option to
implement an integrity_enforce option.
> > > - you call vfs_getxattr rather than directly calling i_op->getxattr, but
> > > the VFS code has additional processing that we specifically do not want
> > > here (e.g. permission checking, fallback to the security module to
> > > supply the canonical value). We only want to fetch the value from the
> > > filesystem here for SELinux-internal use, not apply permission checking
> > > based on the current process or loop back into SELinux again to ask for
> > > the in-core value.
> >
> > The current version of the dummy integrity provider, which has not been
> > posted, does not use vfs_getxattr. Will post the current version here.
>
> Ok, but you should update the comment to note that it isn't about the
> vfs layer being available (not sure what you mean there), but rather
> about avoiding extraneous processing that should only be applied upon
> userspace access to the xattr (like permission checking based on the
> current process' credentials and giving the security module a chance to
> provide or canonicalize the result) and not upon a kernel-internal
> request for it.
Ok
> > > Are we measuring the same thing we verified? Can it change in between?
> > > Can it change after being verified and measured before use? What good
> > > is it?
> >
> > The OS itself protects against an executable file open for execute from
> > being modified. As for other files, you are correct, the file could be
> > modified before being read. The current source forge version of IMA
> > tracks possible violations. This is on my todo list.
>
> So you are assuming that your measurement and verification calls are
> happening after deny_write_access() has occurred; should be noted.
Will do.
> For other files, should be handled by putting the verify+measure at the
> same point where the data is being loaded, acting on the same copy of
> the data in memory rather than done just upon open-time permission
> checking.
Today, if the LSM module wants to know that the file has unequivocally
changed, then yes we would need to do as you suggested. But as we are
not attempting to prevent the file from actually being changed, we
will note it, with a violation and in the PCR, that the file has
definitively changed or could have possibly been changed. When
the mtime bug is fixed, all of the reported violations will be
definitive in that the file changed. This will not imply anything
as to when the change took place(before, during, or after being
read).
> > > > + rc = inode_has_perm(current, inode,
> > > > file_mask_to_av(inode->i_mode, mask), NULL);
> > > > + if (rc != 0)
> > > > + return rc;
> > > > +
> > > > + if (S_ISREG(inode->i_mode) && (mask & MAY_READ)) {
> > > > + struct dentry *dentry;
> > > > +
> > > > + dentry = (!nd || !nd->dentry) ?
> > > > + d_find_alias(inode) : nd->dentry;
> > >
> > > NAK. Either you truly need the dentry (in which case this is the wrong
> > > place for it) or you don't (in which case you shouldn't depend on it).
> >
> > Wouldn't it depend on how you are using the dentry? In this case,
> > the dentry is used to get a file handle in order to measure the file.
>
> You don't want your code to sometimes succeed in measuring and sometimes
> not depending on whether you happen to have a dentry available for the
> inode.
The call to integrity_measure() is in the correct place. Just that it
should be possible to call integrity_measure() with an inode, as well as
with a dentry or file when available.
> > > > +
> > > > + if (dentry) {
> > > > + if (selinux_measure(inode, dentry) == 0)
> > > > + integrity_measure(dentry, NULL,
> > > > + (char *)dentry->d_name.name, mask);
> > > > + if (!nd || !nd->dentry)
> > > > + dput(dentry);
> > > > + }
> > >
> > > So we measure the data at permission check time, but it changes before
> > > it gets read by the actual data consumer (kernel or userland). What do
> > > we gain?
> >
> > Discussed above.
>
> In general, I'm not sure that you are doing measurement and verification
> at the right points, or that the LSM hooks fit well for placement.
The integrity subsystem provides a framework for an integrity provider
to maintain the data/metadata file integrity status, which an LSM module
can request when and as desired. The LSM module can then use the results
as another criteria on which to base access control decisions. It is left
up to the LSM module to decide when it wants to call integrity_measure.
As you suggested there might be additional places from which an LSM module
would want to verify and measure the data, but the current LSM hooks
provide a good starting point.
Mimi Zohar
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 23+ messages in thread