Add checks for template/interface mis-declarations

Interfaces in the refpolicy should not:
  - declare anything (no side effects)
  - use prefix parameters

Add one check to find interfaces that should be declared as a template
and one check to find templates that can be declared as an interface.

Refpolicy findings:

qemu.if:            112: (S): Template qemu_role might be declared as an interface (S-012)
wm.if:              142: (S): Interface wm_dbus_chat should be a template, due to parameter 0 (S-011)
wm.if:              250: (S): Interface wm_write_pipes should be a template, due to parameter 0 (S-011)
gnome.if:           673: (S): Interface gnome_dbus_chat_gkeyringd should be a template, due to parameter 0 (S-011)
gnome.if:           741: (S): Interface gnome_stream_connect_gkeyringd should be a template, due to parameter 0 (S-011)
userdomain.if:     1431: (S): Template userdom_security_admin_template might be declared as an interface (S-012)
kismet.if:           18: (S): Template kismet_role might be declared as an interface (S-012)
dbus.if:            193: (S): Interface dbus_connect_spec_session_bus should be a template, due to parameter 0 (S-011)
dbus.if:            245: (S): Interface dbus_spec_session_bus_client should be a template, due to parameter 0 (S-011)
dbus.if:            298: (S): Interface dbus_send_spec_session_bus should be a template, due to parameter 0 (S-011)
dbus.if:            436: (S): Interface dbus_spec_session_domain should be a template, due to parameter 0 (S-011)
rlogin.if:           32: (S): Template rlogin_read_home_content might be declared as an interface (S-012)
git.if:              18: (S): Template git_role might be declared as an interface (S-012)
Found the following issue counts:
S-011: 8
S-012: 5

Closes: #205

Author: cgzones

README

@@ -168,6 +168,8 @@ CHECK IDS
 	S-008: Unquoted gen_require block
 	S-009: Permission macro suffix does not match class name
 	S-010: Permission macro usage suggested
+	S-011: Interface should be decalred as template
+	S-012: Template can be declared as interface
 
 	W-001: Type or attribute referenced without explicit declaration
 	W-002: Type, attribute or role used but not listed in require block in interface

src/check_hooks.h

@@ -41,6 +41,8 @@ enum style_ids {
 	S_ID_UNQUOTE_GENREQ = 8,
 	S_ID_PERM_SUFFIX    = 9,
 	S_ID_PERMMACRO      = 10,
+	S_ID_TEXT_IF_PARAM  = 11,
+	S_ID_VOID_TEMP_DECL = 12,
 	S_END
 };
 

src/if_checks.c

@@ -232,6 +232,80 @@ struct check_result *check_unquoted_gen_require_block(__attribute__((unused)) co
 	return NULL;
 }
 
