Conditionally expand neverallows

PATCH In review Joshua Brindle 2 participants 4 messages Jul 25, 2006 – Jul 27, 2006 SELinux security module

Share & tools

Joshua Brindle

author

2006-07-25 14:55 UTC #0 lore raw

PATCH 1/2 Conditionally 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.

libsepol/include/sepol/policydb/avtab.h +1 -0

diff -urpN -x 'Change*' -x entries -x '*.orig' -x '*.rej' -x '*.svn*' -x '*.swp' -x '*.o' -x '*.lo' ../../../trunk/libsepol/include/sepol/policydb/avtab.h ./include/sepol/policydb/avtab.h
@@ -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

libsepol/src/expand.c +20 -7

diff -urpN -x 'Change*' -x entries -x '*.orig' -x '*.rej' -x '*.svn*' -x '*.swp' -x '*.o' -x '*.lo' ../../../trunk/libsepol/src/expand.c ./src/expand.c
@@ -41,8 +41,14 @@ typedef struct expand_state {
 	policydb_t *base;
 	policydb_t *out;
 	sepol_handle_t *handle;
+	int expand_neverallow;
 } expand_state_t;
 
+static void expand_state_init(expand_state_t *state)
+{
+	memset(state, 0, sizeof(expand_state_t));
+}
+
 static int type_copy_callback(hashtab_key_t key, hashtab_datum_t datum,
 			      void *data)
 {

@@ -1137,6 +1143,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 +1170,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 +1210,7 @@ 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 +1237,7 @@ 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 +1274,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);

@@ -1306,7 +1317,7 @@ static int cond_avrule_list_copy(policyd
 	while (cur) {
 		if (convert_and_expand_rule(state->handle, dest_pol,
 					    typemap, cur, dest_avtab,
-					    list, other, enabled) != 1) {
+					    list, other, enabled, 0) != 1) {
 			return -1;
 		}

@@ -1897,6 +1908,8 @@ int expand_module(sepol_handle_t * handl
 	expand_state_t state;
 	avrule_block_t *curblock;
 
+	expand_state_init(&state);
+
 	state.verbose = verbose;
 	state.typemap = NULL;
 	state.base = base;

@@ -2033,7 +2046,7 @@ 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))

@@ -2043,7 +2056,7 @@ int expand_module(sepol_handle_t * handl
 				if (convert_and_expand_rule
 				    (state.handle, out, state.typemap,
 				     cur_avrule, &out->te_avtab, NULL, NULL,
-				     0) != 1) {
+				     0, state->expand_neverallow) != 1) {
 					goto cleanup;
 				}
 			}

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

2006-07-25 16:09 UTC #1 lore raw

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

How much memory does it take to expand the neverallow rules for a typical reference policy? Can you post some hashtable stats - this might make the chains very long and hurt performance for looking up non-neverallow rules. These tradeoffs may be fine for analysis, but it would be nice to have some comments explaining the effects for other users of the library.

<snip>

