disallow * and ~ in rules

PATCH In review Joshua Brindle 4 participants 14 messages Jun 23, 2005 – Jun 27, 2005 SELinux security module

Share & tools

Joshua Brindle

author

2005-06-23 16:26 UTC #0 lore raw

PATCH 1/1 disallow * and ~ in rules

The attached patch disallows * and ~ in certain kinds of rules, the list of where they are allowed and where they are not follows. I'm very willing to discuss any ideas or arguments as to why these should or shouldn't be in the list they are in.

* and ~ allowed:
range_trans (I was hoping TCS had an opinion on this) neverallow, dontaudit, auditallow

* and ~ not allowed:
allow rules
type_transition, type_member, type_change
role declarations (to add types to a role)
role transitions

Joshua Brindle
Tresys Technology

Index: policy_parse.y
===================================================================
RCS file: /cvsroot/selinux/nsa/selinux-usr/checkpolicy/policy_parse.y,v retrieving revision 1.31

policy_parse.y +23 ~~-14~~

diff -u -u -p -r1.31 policy_parse.y
@@ -1781,12 +1781,17 @@ static char *type_val_to_name(unsigned i
 static int set_types(ebitmap_t *set,
 		     ebitmap_t *negset,
 		     char *id,
-		     int *add)
+		     int *add, 
+		     char starallowed)
 {
 	type_datum_t *t;
 	unsigned int i;
 
 	if (strcmp(id, "*") == 0) {
+		if (!starallowed) {
+			yyerror("* not allowed in this type of rule");
+			return -1;
+		}
 		/* set all types not in negset */
 		for (i = 0; i < policydbp->p_types.nprim; i++) {
 			if (!ebitmap_get_bit(negset, i))

@@ -1797,6 +1802,10 @@ static int set_types(ebitmap_t *set,
 	}
 
 	if (strcmp(id, "~") == 0) {
+		if (!starallowed) {
+			yyerror("~ not allowed in this type of rule");
+			return -1;
+		}
 		/* complement the set */
 		for (i = 0; i < policydbp->p_types.nprim; i++) {
 			if (ebitmap_get_bit(set, i))

@@ -1893,14 +1902,14 @@ static int define_compute_type(int which
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&stypes, &negset, id, &add))
+		if (set_types(&stypes, &negset, id, &add, 0))
 			return -1;
 	}
 	ebitmap_destroy(&negset);
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&ttypes, &negset, id, &add))
+		if (set_types(&ttypes, &negset, id, &add, 0))
 			return -1;
 	}
 	ebitmap_destroy(&negset);

@@ -2033,14 +2042,14 @@ static cond_av_list_t *define_cond_compu
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&stypes, &negset, id, &add))
+		if (set_types(&stypes, &negset, id, &add, 0))
 			return  COND_ERR;
 	}
 	ebitmap_destroy(&negset);
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&ttypes, &negset, id, &add))
+		if (set_types(&ttypes, &negset, id, &add, 0))
 			return COND_ERR;
 	}
 	ebitmap_destroy(&negset);

@@ -2468,7 +2477,7 @@ static cond_av_list_t *define_cond_te_av
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&stypes, &negset, id, &add))
+		if (set_types(&stypes, &negset, id, &add, which == AVTAB_ALLOWED? 0 : 1 ))
 			return COND_ERR;
 	}
 	ebitmap_destroy(&negset);

@@ -2479,7 +2488,7 @@ static cond_av_list_t *define_cond_te_av
 			self = 1;
 			continue;
 		}
-		if (set_types(&ttypes, &negset, id, &add))
+		if (set_types(&ttypes, &negset, id, &add, which == AVTAB_ALLOWED? 0 : 1 ))
 			return COND_ERR;
 	}
 	ebitmap_destroy(&negset);

@@ -2646,7 +2655,7 @@ static int define_te_avtab(int which)
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&stypes, &negset, id, &add))
+		if (set_types(&stypes, &negset, id, &add, which == AVTAB_ALLOWED? 0 : 1 ))
 			return -1;
 	}
 	ebitmap_destroy(&negset);

@@ -2657,7 +2666,7 @@ static int define_te_avtab(int which)
 			self = 1;
 			continue;
 		}
-		if (set_types(&ttypes, &negset, id, &add))
+		if (set_types(&ttypes, &negset, id, &add, which == AVTAB_ALLOWED? 0 : 1 ))
 			return -1;
 	}
 	ebitmap_destroy(&negset);

