Merge parser: fix rule priority destroying rule permissions for some classes

io_uring and userns mediation are encoding permissions on the class
byte. This is a mistake that should never have been allowed.

With the addition of rule priorities the class byte mediates rule,
that ensure the kernel can determine a class is being mediated is
given the highest priority possible, to ensure class mediation can not
be removed by a deny rule. See
  61b7568e1 ("parser: bug fix mediates_X stub rules.")
for details.

Unfortunately this breaks rule classes that encode permissions on the
class byte, because those rules will always have a lower priority and
the class mediates rule will always be selected over them resulting in
only the class mediates permission being on the rule class state.

Fix this by adding the mediaties class rules for these rule classes
with the lowest priority possible. This means that any rule mediating
the class will wipe out the mediates class rule. So add a new mediates
class rule at the same priority, as the rule being added.

This is a naive implementation and does result in more mediates rules
being added than necessary. The rule class could keep track of the
highest priority rule that had been added, and use that to reduce the
number of mediates rules it adds for the class.

Technically we could also get away with not adding the rules for allow
rules, as the kernel doesn't actually check the encoded permission but
whether the class state is not the trap state. But it is required with
deny rules to ensure the deny rule doesn't result in permissions being
removed from the class, resulting in the kernel thinking it is
unmediated. We also want to ensure that mediation is encoded for other
rule types like prompt, and in the future the kernel could check the
permission so we do want to guarantee that the class state has the
MAY_READ permission on it.

Note: there is another set of classes (file, mqueue, dbus, ...) which
encodes a default rule permission as

  class .* <perm>

this encoding is unfortunate in that it will also add the permission
to the class byte, but also sets up following states with the permission.
thankfully, this accespt anything, including nothing generally isn't
valid in the nothing case (eg. a file without any absolute name). For
this set of classes, the high priority mediates rule just ensures
that the null match case does not have permission.

Fixes: 61b7568e1 parser: bug fix mediates_X stub rules.
Signed-off-by: John Johansen <john.johansen@canonical.com>

MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1307
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: John Johansen <john@jjmx.net>

parser/io_uring.cc

@@ -127,6 +127,13 @@ int io_uring_rule::gen_policy_re(Profile &prof)
 					audit == AUDIT_FORCE ? perms : 0,
 					parseopts))
 			goto fail;
+		/* add a mediates_io_uring rule for every rule added. It
+		 * needs to be the same priority
+		 */
+		if (!prof.policy.rules->add_rule(buf.c_str(), priority,
+					RULE_ALLOW, AA_MAY_READ, 0,
+					parseopts))
+			goto fail;
 
 		if (perms & AA_IO_URING_OVERRIDE_CREDS) {
 			buf = buffer.str(); /* update buf to have label */

parser/parser_regex.c

@@ -1097,6 +1097,17 @@ static const char *deny_file = ".*";
  */
 static int mediates_priority = INT_MAX;
 
+/* some rule types unfortunately encoded permissions on the class byte
+ * to fix the above bug, they need a different solution. The generic
+ * mediates rule will get encoded at the minimum priority, and then
+ * for every rule of those classes a mediates rule of the same priority
+ * will be added. This way the mediates rule never has higher priority,
+ * which would wipe out the rule permissions encoded on the class state,
+ * and it is guaranteed to have the same priority as the highest priority
+ * rule.
+ */
+static int perms_onclass_mediates_priority = INT_MIN;
+
 int process_profile_policydb(Profile *prof)
 {
 	int error = -1;
@@ -1112,7 +1123,7 @@ int process_profile_policydb(Profile *prof)
 	 * to be supported
 	 */
 	if (features_supports_userns &&
-	    !prof->policy.rules->add_rule(mediates_ns, mediates_priority, RULE_ALLOW, AA_MAY_READ, 0, parseopts))
+	    !prof->policy.rules->add_rule(mediates_ns, perms_onclass_mediates_priority, RULE_ALLOW, AA_MAY_READ, 0, parseopts))
 		goto out;
 
 	/* don't add mediated classes to unconfined profiles */
@@ -1148,7 +1159,7 @@ int process_profile_policydb(Profile *prof)
 		    !prof->policy.rules->add_rule(mediates_sysv_mqueue, mediates_priority, RULE_ALLOW, AA_MAY_READ, 0, parseopts))
 			goto out;
 		if (features_supports_io_uring &&
-		    !prof->policy.rules->add_rule(mediates_io_uring, mediates_priority, RULE_ALLOW, AA_MAY_READ, 0, parseopts))
+		    !prof->policy.rules->add_rule(mediates_io_uring, perms_onclass_mediates_priority, RULE_ALLOW, AA_MAY_READ, 0, parseopts))
 			goto out;
 	}
 

parser/userns.cc

@@ -99,6 +99,14 @@ int userns_rule::gen_policy_re(Profile &prof)
 					rule_mode, perms,
 					audit == AUDIT_FORCE ? perms : 0,
 					parseopts))
+
+			goto fail;
+		/* add a mediates_userns rule for every rule added. It
+		 * needs to be the same priority
+		 */
+		if (!prof.policy.rules->add_rule(buf.c_str(), priority,
+					RULE_ALLOW, AA_MAY_READ, 0,
+					parseopts))
 			goto fail;
 	}