[apparmor] [PATCH] Fix: make sure overlapping safe and unsafe exec rules conflict

[John Johansen]

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.

Signed-off-by: John Johansen <john.johansen at canonical.com>
---
 parser/parser_yacc.y                               | 23 +++++++++++-----------
 parser/tst/equality.sh                             | 14 +++++++++++++
 .../change_profile/onx_conflict_unsafe1.sd         |  8 ++++++++
 .../change_profile/onx_conflict_unsafe2.sd         |  8 ++++++++
 4 files changed, 42 insertions(+), 11 deletions(-)
 create mode 100644 parser/tst/simple_tests/change_profile/onx_conflict_unsafe1.sd
 create mode 100644 parser/tst/simple_tests/change_profile/onx_conflict_unsafe2.sd

diff --git a/parser/parser_yacc.y b/parser/parser_yacc.y
index b76634f..3e2bcd2 100644
--- a/parser/parser_yacc.y
+++ b/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 '/'."));
 
diff --git a/parser/tst/equality.sh b/parser/tst/equality.sh
index 25260ad..18bd286 100755
--- a/parser/tst/equality.sh
+++ b/parser/tst/equality.sh
@@ -461,9 +461,23 @@ verify_binary_equality "Deny of ungranted perm" \
 verify_binary_equality "change_profile == 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}, }"
diff --git a/parser/tst/simple_tests/change_profile/onx_conflict_unsafe1.sd b/parser/tst/simple_tests/change_profile/onx_conflict_unsafe1.sd
new file mode 100644
index 0000000..05854ae
--- /dev/null
+++ b/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,
+}
diff --git a/parser/tst/simple_tests/change_profile/onx_conflict_unsafe2.sd b/parser/tst/simple_tests/change_profile/onx_conflict_unsafe2.sd
new file mode 100644
index 0000000..a6c583e
--- /dev/null
+++ b/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,
+}
-- 
2.7.4