@@ -2853,7 +2862,7 @@ static int define_role_types(void)
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&role->types, &negset, id, &add))
+		if (set_types(&role->types, &negset, id, &add, 0))
 			return -1;
 	}
 	ebitmap_destroy(&negset);

@@ -3068,7 +3077,7 @@ static int define_role_trans(void)
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&types, &negset, id, &add))
+		if (set_types(&types, &negset, id, &add, 0))
 			return -1;
 	}
 	ebitmap_destroy(&negset);

@@ -3493,7 +3502,7 @@ static uintptr_t
 				}
 				val = role->value;
 			} else if (expr->attr & CEXPR_TYPE) {
-				if (set_types(&expr->names, &negset, id, &add)) {
+				if (set_types(&expr->names, &negset, id, &add, 1)) {
 					free(expr);
 					return 0;
 				}

@@ -4839,14 +4848,14 @@ static int define_range_trans(void)
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&doms, &negset, id, &add))
+		if (set_types(&doms, &negset, id, &add, 1))
 			return -1;
 	}
 	ebitmap_destroy(&negset);
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&types, &negset, id, &add))
+		if (set_types(&types, &negset, id, &add, 1))
 			return -1;
 	}
 	ebitmap_destroy(&negset);

Stephen Smalley

2005-06-23 17:54 UTC #1 lore raw

Quoted 11 lines · Joshua Brindle

The attached patch disallows * and ~ in certain kinds of rules, the list of where they are allowed and where they are not follows. I'm very willing to discuss any ideas or arguments as to why these should or shouldn't be in the list they are in.
* and ~ allowed:
range_trans (I was hoping TCS had an opinion on this) neverallow, dontaudit, auditallow

Quoted 5 lines

* and ~ not allowed:
allow rules
type_transition, type_member, type_change
role declarations (to add types to a role)
role transitions

The patch only appears to alter their use in type sets, nothing else (e.g. they can still be used for permission sets and role sets). Is that your intent?

For type sets, I'd favor greater consistency, e.g. not allow their use in any type set except neverallow rules and only in them because they are used to catch ill-formed policies. Otherwise, type set exclusion (e.g. domain -foo) is always preferable to set complement (e.g. ~foo), as the latter will include unrelated types (e.g. file types) that are not possible in that field, and a type attribute like domain or file_type should always be used rather than a * for the same reason. Looks like the FC4 strict and targeted policies are clean in this respect, not sure about everything in the example policy.

Signature · 8 lines

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 [email protected] with
the words "unsubscribe selinux" without quotes as the message.

Joshua Brindle

author

2005-06-23 18:47 UTC #2 lore raw

Quoted 18 lines · Stephen Smalley

Quoted 11 lines · Joshua Brindle
The attached patch disallows * and ~ in certain kinds of rules, the list of where they are allowed and where they are not follows. I'm very willing to discuss any ideas or arguments as to why these should or shouldn't be in the list they are in.
* and ~ allowed:
range_trans (I was hoping TCS had an opinion on this) neverallow, dontaudit, auditallow
Quoted 5 lines
* and ~ not allowed:
allow rules
type_transition, type_member, type_change
role declarations (to add types to a role)
role transitions

Quoted 4 lines

The patch only appears to alter their use in type sets, nothing else (e.g. they can still be used for permission sets and role sets). Is that your intent?

This is definitely up for debate. The major problems with type sets was that using * and ~ in types would add rules when types were added, even without assigning attributes or writing explicit rules. Permissions are something different because they are mostly static and it's fairly well known what you are saying you want when you use * for permissions.

Roles are interesting and I hadn't really thought about them. The attached patch disallows * and ~ in role transitions, role allows and user declarations in the role sets.

Quoted 9 lines

For type sets, I'd favor greater consistency, e.g. not allow their use in any type set except neverallow rules and only in them because they are used to catch ill-formed policies. Otherwise, type set exclusion (e.g. domain -foo) is always preferable to set complement (e.g. ~foo), as the latter will include unrelated types (e.g. file types) that are not possible in that field, and a type attribute like domain or file_type should always be used rather than a * for the same reason. Looks like the FC4 strict and targeted policies are clean in this respect, not sure about everything in the example policy.

Ok, I wasn't really sure if * or ~ would be useful in range_trans so I left that for an MLS user to say something. The attached patch disallows it.

