Merge pull request #1222 from cgwalters/lsm-fix

lsm, imgstorage: Rework relabeling

Author: ckyrouac

lib/src/cli.rs

@@ -436,6 +436,14 @@ pub(crate) enum InternalsOpts {
     Fsck,
     /// Perform cleanup actions
     Cleanup,
+    Relabel {
+        #[clap(long)]
+        /// Relabel using this path as root
+        as_path: Option<Utf8PathBuf>,
+
+        /// Relabel this path
+        path: Utf8PathBuf,
+    },
     /// Proxy frontend for the `ostree-ext` CLI.
     OstreeExt {
         #[clap(allow_hyphen_values = true)]
@@ -1221,6 +1229,14 @@ async fn run_from_opt(opt: Opt) -> Result<()> {
                 let sysroot = get_storage().await?;
                 crate::deploy::cleanup(&sysroot).await
             }
+            InternalsOpts::Relabel { as_path, path } => {
+                let root = &Dir::open_ambient_dir("/", cap_std::ambient_authority())?;
+                let path = path.strip_prefix("/")?;
+                let sepolicy =
+                    &ostree::SePolicy::new(&gio::File::for_path("/"), gio::Cancellable::NONE)?;
+                crate::lsm::relabel_recurse(root, path, as_path.as_deref(), sepolicy)?;
+                Ok(())
+            }
             InternalsOpts::BootcInstallCompletion { sysroot, stateroot } => {
                 let rootfs = &Dir::open_ambient_dir("/", cap_std::ambient_authority())?;
                 crate::install::completion::run_from_ostree(rootfs, &sysroot, &stateroot).await

lib/src/imgstorage.rs

@@ -164,8 +164,11 @@ impl Storage {
         Ok(())
     }
 
+    /// Ensure that the LSM (SELinux) labels are set on the bootc-owned
+    /// containers-storage: instance. We use a `LABELED` stamp file for
+    /// idempotence.
     #[context("Labeling imgstorage dirs")]
-    fn label_dirs(root: &Dir, sepolicy: Option<&ostree::SePolicy>) -> Result<()> {
+    fn ensure_labeled(root: &Dir, sepolicy: Option<&ostree::SePolicy>) -> Result<()> {
         if root.try_exists(LABELED)? {
             return Ok(());
         }
@@ -175,28 +178,14 @@ impl Storage {
 
         // recursively set the labels because they were previously set to usr_t,
         // and there is no policy defined to set them to the c/storage labels
-        crate::lsm::ensure_dir_labeled_recurse_policy(
+        crate::lsm::relabel_recurse(
             &root,
             ".",
             Some(Utf8Path::new("/var/lib/containers/storage")),
-            0o755.into(),
-            Some(sepolicy),
+            sepolicy,
         )
         .context("labeling storage root")?;
 
-        let paths = ["overlay", "overlay-images", "overlay-layers", "volumes"];
-        for p in paths {
-            let full_label_path = format!("/var/lib/containers/storage/{}", p);
-            crate::lsm::ensure_dir_labeled_recurse_policy(
-                &root,
-                p,
-                Some(Utf8Path::new(full_label_path.as_str())),
-                0o755.into(),
-                Some(sepolicy),
-            )
-            .context(format!("labeling storage subpath: {}", p))?;
-        }
-
         root.create(LABELED)?;
 
         Ok(())
@@ -224,7 +213,6 @@ impl Storage {
                 .with_context(|| format!("Creating {parent}"))?;
             sysroot.create_dir_all(&tmp).context("Creating tmpdir")?;
             let storage_root = sysroot.open_dir(&tmp).context("Open tmp")?;
-            Self::label_dirs(&storage_root, sepolicy)?;
 
             // There's no explicit API to initialize a containers-storage:
             // root, simply passing a path will attempt to auto-create it.
@@ -234,6 +222,7 @@ impl Storage {
                 .arg("images")
                 .run()
                 .context("Initializing images")?;
+            Self::ensure_labeled(&storage_root, sepolicy)?;
             drop(storage_root);
             sysroot
                 .rename(&tmp, sysroot, subpath)
@@ -242,7 +231,7 @@ impl Storage {
         } else {
             // the storage already exists, make sure it has selinux labels
             let storage_root = sysroot.open_dir(subpath).context("opening storage dir")?;
-            Self::label_dirs(&storage_root, sepolicy)?;
+            Self::ensure_labeled(&storage_root, sepolicy)?;
         }
 
         Self::open(sysroot, run)

lib/src/lsm.rs

@@ -1,3 +1,4 @@
+use std::borrow::Cow;
 use std::io::Write;
 use std::os::fd::AsRawFd;
 use std::os::unix::process::CommandExt;
@@ -230,8 +231,11 @@ pub(crate) fn has_security_selinux(root: &Dir, path: &Utf8Path) -> Result<SELinu
     }
 }
 
+/// Directly set the `security.selinux` extended atttribute on the target
+/// path. Symbolic links are not followed for the target.
+///
+/// Note that this API will work even if SELinux is disabled.
 pub(crate) fn set_security_selinux_path(root: &Dir, path: &Utf8Path, label: &[u8]) -> Result<()> {
-    // TODO: avoid hardcoding a max size here
     let fdpath = format!("/proc/self/fd/{}/", root.as_raw_fd());
     let fdpath = &Path::new(&fdpath).join(path);
     rustix::fs::lsetxattr(
@@ -243,6 +247,9 @@ pub(crate) fn set_security_selinux_path(root: &Dir, path: &Utf8Path, label: &[u8
     Ok(())
 }
 
+/// Given a policy, ensure the target file path has a security.selinux label.
+/// If the path already is labeled, this function is a no-op, even if
+/// the policy would default to a different label.
 pub(crate) fn ensure_labeled(
     root: &Dir,
     path: &Utf8Path,
@@ -251,44 +258,97 @@ pub(crate) fn ensure_labeled(
 ) -> Result<SELinuxLabelState> {
     let r = has_security_selinux(root, path)?;
     if matches!(r, SELinuxLabelState::Unlabeled) {
-        let abspath = Utf8Path::new("/").join(&path);
-        let label = require_label(policy, &abspath, metadata.mode())?;
-        tracing::trace!("Setting label for {path} to {label}");
-        set_security_selinux_path(root, &path, label.as_bytes())?;
+        relabel(root, metadata, path, None, policy)?;
     }
     Ok(r)
 }
 
-pub(crate) fn ensure_dir_labeled_recurse_policy(
+/// Given the policy, relabel the target file or directory.
+/// Optionally, an override for the path can be provided
+/// to set the label as if the target has that filename.
+pub(crate) fn relabel(
     root: &Dir,
-    destname: impl AsRef<Utf8Path>,
+    metadata: &Metadata,
+    path: &Utf8Path,
     as_path: Option<&Utf8Path>,
-    mode: rustix::fs::Mode,
-    policy: Option<&ostree::SePolicy>,
+    policy: &ostree::SePolicy,
+) -> Result<()> {
+    assert!(!path.starts_with("/"));
+    let as_path = as_path
+        .map(Cow::Borrowed)
+        .unwrap_or_else(|| Utf8Path::new("/").join(path).into());
+    let label = require_label(policy, &as_path, metadata.mode())?;
+    tracing::trace!("Setting label for {path} to {label}");
+    set_security_selinux_path(root, &path, label.as_bytes())
+}
+
+pub(crate) fn relabel_recurse_inner(
+    root: &Dir,
+    path: &mut Utf8PathBuf,
+    mut as_path: Option<&mut Utf8PathBuf>,
+    policy: &ostree::SePolicy,
 ) -> Result<()> {
-    ensure_dir_labeled(root, &destname, as_path, mode, policy)?;
-    let mut dest_path = destname.as_ref().to_path_buf();
+    // Relabel this directory
+    let self_meta = root.dir_metadata()?;
+    relabel(
+        root,
+        &self_meta,
+        path,
+        as_path.as_ref().map(|p| p.as_path()),
+        policy,
+    )?;
 
-    for ent in root.read_dir(destname.as_ref())? {
+    // Relabel all children
+    for ent in root.read_dir(&path)? {
         let ent = ent?;
         let metadata = ent.metadata()?;
         let name = ent.file_name();
         let name = name
             .to_str()
             .ok_or_else(|| anyhow::anyhow!("Invalid non-UTF-8 filename: {name:?}"))?;
-        dest_path.push(name);
+        // Extend both copies of the path
+        path.push(name);
+        if let Some(p) = as_path.as_mut() {
+            p.push(name);
+        }
 
         if metadata.is_dir() {
-            ensure_dir_labeled_recurse_policy(root, &dest_path, as_path, mode, policy)?;
+            let as_path = as_path.as_deref_mut();
+            relabel_recurse_inner(root, path, as_path, policy)?;
         } else {
-            ensure_dir_labeled(root, &dest_path, as_path, mode, policy)?
+            let as_path = as_path.as_ref().map(|p| p.as_path());
+            relabel(root, &metadata, &path, as_path, policy)?
+        }
+        // Trim what we added to the path
+        let r = path.pop();
+        assert!(r);
+        if let Some(p) = as_path.as_mut() {
+            let r = p.pop();
+            assert!(r);
         }
-        dest_path.pop();
     }
 
     Ok(())
 }
 
+/// Recursively relabel the target directory.
+pub(crate) fn relabel_recurse(
+    root: &Dir,
+    path: impl AsRef<Utf8Path>,
+    as_path: Option<&Utf8Path>,
+    policy: &ostree::SePolicy,
+) -> Result<()> {
+    let mut path = path.as_ref().to_owned();
+    // This path must be relative, as we access via cap-std
+    assert!(!path.starts_with("/"));
+    let mut as_path = as_path.map(|v| v.to_owned());
+    // But the as_path must be absolute, if provided
+    if let Some(as_path) = as_path.as_deref() {
+        assert!(as_path.starts_with("/"));
+    }
+    relabel_recurse_inner(root, &mut path, as_path.as_mut(), policy)
+}
+
 /// A wrapper for creating a directory, also optionally setting a SELinux label.
 /// The provided `skip` parameter is a device/inode that we will ignore (and not traverse).
 pub(crate) fn ensure_dir_labeled_recurse(

tests/booted/readonly/020-test-relabel.nu

@@ -0,0 +1,26 @@
+use std assert
+use tap.nu
+
+tap begin "Relabel"
+
+let td = mktemp -d -p /var/tmp
+cd $td
+
+mkdir etc/ssh
+touch etc/shadow etc/ssh/ssh_config
+bootc internals relabel --as-path /etc $"(pwd)/etc"
+
+def assert_labels_equal [p] {
+    let base = (getfattr --only-values -n security.selinux $"/($p)")
+    let target = (getfattr --only-values -n security.selinux $p)
+    assert equal $base $target
+}
+
+for path in ["etc", "etc/shadow", "etc/ssh/ssh_config"] {
+    assert_labels_equal $path
+}
+
+cd /
+rm -rf $td
+
+tap ok

tests/booted/test-logically-bound-install.nu

@@ -35,11 +35,22 @@ def test_bootc_image_list [] {
     validate_images $images
 }
 
+# Get just the type (foo_t) from a security context
+def get_file_selinux_type [p] {
+    getfattr --only-values -n security.selinux $p | split row ':' | get 2
+}
+
+# Verify that the SELinux labels on the main "containers-storage:" instance match ours.
+# See the relabeling we do in imgstorage.rs. We only verify types, because the role
+# may depend on the creating user.
 def test_storage_labels [] {
-    let root_labeled = getfattr -n security.selinux /var/lib/containers/storage | grep container_var_lib_t | complete
-    assert equal $root_labeled.exit_code 0
-    let overlay_labeled = getfattr -n security.selinux /usr/lib/bootc/storage/overlay | grep container_ro_file_t | complete
-    assert equal $overlay_labeled.exit_code 0
+    for v in [".", "overlay-images", "defaultNetworkBackend"] {
+        let base = (get_file_selinux_type $"/var/lib/containers/storage/($v)")
+        let target = (get_file_selinux_type $"/usr/lib/bootc/storage/($v)")
+        assert equal $base $target
+    }
+    # Verify the stamp file exists
+    test -f /usr/lib/bootc/storage/.bootc_labeled
 }
 
 test_logically_bound_images_in_storage