mirror of
https://git.FreeBSD.org/src.git
synced 2026-06-02 11:24:32 +00:00
MAC/do: Fix double-free on parse error after "executable paths" feature
parse_rules() has been calling toast_rules() in case of a parse error in
order to deallocate the 'struct rule' objects it has constructed up to
that point.
toast_rules() would take a pointer to a full 'struct rules' object, and
besides freeing all 'struct rule' referenced by it, would also free the
holding 'struct rules' itself.
With the introduction of the "executable paths" feature, and the
embedding of 'struct rules' into 'struct conf', meaning that the
lifecycle for 'struct rules' was no longer independent, toast_rules()
was changed not to free the passed 'struct rules' (as it was a field of
a 'struct conf' object). Unfortunately, this change was not completed
with a reinitialization of the rules list head, so the 'struct conf'
object would continue to reference just-freed rules, which then would be
freed a second time on destruction of that container.
So, make toast_rules() re-initialize the rules list in 'struct rules',
which it logically has been having to do since not freeing the enclosing
'struct rules'. This alone is enough to fix the bug, but let's use the
occasion to change the contract of parse_rules() and bring its herald
comment up-to-date: On error, parse_rules() now simply leaves already
constructed 'struct rule' objects in 'conf'. The latter is eventually
destroyed and the rule objects reclaimed at that point.
Add a test trying to set an invalid rules configuration with the first
rule being valid and the second being invalid, which triggers the bug
(and an immediate panic() on an INVARIANTS kernel).
Reported by: impost0r(ret2plt) <impostor@ret2p.lt>
Reviewed by: markj
Fixes: 9818224174 ("MAC/do: Executable paths feature (GSoC 2025's final state)")
Sponsored by: The FreeBSD Foundation
This commit is contained in:
@@ -390,6 +390,7 @@ toast_rules(struct rules *const rules)
|
||||
free(rule->gids, M_MAC_DO);
|
||||
free(rule, M_MAC_DO);
|
||||
}
|
||||
STAILQ_INIT(head);
|
||||
}
|
||||
|
||||
static inline void
|
||||
@@ -1071,13 +1072,13 @@ einval:
|
||||
/*
|
||||
* Parse rules specification and produce rule structures out of it.
|
||||
*
|
||||
* Must be called with '*parse_error' set to NULL. Returns 0 on success, with
|
||||
* '*rulesp' made to point to a 'struct rule' representing the rules. On error,
|
||||
* the returned value is non-zero and '*rulesp' is unchanged. If 'string' has
|
||||
* length greater or equal to MAX_RULE_STRING_SIZE, ENAMETOOLONG is returned. If
|
||||
* it is not in the expected format, EINVAL is returned. If an error is
|
||||
* returned, '*parse_error' is set to point to a 'struct parse_error' giving an
|
||||
* error message for the problem.
|
||||
* Must be called with '*parse_error' set to NULL. Returns 0 on success,
|
||||
* filling the passed '*rules' with 'struct rule' objects. On error, the
|
||||
* returned value is non-zero, and '*rules' may have been changed. If 'string'
|
||||
* has length greater or equal to MAX_RULE_STRING_SIZE, ENAMETOOLONG is
|
||||
* returned. If it is not in the expected format, EINVAL is returned. If an
|
||||
* error is returned, '*parse_error' is set to point to a 'struct parse_error'
|
||||
* giving an error message for the problem.
|
||||
*
|
||||
* Expected format: A >-colon-separated list of rules of the form
|
||||
* "<from>><target>" (for backwards compatibility, a semi-colon ":" is accepted
|
||||
@@ -1123,7 +1124,6 @@ parse_rules(const char *const string, struct rules *const rules,
|
||||
error = parse_single_rule(rule, rules, parse_error);
|
||||
if (error != 0) {
|
||||
(*parse_error)->pos += rule - copy;
|
||||
toast_rules(rules);
|
||||
goto error;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -72,6 +72,19 @@ rules_wrong_separator_body()
|
||||
sysctl_set_and_check_fails_rules "uid=1001>gid=0:gid=1001>gid=5"
|
||||
}
|
||||
|
||||
# Added after observing a panic() in this situation because of a double-free
|
||||
# after introduction of "exec_paths".
|
||||
atf_test_case non_first_rule_unparseable
|
||||
non_first_rule_unparseable_head()
|
||||
{
|
||||
atf_set descr "Non-first rule wrong"
|
||||
}
|
||||
|
||||
non_first_rule_unparseable_body()
|
||||
{
|
||||
sysctl_set_and_check_fails_rules "gid=1001>uid=0;hello"
|
||||
}
|
||||
|
||||
|
||||
atf_init_test_cases()
|
||||
{
|
||||
@@ -83,4 +96,5 @@ atf_init_test_cases()
|
||||
atf_add_test_case rule_user_names_fail
|
||||
atf_add_test_case rule_group_names_fail
|
||||
atf_add_test_case rules_wrong_separator
|
||||
atf_add_test_case non_first_rule_unparseable
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user