From bab50e162ed829d18dc06da8639392f467b84486 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 22 Mar 2024 12:34:55 -0400 Subject: [PATCH 1/3] install: Change no-SELinux -> SELinux to a warning We believe we have almost all the labeling work here covered, so degrade this to a warning. Signed-off-by: Colin Walters --- .github/workflows/ci.yml | 2 +- lib/src/install.rs | 14 +++----------- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 90b39f55..bb022e86 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -143,6 +143,6 @@ jobs: # TODO fix https://github.com/containers/bootc/pull/137 sudo chattr -i /ostree/deploy/default/deploy/* sudo rm /ostree/deploy/default -rf - sudo podman run --rm -ti --privileged --env BOOTC_SKIP_SELINUX_HOST_CHECK=1 --env RUST_LOG=debug -v /:/target -v /var/lib/containers:/var/lib/containers -v ./usr/bin/bootc:/usr/bin/bootc --pid=host --security-opt label=disable \ + sudo podman run --rm -ti --privileged --env RUST_LOG=debug -v /:/target -v /var/lib/containers:/var/lib/containers -v ./usr/bin/bootc:/usr/bin/bootc --pid=host --security-opt label=disable \ ${image} bootc install to-existing-root sudo podman run --rm -ti --privileged -v /:/target -v ./usr/bin/bootc:/usr/bin/bootc --pid=host --security-opt label=disable ${image} bootc internal-tests verify-selinux /target/ostree --warn diff --git a/lib/src/install.rs b/lib/src/install.rs index 96002fb6..08a58211 100644 --- a/lib/src/install.rs +++ b/lib/src/install.rs @@ -788,10 +788,6 @@ pub(crate) fn reexecute_self_for_selinux_if_needed( let mut ret_did_override = false; // If the target state has SELinux enabled, we need to check the host state. let mut g = None; - // We don't currently quite support installing SELinux enabled systems - // from SELinux disabled hosts, but this environment variable can be set - // to test it out anyways. - let skip_check_envvar = "BOOTC_SKIP_SELINUX_HOST_CHECK"; if srcdata.selinux { let host_selinux = crate::lsm::selinux_enabled()?; tracing::debug!("Target has SELinux, host={host_selinux}"); @@ -807,14 +803,10 @@ pub(crate) fn reexecute_self_for_selinux_if_needed( setup_sys_mount("selinuxfs", SELINUXFS)?; // This will re-execute the current process (once). g = crate::lsm::selinux_ensure_install_or_setenforce()?; - } else if std::env::var_os(skip_check_envvar).is_some() { - eprintln!( - "Host kernel does not have SELinux support, but target enables it by default; {} is set, continuing anyways", - skip_check_envvar - ); } else { - anyhow::bail!( - "Host kernel does not have SELinux support, but target enables it by default" + // This used to be a hard error, but is now a mild warning + crate::utils::medium_visibility_warning( + "Host kernel does not have SELinux support, but target enables it by default; this is less well tested. See https://github.com/containers/bootc/issues/419", ); } } else { From 201c4391a110d1dfe993672915e68797bdfd3950 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 22 Mar 2024 13:18:35 -0400 Subject: [PATCH 2/3] install: Change SELinux state into enum, serialize to aleph We want to support the "installing SELinux target from SELinux-disabled host" - but in case we run into problems, let's serialize the state of things at install time into the aleph data, for the same reason we save other relevant environmental data like the kernel version. Signed-off-by: Colin Walters --- lib/src/install.rs | 68 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 50 insertions(+), 18 deletions(-) diff --git a/lib/src/install.rs b/lib/src/install.rs index 08a58211..26ef7884 100644 --- a/lib/src/install.rs +++ b/lib/src/install.rs @@ -288,9 +288,7 @@ pub(crate) struct SourceInfo { pub(crate) struct State { pub(crate) source: SourceInfo, /// Force SELinux off in target system - pub(crate) override_disable_selinux: bool, - #[allow(dead_code)] - pub(crate) setenforce_guard: Option, + pub(crate) selinux_state: SELinuxFinalState, #[allow(dead_code)] pub(crate) config_opts: InstallConfigOpts, pub(crate) target_imgref: ostree_container::OstreeImageReference, @@ -303,7 +301,7 @@ impl State { #[context("Loading SELinux policy")] pub(crate) fn load_policy(&self) -> Result> { use std::os::fd::AsRawFd; - if !self.source.selinux || self.override_disable_selinux { + if !self.selinux_state.enabled() { return Ok(None); } // We always use the physical container root to bootstrap policy @@ -334,6 +332,8 @@ struct InstallAleph { timestamp: Option>, /// The `uname -r` of the kernel doing the installation kernel: String, + /// The state of SELinux at install time + selinux: String, } /// A mount specification is a subset of a line in `/etc/fstab`. @@ -687,6 +687,7 @@ async fn initialize_ostree_root_from_self( version: imgstate.version().as_ref().map(|s| s.to_string()), timestamp, kernel: uname.release().to_str()?.to_string(), + selinux: state.selinux_state.to_aleph().to_string(), }; Ok(aleph) @@ -777,6 +778,38 @@ impl RootSetup { } } +pub(crate) enum SELinuxFinalState { + /// Host and target both have SELinux, but user forced it off for target + ForceTargetDisabled, + /// Host and target both have SELinux + Enabled(Option), + /// Host has SELinux disabled, target is enabled. + HostDisabled, + /// Neither host or target have SELinux + Disabled, +} + +impl SELinuxFinalState { + /// Returns true if the target system will have SELinux enabled. + pub(crate) fn enabled(&self) -> bool { + match self { + SELinuxFinalState::ForceTargetDisabled | SELinuxFinalState::Disabled => false, + SELinuxFinalState::Enabled(_) | SELinuxFinalState::HostDisabled => true, + } + } + + /// Returns the canonical stringified version of self. This is only used + /// for debugging purposes. + pub(crate) fn to_aleph(&self) -> &'static str { + match self { + SELinuxFinalState::ForceTargetDisabled => "force-target-disabled", + SELinuxFinalState::Enabled(_) => "enabled", + SELinuxFinalState::HostDisabled => "host-disabled", + SELinuxFinalState::Disabled => "disabled", + } + } +} + /// If we detect that the target ostree commit has SELinux labels, /// and we aren't passed an override to disable it, then ensure /// the running process is labeled with install_t so it can @@ -784,16 +817,14 @@ impl RootSetup { pub(crate) fn reexecute_self_for_selinux_if_needed( srcdata: &SourceInfo, override_disable_selinux: bool, -) -> Result<(bool, Option)> { - let mut ret_did_override = false; +) -> Result { // If the target state has SELinux enabled, we need to check the host state. - let mut g = None; if srcdata.selinux { let host_selinux = crate::lsm::selinux_enabled()?; tracing::debug!("Target has SELinux, host={host_selinux}"); - if override_disable_selinux { - ret_did_override = true; - println!("notice: Target has SELinux enabled, overriding to disable") + let r = if override_disable_selinux { + println!("notice: Target has SELinux enabled, overriding to disable"); + SELinuxFinalState::ForceTargetDisabled } else if host_selinux { // /sys/fs/selinuxfs is not normally mounted, so we do that now. // Because SELinux enablement status is cached process-wide and was very likely @@ -802,17 +833,20 @@ pub(crate) fn reexecute_self_for_selinux_if_needed( // so let's just fall through to that. setup_sys_mount("selinuxfs", SELINUXFS)?; // This will re-execute the current process (once). - g = crate::lsm::selinux_ensure_install_or_setenforce()?; + let g = crate::lsm::selinux_ensure_install_or_setenforce()?; + SELinuxFinalState::Enabled(g) } else { // This used to be a hard error, but is now a mild warning crate::utils::medium_visibility_warning( "Host kernel does not have SELinux support, but target enables it by default; this is less well tested. See https://github.com/containers/bootc/issues/419", ); - } + SELinuxFinalState::HostDisabled + }; + Ok(r) } else { tracing::debug!("Target does not enable SELinux"); + Ok(SELinuxFinalState::Disabled) } - Ok((ret_did_override, g)) } /// Trim, flush outstanding writes, and freeze/thaw the target mounted filesystem; @@ -1071,8 +1105,7 @@ async fn prepare_install( setup_sys_mount("efivarfs", EFIVARFS)?; // Now, deal with SELinux state. - let (override_disable_selinux, setenforce_guard) = - reexecute_self_for_selinux_if_needed(&source, config_opts.disable_selinux)?; + let selinux_state = reexecute_self_for_selinux_if_needed(&source, config_opts.disable_selinux)?; println!("Installing image: {:#}", &target_imgref); if let Some(digest) = source.digest.as_deref() { @@ -1094,8 +1127,7 @@ async fn prepare_install( // so we can pass it to worker threads too. Right now this just // combines our command line options along with some bind mounts from the host. let state = Arc::new(State { - override_disable_selinux, - setenforce_guard, + selinux_state, source, config_opts, target_imgref, @@ -1107,7 +1139,7 @@ async fn prepare_install( } async fn install_to_filesystem_impl(state: &State, rootfs: &mut RootSetup) -> Result<()> { - if state.override_disable_selinux { + if matches!(state.selinux_state, SELinuxFinalState::ForceTargetDisabled) { rootfs.kargs.push("selinux=0".to_string()); } From 7842a61493e5281e00101c30c6af5f198dd89947 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 22 Mar 2024 14:13:27 -0400 Subject: [PATCH 3/3] install: Change to consume SELinux guard (and Arc) This avoids a dead code warning on newer rustc. Also, it's just better because if we fail to re-invoke `setenforce 1` this should be a fatal error probably. Signed-off-by: Colin Walters --- lib/src/install.rs | 14 ++++++++++++++ lib/src/lsm.rs | 23 ++++++++++++++++++++--- 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/lib/src/install.rs b/lib/src/install.rs index 26ef7884..14d2a811 100644 --- a/lib/src/install.rs +++ b/lib/src/install.rs @@ -1242,6 +1242,20 @@ pub(crate) async fn install_to_disk(mut opts: InstallToDiskOpts) -> Result<()> { loopback_dev.close()?; } + // At this point, all other threads should be gone. + if let Some(state) = Arc::into_inner(state) { + // If we had invoked `setenforce 0`, then let's re-enable it. + match state.selinux_state { + SELinuxFinalState::Enabled(Some(guard)) => { + guard.consume()?; + } + _ => {} + } + } else { + // This shouldn't happen...but we will make it not fatal right now + tracing::warn!("Failed to consume state Arc"); + } + installation_complete(); Ok(()) diff --git a/lib/src/lsm.rs b/lib/src/lsm.rs index 1d56eb45..d53381a2 100644 --- a/lib/src/lsm.rs +++ b/lib/src/lsm.rs @@ -99,12 +99,29 @@ pub(crate) fn selinux_ensure_install() -> Result { /// gain the `mac_admin` permission (install_t). #[cfg(feature = "install")] #[must_use] -pub(crate) struct SetEnforceGuard; +pub(crate) struct SetEnforceGuard(Option<()>); + +#[cfg(feature = "install")] +impl SetEnforceGuard { + pub(crate) fn new() -> Self { + SetEnforceGuard(Some(())) + } + + pub(crate) fn consume(mut self) -> Result<()> { + // SAFETY: The option cannot have been consumed until now + self.0.take().unwrap(); + // This returns errors + selinux_set_permissive(false) + } +} #[cfg(feature = "install")] impl Drop for SetEnforceGuard { fn drop(&mut self) { - let _ = selinux_set_permissive(false); + // A best-effort attempt to re-enable enforcement on drop (installation failure) + if let Some(()) = self.0.take() { + let _ = selinux_set_permissive(false); + } } } @@ -121,7 +138,7 @@ pub(crate) fn selinux_ensure_install_or_setenforce() -> Result