MAC/do: sysctl_rules(): Set the requesting's thread's jail's rules

Allowing to change the rules specification on a jail other than the
requesting's thread one is a security issue, as it will immedia

MAC/do: sysctl_rules(): Set the requesting's thread's jail's rules

Allowing to change the rules specification on a jail other than the
requesting's thread one is a security issue, as it will immediately
apply to the jail we inherited from and all its other descendants that
inherit from it.

With this change, setting the 'mdo_rules' sysctl in a jail forces that
jail to no more inherit from its parent.

Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47601

show more ...

MAC/do: sysctl_rules(): Always copy the rules specification string

We are not guaranteed that the 'rules' storage stays stable if we don't
hold the prison lock. For this reason, always copy the spe

MAC/do: sysctl_rules(): Always copy the rules specification string

We are not guaranteed that the 'rules' storage stays stable if we don't
hold the prison lock. For this reason, always copy the specification
string (under the lock).

Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47600

show more ...

MAC/do: Remove PR_METHOD_REMOVE method

It isn't really needed, since common jail code destroys jail OSD storage
at jail destruction (via osd_jail_exit()), triggering our destructor
dealloc_osd(). L

MAC/do: Remove PR_METHOD_REMOVE method

It isn't really needed, since common jail code destroys jail OSD storage
at jail destruction (via osd_jail_exit()), triggering our destructor
dealloc_osd(). Leveraging this mechanism is arguably even better as it
causes deallocation to always happen without the 'allprison_lock' lock.

While here, make the static definition of 'methods' top-level, renaming
it to 'osd_methods'.

Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47599

show more ...

MAC/do: Allocate/deallocate rules as a whole

Stop recycling the top-level 'struct rules' already assigned to jails.
This considerably simplifies the code, as now changing rules on a jail
amounts to

MAC/do: Allocate/deallocate rules as a whole

Stop recycling the top-level 'struct rules' already assigned to jails.
This considerably simplifies the code, as now changing rules on a jail
amounts to just changing the OSD pointer.

Also, this is to increase potential concurrency in preparation for
incoming fixes about enforcing rules. Indeed, keeping these changes
relatively simple requires rules assigned to a jail to slightly outlive
resetting them, which is most easily done by just operating on pointers
to separate rules objects.

The (negligible) price to pay for this change is that setting rules on
a jail now systematically needs to allocate memory (and also that the
OSD slot needs to be accessed twice, once to get the old rules to free
them and another one to set the rules, which was already the case before
when memory had to be allocated).

Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47598

show more ...

MAC/do: Factor out setting/destroying rule structures

This generally removes duplicate code and clarifies higher-level
operations, allowing to fix several important bugs.

New (internal) functions:

MAC/do: Factor out setting/destroying rule structures

This generally removes duplicate code and clarifies higher-level
operations, allowing to fix several important bugs.

New (internal) functions:
- ensure_rules(): Ensure that a jail has a populated
'mac_do_osd_jail_slot', and returns the corresponding 'struct rules'.
- dealloc_rules(): Destroy the 'mac_do_osd_jail_slot' slot of a jail.
- set_rules(): Assign already parsed rules to a jail. Leverages
ensure_rules().
- parse_and_set_rules(): Combination of parse_rules() and set_rules().

Bugs fixed in mac_do_prison_set():
- A panic if "mdo" is explicitly passed to JAIL_SYS_NEW but "mdo.rules"
is absent, in which case 'rules_string' wasn't set (setting 'rules' at
this point would do nothing).
- In the JAIL_SYS_NEW case, would release the prison lock and reacquire
it, but still using the same 'rules' pointer that can have been freed
and changed concurrently, as the prison lock is temporary
unlocked. (This is generally a bug of the mac_do_alloc_prison()'s
interface when 'lrp' is not NULL.)

Suppress mac_do_alloc_prison(), as it has the following bugs:
- The interface bug mentioned just above.
- Wrong locking, leading to deadlocks in case of setting jail parameters
multiple times (concurrently or not).
It has been replaced by either parse_and_set_rules(), or by
ensure_rules() directly coupled with prison_unlock().

Rename mac_do_dealloc_prison(), the OSD destructor, to dealloc_osd(),
and make it free the 'struct rules' itself (which was leaking).

While here, in parse_rules(): Clarify the contract by adding comments,
and check (again) for the rules specification's length.

Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47597

show more ...

MAC/do: find_rules(): Clarify the contract

While here, rename an internal variable.

Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision:

MAC/do: find_rules(): Clarify the contract

While here, rename an internal variable.

Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47596

show more ...

MAC/do: Use prison_lock()/prison_unlock()

Instead of fiddling directly with 'pr_mtx'.

Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revisio

MAC/do: Use prison_lock()/prison_unlock()

Instead of fiddling directly with 'pr_mtx'.

Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47595

show more ...

MAC/do: Rename internal mac_do_rule_find() => find_rules()

To simplify, be consistent with the rename 'struct mac_do_rule' =>
'struct rules' and other functions, and because this function is
interna

MAC/do: Rename internal mac_do_rule_find() => find_rules()

To simplify, be consistent with the rename 'struct mac_do_rule' =>
'struct rules' and other functions, and because this function is
internal (and thus is never the first mac_do(4)'s function to appear in
a stack trace).

Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47594

show more ...

MAC/do: Rename private struct 'mac_do_rule' => 'rules'

To simplify and be consistent with 'struct rule'.

Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
D

MAC/do: Rename private struct 'mac_do_rule' => 'rules'

To simplify and be consistent with 'struct rule'.

Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47593

show more ...

MAC/do: Rename rule_is_valid() => rule_applies()

This function checks whether a rule applies in the current context,
i.e., if the subject's users/groups match that of the rule. By
contrast, it does

MAC/do: Rename rule_is_valid() => rule_applies()

This function checks whether a rule applies in the current context,
i.e., if the subject's users/groups match that of the rule. By
contrast, it doesn't check if the rule as specified by the user is valid
(i.e., consistent).

Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47592

show more ...

MAC/do: parse_rules(): Copy input string on its own

Since all callers have to do it, save them that burden and do it in
parse_rules() instead.

While here, replace "strlen(x) == 0" with the simpler

MAC/do: parse_rules(): Copy input string on its own

Since all callers have to do it, save them that burden and do it in
parse_rules() instead.

While here, replace "strlen(x) == 0" with the simpler and more efficient
"x[0] == '\0'".

Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47591

show more ...

MAC/do: Sort header inclusions

In accordance with style(9).

Reviewed by: bapt, emaste
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://review

MAC/do: Sort header inclusions

In accordance with style(9).

Reviewed by: bapt, emaste
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47771

show more ...

mac_do(4): Enhance GID rule validation to check all groups in cr_groups

Previously, the rule validation only checked the primary GID (cr_gid).
This caused issues when applying GID-based rules, as us

mac_do(4): Enhance GID rule validation to check all groups in cr_groups

Previously, the rule validation only checked the primary GID (cr_gid).
This caused issues when applying GID-based rules, as users with matching
secondary groups were not considered valid. This patch modifies both
functions to iterate through all groups in cr_groups to ensure all group
memberships are considered when validating GID-based rules.

For example, a user's primary group is staff (20) and they are also in
the wheel (0) group, this change allows the rule gid=0:any to enable
them to run commands as any user.

Reviewed by: delphij (earlier version), bapt
Differential Revision: https://reviews.freebsd.org/D47304

show more ...