MAC/do: Executable paths features, consistency fixes #38
emaste
bapt
olce
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "olce/olce-mac_do-exec_paths"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
See first commit of the series for a summary.
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)Fixes: XXX ("MAC/do: Executable paths feature (GSoC 2025's final state)") MFC after: 1 month Sponsored by: The FreeBSD FoundationNo functional change. Fixes: XXX ("MAC/do: Executable paths feature (GSoC 2025's final state)") MFC after: 1 month Sponsored by: The FreeBSD FoundationThis 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. Fixes: XXX ("MAC/do: Executable paths feature (GSoC 2025's final state)") MFC after: 1 month Sponsored by: The FreeBSD FoundationOn 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). Fixes: XXX ("MAC/do: Executable paths feature (GSoC 2025's final state)") MFC after: 1 month Sponsored by: The FreeBSD Foundationparse_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(). Fixes: XXX ("MAC/do: Executable paths feature (GSoC 2025's final state)") MFC after: 1 month Sponsored by: The FreeBSD FoundationThe 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().) Fixes: XXX ("MAC/do: Executable paths feature (GSoC 2025's final state)") MFC after: 1 month Sponsored by: The FreeBSD Foundationmac_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()). Fixes: XXX ("MAC/do: Executable paths feature (GSoC 2025's final state)") MFC after: 1 month Sponsored by: The FreeBSD FoundationIn mac_do_jail_get(), computation of 'jsys' had not been updated to take into account executable paths. Fixes: XXX ("MAC/do: Executable paths feature (GSoC 2025's final state)") MFC after: 1 month Sponsored by: The FreeBSD FoundationWhen 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. Fixes: XXX ("MAC/do: Executable paths feature (GSoC 2025's final state)") MFC after: 1 month Sponsored by: The FreeBSD FoundationAs 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. Fixes: XXX ("MAC/do: Executable paths feature (GSoC 2025's final state)") MFC after: 1 month Sponsored by: The FreeBSD FoundationSee previous commit for explanations of what this is fixing. Because we currently only modify a single configuration object per transaction (this might change in the future), 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. Fixes: XXX ("MAC/do: Executable paths feature (GSoC 2025's final state)") MFC after: 1 month Sponsored by: The FreeBSD FoundationThe 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. Fixes: XXX ("MAC/do: Executable paths feature (GSoC 2025's final state)") MFC after: 1 month Sponsored by: The FreeBSD Foundationthe patch is big, but from a quick review it looks good to me.
@ -3,3 +3,2 @@.\".\" Copyright (c) 2024 Baptiste Daroussin <bapt@FreeBSD.org>.\" Copyright (c) 2024 The FreeBSD Foundation.\" Copyright (c) 2024, Baptiste Daroussin <bapt@FreeBSD.org>I don't think I need a coma here
@ -3,3 +3,2 @@** Copyright(c) 2024 Baptiste Daroussin <bapt@FreeBSD.org>* Copyright (c) 2024 The FreeBSD Foundation* Copyright(c) 2024, Baptiste Daroussin <bapt@FreeBSD.org>for one single year I don't think the coma is needed, it looks weird. alsi a space was swallowed.
I'll remove the added comma.
What do you mean by "a space was swallowed"? Is it about no space between "Copyright" and "(c)"? This is also in the original text. I can fix it in passing.
yes this is what I mean
c8e1af301a851499046d