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
(cherry picked from commit b5c9889e369a801ce7c1115f2535ddacbd69800d)
(cherry picked from commit 30f092c40ad4eb592861839f4ffa9e9891abf1d3)
Approved by: re (cperciva)
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
(cherry picked from commit f01d26dec67fb6597438ed765269b85d1099a6fa)
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).
(cherry picked from commit f1ddb6fb8c4d051a205dae3a848776c9d56f86ff)
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
(cherry picked from commit de701f9bdbe0ede691a0439d1c469082b94fe234)
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
(cherry picked from commit c7fc71c6af0761f81ecafdb281dd43a081b3b22f)
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
(cherry picked from commit 4a03b64517b3151064c52e213ebbc068ab1430d1)
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
(cherry picked from commit 2110eef4bf608b6c1facc57c68d02960b6d880c9)
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
(cherry picked from commit 8f7e8726e3f5f20b9eed0ad12fc2d2a4ec304d14)
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
(cherry picked from commit 3d8d91a5b32c219c7ee47840dcacbaf8c7480267)
This new system call allows to set all necessary credentials of
a process in one go: Effective, real and saved UIDs, effective, real and
saved GIDs, supplementary groups and the MAC label. Its advantage over
standard credential-setting system calls (such as setuid(), seteuid(),
etc.) is that it enables MAC modules, such as MAC/do, to restrict the
set of credentials some process may gain in a fine-grained manner.
Traditionally, credential changes rely on setuid binaries that call
multiple credential system calls and in a specific order (setuid() must
be last, so as to remain root for all other credential-setting calls,
which would otherwise fail with insufficient privileges). This
piecewise approach causes the process to transiently hold credentials
that are neither the original nor the final ones. For the kernel to
enforce that only certain transitions of credentials are allowed, either
these possibly non-compliant transient states have to disappear (by
setting all relevant attributes in one go), or the kernel must delay
setting or checking the new credentials. Delaying setting credentials
could be done, e.g., by having some mode where the standard system calls
contribute to building new credentials but without committing them. It
could be started and ended by a special system call. Delaying checking
could mean that, e.g., the kernel only verifies the credentials
transition at the next non-credential-setting system call (we just
mention this possibility for completeness, but are certainly not
endorsing it).
We chose the simpler approach of a new system call, as we don't expect
the set of credentials one can set to change often. It has the
advantages that the traditional system calls' code doesn't have to be
changed and that we can establish a special MAC protocol for it, by
having some cleanup function called just before returning (this is
a requirement for MAC/do), without disturbing the existing ones.
The mac_cred_check_setcred() hook is passed the flags received by
setcred() (including the version) and both the old and new kernel's
'struct ucred' instead of 'struct setcred' as this should simplify
evolving existing hooks as the 'struct setcred' structure evolves. The
mac_cred_setcred_enter() and mac_cred_setcred_exit() hooks are always
called by pairs around potential calls to mac_cred_check_setcred().
They allow MAC modules to allocate/free data they may need in their
mac_cred_check_setcred() hook, as the latter is called under the current
process' lock, rendering sleepable allocations impossible. MAC/do is
going to leverage these in a subsequent commit. A scheme where
mac_cred_check_setcred() could return ERESTART was considered but is
incompatible with proper composition of MAC modules.
While here, add missing includes and declarations for standalone
inclusion of <sys/ucred.h> both from kernel and userspace (for the
latter, it has been working thanks to <bsm/audit.h> already including
<sys/types.h>).
Reviewed by: brooks
Approved by: markj (mentor)
Relnotes: yes
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47618
(cherry picked from commit ddb3eb4efe55e57c206f3534263c77b837aff1dc)
So that administrators can more easily know what the problem is with the
rules they are trying to set.
The new sysctl 'security.mac.do.print_parse_error' controls whether
trying to set sysctl 'security.mac.do.rules' with invalid rules triggers
printing of the error on the system console.
Setting jail parameters directlty reports an error to the calling
process thanks to the VFS options mechanism used by the jail machinery,
so is not controlled by the new sysctl setting.
Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47617
(cherry picked from commit 87c06b7d026f2beeb3c2f695567ef72aa3a427ea)
Supporting group targets is a requirement for MAC/do to be able to
enforce a limited set of valid new groups passed to setgroups().
Additionally, it must be possible for this set of groups to also depend
on the target UID, since users and groups are quite tied in UNIX (users
are automatically placed in only the groups specified through
'/etc/passwd' (primary group) and '/etc/group' (supplementary ones)).
These requirements call for a re-design of the specification of the
rules specification string and of 'struct rule'.
A rules specification string is now a list of rules separated by ';'
(instead of ','). One rule is still composed of a "from" part and
a "to" (or "target") part, both being separated by ':' (as before).
The first part, "from", is matched against the credentials of the
process calling setuid()/setgroups(). Its specification remains
unchanged: It is a '<type>=<id>' clause, where <type> is either "uid" or
"gid" and <id> an UID or GID.
The second part, "to", is now a comma-separated (',') list of
'<flags><type>=<id>' clauses similar to that of the "from" part, with
the extensions that <id> may also be "*" or "any" or ".", and that
<flags> may contain at most one of the '+', '-' and '!' characters when
<type> is GID. "*" and "any" both designate any ID for the <type>, and
are aliases to each other. In front of them, only the "+" flag is
allowed (in addition to the previous rules). "." designates the
process' current IDs for the <type>, as explained below.
For GIDs, an absence of flag indicates that the specified GID is allowed
as the real, effective and/or saved GIDs (the "primary" groups).
Conversely, the presence of any allowed flag indicates that the
specification concerns supplementary groups. The '+' flag in front of
"gid" indicates that the ID is allowed as a supplementary group. The
'!' flag indicates that the ID is mandatory, i.e., must be listed in the
supplementary groups. The '-' flag indicates that the GID must not be
listed in the supplementary groups. A specification with '-' is only
useful in conjunction with a '+'-tagged specification where only one of
them has <id> ".", or if other MAC policies are loaded that would give
access to other, unwanted groups.
"." indicates some ID that the calling process already has on privilege
check. For type "uid", it designates any of the real, effective or
saved UIDs. For type "gid", its effect depends on the presence of one
of the '+', '-' or '!' flags. If no flag is present, it designates any
of the real, effective or saved GIDs. If one is present, it designates
any of the supplementary groups.
If the "to" part doesn't specify any explicit UID, any of the UIDs of
the calling process is implied (it is as if "uid=." had been specified).
Similarly, if it doesn't specify any explicit GID, "gid=.,!gid=." is
assumed, meaning that all the groups of the calling process are implied
and must be present. More precisely, each of the desired real,
effective and saved GIDs must be one of the current real, effective or
saved GID, whereas all others (the supplementary ones) must be the same
as those that are current.
No two clauses in a single "to" list may display the same <id>, except
for GIDs but only if, each time the same <id> appears, it does so with
a different flag (no flag counting as a separate flag) and all the
specified flags are not contradictory (e.g., it is possible to have the
same GID appear with no flag and the "+" flag, but the same GID with
both "+" and "-" will be rejected).
'struct rule' now holds arrays of UIDs (field 'uids') and GIDs (field
'gids') that are admissible as targets, with accompanying flags (such as
MDF_SUPP_MUST, representing the '!' flag). Some flags are also held by
ID type, including flags associated to individual IDs, as MDF_CURRENT in
these flags stands for the process being privilege-checked's current
IDs, to which ID flags apply. As a departure from this scheme, "*" or
"any" as <id> for GIDs is either represented by MDF_ANY or MDF_ANY_SUPP.
This is to make it coexist with a "."/MDF_CURRENT specification for the
other category of groups (among primary and supplementary groups), which
needs to be qualified by the usual GID flags.
This commit contains only the changes to parse the new rules and to
build their representation. The privilege granting part is not fixed
here, beyond what making compilation work requires (and, in preparation
for some subsequent commit, minimal adaptations to the matching logic in
check_setuid()).
Approved by: markj (mentor)
Relnotes: yes
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47616
(cherry picked from commit 6c3def74e2deb825e7dac4ffebaaf651f547e392)
This variable is static and holds the OSD slot number for jails that
MAC/do uses to store rules.
In the same vein as previous renames, simplify it by removing the
redundant prefix, as this name cannot appear in code outside of
'mac_do.c', nor in stack traces on panic.
Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47772
(cherry picked from commit 40a664a463bab87505c8d42816a71202e8ad7bd9)
Have a static constant array mapping numerical ID types to their
canonical representations ('id_type_to_str').
New parse_id_type() that parses a type thanks to 'id_type_to_str' and
with a special case to accept also 'any'.
Have parse_rule_element() use parse_id_type(). A later commit will add
a second call to the latter for the destination ID.
Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47615
(cherry picked from commit 65766063f85d8b8fe8b24a50250a12a122974c26)
Introduce strtoui_strict(), which signals an error on overflow contrary
to the in-kernel strto*() family of functions which have no 'errno' to
set and thus do not allow callers to distinguish a genuine maximum value
on input and overflow.
It is built on top of strtoq() and the 'quad_t' type in order to achieve
this distinction and also to still support negative inputs with the
usual meaning for these functions. See the introduced comments for more
details.
Use strtoui_strict() to read IDs instead of strtol().
Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47614
(cherry picked from commit 0af43c029048e1ad2f8b140a3baf3851785c12d9)
This is in preparation for introducing a common conversion function for
IDs and to simplify code a bit by removing the from-IDs union and not
having to introduce a new one for to-IDs in a later commit.
Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47613
(cherry picked from commit 6aadc7b2ee055fba58984fec715b6e2a754f9d3e)
The ID field was allowed to be empty, which would be then parsed as 0 by
strtol(). There remains bugs in this function, where parsing for from-
or to- IDs accepts spaces and produces 0, but this will conveniently be
fixed in a later commit introducing strtoui_strict().
Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47612
(cherry picked from commit fa4352b74580832d7b501d34d09a564438a82c3d)
Handle JAIL_SYS_DISABLE the same as JAIL_SYS_NEW with an empty rules
specification, coherently with jail_get(). Also accept JAIL_SYS_DISABLE
in "mac.do" without "mac.do.rules" being specified.
The default value for "mac.do", if not passed explicitly, is either
JAIL_SYS_NEW if "mac.do.rules" is present and non-empty, or
JAIL_SYS_DISABLE if present and empty or not present.
Perform all cheap sanity checks in jail_check(), and have these
materialized as well in jail_set() under INVARIANTS. Cheap checks are
type and coherency checks between the values of "mac.do" and
"mac.do.rules". They don't include parsing the "mac.do.rules" string
but just checking its length (when applicable). In a nutshell,
JAIL_SYS_DISABLE and JAIL_SYS_INHERIT are allowed iff "mac.do.rules"
isn't specified or is with an empty string, and JAIL_SYS_NEW is allowed
iff "mac.do.rules" is specified (the latter may be empty, in which case
this is equivalent to JAIL_SYS_DISABLE).
Normally, vfs_getopts() is the function to use to read string options.
Because we need the length of the "mac.do.rules" string to check it, in
order to avoid double search within jail options in jail_check(), we use
vfs_getopt() instead, but perform some additional checks afterwards (the
same as those performed by vfs_getopts()).
Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47610
(cherry picked from commit 11eb32958f2c6e70892201982c1c92a0140d6864)
- Properly fill 'jsys' before copying it out (we would leak bytes from
the kernel stack). When the current jail has its own 'struct rules',
set it to the special value JAIL_SYS_DISABLE if it in fact holds no
rules.
- Don't forget to unlock the jail holding rules on error.
- Correctly return errors.
Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47609
(cherry picked from commit 2a20ce91dc29e5a80f4eeb9352cf3169cd1891b9)
So that we immediately know whether a kernel stack involves MAC/do.
Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47607
(cherry picked from commit 11ba1f2fe2d4e151ffc0a66d03a0691a7b8d2866)
The panic is caused by dereferencing 'element' at a point where it can
be NULL (if string ends at the ':').
Harden and simplify by enforcing the control flow rule in this function
that jumping to the end is reserved for error cases.
Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47605
(cherry picked from commit add521c1a5d21ec84454009d42d1dcd688d77008)
The rules on 'prison0' are initialized in init(), now using
set_empty_rules().
Until the jail is destroyed, they can never be uninitialized by a call
to osd_jail_del(), since the only chain to call it is
mac_do_prison_set() -> remove_rules() -> osd_jail_del(), and
mac_do_prison_set() (method PR_METHOD_SET) can never be called on
'prison0'. This guarantees that find_rules() always find a valid
'rules' pointer to return.
There's no need to do anything special in destroy() for 'prison0', as
osd_jail_deregister() now takes care of it.
Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47603
(cherry picked from commit beb5603c51e0323e267ceff8f83b3c95151f0822)
Now that sysctl_rules() has been fixed to behave.
Reviewed by: bapt
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47602
(cherry picked from commit b3f93680e39b90c02ddabdaf98f9c9a669d24c00)
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
(cherry picked from commit 53d2e0d4854997005271ee60791ab114bd6e0099)
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
(cherry picked from commit 292c814931d975d56d5ffa7c3c85191d56a059c4)
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
(cherry picked from commit 301eeb10dc197986b2b6261b064cbfe96333f7fb)
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
(cherry picked from commit 3186b192e4db7896bae22a9116ab915bf852fa27)
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
(cherry picked from commit bbf8af664dc94804c219cd918788c0c127a5c310)
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
(cherry picked from commit 8ce5770604981a19884604ad532f9528e087c69a)
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
(cherry picked from commit 02ed945ccec43340208d3a9c152fb98f55dbed69)
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
(cherry picked from commit ccae2774897c1f8bb11f696d5895fb686db98176)
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
(cherry picked from commit 2200a3ec711baa98c20a4b65868957dc40912f0f)
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
(cherry picked from commit f0600c41e754f32b388af804fb542b0f0ea89dee)
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
(cherry picked from commit 7937bfbc0ca53fe7cdd0d54414f9296e273a518e)
A string loader tunable requires setting the len parameter to a nonzero
value, typically the size of the string, to have the flag CTLFLAG_TUN
work correctly [1] [2].
Without this fix security.mac.{biba,lomac}.trusted_interfaces would
have no effect at all.
[1] 3da1cf1e88 Extend the meaning of the CTLFLAG_TUN flag to automatically ...
[2] 6a3287f889 Fix regression issue after r267961. Handle special string case ...
Reviewed by: olce, kib
Fixes: af3b2549c4 Pull in r267961 and r267973 again ...
MFC after: 3 days
Differential Revision: https://reviews.freebsd.org/D48898
(cherry picked from commit 7d4c0fac8c7db9c5741ba98a8b3ce3c43feb1cf4)
syscallenter() has a slow path to handle syscall auditing and dtrace
syscall tracing. It uses AUDIT_SYSCALL_ENTER() to check whether to take
the slow path, but this macro also has side effects: it writes the audit
log entry. When systrace (dtrace syscall tracing) is enabled, this
would get short-circuited, and we end up not writing audit log entries.
Introduce a pure macro to check whether auditing is enabled, use it in
syscallenter() instead of AUDIT_SYSCALL_ENTER().
Reviewed by: kib
Reported by: Joe Duin <jd@firexfly.com>
Fixes: 2f7292437d ("Merge audit and systrace checks")
MFC after: 3 days
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D48448
(cherry picked from commit f78fe930854cac6eed55859b45e0a7b5d87189d6)
Needed by the upcoming setcred() system call. More generally, is a step
on the way to support 32-bit compatibility for MAC-related system calls.
Reviewed by: brooks
Approved by: markj (mentor)
MFC after: 2 weeks
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D47878
(cherry picked from commit 3bdc5ba2ac760634056c66c3c98b6b3452258a5b)
This is in preparation for enabling the new setcred() system call to set
a process' MAC label.
No functional change (intended).
MFC after: 2 weeks
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D46905
(cherry picked from commit 8a4d24a39098ed8170a37ca2aa83bf1da1976de1)
Besides simplifying existing code, this will later enable the new
setcred() system call to copy MAC labels.
MFC after: 2 weeks
Approved by: markj (mentor)
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D46904
(cherry picked from commit 2e593dd3b5e1c515d57b3d3f929e544a6622b04a)
Do this only when the headers for these functionalities were included
prior to this one. Indeed, if they need to be included, style(9)
mandates they should have been so before this one.
Remove the common MAC sysctl declaration from
<security/mac/mac_internal.h>, as it is now redundant (all its includers
also include <security/mac/mac_policy.h>).
Remove local such declarations from all policies' files.
Reviewed by: jamie
Approved by: markj (mentor)
MFC after: 5 days
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D46903
(cherry picked from commit db33c6f3ae9d1231087710068ee4ea5398aacca7)
The original changes in 'sys/security/mac_grantbylabel/mac_grantbylabel.c' were
removed as MAC/grantbylabel has not been MFCed.