+struct check_result *check_text_param_in_interface(__attribute__((unused)) const struct
+                                                   check_data *data,
+                                                   const struct
+                                                   policy_node *node)
+{
+	const char *if_name = node->data.str;
+	const struct interface_trait *trait = look_up_in_if_traits_map(if_name);
+	if (!trait || !trait->is_inferred) {
+		return NULL;
+	}
+
+	for (int i = 0; i < TRAIT_MAX_PARAMETERS; i++) {
+		if (trait->parameters[i] == PARAM_TEXT) {
+			return make_check_result('S', S_ID_TEXT_IF_PARAM,
+						 "Interface %s should be a template, due to parameter %d",
+						 if_name,
+						 i);
+		}
+
+		if (trait->parameters[i] == PARAM_INITIAL) {
+			break;
+		}
+	}
+
+	return NULL;
+}
+
+struct check_result *check_unnecessary_template_definition(__attribute__((unused)) const struct
+                                                           check_data *data,
+                                                           const struct
+                                                           policy_node *node)
+{
+	const char *temp_name = node->data.str;
+	const struct interface_trait *trait = look_up_in_if_traits_map(temp_name);
+	if (!trait || !trait->is_inferred) {
+		return NULL;
+	}
+
+	for (int i = 0; i < TRAIT_MAX_PARAMETERS; i++) {
+		if (trait->parameters[i] == PARAM_TEXT) {
+			return NULL;
+		}
+
+		if (trait->parameters[i] == PARAM_INITIAL) {
+			break;
+		}
+	}
+
+	for (const struct policy_node *cur = node->first_child; cur; cur = dfs_next(cur)) {
+		if (cur == node || cur == node->next) {
+			break;
+		}
+
+		if (cur->flavor == NODE_GEN_REQ || cur->flavor == NODE_REQUIRE) {
+			cur = cur->next;
+		}
+
+		if (cur->flavor == NODE_DECL) {
+			return NULL;
+		}
+
+		if (cur->flavor == NODE_IF_CALL) {
+			const struct if_call_data *ic_data = cur->data.ic_data;
+			if (look_up_in_template_map(ic_data->name)) {
+				return NULL;
+			}
+		}
+	}
+
+	return make_check_result('S', S_ID_VOID_TEMP_DECL,
+				 "Template %s might be declared as an interface",
+				 temp_name);
+}
+
 struct check_result *check_name_used_but_not_required_in_if(const struct
                                                             check_data *data,
                                                             const struct

src/if_checks.h

@@ -81,6 +81,30 @@ struct check_result *check_unquoted_gen_require_block(const struct
                                                       const struct
                                                       policy_node *node);
 
+/*********************************************
+* Check that an interface has no text parameter
+* Called on NODE_INTERFACE_DEF nodes
+* data - metadata about the file
+* node - the node to check
+* returns NULL if passed or check_result for issue S-011
+*********************************************/
+struct check_result *check_text_param_in_interface(const struct
+                                                   check_data *data,
+                                                   const struct
+                                                   policy_node *node);
+
+/*********************************************
+* Check for templates that can be declared as interface
+* Called on NODE_TEMP_DEF nodes
+* data - metadata about the file
+* node - the node to check
+* returns NULL if passed or check_result for issue S-012
+*********************************************/
+struct check_result *check_unnecessary_template_definition(const struct
+                                                           check_data *data,
+                                                           const struct
+                                                           policy_node *node);
+
 /*********************************************
 * Check that all names referenced in interface are listed in its require block
 * (or declared in that template)

src/runner.c

@@ -189,6 +189,14 @@ struct checks *register_checks(char level,
 			add_check(NODE_AV_RULE, ck, "S-010",
 			          check_perm_macro_available);
 		}
+		if (CHECK_ENABLED("S-011")) {
+			add_check(NODE_INTERFACE_DEF, ck, "S-011",
+			          check_text_param_in_interface);
+		}
+		if (CHECK_ENABLED("S-012")) {
+			add_check(NODE_TEMP_DEF, ck, "S-012",
+			          check_unnecessary_template_definition);
+		}
 		// FALLTHRU
 	case 'W':
 		if (CHECK_ENABLED("W-001")) {

tests/Makefile.am

@@ -157,6 +157,8 @@ FUNCTIONAL_TEST_FILES=functional/end-to-end.bats \
 			functional/policies/check_triggers/s08.if \
 			functional/policies/check_triggers/s09.pass.te \
 			functional/policies/check_triggers/s09.warn.te \
+			functional/policies/check_triggers/s11.if \
+			functional/policies/check_triggers/s12.if \
 			functional/policies/check_triggers/w01_other.te \
 			functional/policies/check_triggers/w01.te \
 			functional/policies/check_triggers/w02.if \

tests/functional/end-to-end.bats

@@ -188,6 +188,14 @@ test_parse_error_impl() {
 	test_one_check_expect "S-009" "s09.warn.te" 6
 }
 
+@test "S-011" {
+	test_one_check "S-011" "s11.if"
+}
+
+@test "S-012" {
+	test_one_check "S-012" "s12.if"
+}
+
 @test "W-001" {
 	test_one_check_expect "W-001" "w01*" 5
 }

tests/functional/policies/check_triggers/s11.if

@@ -0,0 +1,20 @@
+interface(`foo1', `
+	gen_require(`
+		type foo_t;
+	')
+
+	allow $1 foo_t:file read;
+')
+
+interface(`foo2', `
+	gen_require(`
+		type foo_t;
+	')
+
+	allow $1_t foo_t:file read;
+')
+
+interface(`foo3', `
+	type foo_t;
+	allow $1 foo_t:file read;
+')

tests/functional/policies/check_triggers/s12.if

@@ -0,0 +1,20 @@
+template(`foo1', `
+	gen_require(`
+		type foo_t;
+	')
+
+	allow $1 foo_t:file read;
+')
+
+template(`foo2', `
+	gen_require(`
+		type foo_t;
+	')
+
+	allow $1_t foo_t:file read;
+')
+
+template(`foo3', `
+	type foo_t;
+	allow $1 foo_t:file read;
+')