Optionally expand neverallows

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.

libsepol/src/expand.c +0 -1

@@ -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)

Quoted 8 lines · 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.

libsepol/src/expand.c static int expand_rule_helper(sepol_hand · line 1212

Quoted 9 lines

libsepol/src/expand.c +2 -1

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

Quoted 12 lines

libsepol/src/expand.c +2 -1

@@ -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;
 

libsepol/src/expand.c static int convert_and_expand_rule(sepol · line 1282

Quoted 3 lines

libsepol/src/expand.c +1 -1

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

Quoted 9 lines · Stephen Smalley

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.

From: Karl MacMillan [mailto:[email protected]]

Quoted 2 lines · Joshua Brindle

Quoted 1 line · Stephen Smalley

Did you mean to include the rename for expand_avrule_decls?

Quoted 4 lines

Index: trunk/libsepol/include/sepol/policydb/policydb.h
===================================================================
--- trunk/libsepol/include/sepol/policydb/policydb.h

(revision 951)

Quoted 1 line

+++ trunk/libsepol/include/sepol/policydb/policydb.h

(working copy)

Review line 370

Quoted 6 lines

(quoted diff) +1 -0

@@ -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 :) )

Review line 2165

Quoted 11 lines

(quoted diff) +1 -1

    }

@@ -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?

Quoted 9 lines

Index: trunk/libsepol/src/write.c
===================================================================

libsepol/src/write.c (working copy) renamed from libsepol/src/write.c (revision 951) +0 -0

@@ -1411,6 +1411,9 @@
    struct policy_data pd;
    char *policydb_str;
 

libsepol/src/write.c (working copy) line 1416

Quoted 3 lines

libsepol/src/write.c (working copy) renamed from libsepol/src/write.c (revision 951) +3 -0

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

Quoted 4 lines

From: Karl MacMillan [mailto:[email protected]]

Joshua Brindle wrote:

Quoted 10 lines

Review line 371

Quoted 7 lines

(quoted diff) +1 -0

@@ -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)

Quoted 4 lines

Quoted 4 lines

(quoted diff) +0 -0

    }

@@ -2156,7 +2159,7 @@
        goto cleanup;
    hashtab_map_remove_on_error(state.out->p_types.table,

Quoted 9 lines

Review line 2165

Quoted 7 lines

(quoted diff) +1 -1

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

Quoted 8 lines

Quoted 8 lines

Index: trunk/libsepol/src/write.c
===================================================================

libsepol/src/write.c (working copy) renamed from libsepol/src/write.c (revision 951) +0 -0

@@ -1411,6 +1411,9 @@
    struct policy_data pd;
    char *policydb_str;
 

Quoted 7 lines

libsepol/src/write.c (working copy) line 1417

Quoted 4 lines

libsepol/src/write.c (working copy) renamed from libsepol/src/write.c (revision 951) +3 -0

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

Quoted 14 lines · Joshua Brindle

Quoted 13 lines

From: Karl MacMillan [mailto:[email protected]]

Review line 371

Quoted 7 lines · Joshua Brindle

(quoted diff) +1 -0

@@ -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 :) )

Quoted 4 lines

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?

Quoted 14 lines

Quoted 13 lines

Review line 2165

Quoted 11 lines

(quoted diff) +1 -1

    }

@@ -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?

Quoted 3 lines

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.

Quoted 14 lines

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

Quoted 8 lines

Quoted 8 lines

Index: trunk/libsepol/src/write.c
===================================================================

libsepol/src/write.c (working copy) renamed from libsepol/src/write.c (revision 951) +0 -0

@@ -1411,6 +1411,9 @@
    struct policy_data pd;
    char *policydb_str;
 

Quoted 6 lines

Quoted 6 lines

libsepol/src/write.c (working copy) line 1417

Quoted 4 lines

libsepol/src/write.c (working copy) renamed from libsepol/src/write.c (revision 951) +3 -0

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

Quoted 8 lines

Quoted 1 line

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

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.

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?

Index: trunk/libsepol/src/link.c
===================================================================

libsepol/src/link.c +1 -2

@@ -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];

Quoted 3 lines

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.

From: Stephen Smalley [mailto:[email protected]]

libsepol/src/link.c line 965

Quoted 6 lines · Joshua Brindle

Index: trunk/libsepol/src/link.c
===================================================================

libsepol/src/link.c +0 -0

@@ -965,8 +965,7 @@
 

module->map[SYM_CLASSES][cur_perm->class - 1];

libsepol/src/link.c

Quoted 4 lines

libsepol/src/link.c +0 -2

 			assert(new_perm->class);
 
-			if (new_rule->
-			    specified & (AVRULE_AV | 

AVRULE_NEVERALLOW)) {

libsepol/src/link.c

Quoted 4 lines

libsepol/src/link.c +1 -0

+			if (new_rule->specified & (AVRULE_AV)) {
 				for (i = 0;
 				     i <
 

module->perm_map_len[cur_perm->class - 1];

Quoted 7 lines

Quoted 5 lines

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

Quoted 1 line

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.