| Commit message (Collapse) | Author | Age | Files | Lines |
|---|
| |
|
|
|
|
|
| |
No functional change.
MFC after: 3 days
Sponsored by: The FreeBSD Foundation
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
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: 9818224174c4 ("MAC/do: Executable paths feature (GSoC 2025's final state)")
Sponsored by: The FreeBSD Foundation
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Update years for the Foundation.
While here, remove the initial '/*-' which has been useless for a long
time.
While here, add a missing space on bapt@'s copyright line (approved by
him).
Reviewed by: bapt
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
The kind of tolerance we apply to parsing rules, whose format we have
defined, cannot be applied to paths since blank characters are allowed
there.
There is still the limitation that no escape character is currently
supported, and so it is not possible to configure a path having a ':'
character.
Reviewed by: bapt
Fixes: 9818224174c4 ("MAC/do: Executable paths feature (GSoC 2025's final state)")
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
See the immediately preceding commit for explanations on what this is
fixing.
When setting 'mac.do' to 'inherit' on a jail with 'mac.do.rules' and
'mac.do.exec_paths' also specified in the same call, ensure that the
check that these passed parameters are the same as those to be inherited
is atomic with respect to enabling the inheritance (i.e., removing the
jail's 'struct conf' object). (See previous commit "MAC/do: Fix the
recent logic to set jail parameters, make it more tolerant" as for why
this check exists.)
Because we currently only modify a single configuration object per
transaction, we introduce the parse_and_commit_conf() wrapper around
parse_and_set_conf() to remove duplicated code that would ensue from
calling the latter directly, namely, releasing the 'mac_do_rwl' lock and
freeing the old configuration object (if any).
Taking the 'mac_do_rwl' lock for writing as a way to freeze all accesses
to mac_do(4) configurations was deemed too thin an operation to be worth
wrapping.
Reviewed by: bapt (older version)
Fixes: 9818224174c4 ("MAC/do: Executable paths feature (GSoC 2025's final state)")
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
As mentioned in previous commits "MAC/do: parse_and_set_conf(): Require
the model configuration" and "MAC/do: Sequential consistency for
configuration retrieval", the introduction of the "executable path"
feature, more fundamentally, the fact that there is now more than one
per-jail parameter and that parameters can be independently modified or
copied, causes an atomicity problem in case of concurrent accesses to of
a jail's applicable configuration.
Partially modifying a configuration is indeed akin to
a read-modify-write operation, where the read is either to the current
or an inherited configuration. More precisely, once pointed to by
a jail, a configuration object is immutable, and changing the jail's
configuration means making the jail point to another configuration
object. To change a jail's configuration, a new configuration object is
thus built, and if only some parameters have been explicitly specified,
those that have not been are set by copying the corresponding values
from an existing configuration object (in case of partial modification
of the existing configuration, from the original configuration object
that is going to be replaced; in case of breakage of inheritance or at
jail creation, from the applicable configuration object, which is on an
ancestor jail). This process is not immune to concurrent modifications
because nothing prevents changes of configurations between reading
existing values and setting the new configuration. Thus, some other
thread could change the value of a parameter after a copy of it has been
made into the new object but before that copy is actually installed,
which effectively will erase the other thread's modification.
To avoid this, we introduce support for serializing configuration
changes on a given jail. To this end, we move the jail climbing process
from find_conf() into find_conf_locked(), and make the former call the
latter in a read-locked section. Similarly, we isolate setting
a configuration in the new set_conf_locked() function, and make
set_conf() call it inside a write-locked section. The new *_unlocked()
variants make it possible to prevent any configuration access between
determining and reading an applicable configuration, computing from it
a new configuration object and finally setting it, by holding a write
lock over the whole process (there is a trade-off here, as read-mostly
locks cannot be upgraded), effectively making it atomic and realizing
full sequential consistency of configuration changes. Also, the
'mac_do_rm' global read-mostly lock is made sleepable so that it can be
write-locked over sysctl_handle*() functions or memory allocations
(eases implementation, at the expense of a potential loss of concurrency
which is most probably irrelevant).
No functional change (intended) at this point.
Reviewed by: bapt
Fixes: 9818224174c4 ("MAC/do: Executable paths feature (GSoC 2025's final state)")
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Since the inception of mac_do(4), find_conf(), used to retrieve the
applicable configuration, has been weakly consistent with respect to
concurrent modifications to configuration inheritance that influence its
result (and it has been sequentially consistent with respect to other
configuration modifications, which the initial executable paths feature
and introduction of implicit parameters broke and which will be fixed in
a subsequent commit).
Indeed, find_conf() climbs the jail tree to find an applicable
configuration, which is not an atomic operation. It examines the
current jail's configuration pointer for each browsed jail, which does
not prevent concurrent modifications of the configuration pointer for
jails below or above it. Modifications above the current jail are not
a problem, since if climbing needs to continue (i.e., the current jail
inherits), these modifications will be seen if performed before that
check (and may or may not be seen if performed after that check).
However, modifications below the current jail impair sequential
consistency, because they could be done before other modifications at
the current jail or higher up, and the latter could still be picked up
by the rest of the climb, effectively ignoring that the former should
have blocked the climb earlier, making them look as if they had happened
after for the climbing thread.
As a concrete example of this situation, let's examine a scenario where
some jail A is the parent of some jail B, and B inherits its
configuration from A. An administrator may want to relax the rules only
for jail A but not B. To this end, he first copies the current rules on
B over to A and then relaxes the rules on A. He can intuitively and
reasonably expect that changing B's rules first will prevent A's relaxed
rules to leak to threads in B. Unfortunately, that is not the case: As
explained in the previous paragraph, there can be a time window where
threads from B can still pick up A's new configuration just after it has
been installed. This arguably makes changing inheritance in mac_do(4)
in a fully secure fashion almost impossible.
If preserving fine-grained locking of prisons, we could prevent this
problem by having find_conf(), once it has climbed to a non-NULL pointer
(actual, non-inherited configuration), do another climb to check that it
can reach the same configuration on the same jail again. If the new
climb gives another pointer or jail, it could make it the new candidate
and do a climb check again until the situation stabilizes. A climb
check detects whether changes in jails below that of the candidate
configuration object happened, catching in particular such changes that
happened before changes to the candidate object. However, that process
alone would still be subject to ABA problems, and we would additionally
need to tag each prison with some modification timestamp (global, or
local but necessitating allocating memory during the check) to fix them.
In the end, we considered this direction to be unnecessarily complex,
given that configuration changes are to be rare events and most uses
will just be configuration determination.
Consequently, switch protecting jail configurations with a single
read-mostly lock.
While here, adapt set_conf() to accept NULL as the new configuration
object, and have remove_conf() call it, which removes duplicated code.
While here, add a comment explaining why we do not need to take any more
locks when climbing the jail tree.
Reviewed by: bapt
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
|
|
| |
Once visible, configuration structures must *never* change.
Spell that out in a comment to help future readers/contributors
understand the design.
Reviewed by: bapt
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
|
|
| |
When mac_do(4) is loaded, all jails get the same default configuration
(disabled, with only one allowed executable path: '/usr/bin/mdo').
Share it between all jails instead of creating a separate copy for each.
Reviewed by: bapt
Fixes: 9818224174c4 ("MAC/do: Executable paths feature (GSoC 2025's final state)")
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
|
| |
With additional empty lines.
No functional change (intended).
Reviewed by: bapt
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
|
| |
In mac_do_jail_get(), computation of 'jsys' had not been updated to take
into account executable paths.
Reviewed by: bapt
Fixes: 9818224174c4 ("MAC/do: Executable paths feature (GSoC 2025's final state)")
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
mac_do_jail_create() would create a default configuration on the
just-created jail, erroneously causing mac_do_jail_set() to then
retrieve it and use it as a model when determining the default values
for not-specified parameters, instead of using the configuration
applicable to the parent jail.
Setting a default configuration in mac_do_jail_create() had been done as
a kind of defensive measure to prevent a created jail not to have
a configuration (effectively making it inherit from an ancestor jail,
which is a security hazard except if explicitly requested). However,
this measure was never really effective (osd_jail_call(PR_METHOD_CREATE)
in kern_jail_set() calls the PR_PETHOD_CREATE methods in an unspecified
order, and stops at the first error), so we are forced to rely in any
case on the fact that an error in a PR_METHOD_CREATE or PR_METHOD_SET
method leads to stopping the jail creation process (which is the case
today; see kern_jail_set()).
Reviewed by: bapt
Fixes: 9818224174c4 ("MAC/do: Executable paths feature (GSoC 2025's final state)")
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
The logic introduced in the initial commit for the "executable paths"
feature did not match the specification we discussed in that specifying
an empty value (for rules or executable paths) on "mac.do" being "new"
would be treated as an absence of value and trigger a copy from the
currently applicable configuration, instead of being an override that
deactivates mac_do(4) in the jail. Fix that by distinguishing both
cases.
More generally, a non-explicitly specified parameter is set to the same
value it has in the currently applicable configuration (that of the
closest ancestor jail that has one; 'prison0' (the host) always has
one), with an exception in the disable case.
On disable (explicit: "mac.do" to "disable", implicit: no parameters
passed, or at least one is empty), now accept parameters with
a non-empty value as long as at least one of them is empty (which alone
is enough to disable mac_do(4)). If no parameters are passed, both are
copied from the currently applicable configuration; if none of them is
empty, then the rules are emptied to effectively disable mac_do(4) (see
the inline comment as to why this was chosen).
On explicit enable ("mac.do" to "enable"), allow not specifying any of
the rules and executable paths, in which case both are copied from the
currently applicable configuration (consistently with what is done when
only one is missing). Note that, as mentioned above, not specifying any
of them by default still resolves to disabling mac_do(4) (i.e., on no
explicit "mac.do" parameter).
On (explicit) inheritance, allow specifying non-empty parameters,
provided they match the values we are going to inherit. This enables
re-applying jail parameters' reported values verbatim to the current
jail (idempotence) or, e.g., to some sibling jail.
(While here, make some existing code easier to read by leveraging
is_null_or_empty().)
Reviewed by: bapt
Fixes: 9818224174c4 ("MAC/do: Executable paths feature (GSoC 2025's final state)")
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
| |
Reviewed by: bapt
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
| |
Reviewed by: bapt
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
This change is a prerequisite for the next change in caller
mac_do_jail_set(), which for semantic correctness needs to rely on
a stable model configuration.
The two other callers already call find_conf() to retrieve the
applicable configuration, so for these a second call to find_conf() can
be saved.
However, this does not fix (actually, makes slightly worse) an atomicity
problem when multiple threads concurrently change some jail's
configuration (or the configuration inherited by a jail), which has
existed since the introduction of executable paths due to being able to
change only rules or executable paths independently (and the possibility
of not specifying them and having them copied from the currently
applicable configuration). Before tackling it in later commits, we
first focus on fixing the semantics of configuration changes in the very
next patches.
Reviewed by: bapt
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
parse_and_set_conf() is meant to be used in all situations when there is
a need to set or modify some jail's MAC/do configuration. This entails
passing the information of whether some parameter was explicitly
specified. For example, an administrator setting/modifying jail
parameters may not specify executable paths but only rules, in which
case the executable paths value is copied from the currently-applicable
configuration. The sysctl(8) knobs case always leverages this feature,
since setting a knob changes one parameter at a time.
Currently, a NULL or empty string argument is treated as a non-specified
parameter. This causes a bug where disabling MAC/do in a jail does not
actually work because, to this end, parse_and_set_conf() is passed an
empty string which it then interprets as a request to copy the currently
applicable configuration's value, which may well not be empty.
Fix this problem by only treating NULL as a marker for a non-specified
parameter, in accordance with the original design for this function.
While here, write some documentation to explain the interface. While
here, remove the original herald comment for parse_and_set_rules(),
which was inadvertently pushed apart from the replacing
parse_and_set_conf().
Reviewed by: bapt
Fixes: 9818224174c4 ("MAC/do: Executable paths feature (GSoC 2025's final state)")
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
|
|
| |
Constify in order to let the compiler check that source and destination
arguments are passed in the proper order in the different calls.
No functional change (intended).
Reviewed by: bapt
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
|
|
| |
Configuration objects would be initialized (zeroed, and some
STAILQ_INIT() called) multiple times. Make sure they are so only once,
and add assertions to check that this is actually the case for functions
that expect it.
Reviewed by: bapt
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
| |
And document more clearly their purpose.
Reviewed by: bapt
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
| |
Defensive programming.
Reviewed by: bapt
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
On parsing error, parse_and_set_conf(), introduced with the recent
"executable paths" feature, has been calling drop_conf() on the
being-built configuration. However, that configuration structure is
allocated through alloc_conf(), which does not grab a reference.
Calling drop_conf() on it, which releases a reference, is thus
erroneous, and causes the underlying counter to saturate, translating
into a memory leak.
To fix this bug, make alloc_conf() grab a reference on the newly-created
'struct conf', and rename it to new_conf() to be more in line with what
it does. Keep set_conf() as is, i.e., grabbing an additional reference
on behalf of the jail that is going to hold the configuration.
Consequently, make sure that callers of alloc_conf() unconditionally
drop the reference acquired by the latter before returning (i.e., even
if set_conf() has been called).
While here, since hold_conf() is always used to obtain additional
references on a configuration (new_conf() does not use it, instead
directly setting the use count), add an assertion that it is never used
on a configuration that has no references at all (which indicates that
the configuration has been destroyed).
These changes generally simplify the lifecycle of configurations,
reducing the probability of re-introducing reference mismatches (at the
expense of slightly more reference counting operations, but performance
does not matter here).
Reviewed by: bapt
Fixes: 9818224174c4 ("MAC/do: Executable paths feature (GSoC 2025's final state)")
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
In addition to the applicable configuration, find_conf() was returning
a pointer to the actual jail holding the configuration object, with that
jail's mutex locked in order to ensure liveness of the returned
configuration (if we wouldn't, a concurrent thread modifying the jail's
configuration could destroy this configuration object underneath us).
But:
1. Ensuring configuration stability by owning the holding jail's mutex
requires callers to either keep that mutex locked for a longer period
of time than just accessing the corresponding 'struct prison' (in
general, bad for concurrency with other operations involving jails)
or to perform an additional dance to acquire a real reference in case
the jail's mutex, for some reason (in general, LORs or acquiring
a sleepable lock) must be dropped before use.
2. Most code does not actually need to know which jail holds the
applicable configuration but for unlocking the jail's mutex. Having
to deal with the jail holding the configuration can cause confusion
about which jail (the current one, or the one holding the
configuration) must be used (and actually did in the very initial
version of MAC/do, which had a serious flaw as a consequence).
So, do not keep a lock on the holding jail. Instead, ensure
configuration stability by always acquiring a true reference from the
start and passing it to the caller. Those callers not doing the dance
mentioned above now need to free it when finished (but this need
replaces the one to unlock the prison).
Additionally, only return the holding jail if explicitly requested by
the caller. mac_do_jail_get() is currently the only caller that needs
it, in order to be able to reliably report if the configuration is
inherited.
Reviewed by: bapt
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
| |
This is in preparation for using hold_conf() in find_conf().
Reviewed by: bapt
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Turn the pre-existing comment into an assertion message, with an update
following the introduction of the "executable paths" feature.
Explain in a comment why this situation cannot happen.
Without INVARIANTS, such a situation would cause an immediate panic()
(NULL is dereferenced in the next iteration of the loop), so leave the
check under INVARIANTS only.
Reviewed by: bapt
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
These constants represent buffer sizes in bytes, not string lengths.
While here, move MAX in EXEC_PATHS_MAXLEN at start, like the other
constants. While here, fix the prefix of the old MAC_RULE_STRING_LEN
accordingly.
No functional change.
Reviewed by: bapt
Fixes: 8aac90f18aef ("mac_do: add a new MAC/do policy and mdo(1) utility")
Fixes: 9818224174c4 ("MAC/do: Executable paths feature (GSoC 2025's final state)")
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
|
|
| |
This effectively allows to disable mac_do(4) by setting the executable
paths to an empty string, realizing a symmetry with rules to be
leveraged in subsequent commits.
Reviewed by: bapt
Fixes: 9818224174c4 ("MAC/do: Executable paths feature (GSoC 2025's final state)")
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
The invariant is that parse_*() functions must create a parse error
object and return it through their 'parse_error' argument if and only if
they return an error code (non-zero).
Add assertions checking this invariant in various places, and in
particular in the new parse_exec_paths(), to be future proof.
Change the contract of parse_and_set_conf() so that the caller is
required to pass a NULL '*parse_error'.
Remove useless resetting of '*parse_error' to NULL.
While here, remove a test that is always true thanks to this invariant
and that was recently introduced with the "executable paths" feature.
No functional change intended.
Reviewed by: bapt
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
| |
No functional change (intended).
Reviewed by: bapt
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
| |
No functional change.
Reviewed by: bapt
Fixes: 9818224174c4 ("MAC/do: Executable paths feature (GSoC 2025's final state)")
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
| |
Reviewed by: bapt
Fixes: 9818224174c4 ("MAC/do: Executable paths feature (GSoC 2025's final state)")
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Fix too long lines, declarations not at head of block, improper
indentation and superfluous whitespace coming from the previous commit
introducing the configurable executable paths feature.
While here, fix some older improper comment formatting.
Reviewed by: bapt
Fixes: 6c3def74e2de ("MAC/do: Support multiple users and groups as single rule's targets")
Fixes: 9818224174c4 ("MAC/do: Executable paths feature (GSoC 2025's final state)")
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
| |
Add Kushagra to account for the commit adding the "executable paths"
feature.
Reviewed by: bapt
MFC after: 1 month
Sponsored by: The FreeBSD Foundation
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
By design, mac_do(4) only authorizes credentials change requests if they
are issued by a process spawned from '/usr/bin/mdo'. The executable
paths feature introduces some flexibility by allowing to change that
path, thus allowing another executable to make requests, and to use
multiple such paths (up to 8 in the current implementation). Its
purpose is to enable thin jails scenarios where mdo(1) may not be at its
canonical path ('/usr/bin/mdo') and to allow experimenting with other
userland programs leveraging setcred(2).
Configuration of executable paths is per-jail and intentionally works
completely similarly with rules. It is accessible from within a jail
through the 'security.mac.do.exec_paths' sysctl knob and from outside
a jail through the 'mac.do.exec_paths' jail parameter.
This commit groups the verbatim changes of the following commits that
Kushagra Srivastava, our GSoC 2025 student, created in his GitHub
repository (https://github.com/thesynthax/freebsd-src), branch
'task/exec-paths-refactor':
mac_do(4): Complete refactor of allowed executable paths feature
mac_do(4): Fixed changing security.mac.do.* knobs in inheritance mode
mac_do(4): Debugging rules and exec_paths leak on destroy
mac_do(4): Deep copy rules
mac_do(4): Fixed leak
mac_do(4): fixed various bugs, structs inlined, leaks remain
mac_do(4): MAC/do working in jail, leaks decreased
mac_do(4): MAC/do fixed, works in host and jails, leaks removed
mac_do(4): style
Frozen log for these commits:
https://github.com/OlCe2/freebsd-src/compare/main...14fdc49fb29265fac5d0daf95a13d0dce325c951.
The corresponding pull request is at:
https://github.com/OlCe2/freebsd-src/pull/2.
The GSoC's final state of this code still has a number of problems that
are fixed in subsequent commits. It is however committed separately to
clearly delineate Kushagra's work.
Reviewed by: olce (amendments to come, see above)
MFC after: 1 month
Relnotes: yes
Sponsored by: Google LLC (GSoC 2025)
Sponsored by: The FreeBSD Foundation (review, commit)
Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38
|
| |
|
|
|
|
|
|
| |
No functional change.
MFC after: 3 days
Event: AsiaBSDCon 2026
Sponsored by: The FreeBSD Foundation
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Contrary to my initial belief, vn_fullpath() does return a vnode's path
from the current chroot, and not from the global root (which would have
been a bug also, but without security consequences). This enables
a "confused deputy"-like scenario where a chroot(2) can change which
executable can be authorized by MAC/do, which is even more problematic
for unprivileged chroot(2).
This was found by re-examining the code following two close events:
1. Shawn Webb sent a mail to freebsd-hackers@ on 08/05 saying that in
HardenedBSD they had added a check on P2_NO_NEW_PRIVS (in
mac_do_priv_grant()), which I responded to on 08/20 saying that
P2_NO_NEW_PRIVS was not necessary for mac_do(4), with a correct
reasoning but based on the wrong above-mentioned assumption about
vn_fullpath().
2. I reviewed some code by Kushagra Srivastava (GSoC 2025 student
working on mac_do(4)/mdo(1)) adding the ability to specify which
executables can spawn processes that mac_do(4) may decide to
authorize (others are simply ignored), which currently is hardcoded
to '/usr/bin/mdo'.
MFC after: 3 days
Event: EuroBSDCon 2025
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D52758
|
| |
|
|
|
|
|
|
|
|
|
|
|
| |
As 'cr_gid' was in fact stored in cr_groups[0],
rule_grant_supplementary_groups() would loop only on further elements of
cr_groups[]. Now that cr_groups[0] is not 'cr_gid' anymore, but some
supplementary group, take it into account.
Fixes: be1f7435ef218b1d ("kern: start tracking cr_gid outside of cr_groups[]")
MFC after: 5 days
MFC to: stable/15
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D52271
|
| |
|
|
|
|
|
|
|
|
|
| |
From M_DO to M_MAC_DO. While here, make the descriptions more accurate.
(Commit message by olce@.)
Reviewed by: olce
MFC after: 3 days
Sponsored by: Google LLC (GSoC 2025)
Sponsored by: The FreeBSD Foundation
|
| |
|
|
|
|
|
|
|
|
|
|
|
| |
Switch to using sys/stdarg.h for va_list type and va_* builtins.
Make an attempt to insert the include in a sensible place. Where
style(9) was followed this is easy, where it was ignored, aim for the
first block of sys/*.h headers and don't get too fussy or try to fix
other style bugs.
Reviewed by: imp
Exp-run by: antoine (PR 286274)
Pull Request: https://github.com/freebsd/freebsd-src/pull/1595
|
| |
|
|
|
|
|
|
|
|
|
| |
MDF_HAS_PRIMARY_CLAUSE only concerns groups, not users, and is thus not
set in the latter case.
This change only has an effect on INVARIANTS builds.
PR: 287057
MFC after: 10 minutes
Sponsored by: The FreeBSD Foundation
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Previously, we would accept only ':' as the separator, which makes
parsing of the rule specification harder for humans, especially those
people that are used to UNIX systems where ':' is used as the separator
in PATH. With ':', the <from> and <to> parts can look like two
different elements that are unrelated, especially to these eyes.
Change parse_single_rule() so that '>' is also accepted as a separator
between <from> and <to>, and promote it as the one to use. During
a transition period, we will still allow the use of ':' for backwards
compatibility.
The manual page update comes from separate revision D49628. ':' has
been completely removed from it on purpose.
Reviewed by: bapt, manpages (ziaee)
MFC after: 5 days
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D49627
|
| |
|
|
|
|
|
| |
No functional change.
MFC after: 5 days
Sponsored by: The FreeBSD Foundation
|
| |
|
|
|
|
|
| |
grant_supplementary_group_from_flags() had been used in previous
versions of the recent changes, but recently has not been needed
anymore. It has been kept around just in case deliberately, by analogy
with grant_primary_group_from_flags() (this one still being used).
|
| |
|
|
|
| |
Approved by: emaste (mentor)
Sponsored by: The FreeBSD Foundation
|
| |
|
|
|
|
|
|
|
|
|
| |
We intend MAC/do to authorize transitions based on the "real" identity
information of the calling process, rather than transiently-acquired
effective IDs.
Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47845
|
| |
|
|
|
|
|
|
|
|
|
|
| |
We only browse these forward and never need to remove arbitrary elements
from them.
No functional change (intended).
Reviewed by: bapt, emaste
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47624
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
| |
To this end, we introduce the strsep_noblanks() function, designed to be
a drop-in replacement for strstep(), and use it in place of the latter.
We had taken care of calling strsep() even when the remaining sub-string
was not delimited (i.e., with empty string as its second argument), so
this commit only has mechanical replacements of existing calls.
Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47623
|
| |
|
|
|
|
|
|
|
| |
Use the most common pattern to browse and delete elements of a list, as it reads quicker.
Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47622
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
TL;DR:
Now monitor setcred() calls, and reject or grant them according to the
new rules specification.
Drop monitoring setuid() and setgroups(). As previously explained in
the commit introducing the setcred() system call, MAC/do must know the
entire new credentials while the old ones are still available to be able
to approve or reject the requested changes. To this end, the chosen
approach was to introduce a new system call, setcred(), instead of
modifying existing ones to be able to participate in a "prepare than
commit"-like protocol.
******
The MAC framework typically calls several hooks of its registered
policies as part of the privilege checking/granting process. Each
system call calls some dedicated hook early, to which it usually passes
the same arguments it received, whose goal is to forcibly deny access to
the functionality when needed (i.e., a single deny by any policy
globally denies the access). Then, the system call usually calls
priv_check() or priv_check_cred() an unspecified number of times, each
of which may trigger calls to two generic MAC hooks. The first such
call is to mac_priv_check(), and always happens. Its role is to deny
access early and forcibly, as can be done also in system calls'
dedicated early hooks (with different reach, however). The second,
mac_priv_grant(), is called only if the priv_check*() and
prison_priv_check() generic code doesn't handle the request by itself,
i.e., doesn't explicitly grant access (to the super user, or to all
users for a few specific privileges). It allows any single policy to
grant the requested access (regardless of whether the other policies do
so or not).
MAC/do currently only has an effect on processes spawned from the
'/usr/bin/mdo' executable. It implements all setcred() hooks, called
via mac_cred_setcred_enter(), mac_cred_check_setcred() and
mac_cred_setcred_exit(). In the first one, implemented in
mac_do_setcred_enter(), it checks if MAC/do has to apply to the current
process, allocates (or re-uses) per-thread data to be later used by the
other hooks (those of setcred() and the mac_priv_grant() one, called by
priv_check*()) and fills them with the current context (the rules to
apply). This is both because memory allocations cannot be performed
while holding the process lock and to ensure that all hooks called by
a single setcred() see the same rules to apply (not doing this would be
a security hazard as rules are concurrently changed by the
administrator, as explained in more details below). In the second one
(implemented by mac_do_check_setcred()), it stores in MAC/do's
per-thread data the new credentials. Indeed, the next MAC/do's hook
implementation to be called, mac_do_priv_grant() (implementing the
mac_priv_grant() hook) must have knowledge of the new credentials that
setcred() wants to install in order to validate them (or not), which the
MAC framework can't provide as the priv_check*() API only passes the
current credentials and a specific privilege number to the
mac_priv_check() and mac_priv_grant() hooks. By contrast, the very
point of MAC/do is to grant the privilege of changing credentials not
only based on the current ones but also on the seeked-for ones.
The MAC framework's constraints that mac_priv_grant() hooks are called
without context and that MAC modules must compose (each module may
implement any of the available hooks, and in particular those of
setcred()) impose some aspects of MAC/do's design. Because MAC/do's
rules are tied to jails, accessing the current rules requires holding
the corresponding jail's lock. As other policies might try to grab the
same jail's lock in the same hooks, it is not possible to keep the
rules' jail's lock between mac_do_setcred_enter() and
mac_do_priv_grant() to ensure that the rules are still alive. We have
thus augmented 'struct rules' with a reference count, and its lifecyle
is now decoupled from being referenced or not by a jail. As a thread
enters mac_cred_setcred_enter(), it grabs a hold on the current rules
and keeps a pointer to them in the per-thread data. In its
mac_do_setcred_exit(), MAC/do just "frees" the per-thread data, in
particular by dropping the referenced rules (we wrote "frees" within
guillemets, as in fact the per-thread structure is reused, and only
freed when a thread exits or the module is unloaded).
Additionally, ensuring that all hooks have a consistent view of the
rules to apply might become crucial if we augment MAC/do with forceful
access denial policies in the future (i.e., policies that forcibly
disable access regardless of other MAC policies wanting to grant that
access). Indeed, without the above-mentioned design, if newly installed
rules start to forcibly deny some specific transitions, and some thread
is past the mac_cred_check_setcred() hook but before the
mac_priv_grant() one, the latter may grant some privileges that should
have been rejected first by the former (depending on the content of
user-supplied rules).
A previous version of this change used to implement access denial
mandated by the '!' and '-' GID flags in mac_do_check_setcred() with the
goal to have this rejection prevail over potential other MAC modules
authorizing the transition. However, this approach had two drawbacks.
First, it was incompatible both conceptually and in the current
implementation with multiple rules being treated as an inclusive
disjunction, where any single rule granting access is enough for MAC/do
to grant access. Explicit denial requested by one matching rule could
prevent another rule from granting access. The implementation could
have been fixed, but the conflation of rules being considered as
disjoint for explicit granting but conjunct for forced denial would have
remained. Second, MAC/do applies only to processes spawned from
a particular executable, and imposing system-wide restrictions on only
these processes is conceptually strange and probably not very useful.
In the end, we moved the implementation of explicit access denial into
mac_do_priv_grant(), along with the interpretation of other target
clauses.
The separate definition of 'struct mac_do_data_header' may seem odd, as
it is only used in 'struct mac_do_setcred_data'. It is a remnant of an
earlier version that was not using setcred(), but rather implemented
hooks for setuid() and setgroups(). We however kept it, as it clearly
separates the machinery to pass data from dedicated system call hooks to
priv_grant() from the actual data that MAC/do needs to monitor a call to
setcred() specifically. It may be useful in the future if we evolve
MAC/do to also grant privileges through other system calls (each seen as
a complete credentials transition on its own).
The target supplementary groups are checked with merge-like algorithms
leveraging the fact that all supplementary groups in credentials
('struct ucred') and in each rule ('struct rule') are sorted, avoiding
to start a binary search for each considered GID which is asymptotically
more costly. All access granting/denial is thus at most linear and in
at most the sum of the number of requested groups, currently held ones
and those contained in the rule, per applicable rule. This should be
enough in all practical cases. There is however still room for more
optimizations, without or with changes in rules' data structures, if the
need ever arises.
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47620
|
| |
|
|
|
|
|
|
|
|
|
|
| |
This is going to be used in subsequent commits to keep rules alive even
if disconnected from their jail in the meantime. We'll indeed have to
release the prison lock between two uses (outright rejection, final
granting) where the rules must absolutely stay the same for security reasons.
Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47619
|
