Skip to contentSkip to conversation

Fix some cil_gen_policy() bugs.

PATCH In review Daniel Cashman 2 participants 10 messages Sep 8, 2016 – Sep 9, 2016 SELinux security module

lore
Share & tools
lore

Daniel Cashman

author
#0 lore raw

cover Fix some cil_gen_policy() bugs.

From: dcashman <[email protected]>

cil_gen_policy() appears to exist to generate a policy.conf corresponding to the original SELinux HLL from a cil_db struct. All of libsepol/cil/src/cil_policy.c appears to exist to support this functionality. This patchset provides some fixes for issues encountered when trying to go from android's policy.conf to a CIL representation (via checkpolicy) and then back to the HLL representation via cil_gen_policy().

dcashman (5):
libsepol: cil: Add userrole mapping to cil_gen_policy().
libsepol: cil: Remove duplicate sid policy declaration.
libsepol: cil: Replace sensitivityorder statement.
libsepol: cil: Fix CIL_OP data assignment.
libsepol: cil: Add cil_constraint_expr_to_policy()

 libsepol/cil/src/cil_policy.c | 235 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 224 insertions(+), 11 deletions(-)
Signature · 1 line
2.8.0.rc3.226.g39d4020
author
#1 lore raw

PATCH 1/5 libsepol: cil: Add userrole mapping to cil_gen_policy().

From: dcashman <[email protected]>

Avoid a "No roles associated with user" error produced by cil_userrole_to_policy() when a userrole mapping is present in CIL policy.

Signed-off-by Daniel Cashman <[email protected]>
---
 libsepol/cil/src/cil_policy.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
