From 204c0c5a3a34ac2eb47b863aae20bace48e0ad3c Mon Sep 17 00:00:00 2001 From: John Johansen Date: Thu, 15 Aug 2024 03:51:20 -0700 Subject: [PATCH] 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 .* 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 --- parser/io_uring.cc | 7 +++++++ parser/parser_regex.c | 15 +++++++++++++-- parser/userns.cc | 8 ++++++++ 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/parser/io_uring.cc b/parser/io_uring.cc index 17fa39614..60b8c2579 100644 --- a/parser/io_uring.cc +++ b/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 */ diff --git a/parser/parser_regex.c b/parser/parser_regex.c index 71126c5f9..7810458d5 100644 --- a/parser/parser_regex.c +++ b/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; } diff --git a/parser/userns.cc b/parser/userns.cc index c66ce062e..a2cd9e8eb 100644 --- a/parser/userns.cc +++ b/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; }