[PATCH 00/11] Various CIL patches

[PATCH 00/11] Various CIL patches

[James Carter]

All of these patches (except the first) have been sent to the list in the
last month. The first patch is a slight modification of an earlier patch
sent by Nicolas Iooss. I also corrected a couple of whitespace errors and
updated one patch that would not apply cleanly with one of the earlier
patches applied.

James Carter (11):
  libsepol/cil: Fix out-of-bound read of file context pattern ending
    with "\"
  libsepol/cil: Destroy classperms list when resetting classpermission
  libsepol/cil: Destroy classperm list when resetting map perms
  libsepol/cil: cil_reset_classperms_set() should not reset
    classpermission
  libsepol/cil: Set class field to NULL when resetting struct
    cil_classperms
  libsepol/cil: More strict verification of constraint leaf expressions
  libsepol/cil: Exit with an error if declaration name is a reserved
    word
  libsepol/cil: Allow permission expressions when using map classes
  libsepol/cil: Refactor helper function for cil_gen_node()
  libsepol/cil: Create function cil_add_decl_to_symtab() and refactor
  libsepol/cil: Move check for the shadowing of macro parameters

 libsepol/cil/src/cil_build_ast.c | 132 ++++++++++++-------------------
 libsepol/cil/src/cil_build_ast.h |   2 +
 libsepol/cil/src/cil_copy_ast.c  |  26 +++---
 libsepol/cil/src/cil_post.c      |  11 +++
 libsepol/cil/src/cil_reset_ast.c |  11 ++-
 libsepol/cil/src/cil_verify.c    |  78 +++++++++++++++++-
 libsepol/cil/src/cil_verify.h    |   3 +-
 7 files changed, 160 insertions(+), 103 deletions(-)

-- 
2.26.3

[PATCH 01/11] libsepol/cil: Fix out-of-bound read of file context pattern ending with "\"

[James Carter]

Based on patch by Nicolas Iooss, who writes:
  OSS-Fuzz found a Heap-buffer-overflow in the CIL compiler when trying
  to compile the following policy:

    (sid SID)
    (sidorder(SID))
    (filecon "\" any ())
    (filecon "" any ())

  When cil_post_fc_fill_data() processes "\", it goes beyond the NUL
  terminator of the string. Fix this by returning when '\0' is read
  after a backslash.

To be consistent with the function compute_diffdata() in
refpolicy/support/fc_sort.py, also increment str_len in this case.

Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28484
Reported-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_post.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/libsepol/cil/src/cil_post.c b/libsepol/cil/src/cil_post.c
index d2ecbd43..fd4758dc 100644
--- a/libsepol/cil/src/cil_post.c
+++ b/libsepol/cil/src/cil_post.c
@@ -186,6 +186,13 @@ static void cil_post_fc_fill_data(struct fc_data *fc, const char *path)
 			break;
 		case '\\':
 			c++;
+			if (path[c] == '\0') {
+				if (!fc->meta) {
+					fc->stem_len++;
+				}
+				fc->str_len++;
+				return;
+			}
 			/* FALLTHRU */
 		default:
 			if (!fc->meta) {
-- 
2.26.3

[PATCH 02/11] libsepol/cil: Destroy classperms list when resetting classpermission

[James Carter]

Nicolas Iooss reports:
  A few months ago, OSS-Fuzz found a crash in the CIL compiler, which
  got reported as
  https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28648 (the title
  is misleading, or is caused by another issue that conflicts with the
  one I report in this message). Here is a minimized CIL policy which
  reproduces the issue:

  (class CLASS (PERM))
  (classorder (CLASS))
  (sid SID)
  (sidorder (SID))
  (user USER)
  (role ROLE)
  (type TYPE)
  (category CAT)
  (categoryorder (CAT))
  (sensitivity SENS)
  (sensitivityorder (SENS))
  (sensitivitycategory SENS (CAT))
  (allow TYPE self (CLASS (PERM)))
  (roletype ROLE TYPE)
  (userrole USER ROLE)
  (userlevel USER (SENS))
  (userrange USER ((SENS)(SENS (CAT))))
  (sidcontext SID (USER ROLE TYPE ((SENS)(SENS))))

  (classpermission CLAPERM)

  (optional OPT
      (roletype nonexistingrole nonexistingtype)
      (classpermissionset CLAPERM (CLASS (PERM)))
  )

  The CIL policy fuzzer (which mimics secilc built with clang Address
  Sanitizer) reports:

  ==36541==ERROR: AddressSanitizer: heap-use-after-free on address
  0x603000004f98 at pc 0x56445134c842 bp 0x7ffe2a256590 sp
  0x7ffe2a256588
  READ of size 8 at 0x603000004f98 thread T0
      #0 0x56445134c841 in __cil_verify_classperms
  /selinux/libsepol/src/../cil/src/cil_verify.c:1620:8
      #1 0x56445134a43e in __cil_verify_classpermission
  /selinux/libsepol/src/../cil/src/cil_verify.c:1650:9
      #2 0x56445134a43e in __cil_pre_verify_helper
  /selinux/libsepol/src/../cil/src/cil_verify.c:1715:8
      #3 0x5644513225ac in cil_tree_walk_core
  /selinux/libsepol/src/../cil/src/cil_tree.c:272:9
      #4 0x564451322ab1 in cil_tree_walk
  /selinux/libsepol/src/../cil/src/cil_tree.c:316:7
      #5 0x5644513226af in cil_tree_walk_core
  /selinux/libsepol/src/../cil/src/cil_tree.c:284:9
      #6 0x564451322ab1 in cil_tree_walk
  /selinux/libsepol/src/../cil/src/cil_tree.c:316:7
      #7 0x5644512b88fd in cil_pre_verify
  /selinux/libsepol/src/../cil/src/cil_post.c:2510:7
      #8 0x5644512b88fd in cil_post_process
  /selinux/libsepol/src/../cil/src/cil_post.c:2524:7
      #9 0x5644511856ff in cil_compile
  /selinux/libsepol/src/../cil/src/cil.c:564:7

The classperms list of a classpermission rule is created and filled
in when classpermissionset rules are processed, so it doesn't own any
part of the list and shouldn't retain any of it when it is reset.

Destroy the classperms list (without destroying the data in it)  when
resetting a classpermission rule.

Reported-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_reset_ast.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libsepol/cil/src/cil_reset_ast.c b/libsepol/cil/src/cil_reset_ast.c
index 3da1b9a6..db70a535 100644
--- a/libsepol/cil/src/cil_reset_ast.c
+++ b/libsepol/cil/src/cil_reset_ast.c
@@ -54,7 +54,7 @@ static void cil_reset_classpermission(struct cil_classpermission *cp)
 		return;
 	}
 
-	cil_reset_classperms_list(cp->classperms);
+	cil_list_destroy(&cp->classperms, CIL_FALSE);
 }
 
 static void cil_reset_classperms_set(struct cil_classperms_set *cp_set)