I agree that neverallow has very legitimate reasons for * but dontaudit and auditallow are a little less clear. Perhaps auditallow should allow *, it's conceivable that someone would want to audit all access to an object (though in the current policies and reference policy * would have the same net effect as domain with fewer avtab entries). This may not be the case for all policies though. The attached patch disallows it in all rule types but neverallow.

I forgot to mention constraints, which currently allow * and ~, any thoughts on this?

Joshua

Index: policy_parse.y
===================================================================
RCS file: /cvsroot/selinux/nsa/selinux-usr/checkpolicy/policy_parse.y,v retrieving revision 1.31

policy_parse.y +31 ~~-39~~

diff -u -u -p -r1.31 policy_parse.y
@@ -1781,12 +1781,17 @@ static char *type_val_to_name(unsigned i
 static int set_types(ebitmap_t *set,
 		     ebitmap_t *negset,
 		     char *id,
-		     int *add)
+		     int *add, 
+		     char starallowed)
 {
 	type_datum_t *t;
 	unsigned int i;
 
 	if (strcmp(id, "*") == 0) {
+		if (!starallowed) {
+			yyerror("* not allowed in this type of rule");
+			return -1;
+		}
 		/* set all types not in negset */
 		for (i = 0; i < policydbp->p_types.nprim; i++) {
 			if (!ebitmap_get_bit(negset, i))

@@ -1797,6 +1802,10 @@ static int set_types(ebitmap_t *set,
 	}
 
 	if (strcmp(id, "~") == 0) {
+		if (!starallowed) {
+			yyerror("~ not allowed in this type of rule");
+			return -1;
+		}
 		/* complement the set */
 		for (i = 0; i < policydbp->p_types.nprim; i++) {
 			if (ebitmap_get_bit(set, i))

@@ -1893,14 +1902,14 @@ static int define_compute_type(int which
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&stypes, &negset, id, &add))
+		if (set_types(&stypes, &negset, id, &add, 0))
 			return -1;
 	}
 	ebitmap_destroy(&negset);
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&ttypes, &negset, id, &add))
+		if (set_types(&ttypes, &negset, id, &add, 0))
 			return -1;
 	}
 	ebitmap_destroy(&negset);

@@ -2033,14 +2042,14 @@ static cond_av_list_t *define_cond_compu
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&stypes, &negset, id, &add))
+		if (set_types(&stypes, &negset, id, &add, 0))
 			return  COND_ERR;
 	}
 	ebitmap_destroy(&negset);
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&ttypes, &negset, id, &add))
+		if (set_types(&ttypes, &negset, id, &add, 0))
 			return COND_ERR;
 	}
 	ebitmap_destroy(&negset);

@@ -2468,7 +2477,7 @@ static cond_av_list_t *define_cond_te_av
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&stypes, &negset, id, &add))
+		if (set_types(&stypes, &negset, id, &add, which == -AVTAB_ALLOWED? 1 : 0 ))
 			return COND_ERR;
 	}
 	ebitmap_destroy(&negset);

@@ -2479,7 +2488,7 @@ static cond_av_list_t *define_cond_te_av
 			self = 1;
 			continue;
 		}
-		if (set_types(&ttypes, &negset, id, &add))
+		if (set_types(&ttypes, &negset, id, &add, which == -AVTAB_ALLOWED? 1 : 0 ))
 			return COND_ERR;
 	}
 	ebitmap_destroy(&negset);

@@ -2646,7 +2655,7 @@ static int define_te_avtab(int which)
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&stypes, &negset, id, &add))
+		if (set_types(&stypes, &negset, id, &add, which == -AVTAB_ALLOWED? 1 : 0 ))
 			return -1;
 	}
 	ebitmap_destroy(&negset);

@@ -2657,7 +2666,7 @@ static int define_te_avtab(int which)
 			self = 1;
 			continue;
 		}
-		if (set_types(&ttypes, &negset, id, &add))
+		if (set_types(&ttypes, &negset, id, &add, which == -AVTAB_ALLOWED? 1 : 0 ))
 			return -1;
 	}
 	ebitmap_destroy(&negset);

@@ -2853,7 +2862,7 @@ static int define_role_types(void)
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&role->types, &negset, id, &add))
+		if (set_types(&role->types, &negset, id, &add, 0))
 			return -1;
 	}
 	ebitmap_destroy(&negset);

