Re: [PATCH-v3] libsepol: allow genfscon statements in modules

[Joshua Brindle <method@manicmethod.com>]

Eric Paris wrote:
> This patch provides the libsepol support for the usage of genfscon
> statements in policy modules.  The module must declare/require all of
> the components of the context associated with the declaration but the
> actual validation of that context is delayed until link time.
>
> Comments and criticism appreciated.  (note that this patch may require
> the recent bug fix from sds for mls_level_convert())
>
>   

This work is appreciated, though I wonder if it is enough to just have 
genfscon in modules or if they should be part of the optional block 
grammar (and this part of the avrule_block struct, etc). This, of 
course, would require a module version bump. Other comments below.

> Signed-off-by: Eric Paris <eparis@redhat.com>
> ---
>
> Example module with genfscon statements:
>
> require {
> 	user user_u;
> 	role role_r;
> 	type type2_t;
> 	type type1_t;
> 	sensitivity s0;
> }
>
> genfscon proc /paris0000	user_u:role_r:type1_t:s0
> genfscon proc /paris4444	user_u:role_r:type2_t:s0
> genfscon proc /paris00000	user_u:role_r:type1_t:s0
> genfscon proc /			user_u:role_r:type2_t:s0
>
>  src/link.c     |  244 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  src/policydb.c |   30 +++----
>  2 files changed, 259 insertions(+), 15 deletions(-)
>
> diff -Naupr libsepol-2.0.26.orig/src/link.c libsepol-2.0.26/src/link.c
> --- libsepol-2.0.26.orig/src/link.c	2008-03-27 13:20:03.000000000 -0400
> +++ libsepol-2.0.26/src/link.c	2008-05-07 11:00:42.000000000 -0400
> @@ -33,6 +33,7 @@
>  #include <string.h>
>  #include <assert.h>
>  
> +#include "context.h"
>  #include "debug.h"
>  
>  #undef min
> @@ -2120,6 +2125,234 @@ static int prepare_base(link_state_t * s
>  	return 0;
>  }
>  
> +/*
> + * We are working under the assumption that these types now exist in base
> + * at the very least it should have been taken care of by requires blocks
> + * in the policy module or previously been copied into base by the previous
> + * link time operations (so don't call this too early in the linking process
> + */
> +static int context_copy_and_validate(link_state_t *state, context_struct_t *dst, context_struct_t *src)
> +{
> +	user_datum_t *base_user_datum;
> +	role_datum_t *base_role_datum;
> +	type_datum_t *base_type_datum;
> +	char *key;
> +	int ret;
> +
> +	key = state->cur->policy->p_user_val_to_name[src->user - 1];
> +	base_user_datum = hashtab_search(state->base->p_users.table, key);
> +	assert(base_user_datum);
> +	dst->user = base_user_datum->s.value;
> +
> +	key = state->cur->policy->p_role_val_to_name[src->role - 1];
> +	base_role_datum = hashtab_search(state->base->p_roles.table, key);
> +	assert(base_role_datum);
> +	dst->role = base_role_datum->s.value;
> +
> +	key = state->cur->policy->p_type_val_to_name[src->type - 1];
> +	base_type_datum = hashtab_search(state->base->p_types.table, key);
> +	assert(base_type_datum);
> +	dst->type = base_type_datum->s.value;
> +
> +	ret = mls_context_cpy(dst, src);
> +	if (ret)
> +		return ret;
> +
> +	return context_is_valid(state->base, dst);
> +}
> +
> +static void free_genfs(genfs_t *genfs)
>   

I don't think this should go in link.c

