[RFC]integrity: SELinux patch

[RFC]integrity: SELinux patch

[Mimi Zohar]

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.

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:
	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.

Re: [RFC]integrity: SELinux patch

[Joshua Brindle]

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.

Re: [RFC]integrity: SELinux patch

[Mimi Zohar]

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.

Re: [RFC]integrity: SELinux patch

[Paul Moore]

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.

Re: [RFC]integrity: SELinux patch

[Mimi Zohar]

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.

Re: [RFC]integrity: SELinux patch

[Paul Moore]

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.

Re: [RFC]integrity: SELinux patch

[James Morris]

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.

Re: [RFC]integrity: SELinux patch

[Steve G]

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

Re: [RFC]integrity: SELinux patch

[Mimi Zohar]

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.

Re: [RFC]integrity: SELinux patch

[Steve Grubb]

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.

Re: Integrity auditing

[Steve Grubb]

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

Re: Integrity auditing

[Mimi Zohar]

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

Re: Integrity auditing

[Steve Grubb]

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

Re: Integrity auditing

[Mimi Zohar]

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

Re: Integrity auditing

[Steve Grubb]

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

Re: [RFC]integrity: SELinux patch

[James Morris]

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.

Re: [RFC]integrity: SELinux patch

[Joshua Brindle]

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.

Re: [RFC]integrity: SELinux patch

[James Morris]

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.

Re: [RFC]integrity: SELinux patch

[Mimi Zohar]

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.

Re: [RFC]integrity: SELinux patch

[Stephen Smalley]

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.

Re: [RFC]integrity: SELinux patch

[Mimi Zohar]

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.

Re: [RFC]integrity: SELinux patch

[Stephen Smalley]

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.

Re: [RFC]integrity: SELinux patch

[Mimi Zohar]

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.

[RFC]integrity: SELinux patch

[Mimi Zohar]

This is a second attempt to verify and measure file integrity, by
adding the new Linux Integrity Modules(LIM) API calls to SElinux.
This posting addresses comments previously made on this list. 
I will also post the current set of LIM patches, as well as an
initial integrity.te example. 

The integrity of the SELinux metadata is verified when the xattr
is initially retrieved.  On an integrity failure, normal selinux 
error processing occurs.

This patch defines a new 'integrity' class with the permission 
'measure'.  Measurement calls are made in selinux_file_mmap(), 
selinux_bprm_check_security, and selinux_inode_permission(),
based on policy.  (Additional calls might be required.)

Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
---
Index: linux-2.6.23-rc3-mm1/security/selinux/hooks.c
===================================================================
--- linux-2.6.23-rc3-mm1.orig/security/selinux/hooks.c
+++ linux-2.6.23-rc3-mm1/security/selinux/hooks.c
@@ -69,6 +69,7 @@
 #include <linux/audit.h>
 #include <linux/string.h>
 #include <linux/selinux.h>
+#include <linux/integrity.h>
 #include <linux/mutex.h>
 
 #include "avc.h"