@@ -3002,26 +3011,17 @@ static int set_roles(ebitmap_t *set,
 		     char *id)
 {
 	role_datum_t *r;
-	unsigned int i;
 
 	if (strcmp(id, "*") == 0) {
-		/* set all roles */
-		for (i = 0; i < policydbp->p_roles.nprim; i++) 
-			ebitmap_set_bit(set, i, TRUE);
 		free(id);
-		return 0;
+		yyerror("* is not allowed for role sets");
+		return -1;
 	}
 
 	if (strcmp(id, "~") == 0) {
-		/* complement the set */
-		for (i = 0; i < policydbp->p_roles.nprim; i++) {
-			if (ebitmap_get_bit(set, i))
-				ebitmap_set_bit(set, i, FALSE);
-			else 
-				ebitmap_set_bit(set, i, TRUE);
-		}
 		free(id);
-		return 0;
+		yyerror("~ is not allowed for role sets");
+		return -1;
 	}
 
 	r = hashtab_search(policydbp->p_roles.table, id);

@@ -3068,7 +3068,7 @@ static int define_role_trans(void)
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&types, &negset, id, &add))
+		if (set_types(&types, &negset, id, &add, 0))
 			return -1;
 	}
 	ebitmap_destroy(&negset);

@@ -3493,7 +3493,7 @@ static uintptr_t
 				}
 				val = role->value;
 			} else if (expr->attr & CEXPR_TYPE) {
-				if (set_types(&expr->names, &negset, id, &add)) {
+				if (set_types(&expr->names, &negset, id, &add, 1)) {
 					free(expr);
 					return 0;
 				}

@@ -3862,23 +3862,15 @@ static int set_user_roles(ebitmap_t *set
 	unsigned int i;
 
 	if (strcmp(id, "*") == 0) {
-		/* set all roles */
-		for (i = 0; i < policydbp->p_roles.nprim; i++) 
-			ebitmap_set_bit(set, i, TRUE);
 		free(id);
-		return 0;
+		yyerror("* not allowed in user declarations");
+		return -1;
 	}
 
 	if (strcmp(id, "~") == 0) {
-		/* complement the set */
-		for (i = 0; i < policydbp->p_roles.nprim; i++) {
-			if (ebitmap_get_bit(set, i))
-				ebitmap_set_bit(set, i, FALSE);
-			else 
-				ebitmap_set_bit(set, i, TRUE);
-		}
 		free(id);
-		return 0;
+		yyerror("~ not allowed in user declarations");
+		return -1;
 	}
 
 	r = hashtab_search(policydbp->p_roles.table, id);

@@ -4839,14 +4831,14 @@ static int define_range_trans(void)
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&doms, &negset, id, &add))
+		if (set_types(&doms, &negset, id, &add, 0))
 			return -1;
 	}
 	ebitmap_destroy(&negset);
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&types, &negset, id, &add))
+		if (set_types(&types, &negset, id, &add, 0))
 			return -1;
 	}
 	ebitmap_destroy(&negset);

Stephen Smalley

2005-06-23 19:00 UTC #3 lore raw

Quoted 7 lines · Joshua Brindle

I agree that neverallow has very legitimate reasons for * but dontaudit and auditallow are a little less clear. Perhaps auditallow should allow *, it's conceivable that someone would want to audit all access to an object (though in the current policies and reference policy * would have the same net effect as domain with fewer avtab entries). This may not be the case for all policies though. The attached patch disallows it in all rule types but neverallow.

Yes, I think requiring people to use an attribute like domain or file_type is preferable anyway; otherwise you end up with massive explosion in the set of rules for a lot of types that can't possibly be used in that manner anyway.

Quoted 2 lines

I forgot to mention constraints, which currently allow * and ~, any thoughts on this?

I'd exclude * and ~ entirely in type sets except for neverallow. In every other case, we should be using type attributes (like domain or
file_type) and type set exclusion rather than * or ~. For permission
sets, I think they are useful. For role sets, I'm not sure - we don't have a parallel to attributes for roles, so there is no easy way to say all roles or all roles except X,Y,Z in any other way.

Signature · 8 lines

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 [email protected] with
the words "unsubscribe selinux" without quotes as the message.

Joshua Brindle

author

2005-06-23 19:29 UTC #4 lore raw

Quoted 9 lines · Stephen Smalley

