Exec fallback targets need a transition table entry to hold an ordered list of names rather than one name, but process_strs_entry() decides that per table, and the kernel has to load policy of both vintages.
Detect per entry instead. Also, now correctly refuse to load an empty target (e.g Px -> ""). Add KUnit coverage. Signed-off-by: Maxime Bélair <[email protected]> --- security/apparmor/include/policy_unpack.h | 1 + security/apparmor/policy_unpack.c | 60 ++++++---- security/apparmor/policy_unpack_test.c | 131 ++++++++++++++++++++++ 3 files changed, 171 insertions(+), 21 deletions(-) diff --git a/security/apparmor/include/policy_unpack.h b/security/apparmor/include/policy_unpack.h index c01f6885dbe3..d23d2bdaa4c8 100644 --- a/security/apparmor/include/policy_unpack.h +++ b/security/apparmor/include/policy_unpack.h @@ -212,6 +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); #endif #endif /* __POLICY_INTERFACE_H */ diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index f1fc48e72d0e..883353150431 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -467,23 +467,38 @@ static struct aa_dfa *unpack_dfa(struct aa_ext *e, int flags) return dfa; } -static int process_strs_entry(char *str, int size, bool multi) +/** + * 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 + * + * 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: + * policy predating exec fallback targets is singly terminated, policy + * after it is doubly terminated throughout, and both have to load. + * + * A ':' namespace separator is stored as a \0 and rejoined here, so it does + * not separate two names. + * + * 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) { + char *save = str; + char *pos = str; + char *end; + bool list; int c = 1; if (size <= 0) return -1; - if (multi) { - if (size < 2) - return -2; - /* multi ends with double \0 */ - if (str[size - 2]) - return -3; - } + /* a list iff doubly \0 terminated */ + list = size >= 2 && !str[size - 2]; + if (multi && !list) + return size < 2 ? -2 : -3; - char *save = str; - char *pos = str; - char *end = multi ? str + size - 2 : str + size - 1; + end = list ? str + size - 2 : str + size - 1; /* count # of internal \0 */ while (str < end) { if (str == pos) { @@ -496,10 +511,8 @@ static int process_strs_entry(char *str, int size, bool multi) } if (isspace(*str)) return -5; - if (*str == ':') { - /* :ns_str\0str\0 - * first character after : must be valid - */ + /* :ns\0name and &stack both need a name after the sigil */ + if (*str == ':' || *str == '&') { if (!str[1]) return -6; } @@ -513,8 +526,18 @@ static int process_strs_entry(char *str, int size, bool multi) str++; } /* while */ + if (!list && c > 1) + /* embedded \0 without the terminator that makes it a list */ + return -7; + /* nothing but terminators, or a list ending in an empty name. A + * rejoined trailing ':' is neither - ":ns:" names a ns default. + */ + if (pos == end && !(pos > save && pos[-1] == ':')) + return -8; + return c; } +EXPORT_SYMBOL_IF_KUNIT(aa_process_strs_entry); /** * unpack_strs_table - unpack a profile transition table @@ -558,18 +581,13 @@ 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 = process_strs_entry(str, size2, multi); + c = aa_process_strs_entry(str, size2, multi); if (c <= 0) { AA_DEBUG(DEBUG_UNPACK, "process_strs %d i %d pos %ld", c, i, (unsigned long)(e->pos - saved_pos)); goto fail; } - if (!multi && c > 1) { - AA_DEBUG(DEBUG_UNPACK, "!multi && c > 1"); - /* fail - all other cases with embedded \0 */ - goto fail; - } table[i].strs = str; table[i].count = c; table[i].size = size2; diff --git a/security/apparmor/policy_unpack_test.c b/security/apparmor/policy_unpack_test.c index cf18744dafe2..237b77104388 100644 --- a/security/apparmor/policy_unpack_test.c +++ b/security/apparmor/policy_unpack_test.c @@ -570,6 +570,132 @@ static void policy_unpack_test_unpack_X_out_of_bounds(struct kunit *test) KUNIT_EXPECT_FALSE(test, success); } +/* aa_process_strs_entry() rewrites its argument, so copy per case */ +static char *strs_entry_dup(struct kunit *test, const char *bytes, int size) +{ + char *buf = kunit_kmalloc(test, size, GFP_KERNEL); + + KUNIT_ASSERT_NOT_NULL(test, buf); + memcpy(buf, bytes, size); + + return buf; +} + +static int strs_entry(struct kunit *test, const char *bytes, int size, + bool multi) +{ + return aa_process_strs_entry(strs_entry_dup(test, bytes, size), size, + multi); +} + +/* 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)) + +/* singly terminated: one name, the shape of every cache on disk */ +static void policy_unpack_test_strs_entry_single(struct kunit *test) +{ + KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, "p", false), 1); + KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, "/t//b", false), 1); + /* an embedded \0 without the terminator that would make it a list */ + KUNIT_EXPECT_LT(test, STRS_ENTRY(test, "p\0q", false), 0); +} + +/* doubly terminated entries are lists, however many names they hold */ +static void policy_unpack_test_strs_entry_list(struct kunit *test) +{ + KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, "p\0", false), 1); + KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, "helper\0", false), 1); + KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, "p\0q\0", false), 2); + KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, "p\0q\0r\0", false), 3); + KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, "/t//b\0/t//q\0", false), 2); + /* a '&' element is an element like any other to the unpacker */ + KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, "&foo\0bar\0", false), 2); +} + +/* a ':' separator is stored as \0 and rejoined; it does not split names */ +static void policy_unpack_test_strs_entry_ns_rejoin(struct kunit *test) +{ + char *buf; + + buf = strs_entry_dup(test, ":ns\0p", sizeof(":ns\0p")); + KUNIT_EXPECT_EQ(test, + aa_process_strs_entry(buf, sizeof(":ns\0p"), false), 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), + 2); + KUNIT_EXPECT_STREQ(test, buf, ":ns:p"); + KUNIT_EXPECT_STREQ(test, buf + sizeof(":ns:p"), "q"); + + /* ":ns:" names a namespace's default profile. Its name does not end + * up empty, so the entry is not a list with a trailing empty name. + */ + 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); + KUNIT_EXPECT_STREQ(test, buf, ":ns:"); + + /* the singly terminated spelling of that same target cannot be told + * apart from a one name list, so it reads as ":ns" - which names the + * same thing, the namespace's default profile + */ + buf = strs_entry_dup(test, ":ns\0", sizeof(":ns\0")); + KUNIT_EXPECT_EQ(test, + aa_process_strs_entry(buf, sizeof(":ns\0"), false), 1); + KUNIT_EXPECT_STREQ(test, buf, ":ns"); + + /* a namespaced name that is not the first, which is what + * "Px -> :ns:p fallback=(:ns:q)" emits + */ + buf = strs_entry_dup(test, ":ns\0p\0:ns\0q\0", + sizeof(":ns\0p\0:ns\0q\0")); + KUNIT_EXPECT_EQ(test, + aa_process_strs_entry(buf, sizeof(":ns\0p\0:ns\0q\0"), + false), + 2); + KUNIT_EXPECT_STREQ(test, buf, ":ns:p"); + KUNIT_EXPECT_STREQ(test, buf + sizeof(":ns:p"), ":ns:q"); +} + +/* Exact codes: a bare "< 0" is satisfied by whichever check fires first + * and would not notice a rejection rule going missing. + */ +static void policy_unpack_test_strs_entry_malformed(struct kunit *test) +{ + /* nothing but terminators, in either vintage */ + KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, "", false), -8); + KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, "\0", false), -8); + /* an empty name, leading, interior and trailing */ + KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, "\0q\0", false), -4); + KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, "p\0\0q\0", false), -4); + KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, "p\0\0", false), -8); + /* a name may not start with whitespace, the first one included */ + KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, " p\0", false), -5); + KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, "p\0 q\0", false), -5); + /* a sigil with no name after it */ + KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, ":\0p\0", false), -6); + KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, "p\0:\0", false), -6); + KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, "&\0", false), -6); + 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); +} + +/* the tags table requires a list per entry; only trans detects per entry */ +static void policy_unpack_test_strs_entry_multi(struct kunit *test) +{ + KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, "p\0", true), 1); + KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, "p\0q\0", true), 2); + KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, "p", true), -3); + KUNIT_EXPECT_EQ(test, STRS_ENTRY(test, "p\0q", true), -3); +} + 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), @@ -601,6 +727,11 @@ static struct kunit_case apparmor_policy_unpack_test_cases[] = { KUNIT_CASE(policy_unpack_test_unpack_X_code_match), KUNIT_CASE(policy_unpack_test_unpack_X_code_mismatch), KUNIT_CASE(policy_unpack_test_unpack_X_out_of_bounds), + KUNIT_CASE(policy_unpack_test_strs_entry_single), + KUNIT_CASE(policy_unpack_test_strs_entry_list), + 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), {}, }; -- 2.51.0