@@ -844,6 +845,7 @@ static int inode_doinit_with_dentry(stru
 	char *context = NULL;
 	unsigned len = 0;
 	int rc = 0;
+	int status;
 
 	if (isec->initialized)
 		goto out;
@@ -888,34 +890,12 @@ static int inode_doinit_with_dentry(stru
 		}
 
 		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) {
@@ -929,6 +909,14 @@ static int inode_doinit_with_dentry(stru
 			sid = sbsec->def_sid;
 			rc = 0;
 		} else {
+			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);
+				kfree(context);
+				goto out_unlock;
+			}
 			rc = security_context_to_sid_default(context, rc, &sid,
 			                                     sbsec->def_sid);
 			if (rc) {
@@ -1698,9 +1686,98 @@ static int selinux_bprm_set_security(str
 	return 0;
 }
 
-static int selinux_bprm_check_security (struct linux_binprm *bprm)
+static int selinux_verify_metadata(struct dentry *dentry)
 {
-	return secondary_ops->bprm_check_security(bprm);
+	int rc, status;
+
+	if (!dentry)
+		return 0;
+
+	rc = integrity_verify_metadata(dentry, NULL, NULL, NULL, &status);
+	if (rc == -EOPNOTSUPP)
+		return 0;
+	if (rc < 0)
+		goto out;
+
+	if (status != INTEGRITY_PASS) 	/* FAIL | NO_LABEL */
+		rc = -EACCES;
+out:
+	return rc;
+}
+
+static int selinux_verify_data(struct dentry *dentry, struct file *file)
+{
+	int rc, status;
+
+	if (!dentry && !file)
+		return 0;
+
+	rc = integrity_verify_data(dentry, file, &status);
+	if (rc < 0)
+		return 0;
+
+	if (status != INTEGRITY_PASS)
+		rc = -EACCES;
+
+	return rc;
+}
+
+/*
+ * Measure based on new 'integrity' class policy
+ */
+static void selinux_measure(struct inode *inode, struct dentry *dentry,
+				struct file *file, char *filename,
+				int mask)
+{
+	int rc = 1;
+	struct inode_security_struct *isec = inode->i_security;
+	struct task_security_struct *tsec = current->security;
+	struct av_decision avd;
+
+	if (!S_ISREG(inode->i_mode))
+		return;
+
+	rc = avc_has_perm_noaudit(tsec->sid, isec->sid, SECCLASS_INTEGRITY,
+				  INTEGRITY__MEASURE, AVC_STRICT, &avd);
+	if (rc == 0)
+		integrity_measure(inode, dentry, file, filename, mask);
+#if 0
+	else { /* Only used for debugging */
+		struct avc_audit_data ad;
+
+		AVC_AUDIT_DATA_INIT(&ad,FS);
+		ad.u.fs.mnt = NULL;
+		ad.u.fs.dentry = dentry;
+		avc_audit(tsec->sid, isec->sid, SECCLASS_INTEGRITY,
+			INTEGRITY__MEASURE, &avd, rc, &ad);
+	}
+#endif
+	return;
+}
+
+/* The OS protects against an executable file, already open for write,
+ * from being executed in deny_write_access() and an executable file
+ * already open for execute, from being modified in get_write_access().
+ * So we can be certain that what we verify and measure here is actually
+ * what is being executed.
+ */
+static int selinux_bprm_check_security(struct linux_binprm *bprm)
+{
+	struct dentry *dentry = bprm->file->f_dentry;
+	int rc;
+
+	rc = secondary_ops->bprm_check_security(bprm);
+	if (rc != 0)
+		return rc;
+
+	rc = selinux_verify_metadata(dentry);
+	if (rc == 0) {
+		rc = selinux_verify_data(dentry, bprm->file);
+		if (rc == 0)
+			selinux_measure(dentry->d_inode, dentry, bprm->file,
+					bprm->filename, MAY_EXEC);
+	}
+	return rc;
 }
 

@@ -2249,6 +2326,30 @@ static int selinux_inode_follow_link(str
 	return dentry_has_perm(current, NULL, dentry, FILE__READ);
 }
 
+static char *get_fname(struct dentry *dentry, struct vfsmount *mnt,
+			char **buf)
+{
+	char *fname = NULL;
+	char *path = NULL;
+
+	path = (char *)__get_free_page(GFP_KERNEL);
+	if (path) {
+		fname = d_path(dentry, mnt, path, PAGE_SIZE);
+		*buf = path;
+	}
+
+	if (!fname)	/* no choice, use short name */
+		fname = (!dentry->d_name.name) ?
+		    (char *)dentry->d_iname : (char *)dentry->d_name.name;
+	return fname;
+}
+
+static void free_fname(char *path)
+{
+	if (path)
+		free_page((unsigned long)path);
+}
+
 static int selinux_inode_permission(struct inode *inode, int mask,
 				    struct nameidata *nd)
 {
@@ -2263,8 +2364,29 @@ static int selinux_inode_permission(stru
 		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 (mask & ~MAY_EXEC) { /* measure executables later */
+		struct dentry *dentry = NULL;
+		char *path = NULL;
+		char *fname = NULL;
+
+		/* The file name is not required, but only a hint.
+		 * When possible, supply a fully qualified path name.
+		 */
+		if (nd) {
+			dentry = nd->dentry;
+			fname = get_fname(nd->dentry, nd->mnt, &path);
+		}
+
+		selinux_measure(inode, dentry, NULL, fname, mask);
+		if (path)
+			free_fname(path);
+	}
+	return rc;
 }
 
 static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
@@ -2585,8 +2707,18 @@ static int selinux_file_mmap(struct file
 	if (selinux_checkreqprot)
 		prot = reqprot;
 
-	return file_map_prot_check(file, prot,
-				   (flags & MAP_TYPE) == MAP_SHARED);
+	rc = file_map_prot_check(file, prot, (flags & MAP_TYPE) == MAP_SHARED);
+	if (file && file->f_dentry && rc == 0) {
+		rc = selinux_verify_metadata(file->f_dentry);
+		if (rc == 0) {
+			rc = selinux_verify_data(NULL, file);
+			if (rc == 0)
+				selinux_measure(file->f_dentry->d_inode,
+						  file->f_dentry, file,
+						  NULL, MAY_EXEC);
+		}
+	}
+	return rc;
 }
 
 static int selinux_file_mprotect(struct vm_area_struct *vma,
@@ -2628,7 +2760,6 @@ static int selinux_file_mprotect(struct 
 			return rc;
 	}
 #endif
-
 	return file_map_prot_check(vma->vm_file, prot, vma->vm_flags&VM_SHARED);
 }
 
Index: linux-2.6.23-rc3-mm1/security/selinux/include/av_permissions.h
===================================================================
--- linux-2.6.23-rc3-mm1.orig/security/selinux/include/av_permissions.h
+++ linux-2.6.23-rc3-mm1/security/selinux/include/av_permissions.h
@@ -824,3 +824,4 @@
 #define DCCP_SOCKET__NODE_BIND                    0x00400000UL
 #define DCCP_SOCKET__NAME_CONNECT                 0x00800000UL
 #define MEMPROTECT__MMAP_ZERO                     0x00000001UL
+#define INTEGRITY__MEASURE                        0x00000001UL
Index: linux-2.6.23-rc3-mm1/security/selinux/include/av_perm_to_string.h
===================================================================
--- linux-2.6.23-rc3-mm1.orig/security/selinux/include/av_perm_to_string.h
+++ linux-2.6.23-rc3-mm1/security/selinux/include/av_perm_to_string.h
@@ -159,3 +159,4 @@
    S_(SECCLASS_DCCP_SOCKET, DCCP_SOCKET__NODE_BIND, "node_bind")
    S_(SECCLASS_DCCP_SOCKET, DCCP_SOCKET__NAME_CONNECT, "name_connect")
    S_(SECCLASS_MEMPROTECT, MEMPROTECT__MMAP_ZERO, "mmap_zero")
+   S_(SECCLASS_INTEGRITY, INTEGRITY__MEASURE, "measure")
Index: linux-2.6.23-rc3-mm1/security/selinux/include/flask.h
===================================================================
--- linux-2.6.23-rc3-mm1.orig/security/selinux/include/flask.h
+++ linux-2.6.23-rc3-mm1/security/selinux/include/flask.h
@@ -50,6 +50,7 @@
 #define SECCLASS_KEY                                     58
 #define SECCLASS_DCCP_SOCKET                             60
 #define SECCLASS_MEMPROTECT                              61
+#define SECCLASS_INTEGRITY                               62
 
 /*
  * Security identifier indices for initial entities
Index: linux-2.6.23-rc3-mm1/security/selinux/include/class_to_string.h
===================================================================
--- linux-2.6.23-rc3-mm1.orig/security/selinux/include/class_to_string.h
+++ linux-2.6.23-rc3-mm1/security/selinux/include/class_to_string.h
@@ -64,3 +64,4 @@
     S_(NULL)
     S_("dccp_socket")
     S_("memprotect")
+    S_("integrity")
Index: linux-2.6.23-rc3-mm1/security/selinux/ss/services.c
===================================================================
--- linux-2.6.23-rc3-mm1.orig/security/selinux/ss/services.c
+++ linux-2.6.23-rc3-mm1/security/selinux/ss/services.c
@@ -305,12 +305,12 @@ static int context_struct_compute_av(str
 		    tclass <= SECCLASS_NETLINK_DNRT_SOCKET)
 			tclass = SECCLASS_NETLINK_SOCKET;
 
-	if (!tclass || tclass > policydb.p_classes.nprim) {
-		printk(KERN_ERR "security_compute_av:  unrecognized class %d\n",
-		       tclass);
-		return -EINVAL;
-	}
-	tclass_datum = policydb.class_val_to_struct[tclass - 1];
+//	if (!tclass || tclass > policydb.p_classes.nprim) {
+//		printk(KERN_ERR "security_compute_av:  unrecognized class %d\n",
+//		       tclass);
+//		return -EINVAL;
+//	}
+//	tclass_datum = policydb.class_val_to_struct[tclass - 1];
 
 	/*
 	 * Initialize the access vectors to the default values.
@@ -321,6 +321,10 @@ static int context_struct_compute_av(str
 	avd->auditdeny = 0xffffffff;
 	avd->seqno = latest_granting;
 
+	if (!tclass || tclass > policydb.p_classes.nprim)
+		return 0;
+	tclass_datum = policydb.class_val_to_struct[tclass - 1];
+
 	/*
 	 * If a specific type enforcement rule was defined for
 	 * this permission check, then use it.



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

Re: [RFC]integrity: SELinux patch

[Joshua Brindle]

Mimi Zohar wrote:
> This is a second attempt to verify and measure file integrity, by
> adding the new Linux Integrity Modules(LIM) API calls to SElinux.
> This posting addresses comments previously made on this list. 
> I will also post the current set of LIM patches, as well as an
> initial integrity.te example. 
>
> The integrity of the SELinux metadata is verified when the xattr
> is initially retrieved.  On an integrity failure, normal selinux 
> error processing occurs.
>
> This patch defines a new 'integrity' class with the permission 
> 'measure'.  Measurement calls are made in selinux_file_mmap(), 
> selinux_bprm_check_security, and selinux_inode_permission(),
> based on policy.  (Additional calls might be required.)
>
> Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
> <snip>
>  
> Index: linux-2.6.23-rc3-mm1/security/selinux/include/av_permissions.h
> ===================================================================
> --- linux-2.6.23-rc3-mm1.orig/security/selinux/include/av_permissions.h
> +++ linux-2.6.23-rc3-mm1/security/selinux/include/av_permissions.h
> @@ -824,3 +824,4 @@
>  #define DCCP_SOCKET__NODE_BIND                    0x00400000UL
>  #define DCCP_SOCKET__NAME_CONNECT                 0x00800000UL
>  #define MEMPROTECT__MMAP_ZERO                     0x00000001UL
> +#define INTEGRITY__MEASURE                        0x00000001UL
> Index: linux-2.6.23-rc3-mm1/security/selinux/include/av_perm_to_string.h
> ===================================================================
> --- linux-2.6.23-rc3-mm1.orig/security/selinux/include/av_perm_to_string.h
> +++ linux-2.6.23-rc3-mm1/security/selinux/include/av_perm_to_string.h
> @@ -159,3 +159,4 @@
>     S_(SECCLASS_DCCP_SOCKET, DCCP_SOCKET__NODE_BIND, "node_bind")
>     S_(SECCLASS_DCCP_SOCKET, DCCP_SOCKET__NAME_CONNECT, "name_connect")
>     S_(SECCLASS_MEMPROTECT, MEMPROTECT__MMAP_ZERO, "mmap_zero")
> +   S_(SECCLASS_INTEGRITY, INTEGRITY__MEASURE, "measure")
>   

Do you really need another object class for this? What is wrong with the 
file object class? eg., a rule like:

allow insmod_t modules_object_t : file { read measure };
would require a measurement.

> Index: linux-2.6.23-rc3-mm1/security/selinux/include/flask.h
> ===================================================================
> --- linux-2.6.23-rc3-mm1.orig/security/selinux/include/flask.h
> +++ linux-2.6.23-rc3-mm1/security/selinux/include/flask.h
> @@ -50,6 +50,7 @@
>  #define SECCLASS_KEY                                     58
>  #define SECCLASS_DCCP_SOCKET                             60
>  #define SECCLASS_MEMPROTECT                              61
> +#define SECCLASS_INTEGRITY                               62
>  
>  /*
>   * Security identifier indices for initial entities
> Index: linux-2.6.23-rc3-mm1/security/selinux/include/class_to_string.h
> ===================================================================
> --- linux-2.6.23-rc3-mm1.orig/security/selinux/include/class_to_string.h
> +++ linux-2.6.23-rc3-mm1/security/selinux/include/class_to_string.h
> @@ -64,3 +64,4 @@
>      S_(NULL)
>      S_("dccp_socket")
>      S_("memprotect")
> +    S_("integrity")
> Index: linux-2.6.23-rc3-mm1/security/selinux/ss/services.c
> ===================================================================
> --- linux-2.6.23-rc3-mm1.orig/security/selinux/ss/services.c
> +++ linux-2.6.23-rc3-mm1/security/selinux/ss/services.c
> @@ -305,12 +305,12 @@ static int context_struct_compute_av(str
>  		    tclass <= SECCLASS_NETLINK_DNRT_SOCKET)
>  			tclass = SECCLASS_NETLINK_SOCKET;
>  
> -	if (!tclass || tclass > policydb.p_classes.nprim) {
> -		printk(KERN_ERR "security_compute_av:  unrecognized class %d\n",
> -		       tclass);
> -		return -EINVAL;
> -	}
> -	tclass_datum = policydb.class_val_to_struct[tclass - 1];
> +//	if (!tclass || tclass > policydb.p_classes.nprim) {
> +//		printk(KERN_ERR "security_compute_av:  unrecognized class %d\n",
> +//		       tclass);
> +//		return -EINVAL;
> +//	}
> +//	tclass_datum = policydb.class_val_to_struct[tclass - 1];
>  
>   

Err? Did you mean to submit it like this? This should be fixed by Eric's 
patch to handle unknown classes anyway.

>  	/*
>  	 * Initialize the access vectors to the default values.
> @@ -321,6 +321,10 @@ static int context_struct_compute_av(str
>  	avd->auditdeny = 0xffffffff;
>  	avd->seqno = latest_granting;
>  
> +	if (!tclass || tclass > policydb.p_classes.nprim)
> +		return 0;
> +	tclass_datum = policydb.class_val_to_struct[tclass - 1];
> +
>   

Ditto.


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

Re: [RFC]integrity: SELinux patch

[Mimi Zohar]

On Wed, 2007-08-29 at 00:16 -0400, Joshua Brindle wrote:
> Mimi Zohar wrote:
> > This is a second attempt to verify and measure file integrity, by
> > adding the new Linux Integrity Modules(LIM) API calls to SElinux.
> > This posting addresses comments previously made on this list. 
> > I will also post the current set of LIM patches, as well as an
> > initial integrity.te example. 
> >
> > The integrity of the SELinux metadata is verified when the xattr
> > is initially retrieved.  On an integrity failure, normal selinux 
> > error processing occurs.
> >
> > This patch defines a new 'integrity' class with the permission 
> > 'measure'.  Measurement calls are made in selinux_file_mmap(), 
> > selinux_bprm_check_security, and selinux_inode_permission(),
> > based on policy.  (Additional calls might be required.)
> >
> > Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
> > <snip>
> >  
> > Index: linux-2.6.23-rc3-mm1/security/selinux/include/av_permissions.h
> > ===================================================================
> > --- linux-2.6.23-rc3-mm1.orig/security/selinux/include/av_permissions.h
> > +++ linux-2.6.23-rc3-mm1/security/selinux/include/av_permissions.h
> > @@ -824,3 +824,4 @@
> >  #define DCCP_SOCKET__NODE_BIND                    0x00400000UL
> >  #define DCCP_SOCKET__NAME_CONNECT                 0x00800000UL
> >  #define MEMPROTECT__MMAP_ZERO                     0x00000001UL
> > +#define INTEGRITY__MEASURE                        0x00000001UL
> > Index: linux-2.6.23-rc3-mm1/security/selinux/include/av_perm_to_string.h
> > ===================================================================
> > --- linux-2.6.23-rc3-mm1.orig/security/selinux/include/av_perm_to_string.h
> > +++ linux-2.6.23-rc3-mm1/security/selinux/include/av_perm_to_string.h
> > @@ -159,3 +159,4 @@
> >     S_(SECCLASS_DCCP_SOCKET, DCCP_SOCKET__NODE_BIND, "node_bind")
> >     S_(SECCLASS_DCCP_SOCKET, DCCP_SOCKET__NAME_CONNECT, "name_connect")
> >     S_(SECCLASS_MEMPROTECT, MEMPROTECT__MMAP_ZERO, "mmap_zero")
> > +   S_(SECCLASS_INTEGRITY, INTEGRITY__MEASURE, "measure")
> >   
> 
> Do you really need another object class for this? What is wrong with the 
> file object class? eg., a rule like:
> 
> allow insmod_t modules_object_t : file { read measure };
> would require a measurement.

The original version attempted to add file measure, but even without
adding measure to the policy, files were being measured.  The following
is taken from Stephen's July 19th posting.

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

> > Index: linux-2.6.23-rc3-mm1/security/selinux/include/flask.h
> > ===================================================================
> > --- linux-2.6.23-rc3-mm1.orig/security/selinux/include/flask.h
> > +++ linux-2.6.23-rc3-mm1/security/selinux/include/flask.h
> > @@ -50,6 +50,7 @@
> >  #define SECCLASS_KEY                                     58
> >  #define SECCLASS_DCCP_SOCKET                             60
> >  #define SECCLASS_MEMPROTECT                              61
> > +#define SECCLASS_INTEGRITY                               62
> >  
> >  /*
> >   * Security identifier indices for initial entities
> > Index: linux-2.6.23-rc3-mm1/security/selinux/include/class_to_string.h
> > ===================================================================
> > --- linux-2.6.23-rc3-mm1.orig/security/selinux/include/class_to_string.h
> > +++ linux-2.6.23-rc3-mm1/security/selinux/include/class_to_string.h
> > @@ -64,3 +64,4 @@
> >      S_(NULL)
> >      S_("dccp_socket")
> >      S_("memprotect")
> > +    S_("integrity")
> > Index: linux-2.6.23-rc3-mm1/security/selinux/ss/services.c
> > ===================================================================
> > --- linux-2.6.23-rc3-mm1.orig/security/selinux/ss/services.c
> > +++ linux-2.6.23-rc3-mm1/security/selinux/ss/services.c
> > @@ -305,12 +305,12 @@ static int context_struct_compute_av(str
> >  		    tclass <= SECCLASS_NETLINK_DNRT_SOCKET)
> >  			tclass = SECCLASS_NETLINK_SOCKET;
> >  
> > -	if (!tclass || tclass > policydb.p_classes.nprim) {
> > -		printk(KERN_ERR "security_compute_av:  unrecognized class %d\n",
> > -		       tclass);
> > -		return -EINVAL;
> > -	}
> > -	tclass_datum = policydb.class_val_to_struct[tclass - 1];
> > +//	if (!tclass || tclass > policydb.p_classes.nprim) {
> > +//		printk(KERN_ERR "security_compute_av:  unrecognized class %d\n",
> > +//		       tclass);
> > +//		return -EINVAL;
> > +//	}
> > +//	tclass_datum = policydb.class_val_to_struct[tclass - 1];
> >  
> >   
> 
> Err? Did you mean to submit it like this? This should be fixed by Eric's 
> patch to handle unknown classes anyway.

I'm working off the latest -mm tree and that patch hasn't made it in yet,
as well as some other patches.  For example, additional security class 
numbers have been defined.  So I will need to update SECCLASS_INTEGRITY
as well. The above code was added in order to test the patch. Once the 
basic integrity concept has been reviewed and accepted, I will repost 
based on the latest selinux development source tree.

> >  	/*
> >  	 * Initialize the access vectors to the default values.
> > @@ -321,6 +321,10 @@ static int context_struct_compute_av(str
> >  	avd->auditdeny = 0xffffffff;
> >  	avd->seqno = latest_granting;
> >  
> > +	if (!tclass || tclass > policydb.p_classes.nprim)
> > +		return 0;
> > +	tclass_datum = policydb.class_val_to_struct[tclass - 1];
> > +
> >   
> 
> Ditto.
> 


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

Re: [RFC]integrity: SELinux patch

[Mimi Zohar]

On Wed, 2007-08-29 at 06:14 -0400, Mimi Zohar wrote:
> On Wed, 2007-08-29 at 00:16 -0400, Joshua Brindle wrote:
> > Mimi Zohar wrote:
> > 
> > > Index: linux-2.6.23-rc3-mm1/security/selinux/ss/services.c
> > > ===================================================================
> > > --- linux-2.6.23-rc3-mm1.orig/security/selinux/ss/services.c
> > > +++ linux-2.6.23-rc3-mm1/security/selinux/ss/services.c
> > > @@ -305,12 +305,12 @@ static int context_struct_compute_av(str
> > >  		    tclass <= SECCLASS_NETLINK_DNRT_SOCKET)
> > >  			tclass = SECCLASS_NETLINK_SOCKET;
> > >  
> > > -	if (!tclass || tclass > policydb.p_classes.nprim) {
> > > -		printk(KERN_ERR "security_compute_av:  unrecognized class %d\n",
> > > -		       tclass);
> > > -		return -EINVAL;
> > > -	}
> > > -	tclass_datum = policydb.class_val_to_struct[tclass - 1];
> > > +//	if (!tclass || tclass > policydb.p_classes.nprim) {
> > > +//		printk(KERN_ERR "security_compute_av:  unrecognized class %d\n",
> > > +//		       tclass);
> > > +//		return -EINVAL;
> > > +//	}
> > > +//	tclass_datum = policydb.class_val_to_struct[tclass - 1];
> > >  
> > >   
> > 
> > Err? Did you mean to submit it like this? This should be fixed by Eric's 
> > patch to handle unknown classes anyway.
> 
> I'm working off the latest -mm tree and that patch hasn't made it in yet,
> as well as some other patches.  For example, additional security class 
> numbers have been defined.  So I will need to update SECCLASS_INTEGRITY
> as well. The above code was added in order to test the patch. Once the 
> basic integrity concept has been reviewed and accepted, I will repost 
> based on the latest selinux development source tree.

Ok, so how do I get the latest selinux development source tree?

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.

Re: [RFC]integrity: SELinux patch

[Stephen Smalley]

On Wed, 2007-09-19 at 15:41 -0400, Mimi Zohar wrote:
> On Wed, 2007-08-29 at 06:14 -0400, Mimi Zohar wrote:
> > On Wed, 2007-08-29 at 00:16 -0400, Joshua Brindle wrote:
> > > Mimi Zohar wrote:
> > > 
> > > > Index: linux-2.6.23-rc3-mm1/security/selinux/ss/services.c
> > > > ===================================================================
> > > > --- linux-2.6.23-rc3-mm1.orig/security/selinux/ss/services.c
> > > > +++ linux-2.6.23-rc3-mm1/security/selinux/ss/services.c
> > > > @@ -305,12 +305,12 @@ static int context_struct_compute_av(str
> > > >  		    tclass <= SECCLASS_NETLINK_DNRT_SOCKET)
> > > >  			tclass = SECCLASS_NETLINK_SOCKET;
> > > >  
> > > > -	if (!tclass || tclass > policydb.p_classes.nprim) {
> > > > -		printk(KERN_ERR "security_compute_av:  unrecognized class %d\n",
> > > > -		       tclass);
> > > > -		return -EINVAL;
> > > > -	}
> > > > -	tclass_datum = policydb.class_val_to_struct[tclass - 1];
> > > > +//	if (!tclass || tclass > policydb.p_classes.nprim) {
> > > > +//		printk(KERN_ERR "security_compute_av:  unrecognized class %d\n",
> > > > +//		       tclass);
> > > > +//		return -EINVAL;
> > > > +//	}
> > > > +//	tclass_datum = policydb.class_val_to_struct[tclass - 1];
> > > >  
> > > >   
> > > 
> > > Err? Did you mean to submit it like this? This should be fixed by Eric's 
> > > patch to handle unknown classes anyway.
> > 
> > I'm working off the latest -mm tree and that patch hasn't made it in yet,
> > as well as some other patches.  For example, additional security class 
> > numbers have been defined.  So I will need to update SECCLASS_INTEGRITY
> > as well. The above code was added in order to test the patch. Once the 
> > basic integrity concept has been reviewed and accepted, I will repost 
> > based on the latest selinux development source tree.
> 
> Ok, so how do I get the latest selinux development source tree?

James Morris maintains a selinux git tree on kernel.org, but even that
wouldn't yet have Eric's patch (that patch has been posted a few times
on list, but there are still a couple of changes to be made).

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

Re: [RFC]integrity: SELinux patch

[Mimi Zohar]

On Wed, 2007-09-19 at 17:04 -0400, Stephen Smalley wrote:
> On Wed, 2007-09-19 at 15:41 -0400, Mimi Zohar wrote:
> > On Wed, 2007-08-29 at 06:14 -0400, Mimi Zohar wrote:
> > > On Wed, 2007-08-29 at 00:16 -0400, Joshua Brindle wrote:
> > > > Mimi Zohar wrote:
> > > > 
> > > > > Index: linux-2.6.23-rc3-mm1/security/selinux/ss/services.c
> > > > > ===================================================================
> > > > > --- linux-2.6.23-rc3-mm1.orig/security/selinux/ss/services.c
> > > > > +++ linux-2.6.23-rc3-mm1/security/selinux/ss/services.c
> > > > > @@ -305,12 +305,12 @@ static int context_struct_compute_av(str
> > > > >  		    tclass <= SECCLASS_NETLINK_DNRT_SOCKET)
> > > > >  			tclass = SECCLASS_NETLINK_SOCKET;
> > > > >  
> > > > > -	if (!tclass || tclass > policydb.p_classes.nprim) {
> > > > > -		printk(KERN_ERR "security_compute_av:  unrecognized class %d\n",
> > > > > -		       tclass);
> > > > > -		return -EINVAL;
> > > > > -	}
> > > > > -	tclass_datum = policydb.class_val_to_struct[tclass - 1];
> > > > > +//	if (!tclass || tclass > policydb.p_classes.nprim) {
> > > > > +//		printk(KERN_ERR "security_compute_av:  unrecognized class %d\n",
> > > > > +//		       tclass);
> > > > > +//		return -EINVAL;
> > > > > +//	}
> > > > > +//	tclass_datum = policydb.class_val_to_struct[tclass - 1];
> > > > >  
> > > > >   
> > > > 
> > > > Err? Did you mean to submit it like this? This should be fixed by Eric's 
> > > > patch to handle unknown classes anyway.
> > > 
> > > I'm working off the latest -mm tree and that patch hasn't made it in yet,
> > > as well as some other patches.  For example, additional security class 
> > > numbers have been defined.  So I will need to update SECCLASS_INTEGRITY
> > > as well. The above code was added in order to test the patch. Once the 
> > > basic integrity concept has been reviewed and accepted, I will repost 
> > > based on the latest selinux development source tree.
> > 
> > Ok, so how do I get the latest selinux development source tree?
> 
> James Morris maintains a selinux git tree on kernel.org, but even that
> wouldn't yet have Eric's patch (that patch has been posted a few times
> on list, but there are still a couple of changes to be made).

I could post a patch without commenting out the above lines, so that when
Eric's patch does become available, there won't be any conflicts.  That 
still leaves the issue of which number to use when extending the object 
class in security/selinux/include/flask.h for integrity.  62 is not 
assigned in either the latest -mm or -git, but didn't someone already
post a patch on the SELinux mailing list using it?

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.

Re: [RFC]integrity: SELinux patch

[Stephen Smalley]

On Wed, 2007-09-19 at 21:34 -0400, Mimi Zohar wrote:
> On Wed, 2007-09-19 at 17:04 -0400, Stephen Smalley wrote:
> > On Wed, 2007-09-19 at 15:41 -0400, Mimi Zohar wrote:
> > > On Wed, 2007-08-29 at 06:14 -0400, Mimi Zohar wrote:
> > > > On Wed, 2007-08-29 at 00:16 -0400, Joshua Brindle wrote:
> > > > > Mimi Zohar wrote:
> > > > > 
> > > > > > Index: linux-2.6.23-rc3-mm1/security/selinux/ss/services.c
> > > > > > ===================================================================
> > > > > > --- linux-2.6.23-rc3-mm1.orig/security/selinux/ss/services.c
> > > > > > +++ linux-2.6.23-rc3-mm1/security/selinux/ss/services.c
> > > > > > @@ -305,12 +305,12 @@ static int context_struct_compute_av(str
> > > > > >  		    tclass <= SECCLASS_NETLINK_DNRT_SOCKET)
> > > > > >  			tclass = SECCLASS_NETLINK_SOCKET;
> > > > > >  
> > > > > > -	if (!tclass || tclass > policydb.p_classes.nprim) {
> > > > > > -		printk(KERN_ERR "security_compute_av:  unrecognized class %d\n",
> > > > > > -		       tclass);
> > > > > > -		return -EINVAL;
> > > > > > -	}
> > > > > > -	tclass_datum = policydb.class_val_to_struct[tclass - 1];
> > > > > > +//	if (!tclass || tclass > policydb.p_classes.nprim) {
> > > > > > +//		printk(KERN_ERR "security_compute_av:  unrecognized class %d\n",
> > > > > > +//		       tclass);
> > > > > > +//		return -EINVAL;
> > > > > > +//	}
> > > > > > +//	tclass_datum = policydb.class_val_to_struct[tclass - 1];
> > > > > >  
> > > > > >   
> > > > > 
> > > > > Err? Did you mean to submit it like this? This should be fixed by Eric's 
> > > > > patch to handle unknown classes anyway.
> > > > 
> > > > I'm working off the latest -mm tree and that patch hasn't made it in yet,
> > > > as well as some other patches.  For example, additional security class 
> > > > numbers have been defined.  So I will need to update SECCLASS_INTEGRITY
> > > > as well. The above code was added in order to test the patch. Once the 
> > > > basic integrity concept has been reviewed and accepted, I will repost 
> > > > based on the latest selinux development source tree.
> > > 
> > > Ok, so how do I get the latest selinux development source tree?
> > 
> > James Morris maintains a selinux git tree on kernel.org, but even that
> > wouldn't yet have Eric's patch (that patch has been posted a few times
> > on list, but there are still a couple of changes to be made).
> 
> I could post a patch without commenting out the above lines, so that when
> Eric's patch does become available, there won't be any conflicts.  That 
> still leaves the issue of which number to use when extending the object 
> class in security/selinux/include/flask.h for integrity.  62 is not 
> assigned in either the latest -mm or -git, but didn't someone already
> post a patch on the SELinux mailing list using it?

Don't worry about conflicts with pending patches; just make a clean
patch (where a clean patch follows the usual guidelines and doesn't
include commented-out code) against some well-defined baseline (linus,
mm, or selinux.git).  Using the for-akpm branch of selinux.git makes it
easier for James to merge of course, but as -mm includes that branch, it
should be the same except as other patches in mm might happen to overlap
and except as one happens to lag behind the other.

The capability override patch was using class 62, but that has not been
merged and I wasn't planning on pushing it - there was a fair amount of
concern about whether capability overrides are safe and usable as long
as the distros have to allow selinux to be disabled as an option.  So 62
is still available.  More generally, class/perm values are not truly
reserved until their definitions are committed, much like syscall
values.

In any event, I don't think we are really at the point of just cleaning
up implementation nits in these integrity patches - I think there needs
to be more discussion and work on the overall design, hook placement and
coverage, how it will be used in practice, who would use it, etc.  Which
goes beyond just how it would be integrated with selinux.  I haven't
really seen a compelling argument and concept of operations so far.  As
a general concept, I can see potential value in integrity measurement
(although lots of pitfalls too), but I'm not sure that this approach is
going to yield that value.

I had hoped that others more involved or interested in integrity
measurement might have weighed in on the discussion (hint, hint).

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

Re: [RFC]integrity: SELinux patch

[James Morris]

On Thu, 20 Sep 2007, Stephen Smalley wrote:

> Don't worry about conflicts with pending patches; just make a clean
> patch (where a clean patch follows the usual guidelines and doesn't
> include commented-out code) against some well-defined baseline (linus,
> mm, or selinux.git).  Using the for-akpm branch of selinux.git makes it
> easier for James to merge of course, but as -mm includes that branch, it
> should be the same except as other patches in mm might happen to overlap
> and except as one happens to lag behind the other.

Generally, a patch against the latest Linus git will work fine, as that is 
what -mm is against, and if there are conflicts, they need to be resolved 
there anyway.


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

Re: [RFC]integrity: SELinux patch

[Mimi Zohar]

On Thu, 2007-09-20 at 14:16 -0700, James Morris wrote: 
> On Thu, 20 Sep 2007, Stephen Smalley wrote:
> 
> > Don't worry about conflicts with pending patches; just make a clean
> > patch (where a clean patch follows the usual guidelines and doesn't
> > include commented-out code) against some well-defined baseline (linus,
> > mm, or selinux.git).  Using the for-akpm branch of selinux.git makes it
> > easier for James to merge of course, but as -mm includes that branch, it
> > should be the same except as other patches in mm might happen to overlap
> > and except as one happens to lag behind the other.
> 
> Generally, a patch against the latest Linus git will work fine, as that is 
> what -mm is against, and if there are conflicts, they need to be resolved 
> there anyway.

OK.  Thank you for clarifying the process.  As I still need to post the LIM
patches based on the -mm tree to LKML, for this iteration the SELinux integrity
patch will also be against the latest -mm.  Once we are further along in the 
process, when the LIM patches are back in -mm, I will post a git compatible 
SELinux integrity patch.

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.

Re: [RFC]integrity: SELinux patch

[Mimi Zohar]

On Thu, 2007-09-20 at 09:12 -0400, Stephen Smalley wrote:

> In any event, I don't think we are really at the point of just cleaning
> up implementation nits in these integrity patches - I think there needs
> to be more discussion and work on the overall design, hook placement and
> coverage, how it will be used in practice, who would use it, etc.  Which
> goes beyond just how it would be integrated with selinux.  I haven't
> really seen a compelling argument and concept of operations so far.  As
> a general concept, I can see potential value in integrity measurement
> (although lots of pitfalls too), but I'm not sure that this approach is
> going to yield that value.
> 
> I had hoped that others more involved or interested in integrity
> measurement might have weighed in on the discussion (hint, hint).

We were just about to post the latest LIM patches as an RFC to LMKL.  If
we can get a discussion going here first, that would be great.  I will
repost the latest set of patches.

In terms of design, the integrity provider would be responsible for
maintaining file integrity information, which an LSM module could query
via the LIM integrity_verify_metadata/data() API.  For now, we are
releasing an IMA-only integrity provider, which implements the LIM
integrity_measure() API.  When integrity_measure() is called, IMA
submits the measurement (hash) of the file to the TPM chip, for
inclusion in one of the chip's Platform Configuration Registers (PCR).
IMA also keeps a list of all file names and hashes that have been
submitted to the TPM, which can be viewed through securityfs.

We have a number of enterprise customers who use the old LSM based IMA
(http://sourceforge.net/projects/linux-ima).  They use it to monitor
file integrity and version level on large server farms. Their top two
requests have been to get IMA off of LSM, so that they can use it with
Selinux and AppArmor, and to get it upstream, so that it will be
supported. These customers are using just the measurement/attestation
portion of LIM. The verification of labels and data is more oriented to
enterprise verification of configuration of the labels, and to protect
clients and servers from off-line attacks.

This is a request for comments for updates to the LIM integrity 
service framework, previously accepted into -mm, an IMA-only 
integrity service provider, and a SELinux integrity patch. 

Patch 1/4 integrity: LIM api, hooks, and dummy provider
Patch 2/4 integrity: IMA service provider
Patch 3/4 integrity: TPM internal kernel interface
Patch 4/4 integrity: SELinux LIM calls

Mimi Zohar
David Safford



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

Re: [RFC]integrity: SELinux patch

[Serge E. Hallyn]

Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> This is a second attempt to verify and measure file integrity, by
> adding the new Linux Integrity Modules(LIM) API calls to SElinux.
> This posting addresses comments previously made on this list. 
> I will also post the current set of LIM patches, as well as an
> initial integrity.te example. 
> 
> The integrity of the SELinux metadata is verified when the xattr
> is initially retrieved.  On an integrity failure, normal selinux 
> error processing occurs.
> 
> This patch defines a new 'integrity' class with the permission 
> 'measure'.  Measurement calls are made in selinux_file_mmap(), 
> selinux_bprm_check_security, and selinux_inode_permission(),
> based on policy.  (Additional calls might be required.)

Just curious - wouldn't you want to also define a 'update' permission to
allow policy to permit some domains to update xattrs?  Or does that not
make sense?

> Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
> ---
> Index: linux-2.6.23-rc3-mm1/security/selinux/hooks.c
> ===================================================================
> --- linux-2.6.23-rc3-mm1.orig/security/selinux/hooks.c
> +++ linux-2.6.23-rc3-mm1/security/selinux/hooks.c
> @@ -69,6 +69,7 @@
>  #include <linux/audit.h>
>  #include <linux/string.h>
>  #include <linux/selinux.h>
> +#include <linux/integrity.h>
>  #include <linux/mutex.h>
> 
>  #include "avc.h"
> @@ -844,6 +845,7 @@ static int inode_doinit_with_dentry(stru
>  	char *context = NULL;
>  	unsigned len = 0;
>  	int rc = 0;
> +	int status;
> 
>  	if (isec->initialized)
>  		goto out;
> @@ -888,34 +890,12 @@ static int inode_doinit_with_dentry(stru
>  		}
> 
>  		len = INITCONTEXTLEN;
> -		context = kmalloc(len, GFP_KERNEL);
> -		if (!context) {
> -			rc = -ENOMEM;
> +		rc = integrity_verify_metadata(dentry, XATTR_NAME_SELINUX,
> +				       &context, &len, &status);

Hmm, so in this case shouldn't this be called
integrity_get_and_verify_xattrs() or something?  I guess that's too
long, but I wouldn't have thought integrity_verify_metadata() would also
get the xattrs.

> +		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) {
> @@ -929,6 +909,14 @@ static int inode_doinit_with_dentry(stru
>  			sid = sbsec->def_sid;
>  			rc = 0;
>  		} else {
> +			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);

Seems like something worth using audit for.

> +				kfree(context);
> +				goto out_unlock;
> +			}
>  			rc = security_context_to_sid_default(context, rc, &sid,
>  			                                     sbsec->def_sid);
>  			if (rc) {
> @@ -1698,9 +1686,98 @@ static int selinux_bprm_set_security(str
>  	return 0;
>  }
> 
> -static int selinux_bprm_check_security (struct linux_binprm *bprm)
> +static int selinux_verify_metadata(struct dentry *dentry)
>  {
> -	return secondary_ops->bprm_check_security(bprm);
> +	int rc, status;
> +
> +	if (!dentry)
> +		return 0;
> +
> +	rc = integrity_verify_metadata(dentry, NULL, NULL, NULL, &status);
> +	if (rc == -EOPNOTSUPP)
> +		return 0;
> +	if (rc < 0)
> +		goto out;
> +
> +	if (status != INTEGRITY_PASS) 	/* FAIL | NO_LABEL */
> +		rc = -EACCES;
> +out:
> +	return rc;
> +}
> +
> +static int selinux_verify_data(struct dentry *dentry, struct file *file)
> +{
> +	int rc, status;
> +
> +	if (!dentry && !file)
> +		return 0;
> +
> +	rc = integrity_verify_data(dentry, file, &status);
> +	if (rc < 0)
> +		return 0;
> +
> +	if (status != INTEGRITY_PASS)
> +		rc = -EACCES;
> +
> +	return rc;
> +}
> +
> +/*
> + * Measure based on new 'integrity' class policy
> + */
> +static void selinux_measure(struct inode *inode, struct dentry *dentry,
> +				struct file *file, char *filename,
> +				int mask)
> +{
> +	int rc = 1;
> +	struct inode_security_struct *isec = inode->i_security;
> +	struct task_security_struct *tsec = current->security;
> +	struct av_decision avd;
> +
> +	if (!S_ISREG(inode->i_mode))
> +		return;
> +
> +	rc = avc_has_perm_noaudit(tsec->sid, isec->sid, SECCLASS_INTEGRITY,
> +				  INTEGRITY__MEASURE, AVC_STRICT, &avd);
> +	if (rc == 0)
> +		integrity_measure(inode, dentry, file, filename, mask);
> +#if 0
> +	else { /* Only used for debugging */
> +		struct avc_audit_data ad;
> +
> +		AVC_AUDIT_DATA_INIT(&ad,FS);
> +		ad.u.fs.mnt = NULL;
> +		ad.u.fs.dentry = dentry;
> +		avc_audit(tsec->sid, isec->sid, SECCLASS_INTEGRITY,
> +			INTEGRITY__MEASURE, &avd, rc, &ad);
> +	}
> +#endif
> +	return;
> +}
> +
> +/* The OS protects against an executable file, already open for write,
> + * from being executed in deny_write_access() and an executable file
> + * already open for execute, from being modified in get_write_access().
> + * So we can be certain that what we verify and measure here is actually
> + * what is being executed.
> + */
> +static int selinux_bprm_check_security(struct linux_binprm *bprm)
> +{
> +	struct dentry *dentry = bprm->file->f_dentry;
> +	int rc;
> +
> +	rc = secondary_ops->bprm_check_security(bprm);
> +	if (rc != 0)
> +		return rc;
> +
> +	rc = selinux_verify_metadata(dentry);
> +	if (rc == 0) {
> +		rc = selinux_verify_data(dentry, bprm->file);
> +		if (rc == 0)
> +			selinux_measure(dentry->d_inode, dentry, bprm->file,
> +					bprm->filename, MAY_EXEC);
> +	}
> +	return rc;
>  }
> 
> 
> @@ -2249,6 +2326,30 @@ static int selinux_inode_follow_link(str
>  	return dentry_has_perm(current, NULL, dentry, FILE__READ);
>  }
> 
> +static char *get_fname(struct dentry *dentry, struct vfsmount *mnt,
> +			char **buf)
> +{
> +	char *fname = NULL;
> +	char *path = NULL;
> +
> +	path = (char *)__get_free_page(GFP_KERNEL);
> +	if (path) {
> +		fname = d_path(dentry, mnt, path, PAGE_SIZE);

Ok, this is kind of a drag performance-wise for something which is
purely optional.  How about putting this behind some boot or compile 
option or something?

> +		*buf = path;
> +	}
> +
> +	if (!fname)	/* no choice, use short name */
> +		fname = (!dentry->d_name.name) ?
> +		    (char *)dentry->d_iname : (char *)dentry->d_name.name;
> +	return fname;
> +}
> +
> +static void free_fname(char *path)
> +{
> +	if (path)
> +		free_page((unsigned long)path);
> +}
> +
>  static int selinux_inode_permission(struct inode *inode, int mask,
>  				    struct nameidata *nd)
>  {
> @@ -2263,8 +2364,29 @@ static int selinux_inode_permission(stru
>  		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 (mask & ~MAY_EXEC) { /* measure executables later */
> +		struct dentry *dentry = NULL;
> +		char *path = NULL;
> +		char *fname = NULL;
> +
> +		/* The file name is not required, but only a hint.
> +		 * When possible, supply a fully qualified path name.
> +		 */
> +		if (nd) {
> +			dentry = nd->dentry;
> +			fname = get_fname(nd->dentry, nd->mnt, &path);
> +		}
> +
> +		selinux_measure(inode, dentry, NULL, fname, mask);
> +		if (path)
> +			free_fname(path);
> +	}
> +	return rc;
>  }
> 
>  static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
> @@ -2585,8 +2707,18 @@ static int selinux_file_mmap(struct file
>  	if (selinux_checkreqprot)
>  		prot = reqprot;
> 
> -	return file_map_prot_check(file, prot,
> -				   (flags & MAP_TYPE) == MAP_SHARED);
> +	rc = file_map_prot_check(file, prot, (flags & MAP_TYPE) == MAP_SHARED);
> +	if (file && file->f_dentry && rc == 0) {
> +		rc = selinux_verify_metadata(file->f_dentry);
> +		if (rc == 0) {
> +			rc = selinux_verify_data(NULL, file);
> +			if (rc == 0)
> +				selinux_measure(file->f_dentry->d_inode,
> +						  file->f_dentry, file,
> +						  NULL, MAY_EXEC);
> +		}
> +	}
> +	return rc;
>  }
> 
>  static int selinux_file_mprotect(struct vm_area_struct *vma,
> @@ -2628,7 +2760,6 @@ static int selinux_file_mprotect(struct 
>  			return rc;
>  	}
>  #endif
> -
>  	return file_map_prot_check(vma->vm_file, prot, vma->vm_flags&VM_SHARED);
>  }
> 
> Index: linux-2.6.23-rc3-mm1/security/selinux/include/av_permissions.h
> ===================================================================
> --- linux-2.6.23-rc3-mm1.orig/security/selinux/include/av_permissions.h
> +++ linux-2.6.23-rc3-mm1/security/selinux/include/av_permissions.h
> @@ -824,3 +824,4 @@
>  #define DCCP_SOCKET__NODE_BIND                    0x00400000UL
>  #define DCCP_SOCKET__NAME_CONNECT                 0x00800000UL
>  #define MEMPROTECT__MMAP_ZERO                     0x00000001UL
> +#define INTEGRITY__MEASURE                        0x00000001UL
> Index: linux-2.6.23-rc3-mm1/security/selinux/include/av_perm_to_string.h
> ===================================================================
> --- linux-2.6.23-rc3-mm1.orig/security/selinux/include/av_perm_to_string.h
> +++ linux-2.6.23-rc3-mm1/security/selinux/include/av_perm_to_string.h
> @@ -159,3 +159,4 @@
>     S_(SECCLASS_DCCP_SOCKET, DCCP_SOCKET__NODE_BIND, "node_bind")
>     S_(SECCLASS_DCCP_SOCKET, DCCP_SOCKET__NAME_CONNECT, "name_connect")
>     S_(SECCLASS_MEMPROTECT, MEMPROTECT__MMAP_ZERO, "mmap_zero")
> +   S_(SECCLASS_INTEGRITY, INTEGRITY__MEASURE, "measure")
> Index: linux-2.6.23-rc3-mm1/security/selinux/include/flask.h
> ===================================================================
> --- linux-2.6.23-rc3-mm1.orig/security/selinux/include/flask.h
> +++ linux-2.6.23-rc3-mm1/security/selinux/include/flask.h
> @@ -50,6 +50,7 @@
>  #define SECCLASS_KEY                                     58
>  #define SECCLASS_DCCP_SOCKET                             60
>  #define SECCLASS_MEMPROTECT                              61
> +#define SECCLASS_INTEGRITY                               62
> 
>  /*
>   * Security identifier indices for initial entities
> Index: linux-2.6.23-rc3-mm1/security/selinux/include/class_to_string.h
> ===================================================================
> --- linux-2.6.23-rc3-mm1.orig/security/selinux/include/class_to_string.h
> +++ linux-2.6.23-rc3-mm1/security/selinux/include/class_to_string.h
> @@ -64,3 +64,4 @@
>      S_(NULL)
>      S_("dccp_socket")
>      S_("memprotect")
> +    S_("integrity")
> Index: linux-2.6.23-rc3-mm1/security/selinux/ss/services.c
> ===================================================================
> --- linux-2.6.23-rc3-mm1.orig/security/selinux/ss/services.c
> +++ linux-2.6.23-rc3-mm1/security/selinux/ss/services.c
> @@ -305,12 +305,12 @@ static int context_struct_compute_av(str
>  		    tclass <= SECCLASS_NETLINK_DNRT_SOCKET)
>  			tclass = SECCLASS_NETLINK_SOCKET;
> 
> -	if (!tclass || tclass > policydb.p_classes.nprim) {
> -		printk(KERN_ERR "security_compute_av:  unrecognized class %d\n",
> -		       tclass);
> -		return -EINVAL;
> -	}
> -	tclass_datum = policydb.class_val_to_struct[tclass - 1];
> +//	if (!tclass || tclass > policydb.p_classes.nprim) {
> +//		printk(KERN_ERR "security_compute_av:  unrecognized class %d\n",
> +//		       tclass);
> +//		return -EINVAL;
> +//	}
> +//	tclass_datum = policydb.class_val_to_struct[tclass - 1];

As you can see the deletion is plenty obvious in the patch so please
just delete it here rather than commenting it out :)

(As to the correctness of moving it I can't speak right now)

> 
>  	/*
>  	 * Initialize the access vectors to the default values.
> @@ -321,6 +321,10 @@ static int context_struct_compute_av(str
>  	avd->auditdeny = 0xffffffff;
>  	avd->seqno = latest_granting;
> 
> +	if (!tclass || tclass > policydb.p_classes.nprim)
> +		return 0;
> +	tclass_datum = policydb.class_val_to_struct[tclass - 1];
> +
>  	/*
>  	 * If a specific type enforcement rule was defined for
>  	 * this permission check, then use it.
> 
> 
> 
> --
> 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.

Re: [RFC]integrity: SELinux patch

[Serge E. Hallyn]

Quoting Serge E. Hallyn (serue@us.ibm.com):
> Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> > This is a second attempt to verify and measure file integrity, by
> > adding the new Linux Integrity Modules(LIM) API calls to SElinux.
> > This posting addresses comments previously made on this list. 
> > I will also post the current set of LIM patches, as well as an
> > initial integrity.te example. 
> > 
> > The integrity of the SELinux metadata is verified when the xattr
> > is initially retrieved.  On an integrity failure, normal selinux 
> > error processing occurs.
> > 
> > This patch defines a new 'integrity' class with the permission 
> > 'measure'.  Measurement calls are made in selinux_file_mmap(), 
> > selinux_bprm_check_security, and selinux_inode_permission(),
> > based on policy.  (Additional calls might be required.)
> 
> Just curious - wouldn't you want to also define a 'update' permission to
> allow policy to permit some domains to update xattrs?  Or does that not
> make sense?

Oops, I see, that's what measure is...   nm then.

-serge

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

Re: [RFC]integrity: SELinux patch

[Mimi Zohar]

On Thu, 2007-08-30 at 16:12 -0500, Serge E. Hallyn wrote:
> Quoting Serge E. Hallyn (serue@us.ibm.com):
> > Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> > > This is a second attempt to verify and measure file integrity, by
> > > adding the new Linux Integrity Modules(LIM) API calls to SElinux.
> > > This posting addresses comments previously made on this list. 
> > > I will also post the current set of LIM patches, as well as an
> > > initial integrity.te example. 
> > > 
> > > The integrity of the SELinux metadata is verified when the xattr
> > > is initially retrieved.  On an integrity failure, normal selinux 
> > > error processing occurs.
> > > 
> > > This patch defines a new 'integrity' class with the permission 
> > > 'measure'.  Measurement calls are made in selinux_file_mmap(), 
> > > selinux_bprm_check_security, and selinux_inode_permission(),
> > > based on policy.  (Additional calls might be required.)
> > 
> > Just curious - wouldn't you want to also define a 'update' permission to
> > allow policy to permit some domains to update xattrs?  Or does that not
> > make sense?
> 
> Oops, I see, that's what measure is...   nm then.

A LIM provider that implements integrity_verify_metadata/data would protect 
its own integrity xattrs, just as an LSM module protects its own xattrs, but 
that is not an SElinux issue.  Based on a 'measure' policy, SELinux decides
whether or not to add a measurement to the measurement list and extend the 
PCR value.

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.