Quoted 7 lines · Joshua Brindle
I agree that neverallow has very legitimate reasons for * but dontaudit and auditallow are a little less clear. Perhaps auditallow should allow *, it's conceivable that someone would want to audit all access to an object (though in the current policies and reference policy * would have the same net effect as domain with fewer avtab entries). This may not be the case for all policies though. The attached patch disallows it in all rule types but neverallow.

Quoted 5 lines

Yes, I think requiring people to use an attribute like domain or file_type is preferable anyway; otherwise you end up with massive explosion in the set of rules for a lot of types that can't possibly be used in that manner anyway.

agreed.

Quoted 9 lines

Quoted 2 lines
I forgot to mention constraints, which currently allow * and ~, any thoughts on this?
I'd exclude * and ~ entirely in type sets except for neverallow. In every other case, we should be using type attributes (like domain or
file_type) and type set exclusion rather than * or ~. For permission
sets, I think they are useful. For role sets, I'm not sure - we don't have a parallel to attributes for roles, so there is no easy way to say all roles or all roles except X,Y,Z in any other way.

in the constraint case * seems to be entirely unnecessary. However I'm not convinced that a compliment would never be useful in a constraint.

As for roles, it certainly isn't an issue now but one can easily concieve a policy that creates a role for each user on the system. Then something like allow system_r * would actually make sense (err, more sense than now) but still isn't the best way, which would be adding the allow when the role is created. I don't think it's a problem to remove * and ~ from role sets, at least not yet.

Joshua

--
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.

Stephen Smalley

2005-06-23 20:19 UTC #5 lore raw

Quoted 2 lines · Joshua Brindle

in the constraint case * seems to be entirely unnecessary. However I'm not convinced that a compliment would never be useful in a constraint.

I'm not sure why you'd ever use set complement (~b) rather than type set exclusion (a -b).

Quoted 6 lines

As for roles, it certainly isn't an issue now but one can easily concieve a policy that creates a role for each user on the system. Then something like allow system_r * would actually make sense (err, more sense than now) but still isn't the best way, which would be adding the allow when the role is created. I don't think it's a problem to remove * and ~ from role sets, at least not yet.

If we do retain them (or later restore them), it occurs to me that they need to exclude the implicitly defined object_r; otherwise, they are useless.

Signature · 8 lines

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 [email protected] with
the words "unsubscribe selinux" without quotes as the message.

Joshua Brindle

author

2005-06-23 20:36 UTC #6 lore raw

Quoted 14 lines · you

Quoted 2 lines · Joshua Brindle
in the constraint case * seems to be entirely unnecessary. However I'm not convinced that a compliment would never be useful in a constraint.
I'm not sure why you'd ever use set complement (~b) rather than type set exclusion (a -b).
Quoted 6 lines
As for roles, it certainly isn't an issue now but one can easily concieve a policy that creates a role for each user on the system. Then something like allow system_r * would actually make sense (err, more sense than now) but still isn't the best way, which would be adding the allow when the role is created. I don't think it's a problem to remove * and ~ from role sets, at least not yet.

Quoted 3 lines

If we do retain them (or later restore them), it occurs to me that they need to exclude the implicitly defined object_r; otherwise, they are useless.

Fair enough, attached is hopefully the final patch, * and ~ disabled in all type sets other than neverallow and removed from role sets entirely.

Joshua

Index: policy_parse.y
===================================================================
RCS file: /cvsroot/selinux/nsa/selinux-usr/checkpolicy/policy_parse.y,v retrieving revision 1.31

policy_parse.y +31 ~~-39~~

diff -u -u -p -r1.31 policy_parse.y
@@ -1781,12 +1781,17 @@ static char *type_val_to_name(unsigned i
 static int set_types(ebitmap_t *set,
 		     ebitmap_t *negset,
 		     char *id,
-		     int *add)
+		     int *add, 
+		     char starallowed)
 {
 	type_datum_t *t;
 	unsigned int i;
 
 	if (strcmp(id, "*") == 0) {
+		if (!starallowed) {
+			yyerror("* not allowed in this type of rule");
+			return -1;
+		}
 		/* set all types not in negset */
 		for (i = 0; i < policydbp->p_types.nprim; i++) {
 			if (!ebitmap_get_bit(negset, i))

@@ -1797,6 +1802,10 @@ static int set_types(ebitmap_t *set,
 	}
 
 	if (strcmp(id, "~") == 0) {
+		if (!starallowed) {
+			yyerror("~ not allowed in this type of rule");
+			return -1;
+		}
 		/* complement the set */
 		for (i = 0; i < policydbp->p_types.nprim; i++) {
 			if (ebitmap_get_bit(set, i))

@@ -1893,14 +1902,14 @@ static int define_compute_type(int which
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&stypes, &negset, id, &add))
+		if (set_types(&stypes, &negset, id, &add, 0))
 			return -1;
 	}
 	ebitmap_destroy(&negset);
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&ttypes, &negset, id, &add))
+		if (set_types(&ttypes, &negset, id, &add, 0))
 			return -1;
 	}
 	ebitmap_destroy(&negset);

