Fix: make sure overlapping safe and unsafe exec rules conflict

BugLink: https://launchpad.net/bugs/1588069

Currently

  change_profile /** -> A,
  change_profile unsafe /** -> A,

do not conflict because the safe rules only set the change_profile
permission where the unsafe set unsafe exec. To fix this we have the
safe version set exec bits as well with out setting unsafe exec.
This allows the exec conflict logic to detect any conflicts.

This is safe to do even for older kernels as the exec bits off of the
2nd term encoding in the change_onexec rules are unused.

Test files
  tst/simple_tests/change_profile/onx_no_conflict_safe1.sd
  tst/simple_tests/change_profile/onx_no_conflict_safe2.sd
by Christian Boltz <apparmor@cboltz.de>

Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Tyler Hicks <tyhicks@canonical.com>

parser/parser_yacc.y

@@ -1486,15 +1486,16 @@ change_profile: TOK_CHANGE_PROFILE opt_exec_mode opt_id opt_named_transition TOK
 		char *exec = $3;
 		char *target = $4;
 
-		if (exec_mode != EXEC_MODE_EMPTY) {
-			if (!exec)
-				yyerror(_("Exec condition is required when unsafe or safe keywords are present"));
-
-			if (exec_mode == EXEC_MODE_UNSAFE) {
-				mode |= (AA_EXEC_BITS | ALL_AA_EXEC_UNSAFE);
-			} else if (exec_mode == EXEC_MODE_SAFE &&
-				   !kernel_supports_stacking &&
-				   warnflags & WARN_RULE_DOWNGRADED) {
+		if (exec) {
+			/* exec bits required to trigger rule conflict if
+			 * for overlapping safe and unsafe exec rules
+			 */
+			mode |= AA_EXEC_BITS;
+			if (exec_mode == EXEC_MODE_UNSAFE)
+				mode |= ALL_AA_EXEC_UNSAFE;
+			else if (exec_mode == EXEC_MODE_SAFE &&
+				 !kernel_supports_stacking &&
+				 warnflags & WARN_RULE_DOWNGRADED) {
 				pwarn("downgrading change_profile safe rule to unsafe due to lack of necessary kernel support\n");
 				/**
 				 * No need to do anything because 'unsafe' exec
@@ -1502,8 +1503,8 @@ change_profile: TOK_CHANGE_PROFILE opt_exec_mode opt_id opt_named_transition TOK
 				 * change_profile rules in non-stacking kernels
 				 */
 			}
-		}
-
+		} else if (exec_mode != EXEC_MODE_EMPTY)
+			yyerror(_("Exec condition is required when unsafe or safe keywords are present"));
 		if (exec && !(exec[0] == '/' || strncmp(exec, "@{", 2) == 0))
 			yyerror(_("Exec condition must begin with '/'."));
 

parser/tst/equality.sh

@@ -460,10 +460,24 @@ verify_binary_equality "Deny of ungranted perm" \
 
 verify_binary_equality "change_profile == change_profile -> **" \
 		       "/t { change_profile, }" \
-		       "/t { change_profile -> **, }" \
+		       "/t { change_profile -> **, }"
+
+verify_binary_equality "change_profile /** == change_profile /** -> **" \
 		       "/t { change_profile /**, }" \
 		       "/t { change_profile /** -> **, }"
 
+verify_binary_equality "change_profile /** == change_profile /** -> **" \
+		       "/t { change_profile unsafe /**, }" \
+		       "/t { change_profile unsafe /** -> **, }"
+
+verify_binary_equality "change_profile /** == change_profile /** -> **" \
+		       "/t { change_profile /**, }" \
+		       "/t { change_profile safe /** -> **, }"
+
+verify_binary_inequality "change_profile /** == change_profile /** -> **" \
+			 "/t { change_profile /**, }" \
+			 "/t { change_profile unsafe /**, }"
+
 verify_binary_equality "profile name is hname in rule" \
 	":ns:/hname { signal peer=/hname, }" \
 	":ns:/hname { signal peer=@{profile_name}, }"

parser/tst/simple_tests/change_profile/onx_conflict_unsafe1.sd

@@ -0,0 +1,8 @@
+#
+#=DESCRIPTION test for conflict safe and unsafe exec condition
+#=EXRESULT FAIL
+#
+/usr/bin/foo {
+   change_profile /onexec -> /bin/foo,
+   change_profile unsafe /onexec -> /bin/foo,
+}

parser/tst/simple_tests/change_profile/onx_conflict_unsafe2.sd

@@ -0,0 +1,8 @@
+#
+#=DESCRIPTION test for conflict safe and unsafe exec condition
+#=EXRESULT FAIL
+#
+/usr/bin/foo {
+   change_profile safe /onexec -> /bin/foo,
+   change_profile unsafe /onexec -> /bin/foo,
+}

parser/tst/simple_tests/change_profile/onx_no_conflict_safe1.sd

@@ -0,0 +1,8 @@
+#
+#=DESCRIPTION 'safe' and unspecified exec condition shouldn't conflict because 'safe' is the default
+#=EXRESULT PASS
+#
+/usr/bin/foo {
+   change_profile safe /onexec -> /bin/foo,
+   change_profile /onexec -> /bin/foo,
+}

parser/tst/simple_tests/change_profile/onx_no_conflict_safe2.sd

@@ -0,0 +1,8 @@
+#
+#=DESCRIPTION 'safe' and 'unsafe' for the same exec condition, but different exec targets
+#=EXRESULT PASS
+#
+/usr/bin/foo {
+   change_profile safe /onexec -> /bin/foo,
+   change_profile unsafe /onexec -> /bin/bar,
+}