[PATCH 0/4] Validate kernel object classes and permissions

[PATCH 0/4] Validate kernel object classes and permissions

[Chad Sellers]

This patch replaces the object class and permission validation code in the SELinux security server. The current code validates that the object classes and permissions have not changed between policy loads. This requires a simple reboot to circumvent, and more importantly does not allow for any change in object classes or permissions upon reload (say if a new userspace object manager was added to the system).

The new logic simply verifies the values of object classes and permissions against the kernel headers in security/selinux/include. If a class or permission in the new policy that is defined in the kernel headers does not match the kernel headers, then policy will fail to load. Note that there is no longer any comparison between the old policy and the new policy.

This patch set will soon be followed by patches allowing userspace to query object class and permission values for the running policy, thereby providing full support for adding and modifying object classes and permissions for userspace object managers. However, I wanted to go ahead and send this patch set as it is useful on its own.

Signed-off-by: Chad Sellers <csellers@tresys.com>

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

[PATCH 1/4] Bug fix in polidydb_destroy

[Chad Sellers]

This patch fixes two bugs in policydb_destroy. Two list pointers (policydb.ocontexts[i] and policydb.genfs) were not being reset to NULL when the lists they pointed to were being freed. This caused a problem when the initial policy load failed, as the policydb being destroyed was not a temporary new policydb that was thrown away, but rather was the global (active) policydb. Consequently, later functions, particularly sys_bind->selinux_socket_bind->security_node_sid and do_rw_proc->selinux_sysctl->selinux_proc_get_sid->security_genfs_sid tried to dereference memory that had previously been freed.

Signed-off-by: Chad Sellers <csellers@tresys.com>

---
 security/selinux/ss/policydb.c |    2 ++
 1 file changed, 2 insertions(+)

Index: linux-2.6-discovery/security/selinux/ss/policydb.c
===================================================================
--- linux-2.6-discovery.orig/security/selinux/ss/policydb.c
+++ linux-2.6-discovery/security/selinux/ss/policydb.c
@@ -613,6 +613,7 @@ void policydb_destroy(struct policydb *p
 			c = c->next;
 			ocontext_destroy(ctmp,i);
 		}
+		p->ocontexts[i] = NULL;
 	}
 
 	g = p->genfs;
@@ -628,6 +629,7 @@ void policydb_destroy(struct policydb *p
 		g = g->next;
 		kfree(gtmp);
 	}
+	p->genfs = NULL;
 
 	cond_policydb_destroy(p);
 

--

--
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: [PATCH 1/4] Bug fix in polidydb_destroy

[James Morris]

On Fri, 6 Oct 2006, Chad Sellers wrote:

> This patch fixes two bugs in policydb_destroy. Two list pointers (policydb.ocontexts[i] and policydb.genfs) were not being reset to NULL when the lists they pointed to were being freed. This caused a problem when the initial policy load failed, as the policydb being destroyed was not a temporary new policydb that was thrown away, but rather was the global (active) policydb. Consequently, later functions, particularly sys_bind->selinux_socket_bind->security_node_sid and do_rw_proc->selinux_sysctl->selinux_proc_get_sid->security_genfs_sid tried to dereference memory that had previously been freed.
> 
> Signed-off-by: Chad Sellers <csellers@tresys.com>

Applied to

git://git.infradead.org/~jmorris/selinux-2.6.git
http://git.infradead.org/?p=users/jmorris/selinux-2.6.git;a=summary


If the other patches look ok, I'll start feeding them into an 
selinux-2.6.20 tree.


In the future, please separate bugfixes from features, as well as 
justifying the text of the mail, and using subject lines per:

http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt




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

[PATCH 2/4] Remove current validation mechanism

[Chad Sellers]

This patch removes the current object class and permission validation code, as it makes it impossible to add, remove, or change object classes and permissions on a running system. Note that this validation code is replaced in the fourth patch of this set, but I removed it completely here to make the patches more readable.

Signed-off-by: Chad Sellers <csellers@tresys.com>

---
 security/selinux/ss/services.c |   91 -----------------------------------------
 1 file changed, 91 deletions(-)

Index: linux-2.6-discovery/security/selinux/ss/services.c
===================================================================
--- linux-2.6-discovery.orig/security/selinux/ss/services.c
+++ linux-2.6-discovery/security/selinux/ss/services.c
@@ -1018,89 +1018,6 @@ int security_change_sid(u32 ssid,
 	return security_compute_sid(ssid, tsid, tclass, AVTAB_CHANGE, out_sid);
 }
 