@@ -2033,14 +2042,14 @@ static cond_av_list_t *define_cond_compu
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&stypes, &negset, id, &add))
+		if (set_types(&stypes, &negset, id, &add, 0))
 			return  COND_ERR;
 	}
 	ebitmap_destroy(&negset);
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&ttypes, &negset, id, &add))
+		if (set_types(&ttypes, &negset, id, &add, 0))
 			return COND_ERR;
 	}
 	ebitmap_destroy(&negset);

@@ -2468,7 +2477,7 @@ static cond_av_list_t *define_cond_te_av
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&stypes, &negset, id, &add))
+		if (set_types(&stypes, &negset, id, &add, which == -AVTAB_ALLOWED? 1 : 0 ))
 			return COND_ERR;
 	}
 	ebitmap_destroy(&negset);

@@ -2479,7 +2488,7 @@ static cond_av_list_t *define_cond_te_av
 			self = 1;
 			continue;
 		}
-		if (set_types(&ttypes, &negset, id, &add))
+		if (set_types(&ttypes, &negset, id, &add, which == -AVTAB_ALLOWED? 1 : 0 ))
 			return COND_ERR;
 	}
 	ebitmap_destroy(&negset);

@@ -2646,7 +2655,7 @@ static int define_te_avtab(int which)
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&stypes, &negset, id, &add))
+		if (set_types(&stypes, &negset, id, &add, which == -AVTAB_ALLOWED? 1 : 0 ))
 			return -1;
 	}
 	ebitmap_destroy(&negset);

@@ -2657,7 +2666,7 @@ static int define_te_avtab(int which)
 			self = 1;
 			continue;
 		}
-		if (set_types(&ttypes, &negset, id, &add))
+		if (set_types(&ttypes, &negset, id, &add, which == -AVTAB_ALLOWED? 1 : 0 ))
 			return -1;
 	}
 	ebitmap_destroy(&negset);

@@ -2853,7 +2862,7 @@ static int define_role_types(void)
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&role->types, &negset, id, &add))
+		if (set_types(&role->types, &negset, id, &add, 0))
 			return -1;
 	}
 	ebitmap_destroy(&negset);

@@ -3002,26 +3011,17 @@ static int set_roles(ebitmap_t *set,
 		     char *id)
 {
 	role_datum_t *r;
-	unsigned int i;
 
 	if (strcmp(id, "*") == 0) {
-		/* set all roles */
-		for (i = 0; i < policydbp->p_roles.nprim; i++) 
-			ebitmap_set_bit(set, i, TRUE);
 		free(id);
-		return 0;
+		yyerror("* is not allowed for role sets");
+		return -1;
 	}
 
 	if (strcmp(id, "~") == 0) {
-		/* complement the set */
-		for (i = 0; i < policydbp->p_roles.nprim; i++) {
-			if (ebitmap_get_bit(set, i))
-				ebitmap_set_bit(set, i, FALSE);
-			else 
-				ebitmap_set_bit(set, i, TRUE);
-		}
 		free(id);
-		return 0;
+		yyerror("~ is not allowed for role sets");
+		return -1;
 	}
 
 	r = hashtab_search(policydbp->p_roles.table, id);

@@ -3068,7 +3068,7 @@ static int define_role_trans(void)
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&types, &negset, id, &add))
+		if (set_types(&types, &negset, id, &add, 0))
 			return -1;
 	}
 	ebitmap_destroy(&negset);

