| Message ID | a79e0e6f-e83d-4b4b-a55c-3f2c20b93c83@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selinux_set_callback for policy load not triggering | expand |
On Thu, Oct 17, 2024 at 3:42 PM Matthew Sheets <masheets@linux.microsoft.com> wrote: > > Hi All, > > I am currently working on an update for dbus-broker to trigger reload of > its configuration whenever an SELinux policy load event is seen. > > For some background dbus-broker is comprised of two major elements the > launcher and the broker. To trigger a config reload you can either send > a SIGHUP to the launcher or send a message to the launcher over dbus. > In most cases the launcher will be the brokers parent. > > Here is a link to my current PR: > https://github.com/bus1/dbus-broker/pull/379 > > In this current state things work. The broker will see the POLICY_LOAD > event and properly send a SIGHUP to its parent, but as David pointed out > my initial attempt at the fix is no good since there is no guarantee > that the brokers parent will be the launcher. > > My attempts at moving the callback registration into the launcher have > been less successful. From what my debugging has told me is that the > selinux_set_callback is going through successfully and the function > pointer is correctly pointing to the callback function I define. But > when I trigger a load_policy my callback function is never called. > > I am not familiar with how the callbacks in libselinux work under the > hood so I am unsure about what could be blocking them in this situation. Caveat: I haven't looked deeply so take this with a grain of salt (or two). There are generally two ways of discovering when policy has been reloaded: 1. Create and receive notifications on a SELinux netlink socket, or 2. Map the SELinux status page and poll it for updates to the policy seqno. Internally libselinux has switched to using the status page whenever the kernel supports it since doing so is more efficient (no syscall required to read it once you've mapped the page). As an aside, the status page is also more easily "virtualizable" for SELinux namespaces since it is per-SELinux state/namespace already (the netlink socket can also be virtualized via a separate network namespace if/when my namespace patches land but that requires you to unshare the network namespace too). As far as libselinux APIs are concerned for the status page, you can check for a policy reload or enforcing mode change by calling selinux_status_updated() at any time after having done an initial selinux_status_open(). selinux_status_updated() will call any registered callbacks if enforcing mode or policy was changed, and, it returns an indicator as to whether anything changed since the last time it was called. Or if you choose to use the netlink socket and want to use the libselinux APIs, you'd call avc_netlink_acquire_fd() to create and take ownership of the SELinux netlink socket and then poll/select on it for notifications, and upon receiving them, calling avc_netlink_check_nb() to process them, including calling your callbacks. But you have to do one of those two things in order for your callback to be invoked. I assume dbus and dbus-broker are already doing one of them which is why it works for them but not for the launcher.
On Fri, Oct 18, 2024 at 10:44 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Thu, Oct 17, 2024 at 3:42 PM Matthew Sheets > <masheets@linux.microsoft.com> wrote: > > > > Hi All, > > > > I am currently working on an update for dbus-broker to trigger reload of > > its configuration whenever an SELinux policy load event is seen. > > > > For some background dbus-broker is comprised of two major elements the > > launcher and the broker. To trigger a config reload you can either send > > a SIGHUP to the launcher or send a message to the launcher over dbus. > > In most cases the launcher will be the brokers parent. > > > > Here is a link to my current PR: > > https://github.com/bus1/dbus-broker/pull/379 > > > > In this current state things work. The broker will see the POLICY_LOAD > > event and properly send a SIGHUP to its parent, but as David pointed out > > my initial attempt at the fix is no good since there is no guarantee > > that the brokers parent will be the launcher. > > > > My attempts at moving the callback registration into the launcher have > > been less successful. From what my debugging has told me is that the > > selinux_set_callback is going through successfully and the function > > pointer is correctly pointing to the callback function I define. But > > when I trigger a load_policy my callback function is never called. > > > > I am not familiar with how the callbacks in libselinux work under the > > hood so I am unsure about what could be blocking them in this situation. > > Caveat: I haven't looked deeply so take this with a grain of salt (or two). > There are generally two ways of discovering when policy has been reloaded: > 1. Create and receive notifications on a SELinux netlink socket, or > 2. Map the SELinux status page and poll it for updates to the policy seqno. > > Internally libselinux has switched to using the status page whenever > the kernel supports it since doing so is more efficient (no syscall > required to read it once you've mapped the page). As an aside, the > status page is also more easily "virtualizable" for SELinux namespaces > since it is per-SELinux state/namespace already (the netlink socket > can also be virtualized via a separate network namespace if/when my > namespace patches land but that requires you to unshare the network > namespace too). > > As far as libselinux APIs are concerned for the status page, you can > check for a policy reload or enforcing mode change by calling > selinux_status_updated() at any time after having done an initial > selinux_status_open(). selinux_status_updated() will call any > registered callbacks if enforcing mode or policy was changed, and, it > returns an indicator as to whether anything changed since the last > time it was called. > > Or if you choose to use the netlink socket and want to use the > libselinux APIs, you'd call avc_netlink_acquire_fd() to create and > take ownership of the SELinux netlink socket and then poll/select on > it for notifications, and upon receiving them, calling > avc_netlink_check_nb() to process them, including calling your > callbacks. > > But you have to do one of those two things in order for your callback > to be invoked. I assume dbus and dbus-broker are already doing one of > them which is why it works for them but not for the launcher. Also, both avc_has_perm*() and selinux_check_access() internally call selinux_status_updated() to ensure that they are using the latest enforcing mode and policy. So anything using those libselinux functions would get the status update for free.
On 10/18/2024 7:52 AM, Stephen Smalley wrote: > On Fri, Oct 18, 2024 at 10:44 AM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: >> >> On Thu, Oct 17, 2024 at 3:42 PM Matthew Sheets >> <masheets@linux.microsoft.com> wrote: >>> >>> Hi All, >>> >>> I am currently working on an update for dbus-broker to trigger reload of >>> its configuration whenever an SELinux policy load event is seen. >>> >>> For some background dbus-broker is comprised of two major elements the >>> launcher and the broker. To trigger a config reload you can either send >>> a SIGHUP to the launcher or send a message to the launcher over dbus. >>> In most cases the launcher will be the brokers parent. >>> >>> Here is a link to my current PR: >>> https://github.com/bus1/dbus-broker/pull/379 >>> >>> In this current state things work. The broker will see the POLICY_LOAD >>> event and properly send a SIGHUP to its parent, but as David pointed out >>> my initial attempt at the fix is no good since there is no guarantee >>> that the brokers parent will be the launcher. >>> >>> My attempts at moving the callback registration into the launcher have >>> been less successful. From what my debugging has told me is that the >>> selinux_set_callback is going through successfully and the function >>> pointer is correctly pointing to the callback function I define. But >>> when I trigger a load_policy my callback function is never called. >>> >>> I am not familiar with how the callbacks in libselinux work under the >>> hood so I am unsure about what could be blocking them in this situation. >> >> Caveat: I haven't looked deeply so take this with a grain of salt (or two). >> There are generally two ways of discovering when policy has been reloaded: >> 1. Create and receive notifications on a SELinux netlink socket, or >> 2. Map the SELinux status page and poll it for updates to the policy seqno. >> >> Internally libselinux has switched to using the status page whenever >> the kernel supports it since doing so is more efficient (no syscall >> required to read it once you've mapped the page). As an aside, the >> status page is also more easily "virtualizable" for SELinux namespaces >> since it is per-SELinux state/namespace already (the netlink socket >> can also be virtualized via a separate network namespace if/when my >> namespace patches land but that requires you to unshare the network >> namespace too). >> >> As far as libselinux APIs are concerned for the status page, you can >> check for a policy reload or enforcing mode change by calling >> selinux_status_updated() at any time after having done an initial >> selinux_status_open(). selinux_status_updated() will call any >> registered callbacks if enforcing mode or policy was changed, and, it >> returns an indicator as to whether anything changed since the last >> time it was called. >> >> Or if you choose to use the netlink socket and want to use the >> libselinux APIs, you'd call avc_netlink_acquire_fd() to create and >> take ownership of the SELinux netlink socket and then poll/select on >> it for notifications, and upon receiving them, calling >> avc_netlink_check_nb() to process them, including calling your >> callbacks. >> >> But you have to do one of those two things in order for your callback >> to be invoked. I assume dbus and dbus-broker are already doing one of >> them which is why it works for them but not for the launcher. > > Also, both avc_has_perm*() and selinux_check_access() internally call > selinux_status_updated() to ensure that they are using the latest > enforcing mode and policy. So anything using those libselinux > functions would get the status update for free. Just to close the loop. With the info above I was able to trace down that selinux_status_updated is being called by when the broker calls selinux_check_access. Thus triggering the callback. The launcher has no such check access call. Thanks for your help Stephen.
diff --git a/Makefile b/Makefile index d806160..3b29620 100644 --- a/Makefile +++ b/Makefile @@ -66,6 +66,7 @@ FORCE: MESON_SETUP = \ meson \ setup \ + --prefix=/usr \ --buildtype "debugoptimized" \ --reconfigure \ --warnlevel "2" \ diff --git a/meson_options.txt b/meson_options.txt index 5227a93..3f65964 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -4,6 +4,6 @@ option('docs', type: 'boolean', value: false, description: 'Build documentation' option('launcher', type: 'boolean', value: true, description: 'Build compatibility launcher') option('linux-4-17', type: 'boolean', value: false, description: 'Require linux-4.17 at runtime and make use of its features') option('reference-test', type: 'boolean', value: false, description: 'Run test suite against reference implementation') -option('selinux', type: 'boolean', value: false, description: 'SELinux support') +option('selinux', type: 'boolean', value: true, description: 'SELinux support') option('system-console-users', type: 'array', value: [], description: 'Additional set of names of system-users to be considered at-console') option('tests', type: 'boolean', value: false, description: 'Include tests in the distribution') diff --git a/src/launch/main.c b/src/launch/main.c index ed08e85..46cb5f0 100644 --- a/src/launch/main.c +++ b/src/launch/main.c @@ -9,6 +9,7 @@ #include <systemd/sd-daemon.h> #include "launch/launcher.h" #include "util/error.h" +#include "util/selinux.h" enum { _MAIN_SUCCESS, @@ -164,6 +165,8 @@ int main(int argc, char **argv) { if (r) goto exit; + bus_selinux_register_load_cb(); + sigemptyset(&mask_new); sigaddset(&mask_new, SIGCHLD); sigaddset(&mask_new, SIGTERM); diff --git a/src/util/selinux-fallback.c b/src/util/selinux-fallback.c index 0654a07..7e00c3c 100644 --- a/src/util/selinux-fallback.c +++ b/src/util/selinux-fallback.c @@ -60,3 +60,7 @@ int bus_selinux_init_global(void) { void bus_selinux_deinit_global(void) { return; } + +void bus_selinux_register_load_cb(void) { + return; +} diff --git a/src/util/selinux.c b/src/util/selinux.c index a72cc0a..c9ed99d 100644 --- a/src/util/selinux.c +++ b/src/util/selinux.c @@ -6,6 +6,7 @@ #include <c-stdaux.h> #include <selinux/selinux.h> #include <selinux/avc.h> +#include <signal.h> #include <stdlib.h> #include "util/audit.h" #include "util/error.h" @@ -413,3 +414,50 @@ void bus_selinux_deinit_global(void) { bus_selinux_avc_open = false; } } + +/** + * On a policy reload we need to reparse the SELinux configuration file, since + * this could have changed. Send a SIGHUP to reload all configs. + */ +static int policy_reload_callback(int seqno) { + return raise(SIGHUP); +} + +/** + * bus_selinux_register_load_cb() - register SIGHUP callback for policy load + * + * Will register a call back to raise a SIGHUP when a SELinux policy load + * is seen. + */ +void bus_selinux_register_load_cb(void) { + int r; + if (!is_selinux_enabled()) + return ; + + if (!bus_selinux_status_open) { + r = selinux_status_open(0); + if (r == 0) { + /* + * The status page was successfully opened and can now + * be used for faster selinux status-checks. + */ + bus_selinux_status_open = true; + } else if (r > 0) { + /* + * >0 indicates success but with the netlink-fallback. + * We didn't request the netlink-fallback, so close the + * status-page again and treat it as unavailable. + */ + selinux_status_close(); + } else { + /* + * If the status page could not be opened, treat it as + * unavailable and use the slower fallback functions. + */ + } + } + + selinux_set_callback(SELINUX_CB_POLICYLOAD, (union selinux_callback)policy_reload_callback); + + return; +} diff --git a/src/util/selinux.h b/src/util/selinux.h index 435c8a8..d5452fd 100644 --- a/src/util/selinux.h +++ b/src/util/selinux.h @@ -36,3 +36,5 @@ int bus_selinux_check_send(BusSELinuxRegistry *registry, int bus_selinux_init_global(void); void bus_selinux_deinit_global(void); + +void bus_selinux_register_load_cb(void);