-/*
- * Verify that each permission that is defined under the
- * existing policy is still defined with the same value
- * in the new policy.
- */
-static int validate_perm(void *key, void *datum, void *p)
-{
-	struct hashtab *h;
-	struct perm_datum *perdatum, *perdatum2;
-	int rc = 0;
-
-
-	h = p;
-	perdatum = datum;
-
-	perdatum2 = hashtab_search(h, key);
-	if (!perdatum2) {
-		printk(KERN_ERR "security:  permission %s disappeared",
-		       (char *)key);
-		rc = -ENOENT;
-		goto out;
-	}
-	if (perdatum->value != perdatum2->value) {
-		printk(KERN_ERR "security:  the value of permission %s changed",
-		       (char *)key);
-		rc = -EINVAL;
-	}
-out:
-	return rc;
-}
-
-/*
- * Verify that each class that is defined under the
- * existing policy is still defined with the same
- * attributes in the new policy.
- */
-static int validate_class(void *key, void *datum, void *p)
-{
-	struct policydb *newp;
-	struct class_datum *cladatum, *cladatum2;
-	int rc;
-
-	newp = p;
-	cladatum = datum;
-
-	cladatum2 = hashtab_search(newp->p_classes.table, key);
-	if (!cladatum2) {
-		printk(KERN_ERR "security:  class %s disappeared\n",
-		       (char *)key);
-		rc = -ENOENT;
-		goto out;
-	}
-	if (cladatum->value != cladatum2->value) {
-		printk(KERN_ERR "security:  the value of class %s changed\n",
-		       (char *)key);
-		rc = -EINVAL;
-		goto out;
-	}
-	if ((cladatum->comdatum && !cladatum2->comdatum) ||
-	    (!cladatum->comdatum && cladatum2->comdatum)) {
-		printk(KERN_ERR "security:  the inherits clause for the access "
-		       "vector definition for class %s changed\n", (char *)key);
-		rc = -EINVAL;
-		goto out;
-	}
-	if (cladatum->comdatum) {
-		rc = hashtab_map(cladatum->comdatum->permissions.table, validate_perm,
-		                 cladatum2->comdatum->permissions.table);
-		if (rc) {
-			printk(" in the access vector definition for class "
-			       "%s\n", (char *)key);
-			goto out;
-		}
-	}
-	rc = hashtab_map(cladatum->permissions.table, validate_perm,
-	                 cladatum2->permissions.table);
-	if (rc)
-		printk(" in access vector definition for class %s\n",
-		       (char *)key);
-out:
-	return rc;
-}
-
 /* Clone the SID into the new SID table. */
 static int clone_sid(u32 sid,
 		     struct context *context,
@@ -1265,14 +1182,6 @@ int security_load_policy(void *data, siz
 
 	sidtab_init(&newsidtab);
 
-	/* Verify that the existing classes did not change. */
-	if (hashtab_map(policydb.p_classes.table, validate_class, &newpolicydb)) {
-		printk(KERN_ERR "security:  the definition of an existing "
-		       "class changed\n");
-		rc = -EINVAL;
-		goto err;
-	}
-
 	/* Clone the SID table. */
 	sidtab_shutdown(&sidtab);
 	if (sidtab_map(&sidtab, clone_sid, &newsidtab)) {

--

--
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: [PATCH 2/4] Remove current validation mechanism

[Stephen Smalley]

On Fri, 2006-10-06 at 16:09 -0400, Chad Sellers wrote:
> plain text document attachment (remove_validation.diff)
> This patch removes the current object class and permission validation
> code, as it makes it impossible to add, remove, or change object
> classes and permissions on a running system.

nit:  It is possible to add object classes and permissions to a running
system, as long as they don't disturb existing values (i.e. as long as
they are added at the end).

>  Note that this validation code is replaced in the fourth patch of
> this set, but I removed it completely here to make the patches more
> readable.

-- 
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: [PATCH 2/4] Remove current validation mechanism

[Chad Sellers]

On 10/6/06 5:07 PM, "Stephen Smalley" <sds@tycho.nsa.gov> wrote:

> On Fri, 2006-10-06 at 16:09 -0400, Chad Sellers wrote:
>> plain text document attachment (remove_validation.diff)
>> This patch removes the current object class and permission validation
>> code, as it makes it impossible to add, remove, or change object
>> classes and permissions on a running system.
> 
> nit:  It is possible to add object classes and permissions to a running
> system, as long as they don't disturb existing values (i.e. as long as
> they are added at the end).
> 
Yes, that's true. So, I should have just said change and remove.

>>  Note that this validation code is replaced in the fourth patch of
>> this set, but I removed it completely here to make the patches more
>> readable.


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

[PATCH 3/4] Export object class and permission definitions

[Chad Sellers]

This patch moves the definition of the 3 structs containing object class and permission definitions from avc.c to avc_ss.h so that the security server can access them. The patch also adds a new struct type, defined_classes_perms_t, suitable for allowing the security server to access these data structures from the avc.

Signed-off-by: Chad Sellers <csellers@tresys.com>

---
 security/selinux/avc.c            |   23 +++++++++++------------
 security/selinux/include/avc_ss.h |   24 ++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 12 deletions(-)

Index: linux-2.6-discovery/security/selinux/avc.c
===================================================================
--- linux-2.6-discovery.orig/security/selinux/avc.c
+++ linux-2.6-discovery/security/selinux/avc.c
@@ -32,12 +32,7 @@
 #include "avc.h"
 #include "avc_ss.h"
 
-static const struct av_perm_to_string
-{
-  u16 tclass;
-  u32 value;
-  const char *name;
-} av_perm_to_string[] = {
+static const struct av_perm_to_string_t av_perm_to_string[] = {
 #define S_(c, v, s) { c, v, s },
 #include "av_perm_to_string.h"
 #undef S_
@@ -57,17 +52,21 @@ static const char *class_to_string[] = {
 #undef TE_
 #undef S_
 
-static const struct av_inherit
-{
-    u16 tclass;
-    const char **common_pts;
-    u32 common_base;
-} av_inherit[] = {
+static const struct av_inherit_t av_inherit[] = {
 #define S_(c, i, b) { c, common_##i##_perm_to_string, b },
 #include "av_inherit.h"
 #undef S_
 };
 
+const struct defined_classes_perms_t defined_classes_perms = {
+	av_perm_to_string,
+	ARRAY_SIZE(av_perm_to_string),
+	class_to_string,
+	ARRAY_SIZE(class_to_string),
+	av_inherit,
+	ARRAY_SIZE(av_inherit)
+};
+
 #define AVC_CACHE_SLOTS			512
 #define AVC_DEF_CACHE_THRESHOLD		512
 #define AVC_CACHE_RECLAIM		16
Index: linux-2.6-discovery/security/selinux/include/avc_ss.h
===================================================================
--- linux-2.6-discovery.orig/security/selinux/include/avc_ss.h
+++ linux-2.6-discovery/security/selinux/include/avc_ss.h
@@ -10,5 +10,29 @@
 
 int avc_ss_reset(u32 seqno);
 
+struct av_perm_to_string_t
+{
+	u16 tclass;
+	u32 value;
+	const char *name;
+};
+
+struct av_inherit_t
+{
+	u16 tclass;
+	const char **common_pts;
+	u32 common_base;
+};
+
+struct defined_classes_perms_t
+{
+	const struct av_perm_to_string_t *av_perm_to_string;
+	u32 av_perm_to_string_len;
+	const char **class_to_string;
+	u32 class_to_string_len;
+	const struct av_inherit_t *av_inherit;
+	u32 av_inherit_len;
+};
+
 #endif /* _SELINUX_AVC_SS_H_ */
 

--

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

[PATCH 4/4] Validate kernel object classes and permissions

[Chad Sellers]

This patch replaces the object class and permission validation code to validate against the defined kernel headers, and allow extra classes and permissions that do not conflict with the kernel definitions. Note that validation is now done for all policy loads, not just subsequent loads after the first policy load.

Signed-off-by: Chad Sellers <csellers@tresys.com>

---
 security/selinux/ss/services.c |  161 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 160 insertions(+), 1 deletion(-)

Index: linux-2.6-discovery/security/selinux/ss/services.c
===================================================================
--- linux-2.6-discovery.orig/security/selinux/ss/services.c
+++ linux-2.6-discovery/security/selinux/ss/services.c
@@ -17,9 +17,13 @@
  *
  *      Added support for NetLabel
  *
+ * Updated: Chad Sellers <csellers@tresys.com>
+ *
+ *  Added validation of kernel classes and permissions
+ *
  * Copyright (C) 2006 Hewlett-Packard Development Company, L.P.
  * Copyright (C) 2004-2006 Trusted Computer Solutions, Inc.
- * Copyright (C) 2003 - 2004 Tresys Technology, LLC
+ * Copyright (C) 2003 - 2004, 2006 Tresys Technology, LLC
  * Copyright (C) 2003 Red Hat, Inc., James Morris <jmorris@redhat.com>
  *	This program is free software; you can redistribute it and/or modify
  *  	it under the terms of the GNU General Public License as published by
@@ -53,6 +57,11 @@
 extern void selnl_notify_policyload(u32 seqno);
 unsigned int policydb_loaded_version;
 
+/*
+ * This is declared in avc.c
+ */
+extern const struct defined_classes_perms_t defined_classes_perms;
+
 static DEFINE_RWLOCK(policy_rwlock);
 #define POLICY_RDLOCK read_lock(&policy_rwlock)
 #define POLICY_WRLOCK write_lock_irq(&policy_rwlock)
@@ -1018,6 +1027,138 @@ int security_change_sid(u32 ssid,
 	return security_compute_sid(ssid, tsid, tclass, AVTAB_CHANGE, out_sid);
 }
 
+/*
+ * Verify that each kernel permission that is defined in a
+ * policy class is correct
+ */
+static int validate_class_perm(void *key, void *datum, void *p)
+{
+	struct perm_datum *perdatum;
+	u32 *class, val;
+	char *name;
+	int rc = 0, i;
+
+	class = p;
+	perdatum = datum;
+	name = key;
+	val = 1 << (perdatum->value - 1);
+
+	for (i = 0; i < defined_classes_perms.av_perm_to_string_len; i++) {
+		if ((defined_classes_perms.av_perm_to_string[i].tclass == *class)
+		    && (defined_classes_perms.av_perm_to_string[i].value == val)) {
+			if (strcmp(name, defined_classes_perms.av_perm_to_string[i].name)) {
+				printk(KERN_ERR
+				       "security:  the value of permission %s is incorrect",
+				       (char *)key);
+				rc = -EINVAL;
+			}
+			break;
+		}
+	}
+	return rc;
+}
+
+/*
+ * Verify that each kernel permission that is defined in a
+ * policy common is correct
+ */
+static int validate_common_perm(void *key, void *datum, void *p)
+{
+	struct perm_datum *perdatum;
+	u32 *class, tmp, common_pts_len = 0;
+	char *name;
+	int rc = 0, i;
+
+	class = p;
+	perdatum = datum;
+	name = key;
+
+	for (i = 0; i < defined_classes_perms.av_inherit_len; i++) {
+		if (defined_classes_perms.av_inherit[i].tclass == *class) {
+			tmp = defined_classes_perms.av_inherit[i].common_base;
+			while (!(tmp & 0x01)) {
+				common_pts_len++;
+				tmp >>= 1;
+			}
+			if (perdatum->value > common_pts_len)
+				break;
+			if (strcmp(name, defined_classes_perms.av_inherit[i].
+			     common_pts[perdatum->value - 1])) {
+				printk(KERN_ERR
+				       "security:  the value of permission %s is incorrect",
+				       (char *)key);
+				rc = -EINVAL;
+			}
+			break;
+		}
+	}
+	return rc;
+}
+
+/*
+ * Verify that each kernel class that is defined in the
+ * policy is correct
+ */
+static int validate_class(void *key, void *datum, void *p)
+{
+	struct class_datum *cladatum;
+	int rc, i;
+	char *name;
+	u8 has_common = 0;
+
+	cladatum = datum;
+	name = key;
+
+	if (cladatum->value >= defined_classes_perms.class_to_string_len) {
+		rc = 0;
+		goto out;
+	}
+	if (strcmp(name, defined_classes_perms.class_to_string[cladatum->value])) {
+		printk(KERN_ERR "security:  the value of class %s is incorrect\n",
+		       (char *)key);
+		rc = -EINVAL;
+		goto out;
+	}
+	for (i = 0; i < defined_classes_perms.av_inherit_len; i++) {
+		if (defined_classes_perms.av_inherit[i].tclass == cladatum->value) {
+			has_common = 1;
+			break;
+		}
+	}
+	if (!((cladatum->comdatum && has_common) ||
+	      (!cladatum->comdatum && !has_common))) {
+		if (has_common) {
+			printk(KERN_ERR
+			       "security:  the access vector definition for class"
+			       "%s should have an inherits clause but does not\n",
+			       (char *)key);
+		} else {
+			printk(KERN_ERR
+			       "security:  the access vector definition for class"
+			       "%s should not have an inherits clause but does\n",
+			       (char *)key);
+		}
+		rc = -EINVAL;
+		goto out;
+	}
+	if (cladatum->comdatum) {
+		rc = hashtab_map(cladatum->comdatum->permissions.table,
+				 validate_common_perm, &cladatum->value);
+		if (rc) {
+			printk(" in the access vector definition for class "
+			       "%s\n", (char *)key);
+			goto out;
+		}
+	}
+	rc = hashtab_map(cladatum->permissions.table, validate_class_perm,
+			 &cladatum->value);
+	if (rc)
+		printk(" in access vector definition for class %s\n",
+		       (char *)key);
+out:
+	return rc;
+}
+
 /* Clone the SID into the new SID table. */
 static int clone_sid(u32 sid,
 		     struct context *context,
@@ -1160,6 +1301,16 @@ int security_load_policy(void *data, siz
 			avtab_cache_destroy();
 			return -EINVAL;
 		}
+		/* Verify that the kernel defined classes are correct. */
+		if (hashtab_map(policydb.p_classes.table, validate_class, NULL)) {
+			printk(KERN_ERR
+			       "security:  the definition of a class is incorrect\n");
+			LOAD_UNLOCK;
+			sidtab_destroy(&sidtab);
+			policydb_destroy(&policydb);
+			avtab_cache_destroy();
+			return -EINVAL;
+		}
 		policydb_loaded_version = policydb.policyvers;
 		ss_initialized = 1;
 		seqno = ++latest_granting;
@@ -1182,6 +1333,14 @@ int security_load_policy(void *data, siz
 
 	sidtab_init(&newsidtab);
 
+	/* Verify that the kernel defined classes are correct. */
+	if (hashtab_map(newpolicydb.p_classes.table, validate_class, NULL)) {
+		printk(KERN_ERR
+		       "security:  the definition of a class is incorrect\n");
+		rc = -EINVAL;
+		goto err;
+	}
+
 	/* Clone the SID table. */
 	sidtab_shutdown(&sidtab);
 	if (sidtab_map(&sidtab, clone_sid, &newsidtab)) {

--

--
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: [PATCH 4/4] Validate kernel object classes and permissions

[Stephen Smalley]

On Fri, 2006-10-06 at 16:09 -0400, Chad Sellers wrote:
> plain text document attachment (verify_kernel.diff)
> This patch replaces the object class and permission validation code to
> validate against the defined kernel headers, and allow extra classes
> and permissions that do not conflict with the kernel definitions. Note
> that validation is now done for all policy loads, not just subsequent
> loads after the first policy load.

I haven't looked closely, but it doesn't quite look right to me.  The
old logic would walk the "good" definitions from the old policy,
checking each one against the definitions in the new policy.  This logic
appears to walk the new policy, checking each of its definitions against
the kernel tables.  But what if the new policy simply omits a definition
required by the kernel, e.g. delete the last class or the last five
classes altogether from policy?  Interestingly, the old logic includes
messages like "permission %s disappeared" or "class %s disappeared" but
this logic does not (only messages about incorrect values).  I think you
want it the other way around, i.e. walk the kernel tables and look up
each definition there in the policy being loaded, then see if it is
correct.  That also avoids having to skip around things that are defined
in policy but not in the kernel tables.

> Signed-off-by: Chad Sellers <csellers@tresys.com>
> 
> ---
>  security/selinux/ss/services.c |  161 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 160 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6-discovery/security/selinux/ss/services.c
> ===================================================================
> --- linux-2.6-discovery.orig/security/selinux/ss/services.c
> +++ linux-2.6-discovery/security/selinux/ss/services.c
> @@ -1018,6 +1027,138 @@ int security_change_sid(u32 ssid,
>  	return security_compute_sid(ssid, tsid, tclass, AVTAB_CHANGE, out_sid);
>  }
>  
> +/*
> + * Verify that each kernel permission that is defined in a
> + * policy class is correct
> + */
> +static int validate_class_perm(void *key, void *datum, void *p)
> +{
> +	struct perm_datum *perdatum;
> +	u32 *class, val;
> +	char *name;
> +	int rc = 0, i;
> +
> +	class = p;
> +	perdatum = datum;
> +	name = key;
> +	val = 1 << (perdatum->value - 1);
> +
> +	for (i = 0; i < defined_classes_perms.av_perm_to_string_len; i++) {
> +		if ((defined_classes_perms.av_perm_to_string[i].tclass == *class)
> +		    && (defined_classes_perms.av_perm_to_string[i].value == val)) {
> +			if (strcmp(name, defined_classes_perms.av_perm_to_string[i].name)) {
> +				printk(KERN_ERR
> +				       "security:  the value of permission %s is incorrect",
> +				       (char *)key);
> +				rc = -EINVAL;
> +			}
> +			break;
> +		}
> +	}
> +	return rc;
> +}
> +
> +/*
> + * Verify that each kernel permission that is defined in a
> + * policy common is correct
> + */
> +static int validate_common_perm(void *key, void *datum, void *p)
> +{
> +	struct perm_datum *perdatum;
> +	u32 *class, tmp, common_pts_len = 0;
> +	char *name;
> +	int rc = 0, i;
> +
> +	class = p;
> +	perdatum = datum;
> +	name = key;
> +
> +	for (i = 0; i < defined_classes_perms.av_inherit_len; i++) {
> +		if (defined_classes_perms.av_inherit[i].tclass == *class) {
> +			tmp = defined_classes_perms.av_inherit[i].common_base;
> +			while (!(tmp & 0x01)) {
> +				common_pts_len++;
> +				tmp >>= 1;
> +			}
> +			if (perdatum->value > common_pts_len)
> +				break;
> +			if (strcmp(name, defined_classes_perms.av_inherit[i].
> +			     common_pts[perdatum->value - 1])) {
> +				printk(KERN_ERR
> +				       "security:  the value of permission %s is incorrect",
> +				       (char *)key);
> +				rc = -EINVAL;
> +			}
> +			break;
> +		}
> +	}
> +	return rc;
> +}
> +
> +/*
> + * Verify that each kernel class that is defined in the
> + * policy is correct
> + */
> +static int validate_class(void *key, void *datum, void *p)
> +{
> +	struct class_datum *cladatum;
> +	int rc, i;
> +	char *name;
> +	u8 has_common = 0;
> +
> +	cladatum = datum;
> +	name = key;
> +
> +	if (cladatum->value >= defined_classes_perms.class_to_string_len) {
> +		rc = 0;
> +		goto out;
> +	}
> +	if (strcmp(name, defined_classes_perms.class_to_string[cladatum->value])) {
> +		printk(KERN_ERR "security:  the value of class %s is incorrect\n",
> +		       (char *)key);
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +	for (i = 0; i < defined_classes_perms.av_inherit_len; i++) {
> +		if (defined_classes_perms.av_inherit[i].tclass == cladatum->value) {
> +			has_common = 1;
> +			break;
> +		}
> +	}
> +	if (!((cladatum->comdatum && has_common) ||
> +	      (!cladatum->comdatum && !has_common))) {
> +		if (has_common) {
> +			printk(KERN_ERR
> +			       "security:  the access vector definition for class"
> +			       "%s should have an inherits clause but does not\n",
> +			       (char *)key);
> +		} else {
> +			printk(KERN_ERR
> +			       "security:  the access vector definition for class"
> +			       "%s should not have an inherits clause but does\n",
> +			       (char *)key);
> +		}
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +	if (cladatum->comdatum) {
> +		rc = hashtab_map(cladatum->comdatum->permissions.table,
> +				 validate_common_perm, &cladatum->value);
> +		if (rc) {
> +			printk(" in the access vector definition for class "
> +			       "%s\n", (char *)key);
> +			goto out;
> +		}
> +	}
> +	rc = hashtab_map(cladatum->permissions.table, validate_class_perm,
> +			 &cladatum->value);
> +	if (rc)
> +		printk(" in access vector definition for class %s\n",
> +		       (char *)key);
> +out:
> +	return rc;
> +}
> +
>  /* Clone the SID into the new SID table. */
>  static int clone_sid(u32 sid,
>  		     struct context *context,
> @@ -1160,6 +1301,16 @@ int security_load_policy(void *data, siz
>  			avtab_cache_destroy();
>  			return -EINVAL;
>  		}
> +		/* Verify that the kernel defined classes are correct. */
> +		if (hashtab_map(policydb.p_classes.table, validate_class, NULL)) {
> +			printk(KERN_ERR
> +			       "security:  the definition of a class is incorrect\n");
> +			LOAD_UNLOCK;
> +			sidtab_destroy(&sidtab);
> +			policydb_destroy(&policydb);
> +			avtab_cache_destroy();
> +			return -EINVAL;
> +		}
>  		policydb_loaded_version = policydb.policyvers;
>  		ss_initialized = 1;
>  		seqno = ++latest_granting;
> @@ -1182,6 +1333,14 @@ int security_load_policy(void *data, siz
>  
>  	sidtab_init(&newsidtab);
>  
> +	/* Verify that the kernel defined classes are correct. */
> +	if (hashtab_map(newpolicydb.p_classes.table, validate_class, NULL)) {
> +		printk(KERN_ERR
> +		       "security:  the definition of a class is incorrect\n");
> +		rc = -EINVAL;
> +		goto err;
> +	}
> +
>  	/* Clone the SID table. */
>  	sidtab_shutdown(&sidtab);
>  	if (sidtab_map(&sidtab, clone_sid, &newsidtab)) {
> 
> --
> 
> --
> 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.
-- 
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: [PATCH 4/4] Validate kernel object classes and permissions

[Chad Sellers]

On 10/6/06 5:37 PM, "Stephen Smalley" <sds@tycho.nsa.gov> wrote:

> On Fri, 2006-10-06 at 16:09 -0400, Chad Sellers wrote:
>> plain text document attachment (verify_kernel.diff)
>> This patch replaces the object class and permission validation code to
>> validate against the defined kernel headers, and allow extra classes
>> and permissions that do not conflict with the kernel definitions. Note
>> that validation is now done for all policy loads, not just subsequent
>> loads after the first policy load.
> 
> I haven't looked closely, but it doesn't quite look right to me.  The
> old logic would walk the "good" definitions from the old policy,
> checking each one against the definitions in the new policy.  This logic
> appears to walk the new policy, checking each of its definitions against
> the kernel tables.  But what if the new policy simply omits a definition
> required by the kernel, e.g. delete the last class or the last five
> classes altogether from policy?  Interestingly, the old logic includes
> messages like "permission %s disappeared" or "class %s disappeared" but
> this logic does not (only messages about incorrect values).  I think you
> want it the other way around, i.e. walk the kernel tables and look up
> each definition there in the policy being loaded, then see if it is
> correct.  That also avoids having to skip around things that are defined
> in policy but not in the kernel tables.
> 
I don't think that's the logic we want, though. Part of the eventual idea
here is to help out the "Andrew Morton syndrome," where a kernel developer
has a new kernel class/permission, but no policy for it. If we require all
kernel defined classes/perms to be present in the policy, then we can't
support this situation. The logic in my patch requires that all the kernel
classes defined in the policy are consistent with the kernel, but allows for
the possibility of the kernel having new classes/permissions that aren't in
policy yet. The logic you propose would not allow a kernel developer to load
a policy until that policy supported their new object class/permission.

Chad


>> Signed-off-by: Chad Sellers <csellers@tresys.com>
>> 
>> ---
>>  security/selinux/ss/services.c |  161
>> ++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 160 insertions(+), 1 deletion(-)
>> 
>> Index: linux-2.6-discovery/security/selinux/ss/services.c
>> ===================================================================
>> --- linux-2.6-discovery.orig/security/selinux/ss/services.c
>> +++ linux-2.6-discovery/security/selinux/ss/services.c
>> @@ -1018,6 +1027,138 @@ int security_change_sid(u32 ssid,
>> return security_compute_sid(ssid, tsid, tclass, AVTAB_CHANGE, out_sid);
>>  }
>>  
>> +/*
>> + * Verify that each kernel permission that is defined in a
>> + * policy class is correct
>> + */
>> +static int validate_class_perm(void *key, void *datum, void *p)
>> +{
>> + struct perm_datum *perdatum;
>> + u32 *class, val;
>> + char *name;
>> + int rc = 0, i;
>> +
>> + class = p;
>> + perdatum = datum;
>> + name = key;
>> + val = 1 << (perdatum->value - 1);
>> +
>> + for (i = 0; i < defined_classes_perms.av_perm_to_string_len; i++) {
>> +  if ((defined_classes_perms.av_perm_to_string[i].tclass == *class)
>> +      && (defined_classes_perms.av_perm_to_string[i].value == val)) {
>> +   if (strcmp(name, defined_classes_perms.av_perm_to_string[i].name)) {
>> +    printk(KERN_ERR
>> +           "security:  the value of permission %s is incorrect",
>> +           (char *)key);
>> +    rc = -EINVAL;
>> +   }
>> +   break;
>> +  }
>> + }
>> + return rc;
>> +}
>> +
>> +/*
>> + * Verify that each kernel permission that is defined in a
>> + * policy common is correct
>> + */
>> +static int validate_common_perm(void *key, void *datum, void *p)
>> +{
>> + struct perm_datum *perdatum;
>> + u32 *class, tmp, common_pts_len = 0;
>> + char *name;
>> + int rc = 0, i;
>> +
>> + class = p;
>> + perdatum = datum;
>> + name = key;
>> +
>> + for (i = 0; i < defined_classes_perms.av_inherit_len; i++) {
>> +  if (defined_classes_perms.av_inherit[i].tclass == *class) {
>> +   tmp = defined_classes_perms.av_inherit[i].common_base;
>> +   while (!(tmp & 0x01)) {
>> +    common_pts_len++;
>> +    tmp >>= 1;
>> +   }
>> +   if (perdatum->value > common_pts_len)
>> +    break;
>> +   if (strcmp(name, defined_classes_perms.av_inherit[i].
>> +        common_pts[perdatum->value - 1])) {
>> +    printk(KERN_ERR
>> +           "security:  the value of permission %s is incorrect",
>> +           (char *)key);
>> +    rc = -EINVAL;
>> +   }
>> +   break;
>> +  }
>> + }
>> + return rc;
>> +}
>> +
>> +/*
>> + * Verify that each kernel class that is defined in the
>> + * policy is correct
>> + */
>> +static int validate_class(void *key, void *datum, void *p)
>> +{
>> + struct class_datum *cladatum;
>> + int rc, i;
>> + char *name;
>> + u8 has_common = 0;
>> +
>> + cladatum = datum;
>> + name = key;
>> +
>> + if (cladatum->value >= defined_classes_perms.class_to_string_len) {
>> +  rc = 0;
>> +  goto out;
>> + }
>> + if (strcmp(name, defined_classes_perms.class_to_string[cladatum->value])) {
>> +  printk(KERN_ERR "security:  the value of class %s is incorrect\n",
>> +         (char *)key);
>> +  rc = -EINVAL;
>> +  goto out;
>> + }
>> + for (i = 0; i < defined_classes_perms.av_inherit_len; i++) {
>> +  if (defined_classes_perms.av_inherit[i].tclass == cladatum->value) {
>> +   has_common = 1;
>> +   break;
>> +  }
>> + }
>> + if (!((cladatum->comdatum && has_common) ||
>> +       (!cladatum->comdatum && !has_common))) {
>> +  if (has_common) {
>> +   printk(KERN_ERR
>> +          "security:  the access vector definition for class"
>> +          "%s should have an inherits clause but does not\n",
>> +          (char *)key);
>> +  } else {
>> +   printk(KERN_ERR
>> +          "security:  the access vector definition for class"
>> +          "%s should not have an inherits clause but does\n",
>> +          (char *)key);
>> +  }
>> +  rc = -EINVAL;
>> +  goto out;
>> + }
>> + if (cladatum->comdatum) {
>> +  rc = hashtab_map(cladatum->comdatum->permissions.table,
>> +     validate_common_perm, &cladatum->value);
>> +  if (rc) {
>> +   printk(" in the access vector definition for class "
>> +          "%s\n", (char *)key);
>> +   goto out;
>> +  }
>> + }
>> + rc = hashtab_map(cladatum->permissions.table, validate_class_perm,
>> +    &cladatum->value);
>> + if (rc)
>> +  printk(" in access vector definition for class %s\n",
>> +         (char *)key);
>> +out:
>> + return rc;
>> +}
>> +
>>  /* Clone the SID into the new SID table. */
>>  static int clone_sid(u32 sid,
>>     struct context *context,
>> @@ -1160,6 +1301,16 @@ int security_load_policy(void *data, siz
>> avtab_cache_destroy();
>> return -EINVAL;
>> }
>> +  /* Verify that the kernel defined classes are correct. */
>> +  if (hashtab_map(policydb.p_classes.table, validate_class, NULL)) {
>> +   printk(KERN_ERR
>> +          "security:  the definition of a class is incorrect\n");
>> +   LOAD_UNLOCK;
>> +   sidtab_destroy(&sidtab);
>> +   policydb_destroy(&policydb);
>> +   avtab_cache_destroy();
>> +   return -EINVAL;
>> +  }
>> policydb_loaded_version = policydb.policyvers;
>> ss_initialized = 1;
>> seqno = ++latest_granting;
>> @@ -1182,6 +1333,14 @@ int security_load_policy(void *data, siz
>>  
>> sidtab_init(&newsidtab);
>>  
>> + /* Verify that the kernel defined classes are correct. */
>> + if (hashtab_map(newpolicydb.p_classes.table, validate_class, NULL)) {
>> +  printk(KERN_ERR
>> +         "security:  the definition of a class is incorrect\n");
>> +  rc = -EINVAL;
>> +  goto err;
>> + }
>> +
>> /* Clone the SID table. */
>> sidtab_shutdown(&sidtab);
>> if (sidtab_map(&sidtab, clone_sid, &newsidtab)) {
>> 
>> --
>> 
>> --
>> 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: [PATCH 4/4] Validate kernel object classes and permissions

[Stephen Smalley]

On Fri, 2006-10-06 at 18:25 -0400, Chad Sellers wrote:
> On 10/6/06 5:37 PM, "Stephen Smalley" <sds@tycho.nsa.gov> wrote:
> 
> > On Fri, 2006-10-06 at 16:09 -0400, Chad Sellers wrote:
> >> plain text document attachment (verify_kernel.diff)
> >> This patch replaces the object class and permission validation code to
> >> validate against the defined kernel headers, and allow extra classes
> >> and permissions that do not conflict with the kernel definitions. Note
> >> that validation is now done for all policy loads, not just subsequent
> >> loads after the first policy load.
> > 
> > I haven't looked closely, but it doesn't quite look right to me.  The
> > old logic would walk the "good" definitions from the old policy,
> > checking each one against the definitions in the new policy.  This logic
> > appears to walk the new policy, checking each of its definitions against
> > the kernel tables.  But what if the new policy simply omits a definition
> > required by the kernel, e.g. delete the last class or the last five
> > classes altogether from policy?  Interestingly, the old logic includes
> > messages like "permission %s disappeared" or "class %s disappeared" but
> > this logic does not (only messages about incorrect values).  I think you
> > want it the other way around, i.e. walk the kernel tables and look up
> > each definition there in the policy being loaded, then see if it is
> > correct.  That also avoids having to skip around things that are defined
> > in policy but not in the kernel tables.
> > 
> I don't think that's the logic we want, though. Part of the eventual idea
> here is to help out the "Andrew Morton syndrome," where a kernel developer
> has a new kernel class/permission, but no policy for it. If we require all
> kernel defined classes/perms to be present in the policy, then we can't
> support this situation. The logic in my patch requires that all the kernel
> classes defined in the policy are consistent with the kernel, but allows for
> the possibility of the kernel having new classes/permissions that aren't in
> policy yet. The logic you propose would not allow a kernel developer to load
> a policy until that policy supported their new object class/permission.

The only thing that changes in that future situation (for which no patch
yet exists, and thus this patch should not presume it) is how one
handles the absence of a kernel class or permission in the policy, not
the logic for detecting such an absence.  Your logic won't even detect
the absence, and thus cannot properly deal with it no matter how we want
it handled, whether that means rejecting the policy altogether,
accepting the policy and permitting the undefined permissions, or
accepting the policy and denying the undefined permissions.  Note too
that the handler will likely remain conditional on some policy-defined
flag, so we will always need such detection logic even in that future
situation.  Until we have support for gracefully handling a missing
class or permission definition, we want to retain the safety property of
rejecting such policies.

-- 
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: [PATCH 4/4] Validate kernel object classes and permissions

[Chad Sellers]

On 10/10/06 9:25 AM, "Stephen Smalley" <sds@tycho.nsa.gov> wrote:

> On Fri, 2006-10-06 at 18:25 -0400, Chad Sellers wrote:
>> On 10/6/06 5:37 PM, "Stephen Smalley" <sds@tycho.nsa.gov> wrote:
>> 
>>> On Fri, 2006-10-06 at 16:09 -0400, Chad Sellers wrote:
>>>> plain text document attachment (verify_kernel.diff)
>>>> This patch replaces the object class and permission validation code to
>>>> validate against the defined kernel headers, and allow extra classes
>>>> and permissions that do not conflict with the kernel definitions. Note
>>>> that validation is now done for all policy loads, not just subsequent
>>>> loads after the first policy load.
>>> 
>>> I haven't looked closely, but it doesn't quite look right to me.  The
>>> old logic would walk the "good" definitions from the old policy,
>>> checking each one against the definitions in the new policy.  This logic
>>> appears to walk the new policy, checking each of its definitions against
>>> the kernel tables.  But what if the new policy simply omits a definition
>>> required by the kernel, e.g. delete the last class or the last five
>>> classes altogether from policy?  Interestingly, the old logic includes
>>> messages like "permission %s disappeared" or "class %s disappeared" but
>>> this logic does not (only messages about incorrect values).  I think you
>>> want it the other way around, i.e. walk the kernel tables and look up
>>> each definition there in the policy being loaded, then see if it is
>>> correct.  That also avoids having to skip around things that are defined
>>> in policy but not in the kernel tables.
>>> 
>> I don't think that's the logic we want, though. Part of the eventual idea
>> here is to help out the "Andrew Morton syndrome," where a kernel developer
>> has a new kernel class/permission, but no policy for it. If we require all
>> kernel defined classes/perms to be present in the policy, then we can't
>> support this situation. The logic in my patch requires that all the kernel
>> classes defined in the policy are consistent with the kernel, but allows for
>> the possibility of the kernel having new classes/permissions that aren't in
>> policy yet. The logic you propose would not allow a kernel developer to load
>> a policy until that policy supported their new object class/permission.
> 
> The only thing that changes in that future situation (for which no patch
> yet exists, and thus this patch should not presume it) is how one
> handles the absence of a kernel class or permission in the policy, not
> the logic for detecting such an absence.  Your logic won't even detect
> the absence, and thus cannot properly deal with it no matter how we want
> it handled, whether that means rejecting the policy altogether,
> accepting the policy and permitting the undefined permissions, or
> accepting the policy and denying the undefined permissions.  Note too
> that the handler will likely remain conditional on some policy-defined
> flag, so we will always need such detection logic even in that future
> situation.  Until we have support for gracefully handling a missing
> class or permission definition, we want to retain the safety property of
> rejecting such policies.

You're right. The reality is that this wouldn't have supported the future
case anyway, as it would improperly handle the absence of a new kernel class
combined with the presence of a userspace class using the same value. So,
please ignore the last 3 patches of this patch set (the bugfix in patch 1
should stay, though).

Thanks,
Chad Sellers


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