@@ -3493,7 +3493,7 @@ static uintptr_t
 				}
 				val = role->value;
 			} else if (expr->attr & CEXPR_TYPE) {
-				if (set_types(&expr->names, &negset, id, &add)) {
+				if (set_types(&expr->names, &negset, id, &add, 0)) {
 					free(expr);
 					return 0;
 				}

@@ -3862,23 +3862,15 @@ static int set_user_roles(ebitmap_t *set
 	unsigned int i;
 
 	if (strcmp(id, "*") == 0) {
-		/* set all roles */
-		for (i = 0; i < policydbp->p_roles.nprim; i++) 
-			ebitmap_set_bit(set, i, TRUE);
 		free(id);
-		return 0;
+		yyerror("* not allowed in user declarations");
+		return -1;
 	}
 
 	if (strcmp(id, "~") == 0) {
-		/* complement the set */
-		for (i = 0; i < policydbp->p_roles.nprim; i++) {
-			if (ebitmap_get_bit(set, i))
-				ebitmap_set_bit(set, i, FALSE);
-			else 
-				ebitmap_set_bit(set, i, TRUE);
-		}
 		free(id);
-		return 0;
+		yyerror("~ not allowed in user declarations");
+		return -1;
 	}
 
 	r = hashtab_search(policydbp->p_roles.table, id);

@@ -4839,14 +4831,14 @@ static int define_range_trans(void)
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&doms, &negset, id, &add))
+		if (set_types(&doms, &negset, id, &add, 0))
 			return -1;
 	}
 	ebitmap_destroy(&negset);
 
 	ebitmap_init(&negset);
 	while ((id = queue_remove(id_queue))) {
-		if (set_types(&types, &negset, id, &add))
+		if (set_types(&types, &negset, id, &add, 0))
 			return -1;
 	}
 	ebitmap_destroy(&negset);

Russell Coker

2005-06-24 06:29 UTC #8 lore raw

In reply to Stephen Smalley

Quoted 9 lines · Stephen Smalley

Quoted 7 lines · Joshua Brindle
I agree that neverallow has very legitimate reasons for * but dontaudit and auditallow are a little less clear. Perhaps auditallow should allow *, it's conceivable that someone would want to audit all access to an object (though in the current policies and reference policy * would have the same net effect as domain with fewer avtab entries). This may not be the case for all policies though. The attached patch disallows it in all rule types but neverallow.

Quoted 4 lines

Yes, I think requiring people to use an attribute like domain or file_type is preferable anyway; otherwise you end up with massive explosion in the set of rules for a lot of types that can't possibly be used in that manner anyway.

I agree for dontaudit, but disagree for auditallow.

Sometimes when debugging policy issues I want to see all the accesses to an object. Writing rules that cover everything can be a drag, and running apol also takes time. It's a lot easier to just do: auditallow * foo_t:file *;

Quoted 3 lines

sets, I think they are useful. For role sets, I'm not sure - we don't have a parallel to attributes for roles, so there is no easy way to say all roles or all roles except X,Y,Z in any other way.

Having role attributes would be handy.

The in_user_role() macro is a gross hack, role attributes would remove the need for it.

Signature · 9 lines

http://www.coker.com.au/selinux/   My NSA Security Enhanced Linux packages
http://www.coker.com.au/bonnie++/  Bonnie++ hard drive benchmark
http://www.coker.com.au/postal/    Postal SMTP/POP benchmark
http://www.coker.com.au/~russell/  My home page

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to [email protected] with
the words "unsubscribe selinux" without quotes as the message.

Stephen Smalley

2005-06-24 11:35 UTC #9 lore raw

Quoted 5 lines · Russell Coker

I agree for dontaudit, but disagree for auditallow.
Sometimes when debugging policy issues I want to see all the accesses to an object. Writing rules that cover everything can be a drag, and running apol also takes time. It's a lot easier to just do: auditallow * foo_t:file *;

But that can just as easily be written as:

	auditallow domain foo_t:file *;

with no loss in what it truly provides (and definite improvement in the size of the resulting policy). Note that we aren't eliminating use of * in permission sets, just in type sets and role sets. The problem with * in type sets is that you never truly want all types (except in assertions checking for policy errors); you only want "all process types, i.e. domain" or "all file types, i.e. file_type", etc.

Quoted 4 lines

Having role attributes would be handy.
The in_user_role() macro is a gross hack, role attributes would remove the need for it.

Hmm...need a list somewhere that tracks all requests for improvements to the policy language...

Signature · 8 lines

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 [email protected] with
the words "unsubscribe selinux" without quotes as the message.

Russell Coker

2005-06-24 13:24 UTC #10 lore raw

Quoted 10 lines · Stephen Smalley