libsepol/src/expand.c typedef struct expand_state { · line 51

Quoted 5 lines

libsepol/src/expand.c +5 -0

+static void expand_state_init(expand_state_t *state)
+{
+	memset(state, 0, sizeof(expand_state_t));
+}
+

I assume you've audited everywhere expand is currently called to use this function?

<snip>

Quoted 10 lines

libsepol/src/expand.c +1 -1

@@ -1200,7 +1210,7 @@ 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->

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

Quoted 8 lines

libsepol/src/expand.c +1 -1

@@ -1227,7 +1237,7 @@ 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->

It might be cleaner to have a new define with AVRULE_AV and AVRULE_NEVERALLOW already or'd. I don't feel strongly about it though.

Quoted 6 lines

libsepol/src/expand.c +1 -1

@@ -1264,13 +1274,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,

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

Quoted 10 lines

libsepol/src/expand.c +2 -1

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

Magic return code.

Quoted 20 lines

libsepol/src/expand.c +3 -1

 	ebitmap_init(&stypes);

@@ -1306,7 +1317,7 @@ static int cond_avrule_list_copy(policyd
 	while (cur) {
 		if (convert_and_expand_rule(state->handle, dest_pol,
 					    typemap, cur, dest_avtab,
-					    list, other, enabled) != 1) {
+					    list, other, enabled, 0) != 1) {
 			return -1;
 		}

@@ -1897,6 +1908,8 @@ int expand_module(sepol_handle_t * handl
 	expand_state_t state;
 	avrule_block_t *curblock;
 
+	expand_state_init(&state);
+
 	state.verbose = verbose;
 	state.typemap = NULL;
 	state.base = base;

libsepol/src/expand.c int expand_module(sepol_handle_t * handl · line 2049

Quoted 5 lines

libsepol/src/expand.c +1 -1

@@ -2033,7 +2046,7 @@ 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) {

I think that the copying of the neverallows needs to be factored out completely - it is a strange side effect of expansion. The expansion functions should just, well, expand rules.

Karl

Joshua Brindle

author

2006-07-25 16:47 UTC #2 lore raw

Quoted 2 lines

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

<snip>

Quoted 9 lines

How much memory does it take to expand the neverallow rules for a typical reference policy? Can you post some hashtable stats - this might make the chains very long and hurt performance for looking up non-neverallow rules. These tradeoffs may be fine for analysis, but it would be nice to have some comments explaining the effects for other users of the library.

Other users won't expand neverallows, it is disabled by default and can only be enabled by static users.

Quoted 9 lines

<snip>
Review
Quoted 3 lines
(quoted diff) +3 -0
+static void expand_state_init(expand_state_t *state) {
+	memset(state, 0, sizeof(expand_state_t)); }
+
I assume you've audited everywhere expand is currently called to use this function?

The single place, yes.

Quoted 19 lines

<snip>

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

Quoted 6 lines

libsepol/src/expand.c +1 -1

@@ -1200,7 +1210,7 @@ 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)) {

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

Quoted 9 lines

libsepol/src/expand.c +1 -1

 				if ((retval =
 				     expand_avrule_helper(handle,
 							  source_rule->

@@ -1227,7 +1237,7 @@ 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)) {

Quoted 7 lines

libsepol/src/expand.c static int expand_rule_helper(sepol_hand · line 1216
Quoted 3 lines
libsepol/src/expand.c +0 -0
 				if ((retval =
 				     expand_avrule_helper(handle,
 							  source_rule->
It might be cleaner to have a new define with AVRULE_AV and AVRULE_NEVERALLOW already or'd. I don't feel strongly about it though.

This makes it obvious what is happening, another define would be unneeded indirection

Quoted 14 lines

Review static int convert_and_expand_rule(sepol · line 1274
Quoted 2 lines
(quoted diff) +0 -0
@@ -1264,13 +1274,14 @@ static int convert_and_expand_rule(sepol
 				   policydb_t * dest_pol, 
uint32_t * typemap,
Review static int convert_and_expand_rule(sepol · line 1275
Quoted 1 line
(quoted diff) +0 -0
 				   avrule_t * source_rule, 
avtab_t * dest_avtab,
Quoted 9 lines
libsepol/src/expand.c +2 -1
 				   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;
 

Quoted 8 lines

Review static int convert_and_expand_rule(sepol · line 1284
Quoted 2 lines
(quoted diff) +1 -1
-	if (source_rule->specified & AVRULE_NEVERALLOW)
+	if (!do_neverallow && source_rule->specified & 
AVRULE_NEVERALLOW)
libsepol/src/expand.c static int convert_and_expand_rule(sepol · line 1286
Quoted 2 lines
libsepol/src/expand.c +0 -0
 		return 1;
 
Magic return code.

Indeed, that is an unchanged line.

Quoted 28 lines

libsepol/src/expand.c static int cond_avrule_list_copy(policyd · line 1309
Quoted 6 lines
libsepol/src/expand.c +0 -1
 	ebitmap_init(&stypes);
@@ -1306,7 +1317,7 @@ static int cond_avrule_list_copy(policyd
 	while (cur) {
 		if (convert_and_expand_rule(state->handle, dest_pol,
 					    typemap, cur, dest_avtab,
-					    list, other, 
enabled) != 1) {
Review static int cond_avrule_list_copy(policyd · line 1320
Quoted 1 line
(quoted diff) +1 -0
+					    list, other, 
enabled, 0) != 1) {
Quoted 13 lines
libsepol/src/expand.c +2 -0
 			return -1;
 		}
 
@@ -1897,6 +1908,8 @@ int expand_module(sepol_handle_t * handl
 	expand_state_t state;
 	avrule_block_t *curblock;
 
+	expand_state_init(&state);
+
 	state.verbose = verbose;
 	state.typemap = NULL;
 	state.base = base;
libsepol/src/expand.c int expand_module(sepol_handle_t * handl · line 2049
Quoted 5 lines
libsepol/src/expand.c +1 -1
@@ -2033,7 +2046,7 @@ 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 &

Quoted 6 lines

Review int expand_module(sepol_handle_t * handl · line 2050
Quoted 1 line
(quoted diff) +1 -0
+AVRULE_NEVERALLOW) {
I think that the copying of the neverallows needs to be factored out completely - it is a strange side effect of expansion. The expansion functions should just, well, expand rules.

For static users maybe but the shared API will still be expand for all this stuff, making the internal and external interfaces for expand would be more confusing than having an expand function that does too much (particularly when the common case is that it does all this)

Karl MacMillan

2006-07-27 14:35 UTC #3 lore raw

Quoted 40 lines · Joshua Brindle

Quoted 40 lines
From: Karl MacMillan [mailto:[email protected]]
libsepol/src/expand.c static int cond_avrule_list_copy(policyd · line 1320
Quoted 7 lines
libsepol/src/expand.c +0 -1
 	ebitmap_init(&stypes);
@@ -1306,7 +1317,7 @@ static int cond_avrule_list_copy(policyd
 	while (cur) {
 		if (convert_and_expand_rule(state->handle, dest_pol,
 					    typemap, cur, dest_avtab,
-					    list, other, 
 
enabled) != 1) {
Review static int cond_avrule_list_copy(policyd · line 1322
Quoted 2 lines
(quoted diff) +1 -0
+					    list, other, 
 
enabled, 0) != 1) {
Quoted 13 lines
libsepol/src/expand.c +0 -0
 			return -1;
libsepol/src/expand.c +2 -0
 		}
@@ -1897,6 +1908,8 @@ int expand_module(sepol_handle_t * handl
 	expand_state_t state;
 	avrule_block_t *curblock;
 
+	expand_state_init(&state);
+
 	state.verbose = verbose;
 	state.typemap = NULL;
 	state.base = base;
libsepol/src/expand.c int expand_module(sepol_handle_t * handl · line 2050
Quoted 6 lines
libsepol/src/expand.c +1 -1
@@ -2033,7 +2046,7 @@ 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 &
Quoted 2 lines
(quoted diff) +1 -0
+AVRULE_NEVERALLOW) {
 

Quoted 10 lines

Quoted 3 lines
I think that the copying of the neverallows needs to be factored out completely - it is a strange side effect of expansion. The expansion functions should just, well, expand rules.
For static users maybe but the shared API will still be expand for all this stuff, making the internal and external interfaces for expand would be more confusing than having an expand function that does too much (particularly when the common case is that it does all this)

I can't quite parse this comment. Copying the neverallows when they are not expanded is just strange. A separate function to copy the neverallows should be provided.

Karl