Merge pull request #76 from nix-community/smart-systemd-boot-install

tool: smarter systemd-boot install

nix/modules/lanzaboote.nix

@@ -59,6 +59,7 @@ in
         ''}
   
         ${cfg.package}/bin/lzbt install \
+          --systemd ${pkgs.systemd} \
           --public-key ${cfg.publicKeyFile} \
           --private-key ${cfg.privateKeyFile} \
           --configuration-limit ${toString configurationLimit} \

rust/tool/src/cli.rs

@@ -19,6 +19,10 @@ enum Commands {
 
 #[derive(Parser)]
 struct InstallCommand {
+    /// Systemd path
+    #[arg(long)]
+    systemd: PathBuf,
+
     /// sbsign Public Key
     #[arg(long)]
     public_key: PathBuf,
@@ -60,6 +64,7 @@ fn install(args: InstallCommand) -> Result<()> {
 
     install::Installer::new(
         PathBuf::from(lanzaboote_stub),
+        args.systemd,
         key_pair,
         args.configuration_limit,
         args.esp,

rust/tool/src/install.rs

@@ -1,7 +1,9 @@
+use std::ffi::CStr;
 use std::fs;
 use std::os::unix::prelude::PermissionsExt;
 use std::path::{Path, PathBuf};
 use std::process::Command;
+use std::str::FromStr;
 
 use anyhow::{Context, Result};
 use nix::unistd::sync;
@@ -17,6 +19,7 @@ use crate::utils::SecureTempDirExt;
 pub struct Installer {
     gc_roots: Roots,
     lanzaboote_stub: PathBuf,
+    systemd: PathBuf,
     key_pair: KeyPair,
     configuration_limit: usize,
     esp_paths: EspPaths,
@@ -26,6 +29,7 @@ pub struct Installer {
 impl Installer {
     pub fn new(
         lanzaboote_stub: PathBuf,
+        systemd: PathBuf,
         key_pair: KeyPair,
         configuration_limit: usize,
         esp: PathBuf,
@@ -38,6 +42,7 @@ impl Installer {
         Self {
             gc_roots,
             lanzaboote_stub,
+            systemd,
             key_pair,
             configuration_limit,
             esp_paths,
@@ -66,6 +71,8 @@ impl Installer {
         };
         self.install_links(links)?;
 
+        self.install_systemd_boot()?;
+
         // Only collect garbage in these two directories. This way, no files that do not belong to
         // the NixOS installation are deleted. Lanzatool takes full control over the esp/EFI/nixos
         // directory and deletes ALL files that it doesn't know about. Dual- or multiboot setups
@@ -148,24 +155,17 @@ impl Installer {
             append_initrd_secrets(initrd_secrets_script, &initrd_location)?;
         }
 
-        let systemd_boot = bootspec
+        // The initrd doesn't need to be signed. The stub has its hash embedded and will refuse
-            .toplevel
+        // loading it when the hash mismatches.
-            .0
+        //
-            .join("systemd/lib/systemd/boot/efi/systemd-bootx64.efi");
+        // The initrd and kernel are not forcibly installed because they are not built
-
+        // reproducibly. Forcibly installing (i.e. overwriting) them is likely to break older
-        [
+        // generations that point to the same initrd/kernel because the hash embedded in the stub
-            (&systemd_boot, &self.esp_paths.efi_fallback),
+        // will not match anymore.
-            (&systemd_boot, &self.esp_paths.systemd_boot),
-            (&bootspec.kernel, &esp_gen_paths.kernel),
-        ]
-        .into_iter()
-        .try_for_each(|(from, to)| install_signed(&self.key_pair, from, to))?;
-
-        // The initrd doesn't need to be signed. Lanzaboote has its
-        // hash embedded and will refuse loading it when the hash
-        // mismatches.
         install(&initrd_location, &esp_gen_paths.initrd)
             .context("Failed to install initrd to ESP")?;
+        install_signed(&self.key_pair, &bootspec.kernel, &esp_gen_paths.kernel)
+            .context("Failed to install kernel to ESP.")?;
 
         let lanzaboote_image = pe::lanzaboote_image(
             &tempdir,
@@ -197,6 +197,33 @@ impl Installer {
 
         Ok(())
     }
+
+    /// Install systemd-boot to ESP.
+    ///
+    /// systemd-boot is only updated when a newer version is available OR when the currently
+    /// installed version is not signed. This enables switching to Lanzaboote without having to
+    /// manually delete previous unsigned systemd-boot binaries and minimizes the number of writes
+    /// to the ESP.
+    ///
+    /// Checking for the version also allows us to skip buggy systemd versions in the future.
+    fn install_systemd_boot(&self) -> Result<()> {
+        let systemd_boot = self
+            .systemd
+            .join("lib/systemd/boot/efi/systemd-bootx64.efi");
+
+        let paths = [
+            (&systemd_boot, &self.esp_paths.efi_fallback),
+            (&systemd_boot, &self.esp_paths.systemd_boot),
+        ];
+
+        for (from, to) in paths {
+            if newer_systemd_boot(from, to)? || !&self.key_pair.verify(to) {
+                force_install_signed(&self.key_pair, from, to)
+                    .with_context(|| format!("Failed to install systemd-boot binary to: {to:?}"))?;
+            }
+        }
+        Ok(())
+    }
 }
 
 /// Install a PE file. The PE gets signed in the process.
@@ -206,16 +233,24 @@ fn install_signed(key_pair: &KeyPair, from: &Path, to: &Path) -> Result<()> {
     if to.exists() {
         println!("{} already exists, skipping...", to.display());
     } else {
-        println!("Signing and installing {}...", to.display());
+        force_install_signed(key_pair, from, to)?;
-        ensure_parent_dir(to);
-        key_pair
-            .sign_and_copy(from, to)
-            .with_context(|| format!("Failed to copy and sign file from {:?} to {:?}", from, to))?;
     }
 
     Ok(())
 }
 
+/// Sign and forcibly install a PE file.
+///
+/// If the file already exists at the destination, it is overwritten.
+fn force_install_signed(key_pair: &KeyPair, from: &Path, to: &Path) -> Result<()> {
+    println!("Signing and installing {}...", to.display());
+    ensure_parent_dir(to);
+    key_pair
+        .sign_and_copy(from, to)
+        .with_context(|| format!("Failed to copy and sign file from {from:?} to {to:?}"))?;
+    Ok(())
+}
+
 /// Install an arbitrary file
 ///
 /// The file is only copied if it doesn't exist at the destination
@@ -281,3 +316,57 @@ fn ensure_parent_dir(path: &Path) {
         fs::create_dir_all(parent).ok();
     }
 }
+
+/// Determine if a newer systemd-boot version is available.
+///
+/// "Newer" can mean
+///   (1) no file exists at the destination,
+///   (2) the file at the destination is malformed,
+///   (3) a binary with a higher version is available.
+fn newer_systemd_boot(from: &Path, to: &Path) -> Result<bool> {
+    // If the file doesn't exists at the destination, it should be installed.
+    if !to.exists() {
+        return Ok(true);
+    }
+
+    // If the version from the source binary cannot be read, something is irrecoverably wrong.
+    let from_version = systemd_boot_version(from)
+        .with_context(|| format!("Failed to read systemd-boot version from {from:?}."))?;
+
+    // If the version cannot be read from the destination binary, it is malformed. It should be
+    // forcibly reinstalled.
+    let to_version = match systemd_boot_version(to) {
+        Ok(version) => version,
+        _ => return Ok(true),
+    };
+
+    Ok(from_version > to_version)
+}
+
+/// Read the version of a systemd-boot binary from its `.osrel` section.
+///
+/// The version is parsed into a f32 because systemd does not follow strict semver conventions. A
+/// f32, however, should parse all systemd versions and enables usefully comparing them.
+/// This is a hack and we should replace it with a better solution once we find one.
+fn systemd_boot_version(path: &Path) -> Result<f32> {
+    let file_data = fs::read(path).with_context(|| format!("Failed to read file {path:?}"))?;
+    let section_data = pe::read_section_data(&file_data, ".osrel")
+        .with_context(|| format!("PE section '.osrel' is empty: {path:?}"))?;
+
+    // The `.osrel` section in the systemd-boot binary is a NUL terminated string and thus needs
+    // special handling.
+    let section_data_cstr =
+        CStr::from_bytes_with_nul(section_data).context("Failed to parse C string.")?;
+    let section_data_string = section_data_cstr
+        .to_str()
+        .context("Failed to convert C string to Rust string.")?;
+
+    let os_release = OsRelease::from_str(section_data_string)
+        .with_context(|| format!("Failed to parse os-release from {section_data_string}"))?;
+
+    let version_string = os_release
+        .0
+        .get("VERSION")
+        .context("Failed to extract VERSION key from: {os_release:#?}")?;
+    Ok(f32::from_str(version_string)?)
+}

rust/tool/src/os_release.rs

@@ -13,7 +13,7 @@ use crate::generation::Generation;
 /// The BTreeMap is used over a HashMap, so that the keys are ordered. This is irrelevant for
 /// systemd-boot (which does not care about order when reading the os-release file) but is useful
 /// for testing. Ordered keys allow using snapshot tests.
-pub struct OsRelease(BTreeMap<&'static str, String>);
+pub struct OsRelease(pub BTreeMap<String, String>);
 
 impl OsRelease {
     pub fn from_generation(generation: &Generation) -> Result<Self> {
@@ -25,10 +25,10 @@ impl OsRelease {
         //
         // Because the ID field here does not have the same meaning as in a real os-release file,
         // it is fine to use a dummy value.
-        map.insert("ID", String::from("lanza"));
+        map.insert("ID".into(), String::from("lanza"));
-        map.insert("PRETTY_NAME", generation.spec.bootspec.label.clone());
+        map.insert("PRETTY_NAME".into(), generation.spec.bootspec.label.clone());
         map.insert(
-            "VERSION_ID",
+            "VERSION_ID".into(),
             generation
                 .describe()
                 .context("Failed to describe generation.")?,
@@ -36,6 +36,31 @@ impl OsRelease {
 
         Ok(Self(map))
     }
+
+    /// Parse the string representation of a os-release file.
+    ///
+    /// **Beware before reusing this function!**
+    ///
+    /// This parser might not parse all valid os-release files correctly. It is only designed to
+    /// read the `VERSION` key from the os-release of a systemd-boot binary.
+    pub fn from_str(value: &str) -> Result<Self> {
+        let mut map = BTreeMap::new();
+
+        let key_value_lines = value.lines().map(|x| x.split('=').collect::<Vec<&str>>());
+
+        for kv in key_value_lines {
+            let k = kv
+                .first()
+                .with_context(|| format!("Failed to get first element from {kv:?}"))?;
+            let v = kv
+                .get(1)
+                .map(|s| s.trim_matches('"'))
+                .with_context(|| format!("Failed to get second element from {kv:?}"))?;
+            map.insert(String::from(*k), v.into());
+        }
+
+        Ok(Self(map))
+    }
 }
 
 /// Display OsRelease in the format of an os-release file.
@@ -47,3 +72,21 @@ impl fmt::Display for OsRelease {
         Ok(())
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::ffi::CStr;
+
+    #[test]
+    fn parses_correctly_from_str() -> Result<()> {
+        let os_release_cstr = CStr::from_bytes_with_nul(b"ID=systemd-boot\nVERSION=\"252.1\"\n\0")?;
+        let os_release_str = os_release_cstr.to_str()?;
+        let os_release = OsRelease::from_str(os_release_str)?;
+
+        assert!(os_release.0["ID"] == "systemd-boot");
+        assert!(os_release.0["VERSION"] == "252.1");
+
+        Ok(())
+    }
+}

rust/tool/src/pe.rs

@@ -180,6 +180,24 @@ fn file_size(path: impl AsRef<Path>) -> Result<u64> {
         .size())
 }
 
+/// Read the data from a section of a PE binary.
+///
+/// The binary is supplied as a `u8` slice.
+pub fn read_section_data<'a>(file_data: &'a [u8], section_name: &str) -> Option<&'a [u8]> {
+    let pe_binary = goblin::pe::PE::parse(file_data).ok()?;
+
+    pe_binary
+        .sections
+        .iter()
+        .find(|s| s.name().unwrap() == section_name)
+        .and_then(|s| {
+            let section_start: usize = s.pointer_to_raw_data.try_into().ok()?;
+            assert!(s.virtual_size <= s.size_of_raw_data);
+            let section_end: usize = section_start + usize::try_from(s.virtual_size).ok()?;
+            Some(&file_data[section_start..section_end])
+        })
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

rust/tool/src/signature.rs

@@ -43,4 +43,30 @@ impl KeyPair {
 
         Ok(())
     }
+
+    /// Verify the signature of a PE binary. Return true if the signature was verified.
+    pub fn verify(&self, path: &Path) -> bool {
+        let args: Vec<OsString> = vec![
+            OsString::from("--cert"),
+            self.public_key.clone().into(),
+            path.as_os_str().to_owned(),
+        ];
+
+        let output = match Command::new("sbverify").args(&args).output() {
+            Ok(output) => output,
+            Err(_) => return false,
+        };
+
+        if !output.status.success() {
+            if std::io::stderr().write_all(&output.stderr).is_err() {
+                return false;
+            };
+            println!(
+                "Failed to verify signature using sbverify with args `{:?}`",
+                &args
+            );
+            return false;
+        }
+        true
+    }
 }

rust/tool/tests/common/mod.rs

@@ -1,7 +1,6 @@
 use std::ffi::OsStr;
 use std::fs;
 use std::io::Write;
-use std::os::unix::fs::symlink;
 use std::path::{Path, PathBuf};
 use std::process::Output;
 
@@ -74,7 +73,6 @@ fn setup_toplevel(tmpdir: &Path) -> Result<PathBuf> {
 
     let initrd_path = toplevel.join("initrd");
     let kernel_path = toplevel.join("kernel");
-    let systemd_path = toplevel.join("systemd");
     let nixos_version_path = toplevel.join("nixos-version");
     let kernel_modules_path = toplevel.join("kernel-modules/lib/modules/6.1.1");
 
@@ -84,7 +82,6 @@ fn setup_toplevel(tmpdir: &Path) -> Result<PathBuf> {
     // in isolation this should suffice.
     fs::copy(&test_systemd_stub, initrd_path)?;
     fs::copy(&test_systemd_stub, kernel_path)?;
-    symlink(&test_systemd, systemd_path)?;
     fs::write(nixos_version_path, b"23.05")?;
     fs::create_dir_all(kernel_modules_path)?;
 
@@ -114,6 +111,8 @@ pub fn lanzaboote_install(
     let output = cmd
         .env("LANZABOOTE_STUB", test_systemd_stub)
         .arg("install")
+        .arg("--systemd")
+        .arg(test_systemd)
         .arg("--public-key")
         .arg("tests/fixtures/uefi-keys/db.pem")
         .arg("--private-key")

rust/tool/tests/systemd_boot.rs

@@ -0,0 +1,155 @@
+use sha2::{Digest, Sha256};
+use std::path::{Path, PathBuf};
+use std::process::Command;
+use std::{fs, os::unix::prelude::MetadataExt};
+
+use anyhow::Result;
+use tempfile::tempdir;
+
+mod common;
+
+#[test]
+fn keep_systemd_boot_binaries() -> Result<()> {
+    let esp = tempdir()?;
+    let tmpdir = tempdir()?;
+    let profiles = tempdir()?;
+    let generation_link = common::setup_generation_link(tmpdir.path(), profiles.path(), 1)
+        .expect("Failed to setup generation link");
+
+    let systemd_boot_path = systemd_boot_path(&esp);
+    let systemd_boot_fallback_path = systemd_boot_fallback_path(&esp);
+
+    let output0 = common::lanzaboote_install(0, esp.path(), vec![&generation_link])?;
+    assert!(output0.status.success());
+
+    // Use the modification time instead of a hash because the hash would be the same even if the
+    // file was overwritten.
+    let systemd_boot_mtime0 = mtime(&systemd_boot_path);
+    let systemd_boot_fallback_mtime0 = mtime(&systemd_boot_fallback_path);
+
+    let output1 = common::lanzaboote_install(0, esp.path(), vec![generation_link])?;
+    assert!(output1.status.success());
+
+    let systemd_boot_mtime1 = mtime(&systemd_boot_path);
+    let systemd_boot_fallback_mtime1 = mtime(&systemd_boot_fallback_path);
+
+    assert_eq!(
+        systemd_boot_mtime0, systemd_boot_mtime1,
+        "systemd-boot binary was modified on second install."
+    );
+    assert_eq!(
+        systemd_boot_fallback_mtime0, systemd_boot_fallback_mtime1,
+        "systemd-boot fallback binary was moidified on second install."
+    );
+
+    Ok(())
+}
+
+#[test]
+fn overwrite_malformed_systemd_boot_binaries() -> Result<()> {
+    let esp = tempdir()?;
+    let tmpdir = tempdir()?;
+    let profiles = tempdir()?;
+    let generation_link = common::setup_generation_link(tmpdir.path(), profiles.path(), 1)
+        .expect("Failed to setup generation link");
+
+    let systemd_boot_path = systemd_boot_path(&esp);
+    let systemd_boot_fallback_path = systemd_boot_fallback_path(&esp);
+
+    let output0 = common::lanzaboote_install(0, esp.path(), vec![&generation_link])?;
+    assert!(output0.status.success());
+
+    // Make systemd-boot binaries malformed by truncating them.
+    fs::File::create(&systemd_boot_path)?;
+    fs::File::create(&systemd_boot_fallback_path)?;
+
+    let malformed_systemd_boot_hash = hash_file(&systemd_boot_path);
+    let malformed_systemd_boot_fallback_hash = hash_file(&systemd_boot_fallback_path);
+
+    let output1 = common::lanzaboote_install(0, esp.path(), vec![generation_link])?;
+    assert!(output1.status.success());
+
+    let systemd_boot_hash = hash_file(&systemd_boot_path);
+    let systemd_boot_fallback_hash = hash_file(&systemd_boot_fallback_path);
+
+    assert_ne!(
+        malformed_systemd_boot_hash, systemd_boot_hash,
+        "Malformed systemd-boot binaries were not replaced."
+    );
+    assert_ne!(
+        malformed_systemd_boot_fallback_hash, systemd_boot_fallback_hash,
+        "Maformed systemd-boot fallback binaries were not replaced."
+    );
+
+    Ok(())
+}
+
+#[test]
+fn overwrite_unsigned_systemd_boot_binaries() -> Result<()> {
+    let esp = tempdir()?;
+    let tmpdir = tempdir()?;
+    let profiles = tempdir()?;
+    let generation_link = common::setup_generation_link(tmpdir.path(), profiles.path(), 1)
+        .expect("Failed to setup generation link");
+
+    let systemd_boot_path = systemd_boot_path(&esp);
+    let systemd_boot_fallback_path = systemd_boot_fallback_path(&esp);
+
+    let output0 = common::lanzaboote_install(0, esp.path(), vec![&generation_link])?;
+    assert!(output0.status.success());
+
+    remove_signature(&systemd_boot_path)?;
+    remove_signature(&systemd_boot_fallback_path)?;
+    assert!(!verify_signature(&systemd_boot_path)?);
+    assert!(!verify_signature(&systemd_boot_fallback_path)?);
+
+    let output1 = common::lanzaboote_install(0, esp.path(), vec![generation_link])?;
+    assert!(output1.status.success());
+
+    assert!(verify_signature(&systemd_boot_path)?);
+    assert!(verify_signature(&systemd_boot_fallback_path)?);
+
+    Ok(())
+}
+
+fn systemd_boot_path(esp: &tempfile::TempDir) -> PathBuf {
+    esp.path().join("EFI/systemd/systemd-bootx64.efi")
+}
+
+fn systemd_boot_fallback_path(esp: &tempfile::TempDir) -> PathBuf {
+    esp.path().join("EFI/BOOT/BOOTX64.EFI")
+}
+
+/// Look up the modification time (mtime) of a file.
+fn mtime(path: &Path) -> i64 {
+    fs::metadata(path)
+        .expect("Failed to read modification time.")
+        .mtime()
+}
+
+fn hash_file(path: &Path) -> sha2::digest::Output<Sha256> {
+    Sha256::digest(fs::read(path).expect("Failed to read file to hash."))
+}
+
+/// Remove signature from a signed PE file.
+pub fn remove_signature(path: &Path) -> Result<()> {
+    let output = Command::new("sbattach")
+        .arg("--remove")
+        .arg(path.as_os_str())
+        .output()?;
+    print!("{}", String::from_utf8(output.stdout)?);
+    print!("{}", String::from_utf8(output.stderr)?);
+    Ok(())
+}
+
+/// Verify signature of PE file.
+pub fn verify_signature(path: &Path) -> Result<bool> {
+    let output = Command::new("sbverify")
+        .arg(path.as_os_str())
+        .arg("--cert")
+        .arg("tests/fixtures/uefi-keys/db.pem")
+        .output()?;
+    print!("{}", String::from_utf8(output.stdout)?);
+    print!("{}", String::from_utf8(output.stderr)?);
+    Ok(output.status.success())
+}