[PATCH 1/4] libsepol/cil: Provide option to allow qualified names in declarations

[PATCH 1/4] libsepol/cil: Provide option to allow qualified names in declarations

[James Carter]

Qualified names have "dots" in them. They are generated when a CIL
policy is compiled and come from declarations in blocks. If a kernel
policy is decompiled into a CIL policy, the resulting policy could
have decarations that use qualified names. Compiling this policy would
result in an error because "dots" in declarations are not allowed.

Qualified names in a policy are normally used to refer to the name of
identifiers, blocks, macros, or optionals that are declared in a
different block (that is not a parent). Name resolution is based on
splitting a name based on the "dots", searching the parents up to the
global namespace for the first block using the first part of the name,
using the second part of the name to lookup the next block using the
first block's symbol tables, looking up the third block in the second's
symbol tables, and so on.

To allow the option of using qualified names in declarations:

1) Create a field in the struct cil_db called "qualified_names" which
is set to CIL_TRUE when qualified names are to be used. This field is
checked in cil_verify_name() and "dots" are allowed if qualified names
are being allowed.

2) Only allow the direct lookup of the whole name in the global symbol
table. This means that blocks, blockinherits, blockabstracts, and in-
statements cannot be allowed. Use the "qualified_names" field of the
cil_db to know when using one of these should result in an error.

3) Create the function cil_set_qualified_names() that is used to set
the "qualified_names" field. Export the function in libsepol.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/include/cil/cil.h     |  1 +
 libsepol/cil/src/cil.c             |  6 ++++++
 libsepol/cil/src/cil_build_ast.c   | 24 ++++++++++++++++++++++--
 libsepol/cil/src/cil_internal.h    |  1 +
 libsepol/cil/src/cil_resolve_ast.c |  4 ++--
 libsepol/cil/src/cil_verify.c      | 19 ++++++++++++++-----
 libsepol/cil/src/cil_verify.h      |  2 +-
 libsepol/src/libsepol.map.in       |  1 +
 8 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/libsepol/cil/include/cil/cil.h b/libsepol/cil/include/cil/cil.h
index 92fac6e1..482ca522 100644
--- a/libsepol/cil/include/cil/cil.h
+++ b/libsepol/cil/include/cil/cil.h
@@ -51,6 +51,7 @@ extern int cil_selinuxusers_to_string(cil_db_t *db, char **out, size_t *size);
 extern int cil_filecons_to_string(cil_db_t *db, char **out, size_t *size);
 extern void cil_set_disable_dontaudit(cil_db_t *db, int disable_dontaudit);
 extern void cil_set_multiple_decls(cil_db_t *db, int multiple_decls);
+extern void cil_set_qualified_names(struct cil_db *db, int qualified_names);
 extern void cil_set_disable_neverallow(cil_db_t *db, int disable_neverallow);
 extern void cil_set_preserve_tunables(cil_db_t *db, int preserve_tunables);
 extern int cil_set_handle_unknown(cil_db_t *db, int handle_unknown);
diff --git a/libsepol/cil/src/cil.c b/libsepol/cil/src/cil.c
index 9d5038d9..3f2e6927 100644
--- a/libsepol/cil/src/cil.c
+++ b/libsepol/cil/src/cil.c
@@ -440,6 +440,7 @@ void cil_db_init(struct cil_db **db)
 	(*db)->handle_unknown = -1;
 	(*db)->mls = -1;
 	(*db)->multiple_decls = CIL_FALSE;
+	(*db)->qualified_names = CIL_FALSE;
 	(*db)->target_platform = SEPOL_TARGET_SELINUX;
 	(*db)->policy_version = POLICYDB_VERSION_MAX;
 }
@@ -1872,6 +1873,11 @@ void cil_set_multiple_decls(struct cil_db *db, int multiple_decls)
 	db->multiple_decls = multiple_decls;
 }
 