> +{
> +	ocontext_t *con;
> +	while (genfs) {
> +		con = genfs->head;
> +		while (con) {
> +			free(con->u.name);
> +			con->u.name = NULL;
> +			free(con);
> +			con = con->next;
> +		}
> +		genfs->head = NULL;
> +		free(genfs->fstype);
> +		genfs->fstype = NULL;
> +		genfs = genfs->next;
> +	}
> +}
> +
> +/* merge 2 genfs sets with the same fstype, we also should free everything we
> + * can if it goes badly */
> +static int genfs_merge(link_state_t *state, genfs_t *genfs, genfs_t *base_genfs)
> +{
> +	ocontext_t *con, *next_con, *base_con, *prev;
> +	int len, base_len;
> +
> +	assert(!strcmp(genfs->fstype, base_genfs->fstype));
> +
> +	con = genfs->head;
> +
> +	/*
> +	 * this loop is a bit odd formed because I have a list of items in genfs
> +	 * that I am trying to merge into the list of items in base_genfs.  So
> +	 * we have to grab next_con before we start inserting the object into
> +	 * the other list
> +	 */
> +	while (con) {
> +		next_con = con->next;
> +		base_con = base_genfs->head;
> +
> +		/* insert at head of list because list is blank */
> +		if (!base_con) {
> +			base_genfs->head = con;
> +			con = next_con;
> +			continue;
> +		}
> +
> +		/* test against the first entry in the list */
> +		len = strlen(con->u.name);
> +		base_len = strlen(base_con->u.name);
> +		/* longer name goes at the head */
> +		if (len > base_len) {
> +			con->next = base_con;
> +			base_genfs->head = con;
> +			con = next_con;
> +			continue;
> +		}
> +
> +		/*
> +		 * walk the base_con list until we need to insert between
> +		 * base_con and base_con->next
> +		 */
> +		while (base_con->next) {
> +			base_len = strlen(base_con->next->u.name);
> +			if (len > base_len)
> +				break;
> +			base_con = base_con->next;
> +			if (len < base_len)
> +				continue;
> +			/* (len == base_len) */
> +			if (!strcmp(con->u.name, base_con->u.name) &&
> +			    ((!con->v.sclass || !base_con->v.sclass || con->v.sclass == base_con->v.sclass))) {
>   

I don't get this. Shouldn't it be ((!con->v.sclass && 
!base_con->v.sclass)) ... or am I missing something?

> +				ERR(state->handle, "dup genfs entry (%s,%s)", genfs->fstype, con->u.name);
> +				/*
> +				 * everything earlier in the list is now associated with
> +				 * the base genfs, so we need to leave all of it alone and
> +				 * let it get freed naturaly when we tear down the base.
> +				 */
> +				while (con) {
> +					free(con->u.name);
> +					prev = con;
> +					con = con->next;
> +					free(prev);
> +				}
> +				free(genfs->fstype);
> +				free(genfs);
> +				return -EINVAL;
> +			}
> +		}
> +
> +		if (!base_con->next || (len > base_len)) {
> +			con->next = base_con->next;
> +			base_con->next = con;
> +			con = next_con;
> +			continue;
> +		} else {
> +			assert(0);
> +		}
> +	}
> +	/* free these since we are using the genfs and fstype already in the base */
> +	free(genfs->fstype);
> +	free(genfs);
> +	return 0;
> +}
> +
> +/* so this is going to merge genfs info from modules into the base policy */
> +static int genfs_context_copy_helper(link_state_t *state)
> +{
> +	ocontext_t *c, *newc, *l;
> +	genfs_t *genfs, *newgenfs, *base_genfs;
> +	int cmp, ret;
> +
> +	for (genfs = state->cur->policy->genfs; genfs; genfs = genfs->next) {
> +		/* build a newgenfs, we might free peices and parts
> +		 * later if the base already had them */
> +		newgenfs = calloc(1, sizeof(genfs_t));
>   

initializer preferred over calloc()

> +		if (!newgenfs) {
> +			ERR(state->handle, "Out of memory!");
> +			return -ENOMEM;
> +		}
> +		newgenfs->fstype = strdup(genfs->fstype);
> +		if (!newgenfs->fstype) {
> +			free_genfs(newgenfs);
> +			ERR(state->handle, "Out of memory!");
> +			return -ENOMEM;
> +		}
> +
> +		l = NULL;
> +		for (c = genfs->head; c; c = c->next) {
> +			newc = calloc(1, sizeof(ocontext_t));
>   

ditto

> +			if (!newc) {
> +				free_genfs(newgenfs);
> +				ERR(state->handle, "Out of memory!");
> +				return -ENOMEM;
> +			}
> +			newc->u.name = strdup(c->u.name);
> +			if (!newc->u.name) {
> +				free_genfs(newgenfs);
> +				ERR(state->handle, "Out of memory!");
> +				return -ENOMEM;
> +			}
> +			newc->v.sclass = c->v.sclass;
> +			context_copy_and_validate(state, &newc->context[0], &c->context[0]);
> +
> +			if (l)
> +				l->next = newc;
> +			else
> +				newgenfs->head = newc;
> +			l = newc;
> +		}
> +
> +		base_genfs = state->base->genfs;
> +		/* base had no genfs, insert at front */
> +		if (!base_genfs) {
> +			state->base->genfs = newgenfs;
> +			continue;
> +		}
> +		/* first base entry is after this entry, insert at front */
> +		cmp = strcmp(newgenfs->fstype, base_genfs->fstype);
> +		if (cmp < 0) {
> +			newgenfs->next = base_genfs;
> +			state->base->genfs = newgenfs;
> +			continue;
> +		} else if (cmp == 0) {
> +			/* merge with head of list */
> +			ret = genfs_merge(state, newgenfs, base_genfs);
> +			if (ret)
> +				return ret;
> +			continue;
> +		}
> +		/* move down the list until we either need to be inserted between
> +		 * base_genfs and base_genfs->next or we need to merge with
> +		 * base_genfs->next */
> +		while (base_genfs->next && (strcmp(newgenfs->fstype, base_genfs->next->fstype) > 0))
> +			base_genfs = base_genfs->next;
> +		/* insert between if appropiete */
> +		if (!base_genfs->next || (strcmp(newgenfs->fstype, base_genfs->next->fstype) < 0)) {
> +			newgenfs->next = base_genfs->next;
> +			base_genfs->next = newgenfs;
> +			continue;
> +		}
> +		/* this is the fun part, merge..... */
> +		if (strcmp(newgenfs->fstype, base_genfs->next->fstype) == 0) {
> +			ret = genfs_merge(state, newgenfs, base_genfs->next);
> +			if (ret)
> +				return ret;
> +		} else {
> +			assert(0);
> +		}
> +	}
> +	return 0;
> +}
> +
>  /* Link a set of modules into a base module. This process is somewhat
>   * similar to an actual compiler: it requires a set of order dependent
>   * steps.  The base and every module must have been indexed prior to
> @@ -2202,8 +2435,19 @@ int link_modules(sepol_handle_t * handle
>  	for (i = 0; i < len; i++) {
>  		state.cur = modules[i];
>  		state.cur_mod_name = modules[i]->policy->name;
> -		ret =
> -		    copy_identifiers(&state, modules[i]->policy->symtab, NULL);
> +		ret = copy_identifiers(&state, modules[i]->policy->symtab, NULL);
> +		if (ret) {
> +			retval = ret;
> +			goto cleanup;
> +		}
> +	}
> +
> +	/* copy genfs_context stuff into base */
> +	for (i = 0; i < len; i++) {
> +		state.cur = modules[i];
> +		state.cur_mod_name = modules[i]->policy->name;
> +
> +		ret = genfs_context_copy_helper(&state);
>   

Why isn't this part of copy_module?

>  		if (ret) {
>  			retval = ret;
>  			goto cleanup;
> diff -Naupr libsepol-2.0.26.orig/src/policydb.c libsepol-2.0.26/src/policydb.c
> --- libsepol-2.0.26.orig/src/policydb.c	2008-03-27 13:20:03.000000000 -0400
> +++ libsepol-2.0.26/src/policydb.c	2008-05-07 10:53:52.000000000 -0400
> @@ -1563,8 +1563,9 @@ static int mls_range_to_semantic(mls_ran
>   * Read and validate a security context structure
>   * from a policydb binary representation file.
>   */
> -static int context_read_and_validate(context_struct_t * c,
> -				     policydb_t * p, struct policy_file *fp)
> +static int context_read_and_validate(context_struct_t *c,
> +				     policydb_t *p, struct policy_file *fp,
> +				     uint32_t validate)
>  {
>  	uint32_t buf[3];
>  	int rc;
> @@ -1588,7 +1589,7 @@ static int context_read_and_validate(con
>  		}
>  	}
>  
> -	if (!policydb_context_isvalid(p, c)) {
> +	if (validate && !policydb_context_isvalid(p, c)) {
>  		ERR(fp->handle, "invalid security context");
>  		context_destroy(c);
>  		return -1;
> @@ -2076,7 +2077,7 @@ static int ocontext_read(struct policydb
>  					return -1;
>  				c->sid[0] = le32_to_cpu(buf[0]);
>  				if (context_read_and_validate
> -				    (&c->context[0], p, fp))
> +				    (&c->context[0], p, fp, 1))
>  					return -1;
>  				break;
>  			case OCON_FS:
> @@ -2093,10 +2094,10 @@ static int ocontext_read(struct policydb
>  					return -1;
>  				c->u.name[len] = 0;
>  				if (context_read_and_validate
> -				    (&c->context[0], p, fp))
> +				    (&c->context[0], p, fp, 1))
>  					return -1;
>  				if (context_read_and_validate
> -				    (&c->context[1], p, fp))
> +				    (&c->context[1], p, fp, 1))
>  					return -1;
>  				break;
>  			case OCON_PORT:
> @@ -2107,7 +2108,7 @@ static int ocontext_read(struct policydb
>  				c->u.port.low_port = le32_to_cpu(buf[1]);
>  				c->u.port.high_port = le32_to_cpu(buf[2]);
>  				if (context_read_and_validate
> -				    (&c->context[0], p, fp))
> +				    (&c->context[0], p, fp, 1))
>  					return -1;
>  				break;
>  			case OCON_NODE:
> @@ -2117,7 +2118,7 @@ static int ocontext_read(struct policydb
>  				c->u.node.addr = le32_to_cpu(buf[0]);
>  				c->u.node.mask = le32_to_cpu(buf[1]);
>  				if (context_read_and_validate
> -				    (&c->context[0], p, fp))
> +				    (&c->context[0], p, fp, 1))
>  					return -1;
>  				break;
>  			case OCON_FSUSE:
> @@ -2134,7 +2135,7 @@ static int ocontext_read(struct policydb
>  					return -1;
>  				c->u.name[len] = 0;
>  				if (context_read_and_validate
> -				    (&c->context[0], p, fp))
> +				    (&c->context[0], p, fp, 1))
>  					return -1;
>  				break;
>  			case OCON_NODE6:{
> @@ -2151,7 +2152,7 @@ static int ocontext_read(struct policydb
>  						c->u.node6.mask[k] =
>  						    le32_to_cpu(buf[k + 4]);
>  					if (context_read_and_validate
> -					    (&c->context[0], p, fp))
> +					    (&c->context[0], p, fp, 1))
>  						return -1;
>  					break;
>  				}
> @@ -2173,6 +2174,10 @@ static int genfs_read(policydb_t * p, st
>  	ocontext_t *l, *c, *newc = NULL;
>  	int rc;
>  
> +	/* don't validate full contexts in modules, will do that at link
> +	 * time later */
> +	int validate = (p->policy_type != POLICY_MOD);
> +
>  	rc = next_entry(buf, fp, sizeof(uint32_t));
>  	if (rc < 0)
>  		goto bad;
> @@ -2240,10 +2245,9 @@ static int genfs_read(policydb_t * p, st
>  			if (rc < 0)
>  				goto bad;
>  			newc->v.sclass = le32_to_cpu(buf[0]);
> -			if (context_read_and_validate(&newc->context[0], p, fp))
> +			if (context_read_and_validate(&newc->context[0], p, fp, validate))
>  				goto bad;
> -			for (l = NULL, c = newgenfs->head; c;
> -			     l = c, c = c->next) {
> +			for (l = NULL, c = newgenfs->head; c; l = c, c = c->next) {
>  				if (!strcmp(newc->u.name, c->u.name) &&
>  				    (!c->v.sclass || !newc->v.sclass ||
>  				     newc->v.sclass == c->v.sclass)) {
>
>
>
> --
> 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.