An exec rule whose target is not loaded denies. The only alternatives policy can name are the composite mode letters, ix and ux, and neither is a restricted answer.
/usr/bin/stg Px -> stg fallback=(stg-base, generic-helper), The parser appends the alternatives to the rule's transition table entry, so walk the entry and take the first target that resolves. Refuse an entry holding more than 32 names: every one of them is tried at exec and searches the namespace, and the parser's own limit is not one to rely on. Signed-off-by: Maxime Bélair <[email protected]> --- security/apparmor/domain.c | 47 +++++++++++++++++++---- security/apparmor/include/policy_unpack.h | 2 +- security/apparmor/policy_unpack.c | 23 ++++++++--- security/apparmor/policy_unpack_test.c | 31 ++++++++++----- 4 files changed, 80 insertions(+), 23 deletions(-) diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index 4276abe534ec..28bc82bebcd5 100644 --- a/security/apparmor/domain.c +++ b/security/apparmor/domain.c @@ -506,9 +506,26 @@ static struct aa_label *find_attach(const struct path *path, return &candidate->label; } -static const char *next_name(int xtype, const char *name) +/** + * next_name - step to the next name in a transition table entry + * @ent: the entry being walked (NOT NULL) + * @name: the name within @ent currently being looked at (NOT NULL) + * + * Stepping past the last name lands on an empty name in a list, and on + * the end of the entry in a lone name, so one function serves both. The + * bound is the end of the entry rather than ent->count, so that a count + * disagreeing with the bytes could not walk off it. + * + * Returns: the next name, or NULL if @name was the last + */ +static const char *next_name(struct aa_str_table_ent *ent, const char *name) { - return NULL; + const char *next = name + strlen(name) + 1; + + if (next >= ent->strs + ent->size || !*next) + return NULL; + + return next; } /* the profile list an exec transition attaches against */ @@ -568,7 +585,11 @@ static struct aa_label *x_resolve_elem(struct aa_profile *profile, base = find_attach(path, profile->ns, x_attach_list(profile, xindex), name, info); if (!base) { - *stack = target; + /* keep the first: the search fails alike for every element */ + if (*stack) + aa_put_label(target); + else + *stack = target; return ERR_PTR(-ENOENT); } new = aa_label_merge(base, target, GFP_KERNEL); @@ -590,6 +611,9 @@ static struct aa_label *x_resolve_elem(struct aa_profile *profile, * set only when NULL is returned * @info: info message if there was an error (NOT NULL) * + * An entry may name the rule's own target followed by its fallbacks, tried + * in order; the first that resolves wins. + * * Returns: refcounted label, NULL if the entry named no loaded profile, or * ERR_PTR if it could not be looked up */ @@ -602,19 +626,26 @@ static struct aa_label *x_table_lookup(struct aa_profile *profile, u32 xindex, { struct aa_ruleset *rules = profile->label.rules[0]; struct aa_label *label, *pending = NULL; - u32 xtype = xindex & AA_X_TYPE_MASK; int index = xindex & AA_X_INDEX_MASK; + struct aa_str_table_ent *ent; + const char *saved_info = *info; const char *next; AA_BUG(!lookupname); /* index is guaranteed to be in range, validated at load time */ /* TODO: move lookup parsing to unpack time so this is a straight - * index into the resultant label + * index into the resultant label. Only partly available now: + * an entry is an ordered list of candidates, and a '&' element + * needs an attachment base that is not known until exec. */ - for (next = rules->file->trans.table[index].strs; next; - next = next_name(xtype, next)) { - *lookupname = next; + ent = &rules->file->trans.table[index]; + /* report the target the rule names, not the last fallback tried */ + *lookupname = ent->strs; + + for (next = ent->strs; next; next = next_name(ent, next)) { + /* a stepped-over target did not decide this exec; no-op first pass */ + *info = saved_info; label = x_resolve_elem(profile, path, name, xindex, next, &pending, info); /* resolved, or failed for a reason that is not absence */ diff --git a/security/apparmor/include/policy_unpack.h b/security/apparmor/include/policy_unpack.h index d23d2bdaa4c8..d389d2af96d9 100644 --- a/security/apparmor/include/policy_unpack.h +++ b/security/apparmor/include/policy_unpack.h @@ -212,7 +212,7 @@ bool aa_unpack_array(struct aa_ext *e, const char *name, u16 *size); size_t aa_unpack_blob(struct aa_ext *e, char **blob, const char *name); int aa_unpack_str(struct aa_ext *e, const char **string, const char *name); int aa_unpack_strdup(struct aa_ext *e, char **string, const char *name); -int aa_process_strs_entry(char *str, int size, bool multi); +int aa_process_strs_entry(char *str, int size, bool multi, int max); #endif #endif /* __POLICY_INTERFACE_H */ diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index 883353150431..06b458a926d2 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -467,11 +467,19 @@ static struct aa_dfa *unpack_dfa(struct aa_ext *e, int flags) return dfa; } +/* Bound the names one transition table entry may hold: each is tried at + * exec and can search every profile in the namespace, and a 64KB entry + * holds thousands. The parser's own limit is not one to rely on - a blob + * can be written to .load from a user namespace. + */ +#define AA_MAX_TRANS_NAMES 32 + /** * aa_process_strs_entry - validate a str table entry and count its names * @str: entry to check, rewritten in place (NOT NULL unless @size <= 0) * @size: bytes in @str, including its terminator(s) * @multi: every entry in this table has to be a list + * @max: most names @str may hold, or 0 for no limit * * A singly \0 terminated entry holds one name, a doubly \0 terminated one a * \0 separated list. That is decided per entry rather than per table: @@ -483,7 +491,8 @@ static struct aa_dfa *unpack_dfa(struct aa_ext *e, int flags) * * Returns: number of names in @str, or < 0 if @str is malformed */ -VISIBLE_IF_KUNIT int aa_process_strs_entry(char *str, int size, bool multi) +VISIBLE_IF_KUNIT int aa_process_strs_entry(char *str, int size, bool multi, + int max) { char *save = str; char *pos = str; @@ -534,6 +543,8 @@ VISIBLE_IF_KUNIT int aa_process_strs_entry(char *str, int size, bool multi) */ if (pos == end && !(pos > save && pos[-1] == ':')) return -8; + if (max && c > max) + return -9; return c; } @@ -544,12 +555,13 @@ EXPORT_SYMBOL_IF_KUNIT(aa_process_strs_entry); * @e: serialized data extent information (NOT NULL) * @name: name of table (MAY BE NULL) * @multi: allow multiple strings on a single entry + * @max: most names one entry may hold, or 0 for no limit * @strs: str table to unpack to (NOT NULL) * * Returns: 0 if table successfully unpacked or not present, else error */ static int unpack_strs_table(struct aa_ext *e, const char *name, bool multi, - struct aa_str_table *strs) + int max, struct aa_str_table *strs) { void *saved_pos = e->pos; struct aa_str_table_ent *table = NULL; @@ -581,7 +593,7 @@ static int unpack_strs_table(struct aa_ext *e, const char *name, bool multi, /* aa_unpack_strdup verifies that the last character is * null termination byte. */ - c = aa_process_strs_entry(str, size2, multi); + c = aa_process_strs_entry(str, size2, multi, max); if (c <= 0) { AA_DEBUG(DEBUG_UNPACK, "process_strs %d i %d pos %ld", c, i, @@ -883,7 +895,7 @@ static int unpack_tags(struct aa_ext *e, struct aa_tags_struct *tags, *info = "invalid tags version"; goto fail_reset; } - error = unpack_strs_table(e, "strs", true, &tags->strs); + error = unpack_strs_table(e, "strs", true, 0, &tags->strs); if (error) { *info = "failed to unpack profile tag.strs"; goto fail; @@ -1084,7 +1096,8 @@ static int unpack_pdb(struct aa_ext *e, struct aa_policydb **policy, * transition table may be present even when the dfa is * not. For compatibility reasons unpack and discard. */ - error = unpack_strs_table(e, "xtable", false, &pdb->trans); + error = unpack_strs_table(e, "xtable", false, AA_MAX_TRANS_NAMES, + &pdb->trans); if (error && required_trans) { *info = "failed to unpack profile transition table"; goto fail; diff --git a/security/apparmor/policy_unpack_test.c b/security/apparmor/policy_unpack_test.c index 237b77104388..7df18534cc3b 100644 --- a/security/apparmor/policy_unpack_test.c +++ b/security/apparmor/policy_unpack_test.c @@ -582,17 +582,19 @@ static char *strs_entry_dup(struct kunit *test, const char *bytes, int size) } static int strs_entry(struct kunit *test, const char *bytes, int size, - bool multi) + bool multi, int max) { return aa_process_strs_entry(strs_entry_dup(test, bytes, size), size, - multi); + multi, max); } /* Separators are explicit so the literal's own terminator supplies the * last one and sizeof() is the wire size: "p" is p\0, "p\0" is p\0\0. */ #define STRS_ENTRY(test, lit, multi) \ - strs_entry((test), (lit), sizeof(lit), (multi)) + strs_entry((test), (lit), sizeof(lit), (multi), 0) +#define STRS_ENTRY_MAX(test, lit, max) \ + strs_entry((test), (lit), sizeof(lit), false, (max)) /* singly terminated: one name, the shape of every cache on disk */ static void policy_unpack_test_strs_entry_single(struct kunit *test) @@ -622,12 +624,12 @@ static void policy_unpack_test_strs_entry_ns_rejoin(struct kunit *test) buf = strs_entry_dup(test, ":ns\0p", sizeof(":ns\0p")); KUNIT_EXPECT_EQ(test, - aa_process_strs_entry(buf, sizeof(":ns\0p"), false), 1); + aa_process_strs_entry(buf, sizeof(":ns\0p"), false, 0), 1); KUNIT_EXPECT_STREQ(test, buf, ":ns:p"); buf = strs_entry_dup(test, ":ns\0p\0q\0", sizeof(":ns\0p\0q\0")); KUNIT_EXPECT_EQ(test, - aa_process_strs_entry(buf, sizeof(":ns\0p\0q\0"), false), + aa_process_strs_entry(buf, sizeof(":ns\0p\0q\0"), false, 0), 2); KUNIT_EXPECT_STREQ(test, buf, ":ns:p"); KUNIT_EXPECT_STREQ(test, buf + sizeof(":ns:p"), "q"); @@ -637,7 +639,7 @@ static void policy_unpack_test_strs_entry_ns_rejoin(struct kunit *test) */ buf = strs_entry_dup(test, ":ns\0\0", sizeof(":ns\0\0")); KUNIT_EXPECT_EQ(test, - aa_process_strs_entry(buf, sizeof(":ns\0\0"), false), 1); + aa_process_strs_entry(buf, sizeof(":ns\0\0"), false, 0), 1); KUNIT_EXPECT_STREQ(test, buf, ":ns:"); /* the singly terminated spelling of that same target cannot be told @@ -646,7 +648,7 @@ static void policy_unpack_test_strs_entry_ns_rejoin(struct kunit *test) */ buf = strs_entry_dup(test, ":ns\0", sizeof(":ns\0")); KUNIT_EXPECT_EQ(test, - aa_process_strs_entry(buf, sizeof(":ns\0"), false), 1); + aa_process_strs_entry(buf, sizeof(":ns\0"), false, 0), 1); KUNIT_EXPECT_STREQ(test, buf, ":ns"); /* a namespaced name that is not the first, which is what @@ -656,7 +658,7 @@ static void policy_unpack_test_strs_entry_ns_rejoin(struct kunit *test) sizeof(":ns\0p\0:ns\0q\0")); KUNIT_EXPECT_EQ(test, aa_process_strs_entry(buf, sizeof(":ns\0p\0:ns\0q\0"), - false), + false, 0), 2); KUNIT_EXPECT_STREQ(test, buf, ":ns:p"); KUNIT_EXPECT_STREQ(test, buf + sizeof(":ns:p"), ":ns:q"); @@ -684,7 +686,7 @@ static void policy_unpack_test_strs_entry_malformed(struct kunit *test) KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, "p\0&\0", false), -6); /* an embedded \0 in a single name */ KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, "p\0q", false), -7); - KUNIT_EXPECT_EQ(test, aa_process_strs_entry(NULL, 0, false), -1); + KUNIT_EXPECT_EQ(test, aa_process_strs_entry(NULL, 0, false, 0), -1); } /* the tags table requires a list per entry; only trans detects per entry */ @@ -696,6 +698,16 @@ static void policy_unpack_test_strs_entry_multi(struct kunit *test) KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, "p\0q", true), -3); } +/* the cap on names per entry, 0 meaning uncapped */ +static void policy_unpack_test_strs_entry_max(struct kunit *test) +{ + KUNIT_EXPECT_EQ(test, STRS_ENTRY_MAX(test, "p\0q\0r\0", 3), 3); + KUNIT_EXPECT_EQ(test, STRS_ENTRY_MAX(test, "p\0q\0r\0s\0", 3), -9); + KUNIT_EXPECT_EQ(test, STRS_ENTRY_MAX(test, "p\0q\0r\0s\0", 0), 4); + /* a rejoined ':' is not a separator, so it does not count */ + KUNIT_EXPECT_EQ(test, STRS_ENTRY_MAX(test, ":ns\0p\0q\0", 2), 2); +} + static struct kunit_case apparmor_policy_unpack_test_cases[] = { KUNIT_CASE(policy_unpack_test_inbounds_when_inbounds), KUNIT_CASE(policy_unpack_test_inbounds_when_out_of_bounds), @@ -732,6 +744,7 @@ static struct kunit_case apparmor_policy_unpack_test_cases[] = { KUNIT_CASE(policy_unpack_test_strs_entry_ns_rejoin), KUNIT_CASE(policy_unpack_test_strs_entry_malformed), KUNIT_CASE(policy_unpack_test_strs_entry_multi), + KUNIT_CASE(policy_unpack_test_strs_entry_max), {}, }; -- 2.51.0
