Fix Checkmodule when Writing down to older Module Versions

Fix Checkmodule when Writing down to older Module Versions

[Matthew Ife]

[-- Attachment #1: Type: text/plain, Size: 1265 bytes --]

Stumbled over this one when writing a module from F33 to EL6.

Steps to reproduce:

Try to build the following module then make a module from an older
release:

module test 1.0.0;

require {
  type default_t;
}
attribute_role new_atrole;

Build

$ checkmodule -M -m -c 12 -o test.mod test.te
$ semodule_package -o test.pp -m test.mod
$ semodule_package:  Error while reading policy module from test.mod

With fix:

$ checkmodule -o test.mod -M -m -c12 test.te 
libsepol.policydb_write: Discarding role attribute rules
$ semodule_package -o test.pp -m test.mod

Failure occurs when the current module gets written out as the scope
declaration remains intact.
semodule_package files correctly at policydb.c:3913 doing a hash table
search on a scope key that is not
in the symbol table.

This patch fixes the problem by removing the hashtable entries and
scope declarations properly prior to module write and emits a warning
to the user of the unsupported statements.

Also altered hashtab_map slightly to allow it to be used for
hashtab_remove calls in order to support the patch.

I submitted a pull request for this (there is some spacing/tabbing
issues actually so I can resent a new pull request if necessary).

https://github.com/SELinuxProject/selinux/pull/273

[-- Attachment #2: fix_downwriting_module.patch --]
[-- Type: text/x-patch, Size: 3972 bytes --]

diff --git a/libsepol/src/hashtab.c b/libsepol/src/hashtab.c
index 76b977a9..ff7ef63f 100644
--- a/libsepol/src/hashtab.c
+++ b/libsepol/src/hashtab.c
@@ -230,7 +230,7 @@ int hashtab_map(hashtab_t h,
 
 	for (i = 0; i < h->size; i++) {
 		cur = h->htable[i];
-		while (curi != NULL) {
+		while (cur != NULL) {
 			next = cur->next;
 			ret = apply(cur->key, cur->datum, args);
 			if (ret)
diff --git a/libsepol/src/write.c b/libsepol/src/write.c
index dd418317..6a59a0c3 100644
--- a/libsepol/src/write.c
+++ b/libsepol/src/write.c
@@ -2170,7 +2170,7 @@ static void scope_write_destroy(hashtab_key_t key __attribute__ ((unused)),
     free(cur);
 }
 
-static void type_attr_filter(hashtab_key_t key,
+static int type_attr_filter(hashtab_key_t key,
                  hashtab_datum_t datum, void *args)
 {
     type_datum_t *typdatum = datum;
@@ -2186,9 +2186,11 @@ static void type_attr_filter(hashtab_key_t key,
         if (scope) 
             hashtab_remove(scopetbl, key, scope_write_destroy, scope);
     }
+
+	return 0;
 }
 
-static void role_attr_filter(hashtab_key_t key,
+static int role_attr_filter(hashtab_key_t key,
                  hashtab_datum_t datum, void *args)
 {
     role_datum_t *role = datum;
@@ -2204,6 +2206,8 @@ static void role_attr_filter(hashtab_key_t key,
         if (scope) 
             hashtab_remove(scopetbl, key, scope_write_destroy, scope);
     }
+
+	return 0;
 }
 
 /*
@@ -2337,36 +2341,36 @@ int policydb_write(policydb_t * p, struct policy_file *fp)
 		buf[0] = cpu_to_le32(p->symtab[i].nprim);
 		buf[1] = p->symtab[i].table->nel;
 
-        /*
-         * A special case when writing type/attribute symbol table.
-         * The kernel policy version less than 24 does not support
-         * to load entries of attribute, so we filter the entries
-         * from the table.
-         */
-        if (i == SYM_TYPES &&
-            p->policyvers < POLICYDB_VERSION_BOUNDARY &&
-            p->policy_type == POLICY_KERN) {
-            (void)hashtab_map(p->symtab[i].table, type_attr_filter, p);
-            if (buf[1] != p->symtab[i].table->nel)
+		/*
+		* A special case when writing type/attribute symbol table.
+		* The kernel policy version less than 24 does not support
+		* to load entries of attribute, so we filter the entries
+		* from the table.
+		*/
+		if (i == SYM_TYPES &&
+			p->policyvers < POLICYDB_VERSION_BOUNDARY &&
+			p->policy_type == POLICY_KERN) {
+			(void)hashtab_map(p->symtab[i].table, type_attr_filter, p);
+			if (buf[1] != p->symtab[i].table->nel)
                 WARN(fp->handle, "Discarding type attribute rules");
-            buf[1] = p->symtab[i].table->nel;
-        }
-
-        /*
-         * Another special case when writing role/attribute symbol
-         * table, role attributes are redundant for policy.X, or
-         * when the pp's version is not big enough. So filter the entries
-         * from the table.
-         */
-        if ((i == SYM_ROLES) &&
-            ((p->policy_type == POLICY_KERN) ||
-             (p->policy_type != POLICY_KERN &&
-              p->policyvers < MOD_POLICYDB_VERSION_ROLEATTRIB))) {
-            (void)hashtab_map(p->symtab[i].table, role_attr_filter, p);
+			buf[1] = p->symtab[i].table->nel;
+		}
+
+	/*
+		* Another special case when writing role/attribute symbol
+		* table, role attributes are redundant for policy.X, or
+		* when the pp's version is not big enough. So filter the entries
+		* from the table.
+		*/
+		if ((i == SYM_ROLES) &&
+			((p->policy_type == POLICY_KERN) ||
+			(p->policy_type != POLICY_KERN &&
+			p->policyvers < MOD_POLICYDB_VERSION_ROLEATTRIB))) {
+			(void)hashtab_map(p->symtab[i].table, role_attr_filter, p);
 			if (buf[1] != p->symtab[i].table->nel)
 				WARN(fp->handle, "Discarding role attribute rules");
-            buf[1] = p->symtab[i].table->nel;
-        }
+			buf[1] = p->symtab[i].table->nel;
+		}
 
 		buf[1] = cpu_to_le32(buf[1]);
 		items = put_entry(buf, sizeof(uint32_t), 2, fp);

Re: Fix Checkmodule when Writing down to older Module Versions

[Petr Lautrbach]

Matthew Ife <matthew@ife.onl> writes:

> Stumbled over this one when writing a module from F33 to EL6.
>
> Steps to reproduce:
>
> Try to build the following module then make a module from an older
> release:
>
> module test 1.0.0;
>
> require {
>   type default_t;
> }
> attribute_role new_atrole;
>
> Build
>
> $ checkmodule -M -m -c 12 -o test.mod test.te
> $ semodule_package -o test.pp -m test.mod
> $ semodule_package:  Error while reading policy module from test.mod
>
> With fix:
>
> $ checkmodule -o test.mod -M -m -c12 test.te 
> libsepol.policydb_write: Discarding role attribute rules
> $ semodule_package -o test.pp -m test.mod
>
> Failure occurs when the current module gets written out as the scope
> declaration remains intact.
> semodule_package files correctly at policydb.c:3913 doing a hash table
> search on a scope key that is not
> in the symbol table.
>
> This patch fixes the problem by removing the hashtable entries and
> scope declarations properly prior to module write and emits a warning
> to the user of the unsupported statements.
>
> Also altered hashtab_map slightly to allow it to be used for
> hashtab_remove calls in order to support the patch.
>
> I submitted a pull request for this (there is some spacing/tabbing
> issues actually so I can resent a new pull request if necessary).
>
> https://github.com/SELinuxProject/selinux/pull/273


Hello,

This particular patch can't be currently applied to master's HEAD and
it's also missing Signed-of-by: line

It looks like you've rebased it in
https://github.com/SELinuxProject/selinux/pull/273 , added Signed-of-by
and also added more commits. When you think it's a final version, please
merge/squash related commits - e.g. "Retab me" looks like fix up of the first
commit - and resend the patches or patches again to this list for
review.

Thanks!




> diff --git a/libsepol/src/hashtab.c b/libsepol/src/hashtab.c
> index 76b977a9..ff7ef63f 100644
> --- a/libsepol/src/hashtab.c
> +++ b/libsepol/src/hashtab.c
> @@ -230,7 +230,7 @@ int hashtab_map(hashtab_t h,
>  
>  	for (i = 0; i < h->size; i++) {
>  		cur = h->htable[i];
> -		while (curi != NULL) {
> +		while (cur != NULL) {
>  			next = cur->next;
>  			ret = apply(cur->key, cur->datum, args);
>  			if (ret)
> diff --git a/libsepol/src/write.c b/libsepol/src/write.c
> index dd418317..6a59a0c3 100644
> --- a/libsepol/src/write.c
> +++ b/libsepol/src/write.c
> @@ -2170,7 +2170,7 @@ static void scope_write_destroy(hashtab_key_t key __attribute__ ((unused)),
>      free(cur);
>  }
>  
> -static void type_attr_filter(hashtab_key_t key,
> +static int type_attr_filter(hashtab_key_t key,
>                   hashtab_datum_t datum, void *args)
>  {
>      type_datum_t *typdatum = datum;
> @@ -2186,9 +2186,11 @@ static void type_attr_filter(hashtab_key_t key,
>          if (scope) 
>              hashtab_remove(scopetbl, key, scope_write_destroy, scope);
>      }
> +
> +	return 0;
>  }
>  
> -static void role_attr_filter(hashtab_key_t key,
> +static int role_attr_filter(hashtab_key_t key,
>                   hashtab_datum_t datum, void *args)
>  {
>      role_datum_t *role = datum;
> @@ -2204,6 +2206,8 @@ static void role_attr_filter(hashtab_key_t key,
>          if (scope) 
>              hashtab_remove(scopetbl, key, scope_write_destroy, scope);
>      }
> +
> +	return 0;
>  }
>  
>  /*
> @@ -2337,36 +2341,36 @@ int policydb_write(policydb_t * p, struct policy_file *fp)
>  		buf[0] = cpu_to_le32(p->symtab[i].nprim);
>  		buf[1] = p->symtab[i].table->nel;
>  
> -        /*
> -         * A special case when writing type/attribute symbol table.
> -         * The kernel policy version less than 24 does not support
> -         * to load entries of attribute, so we filter the entries
> -         * from the table.
> -         */
> -        if (i == SYM_TYPES &&
> -            p->policyvers < POLICYDB_VERSION_BOUNDARY &&
> -            p->policy_type == POLICY_KERN) {
> -            (void)hashtab_map(p->symtab[i].table, type_attr_filter, p);
> -            if (buf[1] != p->symtab[i].table->nel)
> +		/*
> +		* A special case when writing type/attribute symbol table.
> +		* The kernel policy version less than 24 does not support
> +		* to load entries of attribute, so we filter the entries
> +		* from the table.
> +		*/
> +		if (i == SYM_TYPES &&
> +			p->policyvers < POLICYDB_VERSION_BOUNDARY &&
> +			p->policy_type == POLICY_KERN) {
> +			(void)hashtab_map(p->symtab[i].table, type_attr_filter, p);
> +			if (buf[1] != p->symtab[i].table->nel)
>                  WARN(fp->handle, "Discarding type attribute rules");
> -            buf[1] = p->symtab[i].table->nel;
> -        }
> -
> -        /*
> -         * Another special case when writing role/attribute symbol
> -         * table, role attributes are redundant for policy.X, or
> -         * when the pp's version is not big enough. So filter the entries
> -         * from the table.
> -         */
> -        if ((i == SYM_ROLES) &&
> -            ((p->policy_type == POLICY_KERN) ||
> -             (p->policy_type != POLICY_KERN &&
> -              p->policyvers < MOD_POLICYDB_VERSION_ROLEATTRIB))) {
> -            (void)hashtab_map(p->symtab[i].table, role_attr_filter, p);
> +			buf[1] = p->symtab[i].table->nel;
> +		}
> +
> +	/*
> +		* Another special case when writing role/attribute symbol
> +		* table, role attributes are redundant for policy.X, or
> +		* when the pp's version is not big enough. So filter the entries
> +		* from the table.
> +		*/
> +		if ((i == SYM_ROLES) &&
> +			((p->policy_type == POLICY_KERN) ||
> +			(p->policy_type != POLICY_KERN &&
> +			p->policyvers < MOD_POLICYDB_VERSION_ROLEATTRIB))) {
> +			(void)hashtab_map(p->symtab[i].table, role_attr_filter, p);
>  			if (buf[1] != p->symtab[i].table->nel)
>  				WARN(fp->handle, "Discarding role attribute rules");
> -            buf[1] = p->symtab[i].table->nel;
> -        }
> +			buf[1] = p->symtab[i].table->nel;
> +		}
>  
>  		buf[1] = cpu_to_le32(buf[1]);
>  		items = put_entry(buf, sizeof(uint32_t), 2, fp);