[PATCH RETRY 2/3] Optionally expand neverallows

[PATCH RETRY 2/3] Optionally expand neverallows

[Joshua Brindle]

The setools team would like to be able to optionally expand neverallow
rules for analysis purposes.  This patch leaves the current behavior
unchanged, but allows a new state variable for the expander to indicate
whether neverallow rules should get expanded, and creates an init
function for the expand_state struct. This has the earlier suggestions
incorporated.

diff -urpN -x 'Change*' -x entries -x '*.orig' -x '*.rej' -x '*.svn*' -x '*.swp' trunk/libsepol/include/sepol/policydb/avtab.h branch/setools_public-policydb-components/libsepol/include/sepol/policydb/avtab.h
--- trunk/libsepol/include/sepol/policydb/avtab.h	2006-07-13 10:19:14.000000000 -0400
+++ trunk/libsepol/include/sepol/policydb/avtab.h	2006-07-13 10:46:33.000000000 -0400
@@ -45,6 +45,7 @@ typedef struct avtab_key {
 #define AVTAB_ALLOWED     1
 #define AVTAB_AUDITALLOW  2
 #define AVTAB_AUDITDENY   4
+#define AVTAB_NEVERALLOW 128
 #define AVTAB_AV         (AVTAB_ALLOWED | AVTAB_AUDITALLOW | AVTAB_AUDITDENY)
 #define AVTAB_TRANSITION 16
 #define AVTAB_MEMBER     32
diff -urpN -x 'Change*' -x entries -x '*.orig' -x '*.rej' -x '*.svn*' -x '*.swp' trunk/libsepol/trunk/libsepol/src/expand.c branch/setools_public-policydb-components/libsepol/trunk/libsepol/src/expand.c
--- trunk/libsepol/src/expand.c	2006-07-20 09:59:25.000000000 -0400
+++ trunk/libsepol/src/expand.c	2006-07-26 13:21:18.000000000 -0400
@@ -41,6 +41,7 @@ typedef struct expand_state {
 	policydb_t *base;
 	policydb_t *out;
 	sepol_handle_t *handle;
+	int expand_neverallow;
 } expand_state_t;
 
 static int type_copy_callback(hashtab_key_t key, hashtab_datum_t datum,
@@ -1137,6 +1138,8 @@ static int expand_avrule_helper(sepol_ha
 		spec = AVTAB_AUDITDENY;
 	} else if (specified & AVRULE_DONTAUDIT) {
 		spec = AVTAB_AUDITDENY;
+	} else if (specified & AVRULE_NEVERALLOW) {
+		spec = AVTAB_NEVERALLOW;
 	} else {
 		assert(0);	/* unreachable */
 	}
@@ -1162,6 +1165,8 @@ static int expand_avrule_helper(sepol_ha
 			avdatump->data |= cur->data;
 		} else if (specified & AVRULE_AUDITALLOW) {
 			avdatump->data |= cur->data;
+		} else if (specified & AVRULE_NEVERALLOW) {
+			avdatump->data |= cur->data;
 		} else if (specified & AVRULE_AUDITDENY) {
 			/* Since a '0' in an auditdeny mask represents
 			 * a permission we do NOT want to audit
@@ -1200,7 +1205,8 @@ static int expand_rule_helper(sepol_hand
 		if (!ebitmap_node_get_bit(snode, i))
 			continue;
 		if (source_rule->flags & RULE_SELF) {
-			if (source_rule->specified & AVRULE_AV) {
+			if (source_rule->
+			    specified & (AVRULE_AV | AVRULE_NEVERALLOW)) {
 				if ((retval =
 				     expand_avrule_helper(handle,
 							  source_rule->
@@ -1227,7 +1233,8 @@ static int expand_rule_helper(sepol_hand
 		ebitmap_for_each_bit(ttypes, tnode, j) {
 			if (!ebitmap_node_get_bit(tnode, j))
 				continue;
-			if (source_rule->specified & AVRULE_AV) {
+			if (source_rule->
+			    specified & (AVRULE_AV | AVRULE_NEVERALLOW)) {
 				if ((retval =
 				     expand_avrule_helper(handle,
 							  source_rule->
@@ -1264,13 +1271,14 @@ static int convert_and_expand_rule(sepol
 				   policydb_t * dest_pol, uint32_t * typemap,
 				   avrule_t * source_rule, avtab_t * dest_avtab,
 				   cond_av_list_t ** cond,
-				   cond_av_list_t ** other, int enabled)
+				   cond_av_list_t ** other, int enabled,
+				   int do_neverallow)
 {
 	int retval;
 	ebitmap_t stypes, ttypes;
 	unsigned char alwaysexpand;
 
-	if (source_rule->specified & AVRULE_NEVERALLOW)
+	if (!do_neverallow && source_rule->specified & AVRULE_NEVERALLOW)
 		return 1;
 
 	ebitmap_init(&stypes);
@@ -2033,7 +2041,8 @@ int expand_module(sepol_handle_t * handl
 		/* copy rules */
 		cur_avrule = decl->avrules;
 		while (cur_avrule != NULL) {
-			if (cur_avrule->specified & AVRULE_NEVERALLOW) {
+			if (!(state->expand_neverallow)
+			    && cur_avrule->specified & AVRULE_NEVERALLOW) {
 				/* copy this over directly so that assertions are checked later */
 				if (copy_neverallow
 				    (out, state.typemap, cur_avrule))



--
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 RETRY 2/3] Optionally expand neverallows

[Stephen Smalley]

On Wed, 2006-07-26 at 14:11 -0400, Joshua Brindle wrote:
> The setools team would like to be able to optionally expand neverallow
> rules for analysis purposes.  This patch leaves the current behavior
> unchanged, but allows a new state variable for the expander to indicate
> whether neverallow rules should get expanded, and creates an init
> function for the expand_state struct. This has the earlier suggestions
> incorporated.

> @@ -1200,7 +1205,8 @@ static int expand_rule_helper(sepol_hand
>  		if (!ebitmap_node_get_bit(snode, i))
>  			continue;
>  		if (source_rule->flags & RULE_SELF) {
> -			if (source_rule->specified & AVRULE_AV) {
> +			if (source_rule->
> +			    specified & (AVRULE_AV | AVRULE_NEVERALLOW)) {
>  				if ((retval =
>  				     expand_avrule_helper(handle,
>  							  source_rule->

I don't see any occurrences of AVRULE_AV by itself after this patch, so
why wouldn't you just fold AVRULE_NEVERALLOW into it?  We don't need to
preserve interface for static users of the library.

> @@ -1264,13 +1271,14 @@ static int convert_and_expand_rule(sepol
>  				   policydb_t * dest_pol, uint32_t * typemap,
>  				   avrule_t * source_rule, avtab_t * dest_avtab,
>  				   cond_av_list_t ** cond,
> -				   cond_av_list_t ** other, int enabled)
> +				   cond_av_list_t ** other, int enabled,
> +				   int do_neverallow)
>  {
>  	int retval;
>  	ebitmap_t stypes, ttypes;
>  	unsigned char alwaysexpand;
>  
> -	if (source_rule->specified & AVRULE_NEVERALLOW)
> +	if (!do_neverallow && source_rule->specified & AVRULE_NEVERALLOW)
>  		return 1;

Hmm...do you want alwaysexpand = 1 for the do_neverallow case?
Otherwise, type attributes might not get expanded.  Does
expand_avtab_insert need to know about AVTAB_NEVERALLOW?

-- 
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 RETRY 2/3] Optionally expand neverallows

[Stephen Smalley]

On Wed, 2006-07-26 at 16:15 -0400, Stephen Smalley wrote:
> On Wed, 2006-07-26 at 14:11 -0400, Joshua Brindle wrote:
> > The setools team would like to be able to optionally expand neverallow
> > rules for analysis purposes.  This patch leaves the current behavior
> > unchanged, but allows a new state variable for the expander to indicate
> > whether neverallow rules should get expanded, and creates an init
> > function for the expand_state struct. This has the earlier suggestions
> > incorporated.
> 
> > @@ -1200,7 +1205,8 @@ static int expand_rule_helper(sepol_hand
> >  		if (!ebitmap_node_get_bit(snode, i))
> >  			continue;
> >  		if (source_rule->flags & RULE_SELF) {
> > -			if (source_rule->specified & AVRULE_AV) {
> > +			if (source_rule->
> > +			    specified & (AVRULE_AV | AVRULE_NEVERALLOW)) {
> >  				if ((retval =
> >  				     expand_avrule_helper(handle,
> >  							  source_rule->
> 
> I don't see any occurrences of AVRULE_AV by itself after this patch, so
> why wouldn't you just fold AVRULE_NEVERALLOW into it?  We don't need to
> preserve interface for static users of the library.
> 
> > @@ -1264,13 +1271,14 @@ static int convert_and_expand_rule(sepol
> >  				   policydb_t * dest_pol, uint32_t * typemap,
> >  				   avrule_t * source_rule, avtab_t * dest_avtab,
> >  				   cond_av_list_t ** cond,
> > -				   cond_av_list_t ** other, int enabled)
> > +				   cond_av_list_t ** other, int enabled,
> > +				   int do_neverallow)
> >  {
> >  	int retval;
> >  	ebitmap_t stypes, ttypes;
> >  	unsigned char alwaysexpand;
> >  
> > -	if (source_rule->specified & AVRULE_NEVERALLOW)
> > +	if (!do_neverallow && source_rule->specified & AVRULE_NEVERALLOW)
> >  		return 1;
> 
> Hmm...do you want alwaysexpand = 1 for the do_neverallow case?
> Otherwise, type attributes might not get expanded.  Does
> expand_avtab_insert need to know about AVTAB_NEVERALLOW?

Also, suppose hypothetically that one of these policydbs with expanded
neverallow rules is passed along to one of the other libsepol functions.
What behavior do you want?  Seems like avtab_write_item() would happily
write it out, but avtab_read_item() would choke on it (no match in
spec_order, so set == 0).  Do you want to be able to save these as
binary policy images and use them later for analysis?

-- 
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 RETRY 2/3] Optionally expand neverallows

[Joshua Brindle]

On Wed, 2006-07-26 at 16:15 -0400, Stephen Smalley wrote:

> I don't see any occurrences of AVRULE_AV by itself after this patch,
> so
> why wouldn't you just fold AVRULE_NEVERALLOW into it?  We don't need
> to
> preserve interface for static users of the library.

ok, applies on top of the prior patch

Index: trunk/libsepol/include/sepol/policydb/policydb.h
===================================================================
--- trunk/libsepol/include/sepol/policydb/policydb.h	(revision 1000)
+++ trunk/libsepol/include/sepol/policydb/policydb.h	(working copy)
@@ -194,12 +194,12 @@
 #define AVRULE_AUDITALLOW  2
 #define AVRULE_AUDITDENY   4
 #define AVRULE_DONTAUDIT   8
-#define AVRULE_AV         (AVRULE_ALLOWED | AVRULE_AUDITALLOW | AVRULE_AUDITDENY | AVRULE_DONTAUDIT)
+#define AVRULE_NEVERALLOW 128
+#define AVRULE_AV         (AVRULE_ALLOWED | AVRULE_AUDITALLOW | AVRULE_AUDITDENY | AVRULE_DONTAUDIT | AVRULE_NEVERALLOW)
 #define AVRULE_TRANSITION 16
 #define AVRULE_MEMBER     32
 #define AVRULE_CHANGE     64
 #define AVRULE_TYPE       (AVRULE_TRANSITION | AVRULE_MEMBER | AVRULE_CHANGE)
-#define AVRULE_NEVERALLOW 128
 	uint32_t specified;
 #define RULE_SELF 1
 	uint32_t flags;
Index: trunk/libsepol/src/expand.c
===================================================================
--- trunk/libsepol/src/expand.c	(revision 1000)
+++ trunk/libsepol/src/expand.c	(working copy)
@@ -1214,8 +1214,7 @@
 		if (!ebitmap_node_get_bit(snode, i))
 			continue;
 		if (source_rule->flags & RULE_SELF) {
-			if (source_rule->
-			    specified & (AVRULE_AV | AVRULE_NEVERALLOW)) {
+			if (source_rule->specified & (AVRULE_AV)) {
 				if ((retval =
 				     expand_avrule_helper(handle,
 							  source_rule->
@@ -1242,8 +1241,7 @@
 		ebitmap_for_each_bit(ttypes, tnode, j) {
 			if (!ebitmap_node_get_bit(tnode, j))
 				continue;
-			if (source_rule->
-			    specified & (AVRULE_AV | AVRULE_NEVERALLOW)) {
+			if (source_rule->specified & (AVRULE_AV)) {
 				if ((retval =
 				     expand_avrule_helper(handle,
 							  source_rule->
Index: trunk/libsepol/src/link.c
===================================================================
--- trunk/libsepol/src/link.c	(revision 951)
+++ trunk/libsepol/src/link.c	(working copy)
@@ -965,8 +965,7 @@
 			    module->map[SYM_CLASSES][cur_perm->class - 1];
 			assert(new_perm->class);
 
-			if (new_rule->
-			    specified & (AVRULE_AV | AVRULE_NEVERALLOW)) {
+			if (new_rule->specified & (AVRULE_AV)) {
 				for (i = 0;
 				     i <
 				     module->perm_map_len[cur_perm->class - 1];


> Hmm...do you want alwaysexpand = 1 for the do_neverallow case?
> Otherwise, type attributes might not get expanded.  Does
> expand_avtab_insert need to know about AVTAB_NEVERALLOW?

No, apol doesn't always want the attributes expanded out, it knows how
to deal with attributes.


--
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 RETRY 2/3] Optionally expand neverallows

[Joshua Brindle]

Stephen Smalley wrote:
>
>
> Also, suppose hypothetically that one of these policydbs with expanded
> neverallow rules is passed along to one of the other libsepol functions.
> What behavior do you want?  Seems like avtab_write_item() would happily
> write it out, but avtab_read_item() would choke on it (no match in
> spec_order, so set == 0).  Do you want to be able to save these as
> binary policy images and use them later for analysis?
>
Right, there is no intention to store out these policydbs. This patch is 
similar to one I already added for access control hooks in libsepol. It 
marks policies as invalid when you do unsupported things to them, and 
also skips assertion and hierarchy checking.

Index: trunk/libsepol/include/sepol/policydb/policydb.h
===================================================================
--- trunk/libsepol/include/sepol/policydb/policydb.h	(revision 951)
+++ trunk/libsepol/include/sepol/policydb/policydb.h	(working copy)
@@ -366,6 +366,7 @@
 	uint32_t policy_type;
 	char *name;
 	char *version;
+	int invalid;
 
 	/* Whether this policydb is mls, should always be set */
 	int mls;
Index: trunk/libsepol/src/expand.c
===================================================================
--- trunk/libsepol/src/expand.c	(revision 997)
+++ trunk/libsepol/src/expand.c	(working copy)
@@ -1904,7 +1904,7 @@
 	return -1;
 }
 
-static int expand_avrule_decls(expand_state_t * state)
+static int copy_and_expand_avrule_block(expand_state_t * state)
 {
 	avrule_block_t *curblock;
 	int retval = -1;
@@ -1936,6 +1936,9 @@
 					ERR(state->handle,
 					    "Error while copying neverallow.");
 			} else {
+				if (cur_avrule->specified & AVRULE_NEVERALLOW) {
+					state->out->invalid = 1;
+				}
 				if (convert_and_expand_rule
 				    (state->handle, state->out, state->typemap,
 				     cur_avrule, &state->out->te_avtab, NULL,
@@ -1974,7 +1977,7 @@
 	state.verbose = verbose;
 	state.expand_neverallow = expand_neverallow;
 
-	return expand_avrule_decls(&state);
+	return copy_and_expand_avrule_block(&state);
 }
 
 /* Linking should always be done before calling expand, even if
@@ -2108,7 +2111,7 @@
 
 	}
 
-	if (expand_avrule_decls(&state) < 0) {
+	if (copy_and_expand_avrule_block(&state) < 0) {
 		ERR(handle, "Error during expand");
 		goto cleanup;
 	}
@@ -2156,7 +2159,7 @@
 		goto cleanup;
 	hashtab_map_remove_on_error(state.out->p_types.table,
 				    type_attr_remove, type_destroy, state.out);
-	if (check) {
+	if (check && !(out->invalid)) {
 		if (hierarchy_check_constraints(handle, state.out))
 			goto cleanup;
 
Index: trunk/libsepol/src/write.c
===================================================================
--- trunk/libsepol/src/write.c	(revision 951)
+++ trunk/libsepol/src/write.c	(working copy)
@@ -1411,6 +1411,9 @@
 	struct policy_data pd;
 	char *policydb_str;
 
+	if (p->invalid)
+		return -1;
+
 	pd.fp = fp;
 	pd.p = 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 RETRY 2/3] Optionally expand neverallows

[Karl MacMillan]

Joshua Brindle wrote:
> Stephen Smalley wrote:
>>
>>
>> Also, suppose hypothetically that one of these policydbs with expanded
>> neverallow rules is passed along to one of the other libsepol functions.
>> What behavior do you want?  Seems like avtab_write_item() would happily
>> write it out, but avtab_read_item() would choke on it (no match in
>> spec_order, so set == 0).  Do you want to be able to save these as
>> binary policy images and use them later for analysis?
>>
> Right, there is no intention to store out these policydbs. This patch 
> is similar to one I already added for access control hooks in 
> libsepol. It marks policies as invalid when you do unsupported things 
> to them, and also skips assertion and hierarchy checking.

Did you mean to include the rename for expand_avrule_decls?
>
> Index: trunk/libsepol/include/sepol/policydb/policydb.h
> ===================================================================
> --- trunk/libsepol/include/sepol/policydb/policydb.h    (revision 951)
> +++ trunk/libsepol/include/sepol/policydb/policydb.h    (working copy)
> @@ -366,6 +366,7 @@
>     uint32_t policy_type;
>     char *name;
>     char *version;
> +    int invalid;
>

Expanding the avrules doesn't really make the policydb invalid, right? 
It just makes it non-standard (tainted :) )

>
>     }
> @@ -2156,7 +2159,7 @@
>         goto cleanup;
>     hashtab_map_remove_on_error(state.out->p_types.table,
>                     type_attr_remove, type_destroy, state.out);
> -    if (check) {
> +    if (check && !(out->invalid)) {
>         if (hierarchy_check_constraints(handle, state.out))
>             goto cleanup;
>

Why disallow hierarchy checking?

> Index: trunk/libsepol/src/write.c
> ===================================================================
> --- trunk/libsepol/src/write.c    (revision 951)
> +++ trunk/libsepol/src/write.c    (working copy)
> @@ -1411,6 +1411,9 @@
>     struct policy_data pd;
>     char *policydb_str;
>
> +    if (p->invalid)
> +        return -1;
> +

A separate return code is needed so that the caller can distinguish 
between general, likely fatal errors and a policydb that can't be 
written because the format doesn't support it.

Karl




--
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 RETRY 2/3] Optionally expand neverallows

[Joshua Brindle]

> From: Karl MacMillan [mailto:kmacmillan@mentalrootkit.com] 
> 
> Joshua Brindle wrote:
> > Stephen Smalley wrote:
> >>
> Did you mean to include the rename for expand_avrule_decls?

Yes, forgot to add that to the description

> >
> > Index: trunk/libsepol/include/sepol/policydb/policydb.h
> > ===================================================================
> > --- trunk/libsepol/include/sepol/policydb/policydb.h    
> (revision 951)
> > +++ trunk/libsepol/include/sepol/policydb/policydb.h    
> (working copy)
> > @@ -366,6 +366,7 @@
> >     uint32_t policy_type;
> >     char *name;
> >     char *version;
> > +    int invalid;
> >
> 
> Expanding the avrules doesn't really make the policydb 
> invalid, right? 
> It just makes it non-standard (tainted :) )
> 

Putting neverallows in the avtab is invalid (the reader will bail if it
tries to read one)

> >
> >     }
> > @@ -2156,7 +2159,7 @@
> >         goto cleanup;
> >     hashtab_map_remove_on_error(state.out->p_types.table,
> >                     type_attr_remove, type_destroy, state.out);
> > -    if (check) {
> > +    if (check && !(out->invalid)) {
> >         if (hierarchy_check_constraints(handle, state.out))
> >             goto cleanup;
> >
> 
> Why disallow hierarchy checking?

It disables all checking for invalid policies, there is no reason to do
checking if you've marked a policy invalid since it can't be written
anyway (invalid will probably mean other things in the future, such as a
policy with disabled rules expanded out and conflicting type transitions
in the avtab for access control checking).

> 
> > Index: trunk/libsepol/src/write.c
> > ===================================================================
> > --- trunk/libsepol/src/write.c    (revision 951)
> > +++ trunk/libsepol/src/write.c    (working copy)
> > @@ -1411,6 +1411,9 @@
> >     struct policy_data pd;
> >     char *policydb_str;
> >
> > +    if (p->invalid)
> > +        return -1;
> > +
> 
> A separate return code is needed so that the caller can 
> distinguish between general, likely fatal errors and a 
> policydb that can't be written because the format doesn't support it.
> 

-2? ;)

-1 is currently used if, for example, policy->policyvers is invalid,
same deal.. These are programming errors not user errors. I'd be fine
with it being an assert() personally.


--
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 RETRY 2/3] Optionally expand neverallows

[Karl MacMillan]

Joshua Brindle wrote:
>> From: Karl MacMillan [mailto:kmacmillan@mentalrootkit.com] 
>>
>> Joshua Brindle wrote:
>>     
>>> @@ -366,6 +366,7 @@
>>>     uint32_t policy_type;
>>>     char *name;
>>>     char *version;
>>> +    int invalid;
>>>
>>>       
>> Expanding the avrules doesn't really make the policydb 
>> invalid, right? 
>> It just makes it non-standard (tainted :) )
>>
>>     
>
> Putting neverallows in the avtab is invalid (the reader will bail if it
> tries to read one)
>
>   

I disagree - the policydb is still valid, otherwise there would be no 
point doing analysis on it. I think the more correct semantic is that 
this policydb cannot be represented by the on disc format. So the 
variable would be better called something like policydb->unsupported_format.

Wait, just because the neverallows are expanded doesn't mean that the 
unexpanded aren't also there, meaning that the policydb could be written 
to disc with no loss of information. This reinforces my notion that the 
policy is valid. Why not just skip saving the neverallows in the format 
and document the behavior?

>>>     }
>>> @@ -2156,7 +2159,7 @@
>>>         goto cleanup;
>>>     hashtab_map_remove_on_error(state.out->p_types.table,
>>>                     type_attr_remove, type_destroy, state.out);
>>> -    if (check) {
>>> +    if (check && !(out->invalid)) {
>>>         if (hierarchy_check_constraints(handle, state.out))
>>>             goto cleanup;
>>>
>>>       
>> Why disallow hierarchy checking?
>>     
>
> It disables all checking for invalid policies, there is no reason to do
> checking if you've marked a policy invalid since it can't be written
> anyway 

1) The policy is still valid.
2) If this is for analysis I would imagine you would want the 
neverallows and _also_ know if the policy violates hierarchy constraints.
3) It is surprising since the caller explicitly requests both the 
checking and the expansion. If these parameters really are exclusive an 
error should be raised rather than silently not performing the action.

> (invalid will probably mean other things in the future, such as a
> policy with disabled rules expanded out and conflicting type transitions
> in the avtab for access control checking).
>
>   
>>> Index: trunk/libsepol/src/write.c
>>> ===================================================================
>>> --- trunk/libsepol/src/write.c    (revision 951)
>>> +++ trunk/libsepol/src/write.c    (working copy)
>>> @@ -1411,6 +1411,9 @@
>>>     struct policy_data pd;
>>>     char *policydb_str;
>>>
>>> +    if (p->invalid)
>>> +        return -1;
>>> +
>>>       
>> A separate return code is needed so that the caller can 
>> distinguish between general, likely fatal errors and a 
>> policydb that can't be written because the format doesn't support it.
>>
>>     
>
> -2? ;)
>
> -1 is currently used if, for example, policy->policyvers is invalid,
> same deal.. These are programming errors not user errors. I'd be fine
> with it being an assert() personally.
>   

See above - I think that the write should just work and ignore the 
expanded neverallows.

Karl


--
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 RETRY 2/3] Optionally expand neverallows

[Stephen Smalley]

On Thu, 2006-07-27 at 14:34 -0400, Joshua Brindle wrote:
> Index: trunk/libsepol/src/link.c
> ===================================================================
> --- trunk/libsepol/src/link.c	(revision 951)
> +++ trunk/libsepol/src/link.c	(working copy)
> @@ -965,8 +965,7 @@
>  			    module->map[SYM_CLASSES][cur_perm->class - 1];
>  			assert(new_perm->class);
>  
> -			if (new_rule->
> -			    specified & (AVRULE_AV | AVRULE_NEVERALLOW)) {
> +			if (new_rule->specified & (AVRULE_AV)) {
>  				for (i = 0;
>  				     i <
>  				     module->perm_map_len[cur_perm->class - 1];
> 
> 
> > Hmm...do you want alwaysexpand = 1 for the do_neverallow case?
> > Otherwise, type attributes might not get expanded.  Does
> > expand_avtab_insert need to know about AVTAB_NEVERALLOW?
> 
> No, apol doesn't always want the attributes expanded out, it knows how
> to deal with attributes.

Hmm...but is apol ok with the fact that attributes will be expanded in
some cases here, since we always expand attributes for:
- type rules
- self rules
- type sets that have negative sets (-) or flags (~)

I'm not sure I understand what apol wants as output; at present, it will
get a mixture of expanded and unexpanded attributes.

-- 
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 RETRY 2/3] Optionally expand neverallows

[Joshua Brindle]

> From: Stephen Smalley [mailto:sds@tycho.nsa.gov] 
> 
> On Thu, 2006-07-27 at 14:34 -0400, Joshua Brindle wrote:
> > Index: trunk/libsepol/src/link.c
> > ===================================================================
> > --- trunk/libsepol/src/link.c	(revision 951)
> > +++ trunk/libsepol/src/link.c	(working copy)
> > @@ -965,8 +965,7 @@
> >  			    
> module->map[SYM_CLASSES][cur_perm->class - 1];
> >  			assert(new_perm->class);
> >  
> > -			if (new_rule->
> > -			    specified & (AVRULE_AV | 
> AVRULE_NEVERALLOW)) {
> > +			if (new_rule->specified & (AVRULE_AV)) {
> >  				for (i = 0;
> >  				     i <
> >  				     
> module->perm_map_len[cur_perm->class - 1];
> > 
> > 
> > > Hmm...do you want alwaysexpand = 1 for the do_neverallow case?
> > > Otherwise, type attributes might not get expanded.  Does 
> > > expand_avtab_insert need to know about AVTAB_NEVERALLOW?
> > 
> > No, apol doesn't always want the attributes expanded out, 
> it knows how 
> > to deal with attributes.
> 
> Hmm...but is apol ok with the fact that attributes will be 
> expanded in some cases here, since we always expand attributes for:
> - type rules
> - self rules
> - type sets that have negative sets (-) or flags (~)
> 
> I'm not sure I understand what apol wants as output; at 
> present, it will get a mixture of expanded and unexpanded attributes.
> 

Not a problem, apol is using the avtab to quickly find rules
(semantically) and resolve them to syntactic source lines if possible.
If the syntactic match doesn't match what the user requested (eg., it is
an indirect match but the user didn't request that) it can filter it out
at that time. The avtab is just a way of quickly looking up the rules
(which is also why they wanted neverallows in the avtab).



--
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 RETRY 2/3] Optionally expand neverallows

[Joshua Brindle]

Karl MacMillan wrote:
> Joshua Brindle wrote:
>>> From: Karl MacMillan [mailto:kmacmillan@mentalrootkit.com]
>>> Joshua Brindle wrote:
>>>    
>>>> @@ -366,6 +366,7 @@
>>>>     uint32_t policy_type;
>>>>     char *name;
>>>>     char *version;
>>>> +    int invalid;
>>>>
>>>>       
>>> Expanding the avrules doesn't really make the policydb invalid, 
>>> right? It jubt makes it non-standard (tainted :) )
>>>
>>>     
>>
>> Putting neverallows in the avtab is invalid (the reader will bail if it
>> tries to read one)
>>
>>   
> 
> I disagree - the policydb is still valid, otherwise there would be no 
> point doing analysis on it. I think the more correct semantic is that 
> this policydb cannot be represented by the on disc format. So the 
> variable would be better called something like 
> policydb->unsupported_format.
> 
> Wait, just because the neverallows are expanded doesn't mean that the 
> unexpanded aren't also there, meaning that the policydb could be written 
> to disc with no loss of information. This reinforces my notion that the 
> policy is valid. Why not just skip saving the neverallows in the format 
> and document the behavior?
> 


Ok, after talking to karl about this offline the basic sentiment is that 
extra work would have to be done to support writing a policy with 
expanded neverallows (namely recalulating the avtab number of elements) 
which is extra work for something that really shouldn't be happening 
anyway. He believes that "invalid" is too strong and suggests that the 
policydb is totally bogus. While this is a matter for interpretation 
what he suggested above (p->unsupported_format) might be better.

>>>>     }
>>>> @@ -2156,7 +2159,7 @@
>>>>         goto cleanup;
>>>>     hashtab_map_remove_on_error(state.out->p_types.table,
>>>>                     type_attr_remove, type_destroy, state.out);
>>>> -    if (check) {
>>>> +    if (check && !(out->invalid)) {
>>>>         if (hierarchy_check_constraints(handle, state.out))
>>>>             goto cleanup;
>>>>
>>>>       
>>> Why disallow hierarchy checking?
>>>     
>>
>> It disables all checking for invalid policies, there is no reason to do
>> checking if you've marked a policy invalid since it can't be written
>> anyway 
> 
> 1) The policy is still valid.
> 2) If this is for analysis I would imagine you would want the 
> neverallows and _also_ know if the policy violates hierarchy constraints.
> 3) It is surprising since the caller explicitly requests both the 
> checking and the expansion. If these parameters really are exclusive an 
> error should be raised rather than silently not performing the action.

I guess I can see this, I doubt that someone who requests an 
unsupported_format would also request checking so the explicit check is 
not necessary.

> 
>> (invalid will probably mean other things in the future, such as a
>> policy with disabled rules expanded out and conflicting type transitions
>> in the avtab for access control checking).
>>
>>  
>>>> Index: trunk/libsepol/src/write.c
>>>> ===================================================================
>>>> --- trunk/libsepol/src/write.c    (revision 951)
>>>> +++ trunk/libsepol/src/write.c    (working copy)
>>>> @@ -1411,6 +1411,9 @@
>>>>     struct policy_data pd;
>>>>     char *policydb_str;
>>>>
>>>> +    if (p->invalid)
>>>> +        return -1;
>>>> +
>>>>       
>>> A separate return code is needed so that the caller can distinguish 
>>> between general, likely fatal errors and a policydb that can't be 
>>> written because the format doesn't support it.
>>>

Ok, can add.

>>>     
>>
>> -2? ;)
>>
>> -1 is currently used if, for example, policy->policyvers is invalid,
>> same deal.. These are programming errors not user errors. I'd be fine
>> with it being an assert() personally.
>>   
> 
> See above - I think that the write should just work and ignore the 
> expanded neverallows.
> 

more work for no benefit, we can make this unsupported since it probably 
won't be written anyway

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