libsepol/cil/src/cil_policy.c +6 -1
index 382129b..324becc 100644
@@ -1155,7 +1155,12 @@ int __cil_gen_policy_node_helper(struct cil_tree_node *node, uint32_t *finished,
 	} else {
 		switch (node->flavor) {
 		case CIL_USER:
-			cil_multimap_insert(users, node->data, NULL, CIL_USERROLE, CIL_NONE);
+			cil_multimap_insert(users, node->data, NULL, CIL_USER, CIL_NONE);
+			break;
+		case CIL_USERROLE: {
+			struct cil_userrole *userrole = node->data;
+			cil_multimap_insert(users, userrole->user, userrole->role, CIL_USER, CIL_ROLE);
+		}
 			break;
 		case CIL_CATALIAS: {
 			struct cil_alias *alias = node->data;
Signature · 1 line
2.8.0.rc3.226.g39d4020
author
#2 lore raw

PATCH 2/5 libsepol: cil: Remove duplicate sid policy declaration.

From: dcashman <[email protected]>

cil_gen_policy() creates two sets of sid declarations from CIL policy due to the combination of sidorder iteration and the CIL_SID case in the AST walk. Remove the sidorder iteration.

Signed-off-by Daniel Cashman <[email protected]>
---
 libsepol/cil/src/cil_policy.c | 4 ----
 1 file changed, 4 deletions(-)
libsepol/cil/src/cil_policy.c +0 -4
index 324becc..d8ef151 100644
@@ -1272,10 +1272,6 @@ int cil_gen_policy(struct cil_db *db)
 
 	policy_file = fopen("policy.conf", "w+");
 
-	cil_list_for_each(item, db->sidorder) {
-		fprintf(file_arr[ISIDS], "sid %s ", ((struct cil_sid*)item->data)->datum.name);
-	}
-
 	cil_list_for_each(item, db->classorder) {
 		struct cil_class *class = item->data;
 		struct cil_tree_node *node = class->datum.nodes->head->data;
Signature · 1 line
2.8.0.rc3.226.g39d4020
author
#3 lore raw

PATCH 3/5 libsepol: cil: Replace sensitivityorder statement.

From: dcashman <[email protected]>

cil_gen_policy() prints a sensitivityorder{}; output statement when generating its policy.conf file from CIL policy. This omits the sensitivity declarations, however, and should instead be represented as a sid declaration block followed by a dominance statement.

Signed-off-by Daniel Cashman <[email protected]>
---
 libsepol/cil/src/cil_policy.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
libsepol/cil/src/cil_policy.c +5 -2
index d8ef151..78b135e 100644
@@ -1301,11 +1301,14 @@ int cil_gen_policy(struct cil_db *db)
 	}
 
 	if (db->sensitivityorder->head != NULL) {
-		fprintf(file_arr[SENS], "sensitivityorder { ");
+		cil_list_for_each(item, db->sensitivityorder) {
+			fprintf(file_arr[SENS], "sensitivity %s;\n", ((struct cil_sens*)item->data)->datum.name);
+		}
+		fprintf(file_arr[SENS], "dominance { ");
 		cil_list_for_each(item, db->sensitivityorder) {
 			fprintf(file_arr[SENS], "%s ", ((struct cil_sens*)item->data)->datum.name);
 		}
-		fprintf(file_arr[SENS], "};\n");
+		fprintf(file_arr[SENS], "}\n");
 	}
 
 	extra_args.users = users;
Signature · 1 line
2.8.0.rc3.226.g39d4020
author
#4 lore raw

PATCH 4/5 libsepol: cil: Fix CIL_OP data assignment.

From: dcashman <[email protected]>

cil_flavor enums stored in cil_list_items are not pointers, but rather the actual enum value. Remove pointer dereferences on this value to avoid segfaults.

Signed-off-by Daniel Cashman <[email protected]>
---
 libsepol/cil/src/cil_policy.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
libsepol/cil/src/cil_policy.c +3 -3
index 78b135e..32b6b41 100644
@@ -470,7 +470,7 @@ void cil_perms_to_policy(FILE **file_arr, uint32_t file_index, struct cil_list *
 			fprintf(file_arr[file_index], " %s", ((struct cil_symtab_datum *)curr->data)->name);
 			break;
 		case CIL_OP: {
-			enum cil_flavor op_flavor = *((enum cil_flavor *)curr->data);
+			enum cil_flavor op_flavor = (enum cil_flavor)curr->data;
 			char *op_str = NULL;
 
 			switch (op_flavor) {

@@ -673,7 +673,7 @@ static int cil_expr_to_string(struct cil_list *expr, char **out)
 		case CIL_OP: {
 			int len;
 			char *expr_str;
-			enum cil_flavor op_flavor = *((enum cil_flavor *)curr->data);
+			enum cil_flavor op_flavor = (enum cil_flavor)curr->data;
 			char *op_str = NULL;
 
 			if (pos == 0) {

@@ -742,7 +742,7 @@ static int cil_expr_to_string(struct cil_list *expr, char **out)
 			break;
 		}
 		case CIL_CONS_OPERAND: {
-			enum cil_flavor operand_flavor = *((enum cil_flavor *)curr->data);
+			enum cil_flavor operand_flavor = (enum cil_flavor)curr->data;
 			char *operand_str = NULL;
 			switch (operand_flavor) {
 			case CIL_CONS_U1:
Signature · 1 line
2.8.0.rc3.226.g39d4020
author
#5 lore raw

PATCH 5/5 libsepol: cil: Add cil_constraint_expr_to_policy()

From: dcashman <[email protected]>

The current cil_expr_to_policy() does not properly hanlde the case where CIL_OP is at the beginning of an expression. Create a new function, cil_constraint_expr_to_policy() rather than modifying the original, since the expression syntax for constraint expressions requires this ability, but the existing cil_expr_to_policy() function has many other consumers.

Signed-off-by Daniel Cashman <[email protected]>
---
 libsepol/cil/src/cil_policy.c | 211 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 210 insertions(+), 1 deletion(-)
libsepol/cil/src/cil_policy.c +210 -1
index 32b6b41..e5ca091 100644
@@ -84,6 +84,8 @@ struct cil_args_booleanif {
 
 int cil_expr_to_policy(FILE **file_arr, uint32_t file_index, struct cil_list *expr);
 
+int cil_constraint_expr_to_policy(FILE **file_arr, uint32_t file_index, struct cil_list *expr);
+
 int cil_combine_policy(FILE **file_arr, FILE *policy_file)
 {
 	char temp[BUFFER];

@@ -515,7 +517,7 @@ void cil_constrain_to_policy_helper(FILE **file_arr, char *kind, struct cil_list
 				fprintf(file_arr[CONSTRAINS], "%s %s", kind, cp->class->datum.name);
 				cil_perms_to_policy(file_arr, CONSTRAINS, cp->perms);
 				fprintf(file_arr[CONSTRAINS], "\n\t");
-				cil_expr_to_policy(file_arr, CONSTRAINS, expr);
+				cil_constraint_expr_to_policy(file_arr, CONSTRAINS, expr);
 				fprintf(file_arr[CONSTRAINS], ";\n");
 			} else { /* MAP */
 				struct cil_list_item *i = NULL;

@@ -811,6 +813,195 @@ exit:
 	return rc;
 }
 
+static int cil_constraint_expr_to_string(struct cil_list *expr, char **out)
+{
+	int rc = SEPOL_ERR;
+	struct cil_list_item *curr;
+	const size_t CONS_DEPTH = 3;
+	char *stack[CONS_DEPTH] = {}; // 1 operator + 1 - 2 operands
+	size_t pos = 0;
+	size_t i;
+
+	cil_list_for_each(curr, expr) {
+		if (pos >= CONS_DEPTH) {
+			rc = SEPOL_ERR;
+			goto exit;
+		}
+		switch (curr->flavor) {
+		case CIL_LIST:
+			rc = cil_constraint_expr_to_string(curr->data, &stack[pos]);
+			if (rc != SEPOL_OK) {
+				goto exit;
+			}
+			pos++;
+			break;
+		case CIL_STRING:
+			stack[pos] = strdup(curr->data);
+			if (!stack[pos]) {
+				cil_log(CIL_ERR, "OOM. Unable to convert cons_expr to string\n");
+				rc = SEPOL_ERR;
+				goto exit;
+			}
+			pos++;
+			break;
+		case CIL_DATUM:
+			stack[pos] = strdup(((struct cil_symtab_datum *)curr->data)->name);
+			if (!stack[pos]) {
+				cil_log(CIL_ERR, "OOM. Unable to convert cons_expr to string\n");
+				rc = SEPOL_ERR;
+				goto exit;
+			}
+			pos++;
+			break;
+		case CIL_OP: {
+			enum cil_flavor op_flavor = (enum cil_flavor)curr->data;
+			char *op_str = NULL;
+
+			if (pos != 0) {
+				/* ops come before the operand(s) */
+				cil_log(CIL_ERR, "CIL_OP encountered at incorrect offset\n");
+				rc = SEPOL_ERR;
+				goto exit;
+			}
+			switch (op_flavor) {
+			case CIL_AND:
+				op_str = CIL_KEY_AND;
+				break;
+			case CIL_OR:
+				op_str = CIL_KEY_OR;
+				break;
+			case CIL_NOT:
+				op_str = CIL_KEY_NOT;
+				break;
+			case CIL_ALL:
+				op_str = CIL_KEY_ALL;
+				break;
+			case CIL_EQ:
+				op_str = CIL_KEY_EQ;
+				break;
+			case CIL_NEQ:
+				op_str = CIL_KEY_NEQ;
+				break;
+			case CIL_XOR:
+				op_str = CIL_KEY_XOR;
+				break;
+			case CIL_CONS_DOM:
+				op_str = CIL_KEY_CONS_DOM;
+				break;
+			case CIL_CONS_DOMBY:
+				op_str = CIL_KEY_CONS_DOMBY;
+				break;
+			case CIL_CONS_INCOMP:
+				op_str = CIL_KEY_CONS_INCOMP;
+				break;
+			default:
+				cil_log(CIL_ERR, "Unknown operator in expression\n");
+				rc = SEPOL_ERR;
+				goto exit;
+				break;
+			}
+			stack[pos] = strdup(op_str);
+			if (!stack[pos]) {
+				cil_log(CIL_ERR, "OOM. Unable to convert cons_expr to string\n");
+				rc = SEPOL_ERR;
+				goto exit;
+			}
+			pos++;
+			break;
+		}
+		case CIL_CONS_OPERAND: {
+			enum cil_flavor operand_flavor = (enum cil_flavor)curr->data;
+			char *operand_str = NULL;
+			switch (operand_flavor) {
+			case CIL_CONS_U1:
+				operand_str = CIL_KEY_CONS_U1;
+				break;
+			case CIL_CONS_U2:
+				operand_str = CIL_KEY_CONS_U2;
+				break;
+			case CIL_CONS_U3:
+				operand_str = CIL_KEY_CONS_U3;
+				break;
+			case CIL_CONS_T1:
+				operand_str = CIL_KEY_CONS_T1;
+				break;
+			case CIL_CONS_T2:
+				operand_str = CIL_KEY_CONS_T2;
+				break;
+			case CIL_CONS_T3:
+				operand_str = CIL_KEY_CONS_T3;
+				break;
+			case CIL_CONS_R1:
+				operand_str = CIL_KEY_CONS_R1;
+				break;
+			case CIL_CONS_R2:
+				operand_str = CIL_KEY_CONS_R2;
+				break;
+			case CIL_CONS_R3:
+				operand_str = CIL_KEY_CONS_R3;
+				break;
+			case CIL_CONS_L1:
+				operand_str = CIL_KEY_CONS_L1;
+				break;
+			case CIL_CONS_L2:
+				operand_str = CIL_KEY_CONS_L2;
+				break;
+			case CIL_CONS_H1:
+				operand_str = CIL_KEY_CONS_H1;
+				break;
+			case CIL_CONS_H2:
+				operand_str = CIL_KEY_CONS_H2;
+				break;
+			default:
+				cil_log(CIL_ERR, "Unknown operand in expression\n");
+				goto exit;
+				break;
+			}
+			stack[pos] = strdup(operand_str);
+			if (!stack[pos]) {
+				cil_log(CIL_ERR, "OOM. Unable to convert cons_expr to string\n");
+				rc = SEPOL_ERR;
+				goto exit;
+			}
+			pos++;
+			break;
+		}
+		default:
+			cil_log(CIL_ERR, "Unknown flavor in expression\n");
+			rc = SEPOL_ERR;
+			goto exit;
+			break;
+		}
+	}
+	if (pos > 3) {
+		cil_log(CIL_ERR, "Illegal CIL expr: more than 3 components\n");
+		rc = SEPOL_ERR;
+		goto exit;
+	}
+	size_t len;
+	char *expr_str;
+	if (!strcmp(stack[0], CIL_KEY_NOT)) {
+		/* All ops take 2 operands except for CIL_KEY_NOT */
+		len = strlen(stack[0]) + strlen(stack[1]) + 4;
+		expr_str = cil_malloc(len);
+		snprintf(expr_str, len, "(%s %s)", stack[0], stack[1]);
+		/* free() done below */
+	} else {
+		len = strlen(stack[0]) + strlen(stack[1]) + strlen(stack[2]) + 5;
+		expr_str = cil_malloc(len);
+		snprintf(expr_str, len, "(%s %s %s)", stack[1], stack[0], stack[2]);
+		/* free() done below */
+	}
+	*out = expr_str;
+	rc = SEPOL_OK;
+exit:
+	for (i = 0; i < pos; i++) {
+		free(stack[i]);
+		stack[i] = NULL;
+	}
+	return rc;
+}
+
 int cil_expr_to_policy(FILE **file_arr, uint32_t file_index, struct cil_list *expr)
 {
 	int rc = SEPOL_ERR;

@@ -829,6 +1020,24 @@ out:
 	return rc;
 }
 
+int cil_constraint_expr_to_policy(FILE **file_arr, uint32_t file_index, struct cil_list *expr)
+{
+	int rc = SEPOL_ERR;
+	char *str_out;
+
+	rc = cil_constraint_expr_to_string(expr, &str_out);
+	if (rc != SEPOL_OK) {
+		goto out;
+	}
+	fprintf(file_arr[file_index], "%s", str_out);
+	free(str_out);
+
+	return SEPOL_OK;
+
+out:
+	return rc;
+}
+
 int __cil_booleanif_node_helper(struct cil_tree_node *node, __attribute__((unused)) uint32_t *finished, void *extra_args)
 {
 	int rc = SEPOL_ERR;
Signature · 1 line
2.8.0.rc3.226.g39d4020
author
#6 lore raw
In reply to Daniel Cashman
Quoted 16 lines · Daniel Cashman

From: dcashman <[email protected]>

cil_gen_policy() appears to exist to generate a policy.conf corresponding to the original SELinux HLL from a cil_db struct. All of libsepol/cil/src/cil_policy.c appears to exist to support this functionality. This patchset provides some fixes for issues encountered when trying to go from android's policy.conf to a CIL representation (via checkpolicy) and then back to the HLL representation via cil_gen_policy().

dcashman (5):
libsepol: cil: Add userrole mapping to cil_gen_policy().
libsepol: cil: Remove duplicate sid policy declaration.
libsepol: cil: Replace sensitivityorder statement.
libsepol: cil: Fix CIL_OP data assignment.
libsepol: cil: Add cil_constraint_expr_to_policy()

Quoted 3 lines
 libsepol/cil/src/cil_policy.c | 235 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 224 insertions(+), 11 deletions(-)

I suspect that the "proper" fix here is to just remove all of libsepol/cil/src/cil_policy.c, so I can put that patch together too if desired.

The patches in this patchset do not address all of the bugs I encountered trying to go from HLL -> CIL -> HLL. Since I was using this as a temporary work-around, I decided to move on and submit these, in case rescuing cil_gen_policy() is desired; the additional changes needed were becoming more invasive (similar to the 5th patch in this set) and less bug-fix-like.

Thank You,
Dan

#7 lore raw
Quoted 17 lines · Daniel Cashman
Quoted 16 lines · Daniel Cashman

From: dcashman <[email protected]>

cil_gen_policy() appears to exist to generate a policy.conf corresponding to the original SELinux HLL from a cil_db struct. All of libsepol/cil/src/cil_policy.c appears to exist to support this functionality. This patchset provides some fixes for issues encountered when trying to go from android's policy.conf to a CIL representation (via checkpolicy) and then back to the HLL representation via cil_gen_policy().

dcashman (5):
libsepol: cil: Add userrole mapping to cil_gen_policy().
libsepol: cil: Remove duplicate sid policy declaration.
libsepol: cil: Replace sensitivityorder statement.
libsepol: cil: Fix CIL_OP data assignment.
libsepol: cil: Add cil_constraint_expr_to_policy()

Quoted 8 lines
Quoted 3 lines
 libsepol/cil/src/cil_policy.c | 235 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 224 insertions(+), 11 deletions(-)

I suspect that the "proper" fix here is to just remove all of libsepol/cil/src/cil_policy.c, so I can put that patch together too if desired.

Yes, that code was used early on to help with debugging the CIL compiler, but hasn't been maintained. I've wanted to go back and fix it, but there didn't seem to be any use case needing it before now.

If that functionality would be valuable to you, I would be glad to work on this.

I think the right course would be to move this out of libsepol like secilc is.

Jim

Quoted 10 lines

The patches in this patchset do not address all of the bugs I encountered trying to go from HLL -> CIL -> HLL. Since I was using this as a temporary work-around, I decided to move on and submit these, in case rescuing cil_gen_policy() is desired; the additional changes needed were becoming more invasive (similar to the 5th patch in this set) and less bug-fix-like.

Thank You,
Dan

Signature · 2 lines
James Carter <[email protected]>
National Security Agency
#8 lore raw
Quoted 27 lines · James Carter
Quoted 17 lines · Daniel Cashman
Quoted 16 lines · Daniel Cashman

From: dcashman <[email protected]>

cil_gen_policy() appears to exist to generate a policy.conf corresponding to the original SELinux HLL from a cil_db struct. All of libsepol/cil/src/cil_policy.c appears to exist to support this functionality. This patchset provides some fixes for issues encountered when trying to go from android's policy.conf to a CIL representation (via checkpolicy) and then back to the HLL representation via cil_gen_policy().

dcashman (5):
libsepol: cil: Add userrole mapping to cil_gen_policy().
libsepol: cil: Remove duplicate sid policy declaration.
libsepol: cil: Replace sensitivityorder statement.
libsepol: cil: Fix CIL_OP data assignment.
libsepol: cil: Add cil_constraint_expr_to_policy()

Quoted 8 lines
Quoted 3 lines
 libsepol/cil/src/cil_policy.c | 235 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 224 insertions(+), 11 deletions(-)

I suspect that the "proper" fix here is to just remove all of libsepol/cil/src/cil_policy.c, so I can put that patch together too if desired.

Quoted 8 lines

Yes, that code was used early on to help with debugging the CIL compiler, but hasn't been maintained. I've wanted to go back and fix it, but there didn't seem to be any use case needing it before now.

If that functionality would be valuable to you, I would be glad to work on this.

I think the right course would be to move this out of libsepol like secilc is.

The caffeine hadn't kicked in yet. cil_policy.c is like cil_binary.c and should stay where it is.

Jim

Quoted 14 lines

Jim

Quoted 10 lines

The patches in this patchset do not address all of the bugs I encountered trying to go from HLL -> CIL -> HLL. Since I was using this as a temporary work-around, I decided to move on and submit these, in case rescuing cil_gen_policy() is desired; the additional changes needed were becoming more invasive (similar to the 5th patch in this set) and less bug-fix-like.

Thank You,
Dan

Signature · 2 lines
James Carter <[email protected]>
National Security Agency
author
#9 lore raw
Quoted 43 lines · James Carter
Quoted 42 lines · James Carter
Quoted 22 lines · Daniel Cashman
Quoted 21 lines · Daniel Cashman

From: dcashman <[email protected]>

cil_gen_policy() appears to exist to generate a policy.conf corresponding to the
original SELinux HLL from a cil_db struct. All of libsepol/cil/src/cil_policy.c
appears to exist to support this functionality. This patchset provides some
fixes for issues encountered when trying to go from android's policy.conf to a
CIL representation (via checkpolicy) and then back to the HLL representation via
cil_gen_policy().

dcashman (5):
libsepol: cil: Add userrole mapping to cil_gen_policy().
libsepol: cil: Remove duplicate sid policy declaration.
libsepol: cil: Replace sensitivityorder statement.
libsepol: cil: Fix CIL_OP data assignment.
libsepol: cil: Add cil_constraint_expr_to_policy()

Quoted 9 lines
Quoted 4 lines
 libsepol/cil/src/cil_policy.c | 235
++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 224 insertions(+), 11 deletions(-)

I suspect that the "proper" fix here is to just remove all of libsepol/cil/src/cil_policy.c, so I can put that patch together too if desired.

Yes, that code was used early on to help with debugging the CIL compiler, but
hasn't been maintained. I've wanted to go back and fix it, but there didn't seem
to be any use case needing it before now.

If that functionality would be valuable to you, I would be glad to work on this.

Quoted 9 lines
Quoted 3 lines

I think the right course would be to move this out of libsepol like secilc is.

The caffeine hadn't kicked in yet. cil_policy.c is like cil_binary.c and should stay where it is.

Jim

Yes, it requires access to the cil_db internals, most-importantly the ast. I'm trying to do similar processing to replace types and attributes to new attributes (what I'm calling 'attributizing') for portions of policy. Thus, I think any changes I make will also have to live in libsepol, although we'll see eventually how acceptable they are for upstream.

As for the usefulness of cil_gen_policy(), my actual desire was to get some CIL -> CIL code, perhaps a cil_write_ast() used as part of a cil_gen_cil() function, that would allow me to make some AST modifications and then produce transformed CIL policy. I noticed cil_gen_policy() as a potential shortcut to allow me to postpone that further. I don't currently see a need for cil_gen_policy() outside of testing other changes, so I submitted the fixes I'd come up with before deciding to continue with another approach.

Dan

Quoted 16 lines
Quoted 14 lines

Jim

Quoted 10 lines

The patches in this patchset do not address all of the bugs I encountered trying to go from HLL -> CIL -> HLL. Since I was using this as a temporary work-around, I decided to move on and submit these, in case rescuing cil_gen_policy() is desired; the additional changes needed were becoming more invasive (similar to the 5th patch in this set) and less bug-fix-like.

Thank You,
Dan