+void cil_set_qualified_names(struct cil_db *db, int qualified_names)
+{
+	db->qualified_names = qualified_names;
+}
+
 void cil_set_target_platform(struct cil_db *db, int target_platform)
 {
 	db->target_platform = target_platform;
diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index baed3e58..9da90883 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -146,7 +146,7 @@ int cil_gen_node(struct cil_db *db, struct cil_tree_node *ast_node, struct cil_s
 	int rc = SEPOL_ERR;
 	symtab_t *symtab = NULL;
 
-	rc = cil_verify_name((const char*)key, nflavor);
+	rc = cil_verify_name(db, (const char*)key, nflavor);
 	if (rc != SEPOL_OK) {
 		goto exit;
 	}
@@ -204,6 +204,11 @@ int cil_gen_block(struct cil_db *db, struct cil_tree_node *parse_current, struct
 		goto exit;
 	}
 
+	if (db->qualified_names) {
+		cil_log(CIL_ERR, "Blocks are not allowed when the option for qualified names is used\n");
+		goto exit;
+	}
+
 	rc = __cil_verify_syntax(parse_current, syntax, syntax_len);
 	if (rc != SEPOL_OK) {
 		goto exit;
@@ -274,6 +279,11 @@ int cil_gen_blockinherit(struct cil_db *db, struct cil_tree_node *parse_current,
 		goto exit;
 	}
 
+	if (db->qualified_names) {
+		cil_log(CIL_ERR, "Block inherit rules are not allowed when the option for qualified names is used\n");
+		goto exit;
+	}
+
 	rc = __cil_verify_syntax(parse_current, syntax, syntax_len);
 	if (rc != SEPOL_OK) {
 		goto exit;
@@ -331,6 +341,11 @@ int cil_gen_blockabstract(struct cil_db *db, struct cil_tree_node *parse_current
 		goto exit;
 	}
 
+	if (db->qualified_names) {
+		cil_log(CIL_ERR, "Block abstract rules are not allowed when the option for qualified names is used\n");
+		goto exit;
+	}
+
 	rc = __cil_verify_syntax(parse_current, syntax, syntax_len);
 	if (rc != SEPOL_OK) {
 		goto exit;
@@ -376,6 +391,11 @@ int cil_gen_in(struct cil_db *db, struct cil_tree_node *parse_current, struct ci
 		goto exit;
 	}
 
+	if (db->qualified_names) {
+		cil_log(CIL_ERR, "In-statements are not allowed when the option for qualified names is used\n");
+		goto exit;
+	}
+
 	rc = __cil_verify_syntax(parse_current, syntax, syntax_len);
 	if (rc != SEPOL_OK) {
 		goto exit;
@@ -5261,7 +5281,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, param->flavor);
+		rc = cil_verify_name(db, param->str, param->flavor);
 		if (rc != SEPOL_OK) {
 			cil_destroy_param(param);
 			goto exit;
diff --git a/libsepol/cil/src/cil_internal.h b/libsepol/cil/src/cil_internal.h
index 8b9aeabf..f184d739 100644
--- a/libsepol/cil/src/cil_internal.h
+++ b/libsepol/cil/src/cil_internal.h
@@ -321,6 +321,7 @@ struct cil_db {
 	int handle_unknown;
 	int mls;
 	int multiple_decls;
+	int qualified_names;
 	int target_platform;
 	int policy_version;
 };
diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index 5245cc15..27efffa6 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -4409,8 +4409,8 @@ int cil_resolve_name_keep_aliases(struct cil_tree_node *ast_node, char *name, en
 
 	*datum = NULL;
 
-	if (strchr(name,'.') == NULL) {
-		/* No '.' in name */
+	if (db->qualified_names || strchr(name,'.') == NULL) {
+		/* Using qualified names or No '.' in name */
 		rc = __cil_resolve_name_helper(db, ast_node->parent, name, sym_index, datum);
 		if (rc != SEPOL_OK) {
 			goto exit;
diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c
index 59397f70..9cb1a6f6 100644
--- a/libsepol/cil/src/cil_verify.c
+++ b/libsepol/cil/src/cil_verify.c
@@ -92,7 +92,7 @@ static int __cil_is_reserved_name(const char *name, enum cil_flavor flavor)
 	return CIL_FALSE;
 }
 
-int cil_verify_name(const char *name, enum cil_flavor flavor)
+int cil_verify_name(struct cil_db *db, const char *name, enum cil_flavor flavor)
 {
 	int rc = SEPOL_ERR;
 	int len;
@@ -116,10 +116,19 @@ int cil_verify_name(const char *name, enum cil_flavor flavor)
 			goto exit;
 	}
 
-	for (i = 1; i < len; i++) {
-		if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-') {
-			cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
-			goto exit;
+	if (db->qualified_names == CIL_FALSE) {
+		for (i = 1; i < len; i++) {
+			if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-') {
+				cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
+				goto exit;
+			}
+		}
+	} else {
+		for (i = 1; i < len; i++) {
+			if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-' && name[i] != '.') {
+				cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
+				goto exit;
+			}
 		}
 	}
 
diff --git a/libsepol/cil/src/cil_verify.h b/libsepol/cil/src/cil_verify.h
index 4ea14f5b..8eb3c463 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, enum cil_flavor flavor);
+int cil_verify_name(struct cil_db *db, 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);
diff --git a/libsepol/src/libsepol.map.in b/libsepol/src/libsepol.map.in
index 2e503bd1..0e05d606 100644
--- a/libsepol/src/libsepol.map.in
+++ b/libsepol/src/libsepol.map.in
@@ -272,4 +272,5 @@ LIBSEPOL_3.0 {
 	cil_write_parse_ast;
 	cil_write_build_ast;
 	cil_write_resolve_ast;
+	cil_set_qualified_names;
 } LIBSEPOL_1.1;
-- 
2.26.3

[PATCH 2/4] secilc: Add support for using qualified names to secilc

[James Carter]

Provide the option "-Q" or "--qualified-names" to indicate that the
policy is using qualified names.

Using qualified names means that declaration names can have "dots"
in them, but blocks, blockinherits, blockabstracts, and in-statements
are not allowed in the policy.

The libsepol function cil_set_qualified_names() is called with the
desired value for the CIL db's "qualified_names" field.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 secilc/secilc.8.xml | 5 +++++
 secilc/secilc.c     | 9 ++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/secilc/secilc.8.xml b/secilc/secilc.8.xml
index 2b734f09..74b78f2b 100644
--- a/secilc/secilc.8.xml
+++ b/secilc/secilc.8.xml
@@ -75,6 +75,11 @@
             <listitem><para>Treat tunables as booleans.</para></listitem>
          </varlistentry>
 
+         <varlistentry>
+            <term><option>-Q, --qualified-names</option></term>
+            <listitem><para>Use qualified names. Do not allow blocks, blockinherits, blockabstracts, or in-statements.</para></listitem>
+         </varlistentry>
+
          <varlistentry>
             <term><option>-m, --multiple-decls</option></term>
             <listitem><para>Allow some statements to be re-declared.</para></listitem>
diff --git a/secilc/secilc.c b/secilc/secilc.c
index 9c78e425..91b619b9 100644
--- a/secilc/secilc.c
+++ b/secilc/secilc.c
@@ -63,6 +63,7 @@ static __attribute__((__noreturn__)) void usage(const char *prog)
 	printf("                                 statement if present in the policy\n");
 	printf("  -D, --disable-dontaudit        do not add dontaudit rules to the binary policy\n");
 	printf("  -P, --preserve-tunables        treat tunables as booleans\n");
+	printf("  -Q, --qualified-names          Use qualified names and do not allow blocks\n");
 	printf("  -m, --multiple-decls           allow some statements to be re-declared\n");
 	printf("  -N, --disable-neverallow       do not check neverallow rules\n");
 	printf("  -G, --expand-generated         Expand and remove auto-generated attributes\n");
@@ -94,6 +95,7 @@ int main(int argc, char *argv[])
 	int multiple_decls = 0;
 	int disable_neverallow = 0;
 	int preserve_tunables = 0;
+	int qualified_names = 0;
 	int handle_unknown = -1;
 	int policyvers = POLICYDB_VERSION_MAX;
 	int attrs_expand_generated = 0;
@@ -115,6 +117,7 @@ int main(int argc, char *argv[])
 		{"multiple-decls", no_argument, 0, 'm'},
 		{"disable-neverallow", no_argument, 0, 'N'},
 		{"preserve-tunables", no_argument, 0, 'P'},
+		{"qualified-names", no_argument, 0, 'Q'},
 		{"output", required_argument, 0, 'o'},
 		{"filecontexts", required_argument, 0, 'f'},
 		{"expand-generated", no_argument, 0, 'G'},
@@ -125,7 +128,7 @@ int main(int argc, char *argv[])
 	int i;
 
 	while (1) {
-		opt_char = getopt_long(argc, argv, "o:f:U:hvt:M:PDmNOc:GX:n", long_opts, &opt_index);
+		opt_char = getopt_long(argc, argv, "o:f:U:hvt:M:PQDmNOc:GX:n", long_opts, &opt_index);
 		if (opt_char == -1) {
 			break;
 		}
@@ -190,6 +193,9 @@ int main(int argc, char *argv[])
 			case 'P':
 				preserve_tunables = 1;
 				break;
+			case 'Q':
+				qualified_names = 1;
+				break;
 			case 'o':
 				output = strdup(optarg);
 				break;
@@ -238,6 +244,7 @@ int main(int argc, char *argv[])
 	cil_set_multiple_decls(db, multiple_decls);
 	cil_set_disable_neverallow(db, disable_neverallow);
 	cil_set_preserve_tunables(db, preserve_tunables);
+	cil_set_qualified_names(db, qualified_names);
 	if (handle_unknown != -1) {
 		rc = cil_set_handle_unknown(db, handle_unknown);
 		if (rc != SEPOL_OK) {
-- 
2.26.3

[PATCH 3/4] libsepol/cil: Add support for using qualified names to secil2tree

[James Carter]

Provide the option "-Q" or "--qualified-names" to indicate that the
policy is using qualified names.

Using qualified names means that declaration names can have "dots"
in them, but blocks, blockinherits, blockabstracts, and in-statements
are not allowed in the policy.

The libsepol function cil_set_qualified_names() is called with the
desired value for the CIL db's "qualified_names" field.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 secilc/secil2tree.8.xml | 5 +++++
 secilc/secil2tree.c     | 9 ++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/secilc/secil2tree.8.xml b/secilc/secil2tree.8.xml
index 81382ffe..6a969ac9 100644
--- a/secilc/secil2tree.8.xml
+++ b/secilc/secil2tree.8.xml
@@ -45,6 +45,11 @@
             <listitem><para>Treat tunables as booleans.</para></listitem>
          </varlistentry>
 
+         <varlistentry>
+            <term><option>-Q, --qualified-names</option></term>
+            <listitem><para>Use qualified names. Blocks, blockinherits, blockabstracts, and in-statements will not be allowed.</para></listitem>
+         </varlistentry>
+
          <varlistentry>
             <term><option>-A, --ast-phase=&lt;phase></option></term>
             <listitem><para>Write AST of phase <emphasis role="italic">phase</emphasis>. Must be <emphasis role="bold">parse</emphasis>, <emphasis role="bold">build</emphasis>, or <emphasis role="bold">resolve</emphasis>. (default: <emphasis role="bold">resolve</emphasis>)</para></listitem>
diff --git a/secilc/secil2tree.c b/secilc/secil2tree.c
index 218d0583..548a5c63 100644
--- a/secilc/secil2tree.c
+++ b/secilc/secil2tree.c
@@ -54,6 +54,7 @@ static __attribute__((__noreturn__)) void usage(const char *prog)
 	printf("Options:\n");
 	printf("  -o, --output=<file>      write AST to <file>. (default: stdout)\n");
 	printf("  -P, --preserve-tunables  treat tunables as booleans\n");
+	printf("  -Q, --qualified-names    Use qualified names and do not allow blocks\n");
 	printf("  -A, --ast-phase=<phase>  write AST of phase <phase>. Phase must be parse, \n");
 	printf("                           build, or resolve. (default: resolve)\n");
 	printf("  -v, --verbose            increment verbosity level\n");
@@ -71,6 +72,7 @@ int main(int argc, char *argv[])
 	char *output = NULL;
 	struct cil_db *db = NULL;
 	int preserve_tunables = 0;
+	int qualified_names = 0;
 	enum write_ast_phase write_ast = WRITE_AST_PHASE_RESOLVE;
 	int opt_char;
 	int opt_index = 0;
@@ -79,6 +81,7 @@ int main(int argc, char *argv[])
 		{"help", no_argument, 0, 'h'},
 		{"verbose", no_argument, 0, 'v'},
 		{"preserve-tunables", no_argument, 0, 'P'},
+		{"qualified-names", no_argument, 0, 'Q'},
 		{"output", required_argument, 0, 'o'},
 		{"ast-phase", required_argument, 0, 'A'},
 		{0, 0, 0, 0}
@@ -86,7 +89,7 @@ int main(int argc, char *argv[])
 	int i;
 
 	while (1) {
-		opt_char = getopt_long(argc, argv, "o:hvPA:", long_opts, &opt_index);
+		opt_char = getopt_long(argc, argv, "o:hvPQA:", long_opts, &opt_index);
 		if (opt_char == -1) {
 			break;
 		}
@@ -97,6 +100,9 @@ int main(int argc, char *argv[])
 			case 'P':
 				preserve_tunables = 1;
 				break;
+			case 'Q':
+				qualified_names = 1;
+				break;
 			case 'o':
 				output = strdup(optarg);
 				break;
@@ -131,6 +137,7 @@ int main(int argc, char *argv[])
 
 	cil_db_init(&db);
 	cil_set_preserve_tunables(db, preserve_tunables);
+	cil_set_qualified_names(db, qualified_names);
 	cil_set_attrs_expand_generated(db, 0);
 	cil_set_attrs_expand_size(db, 0);
 
-- 
2.26.3

[PATCH 4/4] libsepol/cil: Add support for using qualified names to secil2conf

[James Carter]

Provide the option "-Q" or "--qualified-names" to indicate that the
policy is using qualified names.

Using qualified names means that declaration names can have "dots"
in them, but blocks, blockinherits, blockabstracts, and in-statements
are not allowed in the policy.

The libsepol function cil_set_qualified_names() is called with the
desired value for the CIL db's "qualified_names" field.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 secilc/secil2conf.8.xml | 5 +++++
 secilc/secil2conf.c     | 9 ++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/secilc/secil2conf.8.xml b/secilc/secil2conf.8.xml
index 59d87a54..856c1239 100644
--- a/secilc/secil2conf.8.xml
+++ b/secilc/secil2conf.8.xml
@@ -50,6 +50,11 @@
             <listitem><para>Treat tunables as booleans.</para></listitem>
          </varlistentry>
 
+         <varlistentry>
+            <term><option>-Q, --qualified-names</option></term>
+            <listitem><para>Use qualified names. Blocks, blockinherits, blockabstracts, and in-statements will not be allowed.</para></listitem>
+         </varlistentry>
+
          <varlistentry>
             <term><option>-v, --verbose</option></term>
             <listitem><para>Increment verbosity level.</para></listitem>
diff --git a/secilc/secil2conf.c b/secilc/secil2conf.c
index 4e97dd66..7a317ada 100644
--- a/secilc/secil2conf.c
+++ b/secilc/secil2conf.c
@@ -52,6 +52,7 @@ static __attribute__((__noreturn__)) void usage(const char *prog)
 	printf("                                 This will override the (mls boolean) statement\n");
 	printf("                                 if present in the policy\n");
 	printf("  -P, --preserve-tunables        treat tunables as booleans\n");
+	printf("  -Q, --qualified-names          Use qualified names and do not allow blocks\n");
 	printf("  -v, --verbose                  increment verbosity level\n");
 	printf("  -h, --help                     display usage information\n");
 	exit(1);
@@ -68,6 +69,7 @@ int main(int argc, char *argv[])
 	struct cil_db *db = NULL;
 	int mls = -1;
 	int preserve_tunables = 0;
+	int qualified_names = 0;
 	int opt_char;
 	int opt_index = 0;
 	enum cil_log_level log_level = CIL_ERR;
@@ -76,13 +78,14 @@ int main(int argc, char *argv[])
 		{"verbose", no_argument, 0, 'v'},
 		{"mls", required_argument, 0, 'M'},
 		{"preserve-tunables", no_argument, 0, 'P'},
+		{"qualified-names", no_argument, 0, 'Q'},
 		{"output", required_argument, 0, 'o'},
 		{0, 0, 0, 0}
 	};
 	int i;
 
 	while (1) {
-		opt_char = getopt_long(argc, argv, "o:hvM:P", long_opts, &opt_index);
+		opt_char = getopt_long(argc, argv, "o:hvM:PQ", long_opts, &opt_index);
 		if (opt_char == -1) {
 			break;
 		}
@@ -102,6 +105,9 @@ int main(int argc, char *argv[])
 			case 'P':
 				preserve_tunables = 1;
 				break;
+			case 'Q':
+				qualified_names = 1;
+				break;
 			case 'o':
 				output = strdup(optarg);
 				break;
@@ -123,6 +129,7 @@ int main(int argc, char *argv[])
 
 	cil_db_init(&db);
 	cil_set_preserve_tunables(db, preserve_tunables);
+	cil_set_qualified_names(db, qualified_names);
 	cil_set_mls(db, mls);
 	cil_set_attrs_expand_generated(db, 0);
 	cil_set_attrs_expand_size(db, 0);
-- 
2.26.3

Re: [PATCH 1/4] libsepol/cil: Provide option to allow qualified names in declarations

[Dominick Grift]

James Carter <jwcart2@gmail.com> writes:

> Qualified names have "dots" in them. They are generated when a CIL
> policy is compiled and come from declarations in blocks. If a kernel
> policy is decompiled into a CIL policy, the resulting policy could
> have decarations that use qualified names. Compiling this policy would
> result in an error because "dots" in declarations are not allowed.
>
> Qualified names in a policy are normally used to refer to the name of
> identifiers, blocks, macros, or optionals that are declared in a
> different block (that is not a parent). Name resolution is based on
> splitting a name based on the "dots", searching the parents up to the
> global namespace for the first block using the first part of the name,
> using the second part of the name to lookup the next block using the
> first block's symbol tables, looking up the third block in the second's
> symbol tables, and so on.
>
> To allow the option of using qualified names in declarations:
>
> 1) Create a field in the struct cil_db called "qualified_names" which
> is set to CIL_TRUE when qualified names are to be used. This field is
> checked in cil_verify_name() and "dots" are allowed if qualified names
> are being allowed.
>
> 2) Only allow the direct lookup of the whole name in the global symbol
> table. This means that blocks, blockinherits, blockabstracts, and in-
> statements cannot be allowed. Use the "qualified_names" field of the
> cil_db to know when using one of these should result in an error.
>
> 3) Create the function cil_set_qualified_names() that is used to set
> the "qualified_names" field. Export the function in libsepol.

I wonder what the use-case for this functionality is?

>
> Signed-off-by: James Carter <jwcart2@gmail.com>
> ---
>  libsepol/cil/include/cil/cil.h     |  1 +
>  libsepol/cil/src/cil.c             |  6 ++++++
>  libsepol/cil/src/cil_build_ast.c   | 24 ++++++++++++++++++++++--
>  libsepol/cil/src/cil_internal.h    |  1 +
>  libsepol/cil/src/cil_resolve_ast.c |  4 ++--
>  libsepol/cil/src/cil_verify.c      | 19 ++++++++++++++-----
>  libsepol/cil/src/cil_verify.h      |  2 +-
>  libsepol/src/libsepol.map.in       |  1 +
>  8 files changed, 48 insertions(+), 10 deletions(-)
>
> diff --git a/libsepol/cil/include/cil/cil.h b/libsepol/cil/include/cil/cil.h
> index 92fac6e1..482ca522 100644
> --- a/libsepol/cil/include/cil/cil.h
> +++ b/libsepol/cil/include/cil/cil.h
> @@ -51,6 +51,7 @@ extern int cil_selinuxusers_to_string(cil_db_t *db, char **out, size_t *size);
>  extern int cil_filecons_to_string(cil_db_t *db, char **out, size_t *size);
>  extern void cil_set_disable_dontaudit(cil_db_t *db, int disable_dontaudit);
>  extern void cil_set_multiple_decls(cil_db_t *db, int multiple_decls);
> +extern void cil_set_qualified_names(struct cil_db *db, int qualified_names);
>  extern void cil_set_disable_neverallow(cil_db_t *db, int disable_neverallow);
>  extern void cil_set_preserve_tunables(cil_db_t *db, int preserve_tunables);
>  extern int cil_set_handle_unknown(cil_db_t *db, int handle_unknown);
> diff --git a/libsepol/cil/src/cil.c b/libsepol/cil/src/cil.c
> index 9d5038d9..3f2e6927 100644
> --- a/libsepol/cil/src/cil.c
> +++ b/libsepol/cil/src/cil.c
> @@ -440,6 +440,7 @@ void cil_db_init(struct cil_db **db)
>  	(*db)->handle_unknown = -1;
>  	(*db)->mls = -1;
>  	(*db)->multiple_decls = CIL_FALSE;
> +	(*db)->qualified_names = CIL_FALSE;
>  	(*db)->target_platform = SEPOL_TARGET_SELINUX;
>  	(*db)->policy_version = POLICYDB_VERSION_MAX;
>  }
> @@ -1872,6 +1873,11 @@ void cil_set_multiple_decls(struct cil_db *db, int multiple_decls)
>  	db->multiple_decls = multiple_decls;
>  }
>  
> +void cil_set_qualified_names(struct cil_db *db, int qualified_names)
> +{
> +	db->qualified_names = qualified_names;
> +}
> +
>  void cil_set_target_platform(struct cil_db *db, int target_platform)
>  {
>  	db->target_platform = target_platform;
> diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
> index baed3e58..9da90883 100644
> --- a/libsepol/cil/src/cil_build_ast.c
> +++ b/libsepol/cil/src/cil_build_ast.c
> @@ -146,7 +146,7 @@ int cil_gen_node(struct cil_db *db, struct cil_tree_node *ast_node, struct cil_s
>  	int rc = SEPOL_ERR;
>  	symtab_t *symtab = NULL;
>  
> -	rc = cil_verify_name((const char*)key, nflavor);
> +	rc = cil_verify_name(db, (const char*)key, nflavor);
>  	if (rc != SEPOL_OK) {
>  		goto exit;
>  	}
> @@ -204,6 +204,11 @@ int cil_gen_block(struct cil_db *db, struct cil_tree_node *parse_current, struct
>  		goto exit;
>  	}
>  
> +	if (db->qualified_names) {
> +		cil_log(CIL_ERR, "Blocks are not allowed when the option for qualified names is used\n");
> +		goto exit;
> +	}
> +
>  	rc = __cil_verify_syntax(parse_current, syntax, syntax_len);
>  	if (rc != SEPOL_OK) {
>  		goto exit;
> @@ -274,6 +279,11 @@ int cil_gen_blockinherit(struct cil_db *db, struct cil_tree_node *parse_current,
>  		goto exit;
>  	}
>  
> +	if (db->qualified_names) {
> +		cil_log(CIL_ERR, "Block inherit rules are not allowed when the option for qualified names is used\n");
> +		goto exit;
> +	}
> +
>  	rc = __cil_verify_syntax(parse_current, syntax, syntax_len);
>  	if (rc != SEPOL_OK) {
>  		goto exit;
> @@ -331,6 +341,11 @@ int cil_gen_blockabstract(struct cil_db *db, struct cil_tree_node *parse_current
>  		goto exit;
>  	}
>  
> +	if (db->qualified_names) {
> +		cil_log(CIL_ERR, "Block abstract rules are not allowed when the option for qualified names is used\n");
> +		goto exit;
> +	}
> +
>  	rc = __cil_verify_syntax(parse_current, syntax, syntax_len);
>  	if (rc != SEPOL_OK) {
>  		goto exit;
> @@ -376,6 +391,11 @@ int cil_gen_in(struct cil_db *db, struct cil_tree_node *parse_current, struct ci
>  		goto exit;
>  	}
>  
> +	if (db->qualified_names) {
> +		cil_log(CIL_ERR, "In-statements are not allowed when the option for qualified names is used\n");
> +		goto exit;
> +	}
> +
>  	rc = __cil_verify_syntax(parse_current, syntax, syntax_len);
>  	if (rc != SEPOL_OK) {
>  		goto exit;
> @@ -5261,7 +5281,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, param->flavor);
> +		rc = cil_verify_name(db, param->str, param->flavor);
>  		if (rc != SEPOL_OK) {
>  			cil_destroy_param(param);
>  			goto exit;
> diff --git a/libsepol/cil/src/cil_internal.h b/libsepol/cil/src/cil_internal.h
> index 8b9aeabf..f184d739 100644
> --- a/libsepol/cil/src/cil_internal.h
> +++ b/libsepol/cil/src/cil_internal.h
> @@ -321,6 +321,7 @@ struct cil_db {
>  	int handle_unknown;
>  	int mls;
>  	int multiple_decls;
> +	int qualified_names;
>  	int target_platform;
>  	int policy_version;
>  };
> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> index 5245cc15..27efffa6 100644
> --- a/libsepol/cil/src/cil_resolve_ast.c
> +++ b/libsepol/cil/src/cil_resolve_ast.c
> @@ -4409,8 +4409,8 @@ int cil_resolve_name_keep_aliases(struct cil_tree_node *ast_node, char *name, en
>  
>  	*datum = NULL;
>  
> -	if (strchr(name,'.') == NULL) {
> -		/* No '.' in name */
> +	if (db->qualified_names || strchr(name,'.') == NULL) {
> +		/* Using qualified names or No '.' in name */
>  		rc = __cil_resolve_name_helper(db, ast_node->parent, name, sym_index, datum);
>  		if (rc != SEPOL_OK) {
>  			goto exit;
> diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c
> index 59397f70..9cb1a6f6 100644
> --- a/libsepol/cil/src/cil_verify.c
> +++ b/libsepol/cil/src/cil_verify.c
> @@ -92,7 +92,7 @@ static int __cil_is_reserved_name(const char *name, enum cil_flavor flavor)
>  	return CIL_FALSE;
>  }
>  
> -int cil_verify_name(const char *name, enum cil_flavor flavor)
> +int cil_verify_name(struct cil_db *db, const char *name, enum cil_flavor flavor)
>  {
>  	int rc = SEPOL_ERR;
>  	int len;
> @@ -116,10 +116,19 @@ int cil_verify_name(const char *name, enum cil_flavor flavor)
>  			goto exit;
>  	}
>  
> -	for (i = 1; i < len; i++) {
> -		if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-') {
> -			cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
> -			goto exit;
> +	if (db->qualified_names == CIL_FALSE) {
> +		for (i = 1; i < len; i++) {
> +			if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-') {
> +				cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
> +				goto exit;
> +			}
> +		}
> +	} else {
> +		for (i = 1; i < len; i++) {
> +			if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-' && name[i] != '.') {
> +				cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
> +				goto exit;
> +			}
>  		}
>  	}
>  
> diff --git a/libsepol/cil/src/cil_verify.h b/libsepol/cil/src/cil_verify.h
> index 4ea14f5b..8eb3c463 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, enum cil_flavor flavor);
> +int cil_verify_name(struct cil_db *db, 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);
> diff --git a/libsepol/src/libsepol.map.in b/libsepol/src/libsepol.map.in
> index 2e503bd1..0e05d606 100644
> --- a/libsepol/src/libsepol.map.in
> +++ b/libsepol/src/libsepol.map.in
> @@ -272,4 +272,5 @@ LIBSEPOL_3.0 {
>  	cil_write_parse_ast;
>  	cil_write_build_ast;
>  	cil_write_resolve_ast;
> +	cil_set_qualified_names;
>  } LIBSEPOL_1.1;

-- 
gpg --locate-keys dominick.grift@defensec.nl
Key fingerprint = FCD2 3660 5D6B 9D27 7FC6  E0FF DA7E 521F 10F6 4098
https://sks-keyservers.net/pks/lookup?op=get&search=0xDA7E521F10F64098
Dominick Grift

Re: [PATCH 1/4] libsepol/cil: Provide option to allow qualified names in declarations

[James Carter]

On Thu, Jun 24, 2021 at 4:21 PM Dominick Grift
<dominick.grift@defensec.nl> wrote:
>
> James Carter <jwcart2@gmail.com> writes:
>
> > Qualified names have "dots" in them. They are generated when a CIL
> > policy is compiled and come from declarations in blocks. If a kernel
> > policy is decompiled into a CIL policy, the resulting policy could
> > have decarations that use qualified names. Compiling this policy would
> > result in an error because "dots" in declarations are not allowed.
> >
> > Qualified names in a policy are normally used to refer to the name of
> > identifiers, blocks, macros, or optionals that are declared in a
> > different block (that is not a parent). Name resolution is based on
> > splitting a name based on the "dots", searching the parents up to the
> > global namespace for the first block using the first part of the name,
> > using the second part of the name to lookup the next block using the
> > first block's symbol tables, looking up the third block in the second's
> > symbol tables, and so on.
> >
> > To allow the option of using qualified names in declarations:
> >
> > 1) Create a field in the struct cil_db called "qualified_names" which
> > is set to CIL_TRUE when qualified names are to be used. This field is
> > checked in cil_verify_name() and "dots" are allowed if qualified names
> > are being allowed.
> >
> > 2) Only allow the direct lookup of the whole name in the global symbol
> > table. This means that blocks, blockinherits, blockabstracts, and in-
> > statements cannot be allowed. Use the "qualified_names" field of the
> > cil_db to know when using one of these should result in an error.
> >
> > 3) Create the function cil_set_qualified_names() that is used to set
> > the "qualified_names" field. Export the function in libsepol.
>
> I wonder what the use-case for this functionality is?
>

Compiling a kernel policy that has been turned back into CIL.

Having something like
  (block b
     (type t)
      ....
  )
will result in the type "b.t" in the kernel policy and "b.t" is not
valid in a declaration.

Of course, you can turn the kernel policy into a policy.conf and
compile it with checkpolicy, but I would like to be able to compile it
with secilc.

Another use case is with secil2tree. This generates valid CIL if you
use "-A build", but not if you use "-A resolve". One reason the policy
is not valid is that the names are qualified at this point and will
have "dots" in them if there are declarations within blocks in the
policy. (Another reason the resulting policy would not be valid is
that there are still blocks in it even though they have been
essentially "resolved".)

Jim


> >
> > Signed-off-by: James Carter <jwcart2@gmail.com>
> > ---
> >  libsepol/cil/include/cil/cil.h     |  1 +
> >  libsepol/cil/src/cil.c             |  6 ++++++
> >  libsepol/cil/src/cil_build_ast.c   | 24 ++++++++++++++++++++++--
> >  libsepol/cil/src/cil_internal.h    |  1 +
> >  libsepol/cil/src/cil_resolve_ast.c |  4 ++--
> >  libsepol/cil/src/cil_verify.c      | 19 ++++++++++++++-----
> >  libsepol/cil/src/cil_verify.h      |  2 +-
> >  libsepol/src/libsepol.map.in       |  1 +
> >  8 files changed, 48 insertions(+), 10 deletions(-)
> >
> > diff --git a/libsepol/cil/include/cil/cil.h b/libsepol/cil/include/cil/cil.h
> > index 92fac6e1..482ca522 100644
> > --- a/libsepol/cil/include/cil/cil.h
> > +++ b/libsepol/cil/include/cil/cil.h
> > @@ -51,6 +51,7 @@ extern int cil_selinuxusers_to_string(cil_db_t *db, char **out, size_t *size);
> >  extern int cil_filecons_to_string(cil_db_t *db, char **out, size_t *size);
> >  extern void cil_set_disable_dontaudit(cil_db_t *db, int disable_dontaudit);
> >  extern void cil_set_multiple_decls(cil_db_t *db, int multiple_decls);
> > +extern void cil_set_qualified_names(struct cil_db *db, int qualified_names);
> >  extern void cil_set_disable_neverallow(cil_db_t *db, int disable_neverallow);
> >  extern void cil_set_preserve_tunables(cil_db_t *db, int preserve_tunables);
> >  extern int cil_set_handle_unknown(cil_db_t *db, int handle_unknown);
> > diff --git a/libsepol/cil/src/cil.c b/libsepol/cil/src/cil.c
> > index 9d5038d9..3f2e6927 100644
> > --- a/libsepol/cil/src/cil.c
> > +++ b/libsepol/cil/src/cil.c
> > @@ -440,6 +440,7 @@ void cil_db_init(struct cil_db **db)
> >       (*db)->handle_unknown = -1;
> >       (*db)->mls = -1;
> >       (*db)->multiple_decls = CIL_FALSE;
> > +     (*db)->qualified_names = CIL_FALSE;
> >       (*db)->target_platform = SEPOL_TARGET_SELINUX;
> >       (*db)->policy_version = POLICYDB_VERSION_MAX;
> >  }
> > @@ -1872,6 +1873,11 @@ void cil_set_multiple_decls(struct cil_db *db, int multiple_decls)
> >       db->multiple_decls = multiple_decls;
> >  }
> >
> > +void cil_set_qualified_names(struct cil_db *db, int qualified_names)
> > +{
> > +     db->qualified_names = qualified_names;
> > +}
> > +
> >  void cil_set_target_platform(struct cil_db *db, int target_platform)
> >  {
> >       db->target_platform = target_platform;
> > diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
> > index baed3e58..9da90883 100644
> > --- a/libsepol/cil/src/cil_build_ast.c
> > +++ b/libsepol/cil/src/cil_build_ast.c
> > @@ -146,7 +146,7 @@ int cil_gen_node(struct cil_db *db, struct cil_tree_node *ast_node, struct cil_s
> >       int rc = SEPOL_ERR;
> >       symtab_t *symtab = NULL;
> >
> > -     rc = cil_verify_name((const char*)key, nflavor);
> > +     rc = cil_verify_name(db, (const char*)key, nflavor);
> >       if (rc != SEPOL_OK) {
> >               goto exit;
> >       }
> > @@ -204,6 +204,11 @@ int cil_gen_block(struct cil_db *db, struct cil_tree_node *parse_current, struct
> >               goto exit;
> >       }
> >
> > +     if (db->qualified_names) {
> > +             cil_log(CIL_ERR, "Blocks are not allowed when the option for qualified names is used\n");
> > +             goto exit;
> > +     }
> > +
> >       rc = __cil_verify_syntax(parse_current, syntax, syntax_len);
> >       if (rc != SEPOL_OK) {
> >               goto exit;
> > @@ -274,6 +279,11 @@ int cil_gen_blockinherit(struct cil_db *db, struct cil_tree_node *parse_current,
> >               goto exit;
> >       }
> >
> > +     if (db->qualified_names) {
> > +             cil_log(CIL_ERR, "Block inherit rules are not allowed when the option for qualified names is used\n");
> > +             goto exit;
> > +     }
> > +
> >       rc = __cil_verify_syntax(parse_current, syntax, syntax_len);
> >       if (rc != SEPOL_OK) {
> >               goto exit;
> > @@ -331,6 +341,11 @@ int cil_gen_blockabstract(struct cil_db *db, struct cil_tree_node *parse_current
> >               goto exit;
> >       }
> >
> > +     if (db->qualified_names) {
> > +             cil_log(CIL_ERR, "Block abstract rules are not allowed when the option for qualified names is used\n");
> > +             goto exit;
> > +     }
> > +
> >       rc = __cil_verify_syntax(parse_current, syntax, syntax_len);
> >       if (rc != SEPOL_OK) {
> >               goto exit;
> > @@ -376,6 +391,11 @@ int cil_gen_in(struct cil_db *db, struct cil_tree_node *parse_current, struct ci
> >               goto exit;
> >       }
> >
> > +     if (db->qualified_names) {
> > +             cil_log(CIL_ERR, "In-statements are not allowed when the option for qualified names is used\n");
> > +             goto exit;
> > +     }
> > +
> >       rc = __cil_verify_syntax(parse_current, syntax, syntax_len);
> >       if (rc != SEPOL_OK) {
> >               goto exit;
> > @@ -5261,7 +5281,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, param->flavor);
> > +             rc = cil_verify_name(db, param->str, param->flavor);
> >               if (rc != SEPOL_OK) {
> >                       cil_destroy_param(param);
> >                       goto exit;
> > diff --git a/libsepol/cil/src/cil_internal.h b/libsepol/cil/src/cil_internal.h
> > index 8b9aeabf..f184d739 100644
> > --- a/libsepol/cil/src/cil_internal.h
> > +++ b/libsepol/cil/src/cil_internal.h
> > @@ -321,6 +321,7 @@ struct cil_db {
> >       int handle_unknown;
> >       int mls;
> >       int multiple_decls;
> > +     int qualified_names;
> >       int target_platform;
> >       int policy_version;
> >  };
> > diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> > index 5245cc15..27efffa6 100644
> > --- a/libsepol/cil/src/cil_resolve_ast.c
> > +++ b/libsepol/cil/src/cil_resolve_ast.c
> > @@ -4409,8 +4409,8 @@ int cil_resolve_name_keep_aliases(struct cil_tree_node *ast_node, char *name, en
> >
> >       *datum = NULL;
> >
> > -     if (strchr(name,'.') == NULL) {
> > -             /* No '.' in name */
> > +     if (db->qualified_names || strchr(name,'.') == NULL) {
> > +             /* Using qualified names or No '.' in name */
> >               rc = __cil_resolve_name_helper(db, ast_node->parent, name, sym_index, datum);
> >               if (rc != SEPOL_OK) {
> >                       goto exit;
> > diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c
> > index 59397f70..9cb1a6f6 100644
> > --- a/libsepol/cil/src/cil_verify.c
> > +++ b/libsepol/cil/src/cil_verify.c
> > @@ -92,7 +92,7 @@ static int __cil_is_reserved_name(const char *name, enum cil_flavor flavor)
> >       return CIL_FALSE;
> >  }
> >
> > -int cil_verify_name(const char *name, enum cil_flavor flavor)
> > +int cil_verify_name(struct cil_db *db, const char *name, enum cil_flavor flavor)
> >  {
> >       int rc = SEPOL_ERR;
> >       int len;
> > @@ -116,10 +116,19 @@ int cil_verify_name(const char *name, enum cil_flavor flavor)
> >                       goto exit;
> >       }
> >
> > -     for (i = 1; i < len; i++) {
> > -             if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-') {
> > -                     cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
> > -                     goto exit;
> > +     if (db->qualified_names == CIL_FALSE) {
> > +             for (i = 1; i < len; i++) {
> > +                     if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-') {
> > +                             cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
> > +                             goto exit;
> > +                     }
> > +             }
> > +     } else {
> > +             for (i = 1; i < len; i++) {
> > +                     if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-' && name[i] != '.') {
> > +                             cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
> > +                             goto exit;
> > +                     }
> >               }
> >       }
> >
> > diff --git a/libsepol/cil/src/cil_verify.h b/libsepol/cil/src/cil_verify.h
> > index 4ea14f5b..8eb3c463 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, enum cil_flavor flavor);
> > +int cil_verify_name(struct cil_db *db, 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);
> > diff --git a/libsepol/src/libsepol.map.in b/libsepol/src/libsepol.map.in
> > index 2e503bd1..0e05d606 100644
> > --- a/libsepol/src/libsepol.map.in
> > +++ b/libsepol/src/libsepol.map.in
> > @@ -272,4 +272,5 @@ LIBSEPOL_3.0 {
> >       cil_write_parse_ast;
> >       cil_write_build_ast;
> >       cil_write_resolve_ast;
> > +     cil_set_qualified_names;
> >  } LIBSEPOL_1.1;
>
> --
> gpg --locate-keys dominick.grift@defensec.nl
> Key fingerprint = FCD2 3660 5D6B 9D27 7FC6  E0FF DA7E 521F 10F6 4098
> https://sks-keyservers.net/pks/lookup?op=get&search=0xDA7E521F10F64098
> Dominick Grift

Re: [PATCH 1/4] libsepol/cil: Provide option to allow qualified names in declarations

[Dominick Grift]

James Carter <jwcart2@gmail.com> writes:

> On Thu, Jun 24, 2021 at 4:21 PM Dominick Grift
> <dominick.grift@defensec.nl> wrote:
>>
>> James Carter <jwcart2@gmail.com> writes:
>>
>> > Qualified names have "dots" in them. They are generated when a CIL
>> > policy is compiled and come from declarations in blocks. If a kernel
>> > policy is decompiled into a CIL policy, the resulting policy could
>> > have decarations that use qualified names. Compiling this policy would
>> > result in an error because "dots" in declarations are not allowed.
>> >
>> > Qualified names in a policy are normally used to refer to the name of
>> > identifiers, blocks, macros, or optionals that are declared in a
>> > different block (that is not a parent). Name resolution is based on
>> > splitting a name based on the "dots", searching the parents up to the
>> > global namespace for the first block using the first part of the name,
>> > using the second part of the name to lookup the next block using the
>> > first block's symbol tables, looking up the third block in the second's
>> > symbol tables, and so on.
>> >
>> > To allow the option of using qualified names in declarations:
>> >
>> > 1) Create a field in the struct cil_db called "qualified_names" which
>> > is set to CIL_TRUE when qualified names are to be used. This field is
>> > checked in cil_verify_name() and "dots" are allowed if qualified names
>> > are being allowed.
>> >
>> > 2) Only allow the direct lookup of the whole name in the global symbol
>> > table. This means that blocks, blockinherits, blockabstracts, and in-
>> > statements cannot be allowed. Use the "qualified_names" field of the
>> > cil_db to know when using one of these should result in an error.
>> >
>> > 3) Create the function cil_set_qualified_names() that is used to set
>> > the "qualified_names" field. Export the function in libsepol.
>>
>> I wonder what the use-case for this functionality is?
>>
>
> Compiling a kernel policy that has been turned back into CIL.
>
> Having something like
>   (block b
>      (type t)
>       ....
>   )
> will result in the type "b.t" in the kernel policy and "b.t" is not
> valid in a declaration.
>
> Of course, you can turn the kernel policy into a policy.conf and
> compile it with checkpolicy, but I would like to be able to compile it
> with secilc.
>
> Another use case is with secil2tree. This generates valid CIL if you
> use "-A build", but not if you use "-A resolve". One reason the policy
> is not valid is that the names are qualified at this point and will
> have "dots" in them if there are declarations within blocks in the
> policy. (Another reason the resulting policy would not be valid is
> that there are still blocks in it even though they have been
> essentially "resolved".)

Understood, thanks.

>
> Jim
>
>
>> >
>> > Signed-off-by: James Carter <jwcart2@gmail.com>
>> > ---
>> >  libsepol/cil/include/cil/cil.h     |  1 +
>> >  libsepol/cil/src/cil.c             |  6 ++++++
>> >  libsepol/cil/src/cil_build_ast.c   | 24 ++++++++++++++++++++++--
>> >  libsepol/cil/src/cil_internal.h    |  1 +
>> >  libsepol/cil/src/cil_resolve_ast.c |  4 ++--
>> >  libsepol/cil/src/cil_verify.c      | 19 ++++++++++++++-----
>> >  libsepol/cil/src/cil_verify.h      |  2 +-
>> >  libsepol/src/libsepol.map.in       |  1 +
>> >  8 files changed, 48 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/libsepol/cil/include/cil/cil.h b/libsepol/cil/include/cil/cil.h
>> > index 92fac6e1..482ca522 100644
>> > --- a/libsepol/cil/include/cil/cil.h
>> > +++ b/libsepol/cil/include/cil/cil.h
>> > @@ -51,6 +51,7 @@ extern int cil_selinuxusers_to_string(cil_db_t *db, char **out, size_t *size);
>> >  extern int cil_filecons_to_string(cil_db_t *db, char **out, size_t *size);
>> >  extern void cil_set_disable_dontaudit(cil_db_t *db, int disable_dontaudit);
>> >  extern void cil_set_multiple_decls(cil_db_t *db, int multiple_decls);
>> > +extern void cil_set_qualified_names(struct cil_db *db, int qualified_names);
>> >  extern void cil_set_disable_neverallow(cil_db_t *db, int disable_neverallow);
>> >  extern void cil_set_preserve_tunables(cil_db_t *db, int preserve_tunables);
>> >  extern int cil_set_handle_unknown(cil_db_t *db, int handle_unknown);
>> > diff --git a/libsepol/cil/src/cil.c b/libsepol/cil/src/cil.c
>> > index 9d5038d9..3f2e6927 100644
>> > --- a/libsepol/cil/src/cil.c
>> > +++ b/libsepol/cil/src/cil.c
>> > @@ -440,6 +440,7 @@ void cil_db_init(struct cil_db **db)
>> >       (*db)->handle_unknown = -1;
>> >       (*db)->mls = -1;
>> >       (*db)->multiple_decls = CIL_FALSE;
>> > +     (*db)->qualified_names = CIL_FALSE;
>> >       (*db)->target_platform = SEPOL_TARGET_SELINUX;
>> >       (*db)->policy_version = POLICYDB_VERSION_MAX;
>> >  }
>> > @@ -1872,6 +1873,11 @@ void cil_set_multiple_decls(struct cil_db *db, int multiple_decls)
>> >       db->multiple_decls = multiple_decls;
>> >  }
>> >
>> > +void cil_set_qualified_names(struct cil_db *db, int qualified_names)
>> > +{
>> > +     db->qualified_names = qualified_names;
>> > +}
>> > +
>> >  void cil_set_target_platform(struct cil_db *db, int target_platform)
>> >  {
>> >       db->target_platform = target_platform;
>> > diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
>> > index baed3e58..9da90883 100644
>> > --- a/libsepol/cil/src/cil_build_ast.c
>> > +++ b/libsepol/cil/src/cil_build_ast.c
>> > @@ -146,7 +146,7 @@ int cil_gen_node(struct cil_db *db, struct cil_tree_node *ast_node, struct cil_s
>> >       int rc = SEPOL_ERR;
>> >       symtab_t *symtab = NULL;
>> >
>> > -     rc = cil_verify_name((const char*)key, nflavor);
>> > +     rc = cil_verify_name(db, (const char*)key, nflavor);
>> >       if (rc != SEPOL_OK) {
>> >               goto exit;
>> >       }
>> > @@ -204,6 +204,11 @@ int cil_gen_block(struct cil_db *db, struct cil_tree_node *parse_current, struct
>> >               goto exit;
>> >       }
>> >
>> > +     if (db->qualified_names) {
>> > +             cil_log(CIL_ERR, "Blocks are not allowed when the option for qualified names is used\n");
>> > +             goto exit;
>> > +     }
>> > +
>> >       rc = __cil_verify_syntax(parse_current, syntax, syntax_len);
>> >       if (rc != SEPOL_OK) {
>> >               goto exit;
>> > @@ -274,6 +279,11 @@ int cil_gen_blockinherit(struct cil_db *db, struct cil_tree_node *parse_current,
>> >               goto exit;
>> >       }
>> >
>> > +     if (db->qualified_names) {
>> > +             cil_log(CIL_ERR, "Block inherit rules are not allowed when the option for qualified names is used\n");
>> > +             goto exit;
>> > +     }
>> > +
>> >       rc = __cil_verify_syntax(parse_current, syntax, syntax_len);
>> >       if (rc != SEPOL_OK) {
>> >               goto exit;
>> > @@ -331,6 +341,11 @@ int cil_gen_blockabstract(struct cil_db *db, struct cil_tree_node *parse_current
>> >               goto exit;
>> >       }
>> >
>> > +     if (db->qualified_names) {
>> > +             cil_log(CIL_ERR, "Block abstract rules are not allowed when the option for qualified names is used\n");
>> > +             goto exit;
>> > +     }
>> > +
>> >       rc = __cil_verify_syntax(parse_current, syntax, syntax_len);
>> >       if (rc != SEPOL_OK) {
>> >               goto exit;
>> > @@ -376,6 +391,11 @@ int cil_gen_in(struct cil_db *db, struct cil_tree_node *parse_current, struct ci
>> >               goto exit;
>> >       }
>> >
>> > +     if (db->qualified_names) {
>> > +             cil_log(CIL_ERR, "In-statements are not allowed when the option for qualified names is used\n");
>> > +             goto exit;
>> > +     }
>> > +
>> >       rc = __cil_verify_syntax(parse_current, syntax, syntax_len);
>> >       if (rc != SEPOL_OK) {
>> >               goto exit;
>> > @@ -5261,7 +5281,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, param->flavor);
>> > +             rc = cil_verify_name(db, param->str, param->flavor);
>> >               if (rc != SEPOL_OK) {
>> >                       cil_destroy_param(param);
>> >                       goto exit;
>> > diff --git a/libsepol/cil/src/cil_internal.h b/libsepol/cil/src/cil_internal.h
>> > index 8b9aeabf..f184d739 100644
>> > --- a/libsepol/cil/src/cil_internal.h
>> > +++ b/libsepol/cil/src/cil_internal.h
>> > @@ -321,6 +321,7 @@ struct cil_db {
>> >       int handle_unknown;
>> >       int mls;
>> >       int multiple_decls;
>> > +     int qualified_names;
>> >       int target_platform;
>> >       int policy_version;
>> >  };
>> > diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
>> > index 5245cc15..27efffa6 100644
>> > --- a/libsepol/cil/src/cil_resolve_ast.c
>> > +++ b/libsepol/cil/src/cil_resolve_ast.c
>> > @@ -4409,8 +4409,8 @@ int cil_resolve_name_keep_aliases(struct cil_tree_node *ast_node, char *name, en
>> >
>> >       *datum = NULL;
>> >
>> > -     if (strchr(name,'.') == NULL) {
>> > -             /* No '.' in name */
>> > +     if (db->qualified_names || strchr(name,'.') == NULL) {
>> > +             /* Using qualified names or No '.' in name */
>> >               rc = __cil_resolve_name_helper(db, ast_node->parent, name, sym_index, datum);
>> >               if (rc != SEPOL_OK) {
>> >                       goto exit;
>> > diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c
>> > index 59397f70..9cb1a6f6 100644
>> > --- a/libsepol/cil/src/cil_verify.c
>> > +++ b/libsepol/cil/src/cil_verify.c
>> > @@ -92,7 +92,7 @@ static int __cil_is_reserved_name(const char *name, enum cil_flavor flavor)
>> >       return CIL_FALSE;
>> >  }
>> >
>> > -int cil_verify_name(const char *name, enum cil_flavor flavor)
>> > +int cil_verify_name(struct cil_db *db, const char *name, enum cil_flavor flavor)
>> >  {
>> >       int rc = SEPOL_ERR;
>> >       int len;
>> > @@ -116,10 +116,19 @@ int cil_verify_name(const char *name, enum cil_flavor flavor)
>> >                       goto exit;
>> >       }
>> >
>> > -     for (i = 1; i < len; i++) {
>> > -             if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-') {
>> > -                     cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
>> > -                     goto exit;
>> > +     if (db->qualified_names == CIL_FALSE) {
>> > +             for (i = 1; i < len; i++) {
>> > +                     if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-') {
>> > +                             cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
>> > +                             goto exit;
>> > +                     }
>> > +             }
>> > +     } else {
>> > +             for (i = 1; i < len; i++) {
>> > +                     if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-' && name[i] != '.') {
>> > +                             cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
>> > +                             goto exit;
>> > +                     }
>> >               }
>> >       }
>> >
>> > diff --git a/libsepol/cil/src/cil_verify.h b/libsepol/cil/src/cil_verify.h
>> > index 4ea14f5b..8eb3c463 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, enum cil_flavor flavor);
>> > +int cil_verify_name(struct cil_db *db, 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);
>> > diff --git a/libsepol/src/libsepol.map.in b/libsepol/src/libsepol.map.in
>> > index 2e503bd1..0e05d606 100644
>> > --- a/libsepol/src/libsepol.map.in
>> > +++ b/libsepol/src/libsepol.map.in
>> > @@ -272,4 +272,5 @@ LIBSEPOL_3.0 {
>> >       cil_write_parse_ast;
>> >       cil_write_build_ast;
>> >       cil_write_resolve_ast;
>> > +     cil_set_qualified_names;
>> >  } LIBSEPOL_1.1;
>>
>> --
>> gpg --locate-keys dominick.grift@defensec.nl
>> Key fingerprint = FCD2 3660 5D6B 9D27 7FC6  E0FF DA7E 521F 10F6 4098
>> https://sks-keyservers.net/pks/lookup?op=get&search=0xDA7E521F10F64098
>> Dominick Grift

-- 
gpg --locate-keys dominick.grift@defensec.nl
Key fingerprint = FCD2 3660 5D6B 9D27 7FC6  E0FF DA7E 521F 10F6 4098
https://sks-keyservers.net/pks/lookup?op=get&search=0xDA7E521F10F64098
Dominick Grift

Re: [PATCH 1/4] libsepol/cil: Provide option to allow qualified names in declarations

[Nicolas Iooss]

On Thu, Jun 24, 2021 at 9:59 PM James Carter <jwcart2@gmail.com> wrote:
>
> Qualified names have "dots" in them. They are generated when a CIL
> policy is compiled and come from declarations in blocks. If a kernel
> policy is decompiled into a CIL policy, the resulting policy could
> have decarations that use qualified names. Compiling this policy would

Misspelling: decarations -> declarations

> result in an error because "dots" in declarations are not allowed.
>
> Qualified names in a policy are normally used to refer to the name of
> identifiers, blocks, macros, or optionals that are declared in a
> different block (that is not a parent). Name resolution is based on
> splitting a name based on the "dots", searching the parents up to the
> global namespace for the first block using the first part of the name,
> using the second part of the name to lookup the next block using the
> first block's symbol tables, looking up the third block in the second's
> symbol tables, and so on.
>
> To allow the option of using qualified names in declarations:
>
> 1) Create a field in the struct cil_db called "qualified_names" which
> is set to CIL_TRUE when qualified names are to be used. This field is
> checked in cil_verify_name() and "dots" are allowed if qualified names
> are being allowed.
>
> 2) Only allow the direct lookup of the whole name in the global symbol
> table. This means that blocks, blockinherits, blockabstracts, and in-
> statements cannot be allowed. Use the "qualified_names" field of the
> cil_db to know when using one of these should result in an error.
>
> 3) Create the function cil_set_qualified_names() that is used to set
> the "qualified_names" field. Export the function in libsepol.
>
> Signed-off-by: James Carter <jwcart2@gmail.com>
> ---
>  libsepol/cil/include/cil/cil.h     |  1 +
>  libsepol/cil/src/cil.c             |  6 ++++++
>  libsepol/cil/src/cil_build_ast.c   | 24 ++++++++++++++++++++++--
>  libsepol/cil/src/cil_internal.h    |  1 +
>  libsepol/cil/src/cil_resolve_ast.c |  4 ++--
>  libsepol/cil/src/cil_verify.c      | 19 ++++++++++++++-----
>  libsepol/cil/src/cil_verify.h      |  2 +-
>  libsepol/src/libsepol.map.in       |  1 +
>  8 files changed, 48 insertions(+), 10 deletions(-)
>
[...]
> diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c
> index 59397f70..9cb1a6f6 100644
> --- a/libsepol/cil/src/cil_verify.c
> +++ b/libsepol/cil/src/cil_verify.c
> @@ -92,7 +92,7 @@ static int __cil_is_reserved_name(const char *name, enum cil_flavor flavor)
>         return CIL_FALSE;
>  }
>
> -int cil_verify_name(const char *name, enum cil_flavor flavor)
> +int cil_verify_name(struct cil_db *db, const char *name, enum cil_flavor flavor)
>  {
>         int rc = SEPOL_ERR;
>         int len;
> @@ -116,10 +116,19 @@ int cil_verify_name(const char *name, enum cil_flavor flavor)
>                         goto exit;
>         }
>
> -       for (i = 1; i < len; i++) {
> -               if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-') {
> -                       cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
> -                       goto exit;
> +       if (db->qualified_names == CIL_FALSE) {
> +               for (i = 1; i < len; i++) {
> +                       if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-') {
> +                               cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
> +                               goto exit;
> +                       }
> +               }
> +       } else {
> +               for (i = 1; i < len; i++) {
> +                       if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-' && name[i] != '.') {
> +                               cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
> +                               goto exit;
> +                       }
>                 }
>         }

As cil_verify_name does not modify db (and would be seen as a function
which does not modify anything and only checks some rules), it would
be better to add a const qualifier:

    int cil_verify_name(const struct cil_db *db, const char *name,
enum cil_flavor flavor)

Other than that, the documentation of the new option,
"--qualified-names  Use qualified names." make me feel like the
wording can be improved. More precisely, a commit message contains
"Using qualified names means that declaration names can have dots in
them" and this definition should also be in the documentation. So I am
suggesting to replace the documentation with:

    Allow names containing dots (qualified names). Blocks,
blockinherits, blockabstracts, and in-statements will not be allowed.

Other than that, when I tested "secil2tree -A resolve -o test.cil
secilc/test/policy.cil && secilc -Q test.cil" I encountered other
errors, which are not related to these patches. When modifying the
resulting "test.cil" to make it compile (by removing blocks and
renaming some objects), option -Q worked fine. It would be nice if
such a command was integrated in secilc's tests, in order to prevent
future regressions. Nevertheless such work can be done once this
series is merged.

Cheers,
Nicolas

Re: [PATCH 1/4] libsepol/cil: Provide option to allow qualified names in declarations

[James Carter]

On Sat, Jun 26, 2021 at 5:46 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Thu, Jun 24, 2021 at 9:59 PM James Carter <jwcart2@gmail.com> wrote:
> >
> > Qualified names have "dots" in them. They are generated when a CIL
> > policy is compiled and come from declarations in blocks. If a kernel
> > policy is decompiled into a CIL policy, the resulting policy could
> > have decarations that use qualified names. Compiling this policy would
>
> Misspelling: decarations -> declarations
>

Thanks, I'll fix that in v2.

> > result in an error because "dots" in declarations are not allowed.
> >
> > Qualified names in a policy are normally used to refer to the name of
> > identifiers, blocks, macros, or optionals that are declared in a
> > different block (that is not a parent). Name resolution is based on
> > splitting a name based on the "dots", searching the parents up to the
> > global namespace for the first block using the first part of the name,
> > using the second part of the name to lookup the next block using the
> > first block's symbol tables, looking up the third block in the second's
> > symbol tables, and so on.
> >
> > To allow the option of using qualified names in declarations:
> >
> > 1) Create a field in the struct cil_db called "qualified_names" which
> > is set to CIL_TRUE when qualified names are to be used. This field is
> > checked in cil_verify_name() and "dots" are allowed if qualified names
> > are being allowed.
> >
> > 2) Only allow the direct lookup of the whole name in the global symbol
> > table. This means that blocks, blockinherits, blockabstracts, and in-
> > statements cannot be allowed. Use the "qualified_names" field of the
> > cil_db to know when using one of these should result in an error.
> >
> > 3) Create the function cil_set_qualified_names() that is used to set
> > the "qualified_names" field. Export the function in libsepol.
> >
> > Signed-off-by: James Carter <jwcart2@gmail.com>
> > ---
> >  libsepol/cil/include/cil/cil.h     |  1 +
> >  libsepol/cil/src/cil.c             |  6 ++++++
> >  libsepol/cil/src/cil_build_ast.c   | 24 ++++++++++++++++++++++--
> >  libsepol/cil/src/cil_internal.h    |  1 +
> >  libsepol/cil/src/cil_resolve_ast.c |  4 ++--
> >  libsepol/cil/src/cil_verify.c      | 19 ++++++++++++++-----
> >  libsepol/cil/src/cil_verify.h      |  2 +-
> >  libsepol/src/libsepol.map.in       |  1 +
> >  8 files changed, 48 insertions(+), 10 deletions(-)
> >
> [...]
> > diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c
> > index 59397f70..9cb1a6f6 100644
> > --- a/libsepol/cil/src/cil_verify.c
> > +++ b/libsepol/cil/src/cil_verify.c
> > @@ -92,7 +92,7 @@ static int __cil_is_reserved_name(const char *name, enum cil_flavor flavor)
> >         return CIL_FALSE;
> >  }
> >
> > -int cil_verify_name(const char *name, enum cil_flavor flavor)
> > +int cil_verify_name(struct cil_db *db, const char *name, enum cil_flavor flavor)
> >  {
> >         int rc = SEPOL_ERR;
> >         int len;
> > @@ -116,10 +116,19 @@ int cil_verify_name(const char *name, enum cil_flavor flavor)
> >                         goto exit;
> >         }
> >
> > -       for (i = 1; i < len; i++) {
> > -               if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-') {
> > -                       cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
> > -                       goto exit;
> > +       if (db->qualified_names == CIL_FALSE) {
> > +               for (i = 1; i < len; i++) {
> > +                       if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-') {
> > +                               cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
> > +                               goto exit;
> > +                       }
> > +               }
> > +       } else {
> > +               for (i = 1; i < len; i++) {
> > +                       if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-' && name[i] != '.') {
> > +                               cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
> > +                               goto exit;
> > +                       }
> >                 }
> >         }
>
> As cil_verify_name does not modify db (and would be seen as a function
> which does not modify anything and only checks some rules), it would
> be better to add a const qualifier:
>
>     int cil_verify_name(const struct cil_db *db, const char *name,
> enum cil_flavor flavor)
>

Good suggestion.

> Other than that, the documentation of the new option,
> "--qualified-names  Use qualified names." make me feel like the
> wording can be improved. More precisely, a commit message contains
> "Using qualified names means that declaration names can have dots in
> them" and this definition should also be in the documentation. So I am
> suggesting to replace the documentation with:
>
>     Allow names containing dots (qualified names). Blocks,
> blockinherits, blockabstracts, and in-statements will not be allowed.
>

Sounds clearer.

> Other than that, when I tested "secil2tree -A resolve -o test.cil
> secilc/test/policy.cil && secilc -Q test.cil" I encountered other
> errors, which are not related to these patches. When modifying the
> resulting "test.cil" to make it compile (by removing blocks and
> renaming some objects), option -Q worked fine. It would be nice if
> such a command was integrated in secilc's tests, in order to prevent
> future regressions. Nevertheless such work can be done once this
> series is merged.
>

I might not have been clear. This patch doesn't allow you to compile
after using "-A resolve". I will be working on that next.
I am thinking of creating a generic block "<block>", so blocks that
are resolved can be re-written using that.

So when writing out the AST for the resolve phase,

(block b
  (type t)
  (allow t self (CLASS (PERM)))
)

would be written as

;block b
(<block>
  (type b.t)
  (allow b.t self (CLASS (PERM)))
)

or, maybe I would also create a comment statement that wouldn't be
discarded by the parser

(<comment> "block b")
(<block>
  (type b.t)
  (allow b.t self (CLASS (PERM)))
)

I need to do similar things to blockinherits, blockabstracts, and
in-statements. I also would like to be able add a comment when an
optional is disabled.

I am open to suggestions.

Thanks for the review, I will work on a v2.
Jim


> Cheers,
> Nicolas
>

Re: [PATCH 1/4] libsepol/cil: Provide option to allow qualified names in declarations

[Nicolas Iooss]

On Mon, Jun 28, 2021 at 10:48 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Sat, Jun 26, 2021 at 5:46 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> >
> > On Thu, Jun 24, 2021 at 9:59 PM James Carter <jwcart2@gmail.com> wrote:
> > >
> > > Qualified names have "dots" in them. They are generated when a CIL
> > > policy is compiled and come from declarations in blocks. If a kernel
> > > policy is decompiled into a CIL policy, the resulting policy could
> > > have decarations that use qualified names. Compiling this policy would
> >
> > Misspelling: decarations -> declarations
> >
>
> Thanks, I'll fix that in v2.
>
> > > result in an error because "dots" in declarations are not allowed.
> > >
> > > Qualified names in a policy are normally used to refer to the name of
> > > identifiers, blocks, macros, or optionals that are declared in a
> > > different block (that is not a parent). Name resolution is based on
> > > splitting a name based on the "dots", searching the parents up to the
> > > global namespace for the first block using the first part of the name,
> > > using the second part of the name to lookup the next block using the
> > > first block's symbol tables, looking up the third block in the second's
> > > symbol tables, and so on.
> > >
> > > To allow the option of using qualified names in declarations:
> > >
> > > 1) Create a field in the struct cil_db called "qualified_names" which
> > > is set to CIL_TRUE when qualified names are to be used. This field is
> > > checked in cil_verify_name() and "dots" are allowed if qualified names
> > > are being allowed.
> > >
> > > 2) Only allow the direct lookup of the whole name in the global symbol
> > > table. This means that blocks, blockinherits, blockabstracts, and in-
> > > statements cannot be allowed. Use the "qualified_names" field of the
> > > cil_db to know when using one of these should result in an error.
> > >
> > > 3) Create the function cil_set_qualified_names() that is used to set
> > > the "qualified_names" field. Export the function in libsepol.
> > >
> > > Signed-off-by: James Carter <jwcart2@gmail.com>
> > > ---
> > >  libsepol/cil/include/cil/cil.h     |  1 +
> > >  libsepol/cil/src/cil.c             |  6 ++++++
> > >  libsepol/cil/src/cil_build_ast.c   | 24 ++++++++++++++++++++++--
> > >  libsepol/cil/src/cil_internal.h    |  1 +
> > >  libsepol/cil/src/cil_resolve_ast.c |  4 ++--
> > >  libsepol/cil/src/cil_verify.c      | 19 ++++++++++++++-----
> > >  libsepol/cil/src/cil_verify.h      |  2 +-
> > >  libsepol/src/libsepol.map.in       |  1 +
> > >  8 files changed, 48 insertions(+), 10 deletions(-)
> > >
> > [...]
> > > diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c
> > > index 59397f70..9cb1a6f6 100644
> > > --- a/libsepol/cil/src/cil_verify.c
> > > +++ b/libsepol/cil/src/cil_verify.c
> > > @@ -92,7 +92,7 @@ static int __cil_is_reserved_name(const char *name, enum cil_flavor flavor)
> > >         return CIL_FALSE;
> > >  }
> > >
> > > -int cil_verify_name(const char *name, enum cil_flavor flavor)
> > > +int cil_verify_name(struct cil_db *db, const char *name, enum cil_flavor flavor)
> > >  {
> > >         int rc = SEPOL_ERR;
> > >         int len;
> > > @@ -116,10 +116,19 @@ int cil_verify_name(const char *name, enum cil_flavor flavor)
> > >                         goto exit;
> > >         }
> > >
> > > -       for (i = 1; i < len; i++) {
> > > -               if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-') {
> > > -                       cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
> > > -                       goto exit;
> > > +       if (db->qualified_names == CIL_FALSE) {
> > > +               for (i = 1; i < len; i++) {
> > > +                       if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-') {
> > > +                               cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
> > > +                               goto exit;
> > > +                       }
> > > +               }
> > > +       } else {
> > > +               for (i = 1; i < len; i++) {
> > > +                       if (!isalnum(name[i]) && name[i] != '_' && name[i] != '-' && name[i] != '.') {
> > > +                               cil_log(CIL_ERR, "Invalid character \"%c\" in %s\n", name[i], name);
> > > +                               goto exit;
> > > +                       }
> > >                 }
> > >         }
> >
> > As cil_verify_name does not modify db (and would be seen as a function
> > which does not modify anything and only checks some rules), it would
> > be better to add a const qualifier:
> >
> >     int cil_verify_name(const struct cil_db *db, const char *name,
> > enum cil_flavor flavor)
> >
>
> Good suggestion.
>
> > Other than that, the documentation of the new option,
> > "--qualified-names  Use qualified names." make me feel like the
> > wording can be improved. More precisely, a commit message contains
> > "Using qualified names means that declaration names can have dots in
> > them" and this definition should also be in the documentation. So I am
> > suggesting to replace the documentation with:
> >
> >     Allow names containing dots (qualified names). Blocks,
> > blockinherits, blockabstracts, and in-statements will not be allowed.
> >
>
> Sounds clearer.
>
> > Other than that, when I tested "secil2tree -A resolve -o test.cil
> > secilc/test/policy.cil && secilc -Q test.cil" I encountered other
> > errors, which are not related to these patches. When modifying the
> > resulting "test.cil" to make it compile (by removing blocks and
> > renaming some objects), option -Q worked fine. It would be nice if
> > such a command was integrated in secilc's tests, in order to prevent
> > future regressions. Nevertheless such work can be done once this
> > series is merged.
> >
>
> I might not have been clear. This patch doesn't allow you to compile
> after using "-A resolve". I will be working on that next.
> I am thinking of creating a generic block "<block>", so blocks that
> are resolved can be re-written using that.
>
> So when writing out the AST for the resolve phase,
>
> (block b
>   (type t)
>   (allow t self (CLASS (PERM)))
> )
>
> would be written as
>
> ;block b
> (<block>
>   (type b.t)
>   (allow b.t self (CLASS (PERM)))
> )
>
> or, maybe I would also create a comment statement that wouldn't be
> discarded by the parser
>
> (<comment> "block b")
> (<block>
>   (type b.t)
>   (allow b.t self (CLASS (PERM)))
> )
>
> I need to do similar things to blockinherits, blockabstracts, and
> in-statements. I also would like to be able add a comment when an
> optional is disabled.
>
> I am open to suggestions.

These suggestions look good to me. Actually I even began writing a
small patch to show resolved blockinherit statements as "( ;
blockinherit b", because otherwise the parentheses are wrong in the
output of "secil2tree -A resolve secilc/test/policy.cil".

About introducing a new comment statement, it sounds like a whole new
feature just designed to make a niche tool work in a simple way. In my
humble opinion I prefer seeing regular comments in secil2tree dumps,
which has the advantage of being an existing syntax, being easy to
read, etc. But this is what I feel and of course if you prefer the way
you suggest or if someone else has a different preference, it would be
all right. And I might have missed the whole point of introducing
comments which are not discarded by CIL parsers and what advantage
this approach gives.

Cheers,
Nicolas