-- 
2.26.3

[PATCH 03/11] libsepol/cil: Destroy classperm list when resetting map perms

[James Carter]

Map perms share the same struct as regular perms, but only the
map perms use the classperms field. This field is a pointer to a
list of classperms that is created and added to when resolving
classmapping rules, so the map permission doesn't own any of the
data in the list and this list should be destroyed when the AST is
reset.

When resetting a perm, destroy the classperms list without destroying
the data in the list.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_reset_ast.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libsepol/cil/src/cil_reset_ast.c b/libsepol/cil/src/cil_reset_ast.c
index db70a535..89f91e56 100644
--- a/libsepol/cil/src/cil_reset_ast.c
+++ b/libsepol/cil/src/cil_reset_ast.c
@@ -36,7 +36,7 @@ static void cil_reset_class(struct cil_class *class)
 
 static void cil_reset_perm(struct cil_perm *perm)
 {
-	cil_reset_classperms_list(perm->classperms);
+	cil_list_destroy(&perm->classperms, CIL_FALSE);
 }
 
 static inline void cil_reset_classperms(struct cil_classperms *cp)
-- 
2.26.3

[PATCH 04/11] libsepol/cil: cil_reset_classperms_set() should not reset classpermission

[James Carter]

In struct cil_classperms_set, the set field is a pointer to a
struct cil_classpermission which is looked up in the symbol table.
Since the cil_classperms_set does not create the cil_classpermission,
it should not reset it.

Set the set field to NULL instead of resetting the classpermission
that it points to.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_reset_ast.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/libsepol/cil/src/cil_reset_ast.c b/libsepol/cil/src/cil_reset_ast.c
index 89f91e56..1d9ca704 100644
--- a/libsepol/cil/src/cil_reset_ast.c
+++ b/libsepol/cil/src/cil_reset_ast.c
@@ -59,7 +59,11 @@ static void cil_reset_classpermission(struct cil_classpermission *cp)
 
 static void cil_reset_classperms_set(struct cil_classperms_set *cp_set)
 {
-	cil_reset_classpermission(cp_set->set);
+	if (cp_set == NULL) {
+		return;
+	}
+
+	cp_set->set = NULL;
 }
 
 static inline void cil_reset_classperms_list(struct cil_list *cp_list)
-- 
2.26.3

[PATCH 05/11] libsepol/cil: Set class field to NULL when resetting struct cil_classperms

[James Carter]

The class field of a struct cil_classperms points to the class looked
up in the symbol table, so that field should be set to NULL when
the cil_classperms is reset.

Set the class field to NULL when resetting the struct cil_classperms.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_reset_ast.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libsepol/cil/src/cil_reset_ast.c b/libsepol/cil/src/cil_reset_ast.c
index 1d9ca704..76405aba 100644
--- a/libsepol/cil/src/cil_reset_ast.c
+++ b/libsepol/cil/src/cil_reset_ast.c
@@ -45,6 +45,7 @@ static inline void cil_reset_classperms(struct cil_classperms *cp)
 		return;
 	}
 
+	cp->class = NULL;
 	cil_list_destroy(&cp->perms, CIL_FALSE);
 }
 
-- 
2.26.3

[PATCH 06/11] libsepol/cil: More strict verification of constraint leaf expressions

[James Carter]

In constraint expressions u1, u3, r1, r3, t1, and t3 are never
allowed on the right side of an expression, but there were no checks
to verify that they were not used on the right side. The result was
that the expression "(eq t1 t1)" would be silently turned into
"(eq t1 t2)" when the binary policy was created.