Quoted 6 lines · Russell Coker
I agree for dontaudit, but disagree for auditallow.
Sometimes when debugging policy issues I want to see all the accesses to an object. Writing rules that cover everything can be a drag, and running apol also takes time. It's a lot easier to just do: auditallow * foo_t:file *;
But that can just as easily be written as:
	auditallow domain foo_t:file *;

I just did a quick test and found a .3% size difference. I'm surprised, I had expected that as auditallow doesn't permit an operation the rules would not result in anything in the binary policy for an operation which is not permitted. Does the current behavior really make sense?

Quoted 7 lines

Quoted 4 lines
Having role attributes would be handy.
The in_user_role() macro is a gross hack, role attributes would remove the need for it.
Hmm...need a list somewhere that tracks all requests for improvements to the policy language...

I think that Tresys have an internal list. When I was at the Symposium they were telling me about their work to implement a feature I had forgotten about requesting.

Signature · 9 lines

http://www.coker.com.au/selinux/   My NSA Security Enhanced Linux packages
http://www.coker.com.au/bonnie++/  Bonnie++ hard drive benchmark
http://www.coker.com.au/postal/    Postal SMTP/POP benchmark
http://www.coker.com.au/~russell/  My home page

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to [email protected] with
the words "unsubscribe selinux" without quotes as the message.

Stephen Smalley

2005-06-24 13:29 UTC #11 lore raw

Quoted 4 lines · Russell Coker

I just did a quick test and found a .3% size difference. I'm surprised, I had expected that as auditallow doesn't permit an operation the rules would not result in anything in the binary policy for an operation which is not permitted. Does the current behavior really make sense?

checkpolicy isn't that smart, at least not presently ;)
Same core logic is shared for allow/auditallow/auditdeny/dontaudit, see te_avtab_helper() in policy_parse.y. Yes, we could have it optimize away auditallow entries for which no corresponding allow entry exists.

Signature · 8 lines

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 [email protected] with
the words "unsubscribe selinux" without quotes as the message.

Stephen Smalley

2005-06-24 13:59 UTC #7 lore raw

In reply to Joshua Brindle

Quoted 2 lines · Joshua Brindle

Fair enough, attached is hopefully the final patch, * and ~ disabled in all type sets other than neverallow and removed from role sets entirely.

Thanks, merged (checkpolicy 1.25.1).

Signature · 8 lines

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 [email protected] with
the words "unsubscribe selinux" without quotes as the message.

Karl MacMillan

2005-06-24 14:29 UTC #12 lore raw

In reply to Stephen Smalley

Quoted 24 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Stephen Smalley
Sent: Friday, June 24, 2005 7:35 AM
To: [email protected]
Cc: Joshua Brindle; selinux
Subject: Re: [PATCH] disallow * and ~ in rules
Quoted 5 lines · Russell Coker
I agree for dontaudit, but disagree for auditallow.
Sometimes when debugging policy issues I want to see all the accesses to an object. Writing rules that cover everything can be a drag, and running apol also takes time. It's a lot easier to just do: auditallow * foo_t:file *;
But that can just as easily be written as:
	auditallow domain foo_t:file *;
with no loss in what it truly provides (and definite improvement in the size of the resulting policy). Note that we aren't eliminating use of * in permission sets, just in type sets and role sets. The problem with * in type sets is that you never truly want all types (except in assertions checking for policy errors); you only want "all process types, i.e. domain" or "all file types, i.e. file_type", etc.

Quoted 8 lines

Quoted 4 lines
Having role attributes would be handy.
The in_user_role() macro is a gross hack, role attributes would remove the need for it.
Hmm...need a list somewhere that tracks all requests for improvements to the policy language...

We can put this on the policy server sf site as long as everyone knows that we can't address all of these requests as part of that project.

Karl

---
Karl MacMillan
Tresys Technology
http://www.tresys.com
(410) 290-1411 ext 134

Quoted 9 lines

--
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 [email protected] with the words "unsubscribe selinux" without quotes as the message.

Joshua Brindle

author

2005-06-27 15:06 UTC #13 lore raw

<snip>

Quoted 7 lines

Quoted 3 lines
Hmm...need a list somewhere that tracks all requests for improvements to the policy language...
We can put this on the policy server sf site as long as everyone knows that we can't address all of these requests as part of that project.

http://sepolicy-server.sourceforge.net/index.php?page=compiler-wishlist