Verify that u1, u3, r1, r3, t1, and t3 are not used on the right
side of a constraint expression.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_verify.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c
index 09e3daf9..2707b6c9 100644
--- a/libsepol/cil/src/cil_verify.c
+++ b/libsepol/cil/src/cil_verify.c
@@ -227,7 +227,13 @@ int cil_verify_constraint_leaf_expr_syntax(enum cil_flavor l_flavor, enum cil_fl
 			}
 		}
 	} else {
-		if (r_flavor == CIL_CONS_U2) {
+		if (r_flavor == CIL_CONS_U1 || r_flavor == CIL_CONS_R1 || r_flavor == CIL_CONS_T1) {
+			cil_log(CIL_ERR, "u1, r1, and t1 are not allowed on the right side\n");
+			goto exit;
+		} else if (r_flavor == CIL_CONS_U3 || r_flavor == CIL_CONS_R3 || r_flavor == CIL_CONS_T3) {
+			cil_log(CIL_ERR, "u3, r3, and t3 are not allowed on the right side\n");
+			goto exit;
+		} else if (r_flavor == CIL_CONS_U2) {
 			if (op != CIL_EQ && op != CIL_NEQ) {
 				cil_log(CIL_ERR, "u2 on the right side must be used with eq or neq as the operator\n");
 				goto exit;
-- 
2.26.3

[PATCH 07/11 v2] libsepol/cil: Exit with an error if declaration name is a reserved word

[James Carter]

When CIL parses sets or conditional expressions, any identifier that
matches an operator name will always be taken as an operator. If a
declaration has the same name as an operator, then there is the
possibility of causing either confusion or a syntax error if it is
used in an expression. The potential for problems is much greater
than any possible advantage in allowing a declaration to share the
name of a reserved word.

Create a new function, __cil_is_reserved_name() that is called when
an identifier is declared and its name is being validated. In this
function, check if the declaration has the same name as a reserved
word for an expression operator that can be used with the identifer's
flavor and exit with an error if it does.

Also, move the check for types, type aliases, and type attributes
matching the reserved word "self" to this new function.

Finally, change the name of the function __cil_verify_name() to
cil_verify_name(), since this function is neither static nor a
helper function.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
v2:
 Remove the check for u1, u2, u3, r1, r2, r3, t1, t2, and t3
 Removed example because that really shows a different bug
 Reworded the explanation

 libsepol/cil/src/cil_build_ast.c | 28 ++---------------
 libsepol/cil/src/cil_verify.c    | 52 +++++++++++++++++++++++++++++++-
 libsepol/cil/src/cil_verify.h    |  2 +-
 3 files changed, 54 insertions(+), 28 deletions(-)

diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index 4e53f06a..e57de662 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -114,7 +114,7 @@ int cil_gen_node(struct cil_db *db, struct cil_tree_node *ast_node, struct cil_s
 	symtab_t *symtab = NULL;
 	struct cil_symtab_datum *prev;
 
-	rc = __cil_verify_name((const char*)key);
+	rc = cil_verify_name((const char*)key, nflavor);
 	if (rc != SEPOL_OK) {
 		goto exit;
 	}
@@ -1953,12 +1953,6 @@ int cil_gen_roleattribute(struct cil_db *db, struct cil_tree_node *parse_current
 		goto exit;
 	}
 
-	if (parse_current->next->data == CIL_KEY_SELF) {
-		cil_log(CIL_ERR, "The keyword '%s' is reserved\n", CIL_KEY_SELF);
-		rc = SEPOL_ERR;
-		goto exit;
-	}
-
 	cil_roleattribute_init(&attr);
 
 	key = parse_current->next->data;
@@ -2337,12 +2331,6 @@ int cil_gen_type(struct cil_db *db, struct cil_tree_node *parse_current, struct
 		goto exit;
 	}
 
-	if (parse_current->next->data == CIL_KEY_SELF) {
-		cil_log(CIL_ERR, "The keyword '%s' is reserved\n", CIL_KEY_SELF);
-		rc = SEPOL_ERR;
-		goto exit;
-	}
-
 	cil_type_init(&type);
 
 	key = parse_current->next->data;
@@ -2391,12 +2379,6 @@ int cil_gen_typeattribute(struct cil_db *db, struct cil_tree_node *parse_current
 		goto exit;
 	}
 
-	if (parse_current->next->data == CIL_KEY_SELF) {
-		cil_log(CIL_ERR, "The keyword '%s' is reserved\n", CIL_KEY_SELF);
-		rc = SEPOL_ERR;
-		goto exit;
-	}
-
 	cil_typeattribute_init(&attr);
 
 	key = parse_current->next->data;
@@ -3048,12 +3030,6 @@ int cil_gen_alias(struct cil_db *db, struct cil_tree_node *parse_current, struct
 		goto exit;
 	}
 
-	if (flavor == CIL_TYPEALIAS && parse_current->next->data == CIL_KEY_SELF) {
-		cil_log(CIL_ERR, "The keyword '%s' is reserved\n", CIL_KEY_SELF);
-		rc = SEPOL_ERR;
-		goto exit;
-	}
-
 	cil_alias_init(&alias);
 
 	key = parse_current->next->data;
@@ -5278,7 +5254,7 @@ int cil_gen_macro(struct cil_db *db, struct cil_tree_node *parse_current, struct
 
 		param->str =  current_item->cl_head->next->data;
 
-		rc = __cil_verify_name(param->str);
+		rc = cil_verify_name(param->str, param->flavor);
 		if (rc != SEPOL_OK) {
 			cil_destroy_param(param);
 			goto exit;
diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c
index 2707b6c9..6e46fef6 100644
--- a/libsepol/cil/src/cil_verify.c
+++ b/libsepol/cil/src/cil_verify.c
@@ -47,7 +47,51 @@
 
 #include "cil_verify.h"
 
-int __cil_verify_name(const char *name)
+static int __cil_is_reserved_name(const char *name, enum cil_flavor flavor)
+{
+	switch (flavor) {
+	case CIL_BOOL:
+	case CIL_TUNABLE:
+		if ((name == CIL_KEY_EQ) || (name == CIL_KEY_NEQ))
+			return CIL_TRUE;
+		break;
+	case CIL_PERM:
+	case CIL_MAP_PERM:
+	case CIL_USER:
+	case CIL_USERATTRIBUTE:
+	case CIL_ROLE:
+	case CIL_ROLEATTRIBUTE:
+		if ((name == CIL_KEY_ALL))
+			return CIL_TRUE;
+		break;
+	case CIL_TYPE:
+	case CIL_TYPEATTRIBUTE:
+	case CIL_TYPEALIAS:
+		if ((name == CIL_KEY_ALL) || (name == CIL_KEY_SELF))
+			return CIL_TRUE;
+		break;
+	case CIL_CAT:
+	case CIL_CATSET:
+	case CIL_CATALIAS:
+	case CIL_PERMISSIONX:
+		if ((name == CIL_KEY_ALL) || (name == CIL_KEY_RANGE))
+			return CIL_TRUE;
+		break;
+	default:
+		/* All of these are not used in expressions */
+		return CIL_FALSE;
+		break;
+	}
+
+	/* Everything not under the default case is also checked for these */
+	if ((name == CIL_KEY_AND) || (name == CIL_KEY_OR) || (name == CIL_KEY_NOT) || (name == CIL_KEY_XOR)) {
+		return CIL_TRUE;
+	}
+
+	return CIL_FALSE;
+}
+
+int cil_verify_name(const char *name, enum cil_flavor flavor)
 {
 	int rc = SEPOL_ERR;
 	int len;
@@ -77,6 +121,12 @@ int __cil_verify_name(const char *name)
 			goto exit;
 		}
 	}
+
+	if (__cil_is_reserved_name(name, flavor)) {
+		cil_log(CIL_ERR, "Name %s is a reserved word\n", name);
+		goto exit;
+	}
+
 	return SEPOL_OK;
 
 exit:
diff --git a/libsepol/cil/src/cil_verify.h b/libsepol/cil/src/cil_verify.h
index 905761b0..1887ae3f 100644
--- a/libsepol/cil/src/cil_verify.h
+++ b/libsepol/cil/src/cil_verify.h
@@ -56,7 +56,7 @@ struct cil_args_verify {
 	int *pass;
 };
 
-int __cil_verify_name(const char *name);
+int cil_verify_name(const char *name, enum cil_flavor flavor);
 int __cil_verify_syntax(struct cil_tree_node *parse_current, enum cil_syntax s[], int len);
 int cil_verify_expr_syntax(struct cil_tree_node *current, enum cil_flavor op, enum cil_flavor expr_flavor);
 int cil_verify_constraint_leaf_expr_syntax(enum cil_flavor l_flavor, enum cil_flavor r_flavor, enum cil_flavor op, enum cil_flavor expr_flavor);
-- 
2.26.3

[PATCH 08/11] libsepol/cil: Allow permission expressions when using map classes

[James Carter]

The following policy will cause a segfault:
  (class CLASS (PERM))
  (class C (P1 P2 P3))
  (classorder (CLASS C))
  (sid SID)
  (sidorder (SID))
  (user USER)
  (role ROLE)
  (type TYPE)
  (category CAT)
  (categoryorder (CAT))
  (sensitivity SENS)
  (sensitivityorder (SENS))
  (sensitivitycategory SENS (CAT))
  (allow TYPE self (CLASS (PERM)))
  (roletype ROLE TYPE)
  (userrole USER ROLE)
  (userlevel USER (SENS))
  (userrange USER ((SENS)(SENS (CAT))))
  (sidcontext SID (USER ROLE TYPE ((SENS)(SENS))))

  (classmap CM (PM1 PM2 PM3))
  (classmapping CM PM1 (C (P1)))
  (classmapping CM PM2 (C (P2)))
  (classmapping CM PM3 (C (P3)))
  (allow TYPE self (CM (and (all) (not PM2))))

The problem is that, while permission expressions are allowed for
normal classes, map classes are expected to only have permission
lists and no check is done to verify that only a permission list
is being used.

When the above policy is parsed, the "and" and "all" are seen as
expression operators, but when the map permissions are converted to
normal class and permissions, the permission expression is assumed
to be a list of datums and since the operators are not datums a
segfault is the result.

There is no reason to limit map classes to only using a list of
permissions and, in fact, it would be better to be able to use them
in the same way normal classes are used.

Allow permissions expressions to be used for map classes by first
evaluating the permission expression and then converting the
resulting list to normal classes and permissions.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_post.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/libsepol/cil/src/cil_post.c b/libsepol/cil/src/cil_post.c
index fd4758dc..05842b64 100644
--- a/libsepol/cil/src/cil_post.c
+++ b/libsepol/cil/src/cil_post.c
@@ -2137,6 +2137,10 @@ static int __evaluate_classperms_list(struct cil_list *classperms, struct cil_db
 				}
 			} else { /* MAP */
 				struct cil_list_item *i = NULL;
+				rc = __evaluate_classperms(cp, db);
+				if (rc != SEPOL_OK) {
+					goto exit;
+				}
 				cil_list_for_each(i, cp->perms) {
 					struct cil_perm *cmp = i->data;
 					rc = __evaluate_classperms_list(cmp->classperms, db);
-- 
2.26.3

[PATCH 09/11] libsepol/cil: Refactor helper function for cil_gen_node()

[James Carter]

Change the name of cil_is_datum_multiple_decl() to
cil_allow_multiple_decls() and make it static. The new function
takes the CIL db and the flavors of the old and new datum as
arguments. Also, put all of the logic of determining if multiple
declarations are allowed into the new function. Finally, update
the call from cil_gen_node().

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_build_ast.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index e57de662..14cdce14 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -82,30 +82,24 @@ exit:
 	return rc;
 }
 
-/*
- * Determine whether or not multiple declarations of the same key can share a
- * datum, given the new datum and the one already present in a given symtab.
- */
-int cil_is_datum_multiple_decl(__attribute__((unused)) struct cil_symtab_datum *cur,
-                               struct cil_symtab_datum *old,
-                               enum cil_flavor f)
+static int cil_allow_multiple_decls(struct cil_db *db, enum cil_flavor f_new, enum cil_flavor f_old)
 {
-	int rc = CIL_FALSE;
+	if (f_new != f_old) {
+		return CIL_FALSE;
+	}
 
-	switch (f) {
+	switch (f_new) {
 	case CIL_TYPE:
 	case CIL_TYPEATTRIBUTE:
-		if (!old || f != FLAVOR(old)) {
-			rc = CIL_FALSE;
-		} else {
-			/* type and typeattribute statements insert empty datums */
-			rc = CIL_TRUE;
+		if (db->multiple_decls) {
+			return CIL_TRUE;
 		}
 		break;
 	default:
 		break;
 	}
-	return rc;
+
+	return CIL_FALSE;
 }
 
 int cil_gen_node(struct cil_db *db, struct cil_tree_node *ast_node, struct cil_symtab_datum *datum, hashtab_key_t key, enum cil_sym_index sflavor, enum cil_flavor nflavor)
@@ -135,8 +129,7 @@ int cil_gen_node(struct cil_db *db, struct cil_tree_node *ast_node, struct cil_s
 				cil_log(CIL_ERR, "Re-declaration of %s %s, but previous declaration could not be found\n",cil_node_to_string(ast_node), key);
 				goto exit;
 			}
-			if (!db->multiple_decls ||
-			    !cil_is_datum_multiple_decl(datum, prev, nflavor)) {
+			if (!cil_allow_multiple_decls(db, nflavor, FLAVOR(prev))) {
 				/* multiple_decls not ok, ret error */
 				struct cil_tree_node *node = NODE(prev);
 				cil_log(CIL_ERR, "Re-declaration of %s %s\n",
-- 
2.26.3

[PATCH 10/11] libsepol/cil: Create function cil_add_decl_to_symtab() and refactor

[James Carter]

The functionality of adding a declaration to a symbol table is also
needed in __cil_copy_node_helper() and not just cil_gen_node().

Create a new function called cil_add_decl_to_symtab() to add a
declaration to a symtab and refactor cil_gen_node() and
__cil_copy_node_helper() to use the new function.

By using the new function, __cil_copy_node_helper() will now allow
duplicate declarations when appropriate.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_build_ast.c | 63 +++++++++++++++++++-------------
 libsepol/cil/src/cil_build_ast.h |  2 +
 libsepol/cil/src/cil_copy_ast.c  |  6 ++-
 3 files changed, 45 insertions(+), 26 deletions(-)

diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index 14cdce14..ec81db55 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -102,11 +102,45 @@ static int cil_allow_multiple_decls(struct cil_db *db, enum cil_flavor f_new, en
 	return CIL_FALSE;
 }
 
+int cil_add_decl_to_symtab(struct cil_db *db, symtab_t *symtab, hashtab_key_t key, struct cil_symtab_datum *datum, struct cil_tree_node *node)
+{
+	int rc;
+
+	if (symtab == NULL || datum == NULL || node == NULL) {
+		return SEPOL_ERR;
+	}
+
+	rc = cil_symtab_insert(symtab, key, datum, node);
+	if (rc == SEPOL_EEXIST) {
+		struct cil_symtab_datum *prev;
+		rc = cil_symtab_get_datum(symtab, key, &prev);
+		if (rc != SEPOL_OK) {
+			cil_log(CIL_ERR, "Re-declaration of %s %s, but previous declaration could not be found\n",cil_node_to_string(node), key);
+			return SEPOL_ERR;
+		}
+		if (!cil_allow_multiple_decls(db, node->flavor, FLAVOR(prev))) {
+			/* multiple_decls not ok, ret error */
+			struct cil_tree_node *n = NODE(prev);
+			cil_log(CIL_ERR, "Re-declaration of %s %s\n",
+				cil_node_to_string(node), key);
+			cil_tree_log(node, CIL_ERR, "Previous declaration of %s",
+				     cil_node_to_string(n));
+			return SEPOL_ERR;
+		}
+		/* multiple_decls is enabled and works for this datum type, add node */
+		cil_list_append(prev->nodes, CIL_NODE, node);
+		node->data = prev;
+		cil_symtab_datum_destroy(datum);
+		free(datum);
+	}
+
+	return SEPOL_OK;
+}
+
 int cil_gen_node(struct cil_db *db, struct cil_tree_node *ast_node, struct cil_symtab_datum *datum, hashtab_key_t key, enum cil_sym_index sflavor, enum cil_flavor nflavor)
 {
 	int rc = SEPOL_ERR;
 	symtab_t *symtab = NULL;
-	struct cil_symtab_datum *prev;
 
 	rc = cil_verify_name((const char*)key, nflavor);
 	if (rc != SEPOL_OK) {
@@ -121,30 +155,9 @@ int cil_gen_node(struct cil_db *db, struct cil_tree_node *ast_node, struct cil_s
 	ast_node->data = datum;
 	ast_node->flavor = nflavor;
 
-	if (symtab != NULL) {
-		rc = cil_symtab_insert(symtab, (hashtab_key_t)key, datum, ast_node);
-		if (rc == SEPOL_EEXIST) {
-			rc = cil_symtab_get_datum(symtab, (hashtab_key_t)key, &prev);
-			if (rc != SEPOL_OK) {
-				cil_log(CIL_ERR, "Re-declaration of %s %s, but previous declaration could not be found\n",cil_node_to_string(ast_node), key);
-				goto exit;
-			}
-			if (!cil_allow_multiple_decls(db, nflavor, FLAVOR(prev))) {
-				/* multiple_decls not ok, ret error */
-				struct cil_tree_node *node = NODE(prev);
-				cil_log(CIL_ERR, "Re-declaration of %s %s\n",
-					cil_node_to_string(ast_node), key);
-				cil_tree_log(node, CIL_ERR, "Previous declaration of %s",
-					cil_node_to_string(node));
-				rc = SEPOL_ERR;
-				goto exit;
-			}
-			/* multiple_decls is enabled and works for this datum type, add node */
-			cil_list_append(prev->nodes, CIL_NODE, ast_node);
-			ast_node->data = prev;
-			cil_symtab_datum_destroy(datum);
-			free(datum);
-		}
+	rc = cil_add_decl_to_symtab(db, symtab, key, datum, ast_node);
+	if (rc != SEPOL_OK) {
+		goto exit;
 	}
 
 	if (ast_node->parent->flavor == CIL_MACRO) {
diff --git a/libsepol/cil/src/cil_build_ast.h b/libsepol/cil/src/cil_build_ast.h
index 8153e51e..fd9053ce 100644
--- a/libsepol/cil/src/cil_build_ast.h
+++ b/libsepol/cil/src/cil_build_ast.h
@@ -37,6 +37,8 @@
 #include "cil_tree.h"
 #include "cil_list.h"
 
+int cil_add_decl_to_symtab(struct cil_db *db, symtab_t *symtab, hashtab_key_t key, struct cil_symtab_datum *datum, struct cil_tree_node *node);
+
 int cil_gen_node(struct cil_db *db, struct cil_tree_node *ast_node, struct cil_symtab_datum *datum, hashtab_key_t key, enum cil_sym_index sflavor, enum cil_flavor nflavor);
 int cil_parse_to_list(struct cil_tree_node *parse_cl_head, struct cil_list *ast_cl, enum cil_flavor flavor);
 
diff --git a/libsepol/cil/src/cil_copy_ast.c b/libsepol/cil/src/cil_copy_ast.c
index ed967861..12bc553c 100644
--- a/libsepol/cil/src/cil_copy_ast.c
+++ b/libsepol/cil/src/cil_copy_ast.c
@@ -2031,7 +2031,11 @@ int __cil_copy_node_helper(struct cil_tree_node *orig, __attribute__((unused)) u
 				rc = SEPOL_ERR;
 				goto exit;
 			}
-			rc = cil_symtab_insert(symtab, ((struct cil_symtab_datum*)orig->data)->name, ((struct cil_symtab_datum*)data), new);
+
+			rc = cil_add_decl_to_symtab(db, symtab, DATUM(orig->data)->name, DATUM(data), new);
+			if (rc != SEPOL_OK) {
+				goto exit;
+			}
 
 			namespace = new;
 			while (namespace->flavor != CIL_MACRO && namespace->flavor != CIL_BLOCK && namespace->flavor != CIL_ROOT) {
-- 
2.26.3

[PATCH 11/11] libsepol/cil: Move check for the shadowing of macro parameters

[James Carter]

In cil_gen_node(), after the declaration is added to the symbol
table, if the parent is a macro, then a check is made to ensure
the declaration does not shadow any of the macro's parameters.
This check also needs to be done when copying the AST.

Move the check for the shadowing of macro parameters to its own
function, cil_verify_decl_does_not_shadow_macro_parameter(), and
refactor cil_gen_node() and __cil_copy_node_helper() to use the
new function.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_build_ast.c | 16 +++-------------
 libsepol/cil/src/cil_copy_ast.c  | 20 ++++----------------
 libsepol/cil/src/cil_verify.c    | 18 ++++++++++++++++++
 libsepol/cil/src/cil_verify.h    |  1 +
 4 files changed, 26 insertions(+), 29 deletions(-)

diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index ec81db55..a4a2baa0 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -161,19 +161,9 @@ int cil_gen_node(struct cil_db *db, struct cil_tree_node *ast_node, struct cil_s
 	}
 
 	if (ast_node->parent->flavor == CIL_MACRO) {
-		struct cil_list_item *item;
-		struct cil_list *param_list = ((struct cil_macro*)ast_node->parent->data)->params;
-		if (param_list != NULL) {
-			cil_list_for_each(item, param_list) {
-				struct cil_param *param = item->data;
-				if (param->flavor == ast_node->flavor) {
-					if (param->str == key) {
-						cil_log(CIL_ERR, "%s %s shadows a macro parameter in macro declaration\n", cil_node_to_string(ast_node), key);
-						rc = SEPOL_ERR;
-						goto exit;
-					}
-				}
-			}
+		rc = cil_verify_decl_does_not_shadow_macro_parameter(ast_node->parent->data, ast_node, key);
+		if (rc != SEPOL_OK) {
+			goto exit;
 		}
 	}
 
diff --git a/libsepol/cil/src/cil_copy_ast.c b/libsepol/cil/src/cil_copy_ast.c
index 12bc553c..954eab33 100644
--- a/libsepol/cil/src/cil_copy_ast.c
+++ b/libsepol/cil/src/cil_copy_ast.c
@@ -40,6 +40,7 @@
 #include "cil_copy_ast.h"
 #include "cil_build_ast.h"
 #include "cil_strpool.h"
+#include "cil_verify.h"
 
 struct cil_args_copy {
 	struct cil_tree_node *dest;
@@ -1716,7 +1717,6 @@ int __cil_copy_node_helper(struct cil_tree_node *orig, __attribute__((unused)) u
 	struct cil_db *db = NULL;
 	struct cil_args_copy *args = NULL;
 	struct cil_tree_node *namespace = NULL;
-	struct cil_param *param = NULL;
 	enum cil_sym_index sym_index = CIL_SYM_UNKNOWN;
 	symtab_t *symtab = NULL;
 	void *data = NULL;
@@ -2043,21 +2043,9 @@ int __cil_copy_node_helper(struct cil_tree_node *orig, __attribute__((unused)) u
 			}
 
 			if (namespace->flavor == CIL_MACRO) {
-				struct cil_macro *macro = namespace->data;
-				struct cil_list *param_list = macro->params;
-				if (param_list != NULL) {
-					struct cil_list_item *item;
-					cil_list_for_each(item, param_list) {
-						param = item->data;
-						if (param->flavor == new->flavor) {
-							if (param->str == ((struct cil_symtab_datum*)new->data)->name) {
-								cil_tree_log(orig, CIL_ERR, "%s %s shadows a macro parameter", cil_node_to_string(new), ((struct cil_symtab_datum*)orig->data)->name);
-								cil_tree_log(namespace, CIL_ERR, "Note: macro declaration");
-								rc = SEPOL_ERR;
-								goto exit;
-							}
-						}
-					}
+				rc = cil_verify_decl_does_not_shadow_macro_parameter(namespace->data, orig, DATUM(orig->data)->name);
+				if (rc != SEPOL_OK) {
+					goto exit;
 				}
 			}
 		}
diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c
index 6e46fef6..5517461c 100644
--- a/libsepol/cil/src/cil_verify.c
+++ b/libsepol/cil/src/cil_verify.c
@@ -412,6 +412,24 @@ int cil_verify_conditional_blocks(struct cil_tree_node *current)
 	return SEPOL_OK;
 }
 
+int cil_verify_decl_does_not_shadow_macro_parameter(struct cil_macro *macro, struct cil_tree_node *node, const char *name)
+{
+	struct cil_list_item *item;
+	struct cil_list *param_list = macro->params;
+	if (param_list != NULL) {
+		cil_list_for_each(item, param_list) {
+			struct cil_param *param = item->data;
+			if (param->flavor == node->flavor) {
+				if (param->str == name) {
+					cil_log(CIL_ERR, "%s %s shadows a macro parameter in macro declaration\n", cil_node_to_string(node), name);
+					return SEPOL_ERR;
+				}
+			}
+		}
+	}
+	return SEPOL_OK;
+}
+
 int cil_verify_no_self_reference(struct cil_symtab_datum *datum, struct cil_list *datum_list)
 {
 	struct cil_list_item *i;
diff --git a/libsepol/cil/src/cil_verify.h b/libsepol/cil/src/cil_verify.h
index 1887ae3f..c497018f 100644
--- a/libsepol/cil/src/cil_verify.h
+++ b/libsepol/cil/src/cil_verify.h
@@ -62,6 +62,7 @@ int cil_verify_expr_syntax(struct cil_tree_node *current, enum cil_flavor op, en
 int cil_verify_constraint_leaf_expr_syntax(enum cil_flavor l_flavor, enum cil_flavor r_flavor, enum cil_flavor op, enum cil_flavor expr_flavor);
 int cil_verify_constraint_expr_syntax(struct cil_tree_node *current, enum cil_flavor op);
 int cil_verify_conditional_blocks(struct cil_tree_node *current);
+int cil_verify_decl_does_not_shadow_macro_parameter(struct cil_macro *macro, struct cil_tree_node *node, const char *name);
 int cil_verify_no_self_reference(struct cil_symtab_datum *datum, struct cil_list *datum_list);
 int __cil_verify_ranges(struct cil_list *list);
 int __cil_verify_ordered_node_helper(struct cil_tree_node *node, uint32_t *finished, void *extra_args);
-- 
2.26.3

Re: [PATCH 00/11] Various CIL patches

[James Carter]

On Thu, Apr 8, 2021 at 3:16 PM James Carter <jwcart2@gmail.com> wrote:
>
> All of these patches (except the first) have been sent to the list in the
> last month. The first patch is a slight modification of an earlier patch
> sent by Nicolas Iooss. I also corrected a couple of whitespace errors and
> updated one patch that would not apply cleanly with one of the earlier
> patches applied.
>
> James Carter (11):
>   libsepol/cil: Fix out-of-bound read of file context pattern ending
>     with "\"
>   libsepol/cil: Destroy classperms list when resetting classpermission
>   libsepol/cil: Destroy classperm list when resetting map perms
>   libsepol/cil: cil_reset_classperms_set() should not reset
>     classpermission
>   libsepol/cil: Set class field to NULL when resetting struct
>     cil_classperms
>   libsepol/cil: More strict verification of constraint leaf expressions
>   libsepol/cil: Exit with an error if declaration name is a reserved
>     word
>   libsepol/cil: Allow permission expressions when using map classes
>   libsepol/cil: Refactor helper function for cil_gen_node()
>   libsepol/cil: Create function cil_add_decl_to_symtab() and refactor
>   libsepol/cil: Move check for the shadowing of macro parameters
>
>  libsepol/cil/src/cil_build_ast.c | 132 ++++++++++++-------------------
>  libsepol/cil/src/cil_build_ast.h |   2 +
>  libsepol/cil/src/cil_copy_ast.c  |  26 +++---
>  libsepol/cil/src/cil_post.c      |  11 +++
>  libsepol/cil/src/cil_reset_ast.c |  11 ++-
>  libsepol/cil/src/cil_verify.c    |  78 +++++++++++++++++-
>  libsepol/cil/src/cil_verify.h    |   3 +-
>  7 files changed, 160 insertions(+), 103 deletions(-)
>
> --
> 2.26.3
>

Most of these patches were sent out almost a month ago. I am planning
on merging them next week.
Jim

Re: [PATCH 00/11] Various CIL patches

[Petr Lautrbach]

James Carter <jwcart2@gmail.com> writes:

> On Thu, Apr 8, 2021 at 3:16 PM James Carter <jwcart2@gmail.com> wrote:
>>
>> All of these patches (except the first) have been sent to the list in the
>> last month. The first patch is a slight modification of an earlier patch
>> sent by Nicolas Iooss. I also corrected a couple of whitespace errors and
>> updated one patch that would not apply cleanly with one of the earlier
>> patches applied.
>>
>> James Carter (11):
>>   libsepol/cil: Fix out-of-bound read of file context pattern ending
>>     with "\"
>>   libsepol/cil: Destroy classperms list when resetting classpermission
>>   libsepol/cil: Destroy classperm list when resetting map perms
>>   libsepol/cil: cil_reset_classperms_set() should not reset
>>     classpermission
>>   libsepol/cil: Set class field to NULL when resetting struct
>>     cil_classperms
>>   libsepol/cil: More strict verification of constraint leaf expressions
>>   libsepol/cil: Exit with an error if declaration name is a reserved
>>     word
>>   libsepol/cil: Allow permission expressions when using map classes
>>   libsepol/cil: Refactor helper function for cil_gen_node()
>>   libsepol/cil: Create function cil_add_decl_to_symtab() and refactor
>>   libsepol/cil: Move check for the shadowing of macro parameters
>>
>>  libsepol/cil/src/cil_build_ast.c | 132 ++++++++++++-------------------
>>  libsepol/cil/src/cil_build_ast.h |   2 +
>>  libsepol/cil/src/cil_copy_ast.c  |  26 +++---
>>  libsepol/cil/src/cil_post.c      |  11 +++
>>  libsepol/cil/src/cil_reset_ast.c |  11 ++-
>>  libsepol/cil/src/cil_verify.c    |  78 +++++++++++++++++-
>>  libsepol/cil/src/cil_verify.h    |   3 +-
>>  7 files changed, 160 insertions(+), 103 deletions(-)
>>
>> --
>> 2.26.3
>>
>
> Most of these patches were sent out almost a month ago. I am planning
> on merging them next week.
> Jim

I didn't check the content of the changes, but I ran a regression test
suite we use in Red Hat on this and "Update checks for invalid rules in
blocks" patchsets and it didn't discover any problem (just a minor
trailing whitespace I reported last week)

Acked-by: Petr Lautrbach <plautrba@redhat.com>

I believe you can merge it/

Thanks!


Petr

Re: [PATCH 00/11] Various CIL patches

[James Carter]

On Mon, Apr 19, 2021 at 12:20 PM Petr Lautrbach <plautrba@redhat.com> wrote:
>
> James Carter <jwcart2@gmail.com> writes:
>
> > On Thu, Apr 8, 2021 at 3:16 PM James Carter <jwcart2@gmail.com> wrote:
> >>
> >> All of these patches (except the first) have been sent to the list in the
> >> last month. The first patch is a slight modification of an earlier patch
> >> sent by Nicolas Iooss. I also corrected a couple of whitespace errors and
> >> updated one patch that would not apply cleanly with one of the earlier
> >> patches applied.
> >>
> >> James Carter (11):
> >>   libsepol/cil: Fix out-of-bound read of file context pattern ending
> >>     with "\"
> >>   libsepol/cil: Destroy classperms list when resetting classpermission
> >>   libsepol/cil: Destroy classperm list when resetting map perms
> >>   libsepol/cil: cil_reset_classperms_set() should not reset
> >>     classpermission
> >>   libsepol/cil: Set class field to NULL when resetting struct
> >>     cil_classperms
> >>   libsepol/cil: More strict verification of constraint leaf expressions
> >>   libsepol/cil: Exit with an error if declaration name is a reserved
> >>     word
> >>   libsepol/cil: Allow permission expressions when using map classes
> >>   libsepol/cil: Refactor helper function for cil_gen_node()
> >>   libsepol/cil: Create function cil_add_decl_to_symtab() and refactor
> >>   libsepol/cil: Move check for the shadowing of macro parameters
> >>
> >>  libsepol/cil/src/cil_build_ast.c | 132 ++++++++++++-------------------
> >>  libsepol/cil/src/cil_build_ast.h |   2 +
> >>  libsepol/cil/src/cil_copy_ast.c  |  26 +++---
> >>  libsepol/cil/src/cil_post.c      |  11 +++
> >>  libsepol/cil/src/cil_reset_ast.c |  11 ++-
> >>  libsepol/cil/src/cil_verify.c    |  78 +++++++++++++++++-
> >>  libsepol/cil/src/cil_verify.h    |   3 +-
> >>  7 files changed, 160 insertions(+), 103 deletions(-)
> >>
> >> --
> >> 2.26.3
> >>
> >
> > Most of these patches were sent out almost a month ago. I am planning
> > on merging them next week.
> > Jim
>
> I didn't check the content of the changes, but I ran a regression test
> suite we use in Red Hat on this and "Update checks for invalid rules in
> blocks" patchsets and it didn't discover any problem (just a minor
> trailing whitespace I reported last week)
>
> Acked-by: Petr Lautrbach <plautrba@redhat.com>
>
> I believe you can merge it/
>
> Thanks!
>
>
> Petr
>

I have merged both of these.
Thanks,
Jim