Merge pull request #11 from blitz/secure-pe-assembling

lanzatool: perform secure assembling for lanzaboote_image and PE wrapping

flake.nix

@@ -226,6 +226,29 @@
             assert "Secure Boot: enabled (user)" in machine.succeed("bootctl status")
           '';
         };
+        # So, this is the responsibility of the lanzatool install
+        # to run the append-initrd-secret script
+        # This test assert that lanzatool still do the right thing
+        # preDeviceCommands should not have any root filesystem mounted
+        # so it should not be able to find /etc/iamasecret, other than the
+        # initrd's one.
+        # which should exist IF lanzatool do the right thing.
+        lanzaboote-with-initrd-secrets = mkSecureBootTest {
+          name = "signed-files-boot-with-secrets-under-secureboot";
+          machine = { ... }: {
+            boot.initrd.secrets = {
+              "/etc/iamasecret" = (pkgs.writeText "iamsecret" "this is a very secure secret");
+            };
+
+            boot.initrd.preDeviceCommands = ''
+              grep "this is a very secure secret" /etc/iamasecret
+            '';
+          };
+          testScript = ''
+            machine.start()
+            assert "Secure Boot: enabled (user)" in machine.succeed("bootctl status")
+          '';
+        };
         is-initrd-secured = mkUnsignedTest {
           name = "unsigned-initrd-do-not-boot-under-secureboot";
           path = {

nix/lanzaboote.nix

@@ -33,6 +33,9 @@ in
   };
 
   config = mkIf cfg.enable {
+    # bootspec is putting at false
+    # until we fix this upstream, we will mkForce it.
+    boot.loader.supportsInitrdSecrets = mkForce true;
     boot.loader.external = {
       enable = true;
       passBootspec = true;

rust/lanzatool/src/install.rs

@@ -10,6 +10,10 @@ use crate::esp::EspPaths;
 use crate::pe;
 use crate::signer::Signer;
 
+use tempfile::tempdir;
+
+use std::process::Command;
+
 pub fn install(
     public_key: &Path,
     private_key: &Path,
@@ -31,7 +35,16 @@ pub fn install(
 
     let kernel_cmdline = assemble_kernel_cmdline(bootspec_doc.init, bootspec_doc.kernel_params);
 
+    // prepare a secure temporary directory
+    // permission bits are not set, because when files below
+    // are opened, they are opened with 600 mode bits.
+    // hence, they cannot be read except by the current user
+    // which is assumed to be root in most cases.
+    // TODO(Raito): prove to niksnur this is actually acceptable.
+    let secure_temp_dir = tempdir()?;
+
     let lanzaboote_image = pe::lanzaboote_image(
+        &secure_temp_dir,
         lanzaboote_stub,
         &bootspec_doc.extension.os_release,
         &kernel_cmdline,
@@ -43,8 +56,14 @@ pub fn install(
 
     println!("Wrapping initrd into a PE binary...");
 
+    let initrd_location = secure_temp_dir.path().join("initrd");
+    copy(&bootspec_doc.initrd, &initrd_location)?;
+    if let Some(initrd_secrets_script) = bootspec_doc.initrd_secrets {
+        append_initrd_secrets(&initrd_secrets_script,
+            &initrd_location)?;
+    }
     let wrapped_initrd =
-        pe::wrap_initrd(initrd_stub, &bootspec_doc.initrd).context("Failed to assemble stub")?;
+        pe::wrap_initrd(&secure_temp_dir, initrd_stub, &initrd_location).context("Failed to assemble stub")?;
 
     println!("Copy files to EFI system partition...");
 
@@ -65,6 +84,9 @@ pub fn install(
         copy(&source, &target)?;
     }
 
+    // TODO: we should implement sign_and_copy which would be secure
+    // by construction for TOCTOU.
+
     println!("Signing files...");
 
     let signer = Signer::new(&public_key, &private_key);
@@ -93,6 +115,20 @@ pub fn install(
     Ok(())
 }
 
+pub fn append_initrd_secrets(append_initrd_secrets_path: &Path, initrd_path: &PathBuf) -> Result<()> {
+    let status = Command::new(append_initrd_secrets_path)
+        .args(vec![
+            initrd_path
+        ])
+        .status()
+        .context("Failed to append initrd secrets")?;
+    if !status.success() {
+        return Err(anyhow::anyhow!("Failed to append initrd secrets with args `{:?}`", vec![append_initrd_secrets_path, initrd_path]).into());
+    }
+
+    Ok(())
+}
+
 fn assemble_kernel_cmdline(init: PathBuf, kernel_params: Vec<String>) -> Vec<String> {
     let init_string = init
         .into_os_string()

rust/lanzatool/src/pe.rs

@@ -1,16 +1,19 @@
 use std::fs;
 use std::io::Write;
+use std::os::unix::prelude::OpenOptionsExt;
 use std::os::unix::fs::MetadataExt;
 use std::path::{Path, PathBuf};
 use std::process::Command;
 
 use anyhow::{Context, Result};
 use goblin::pe::PE;
-use tempfile::NamedTempFile;
 
 use crate::utils;
 
+use tempfile::TempDir;
+
 pub fn lanzaboote_image(
+    target_dir: &TempDir,
     lanzaboote_stub: &Path,
     os_release: &Path,
     kernel_cmdline: &[String],
@@ -20,9 +23,15 @@ pub fn lanzaboote_image(
 ) -> Result<PathBuf> {
     // objcopy copies files into the PE binary. That's why we have to write the contents
     // of some bootspec properties to disk
-    let kernel_cmdline_file = write_to_tmp(kernel_cmdline.join(" "))?;
-    let kernel_path_file = write_to_tmp(esp_relative_path_string(esp, kernel_path))?;
-    let initrd_path_file = write_to_tmp(esp_relative_path_string(esp, initrd_path))?;
+    let (kernel_cmdline_file, _) = write_to_tmp(&target_dir,
+        "kernel-cmdline",
+        kernel_cmdline.join(" "))?;
+    let (kernel_path_file, _) = write_to_tmp(&target_dir, 
+        "kernel-esp-path",
+        esp_relative_path_string(esp, kernel_path))?;
+    let (initrd_path_file, _) = write_to_tmp(&target_dir, 
+        "initrd-esp-path",
+        esp_relative_path_string(esp, initrd_path))?;
 
     let os_release_offs = stub_offset(lanzaboote_stub)?;
     let kernel_cmdline_offs = os_release_offs + file_size(&os_release)?;
@@ -36,20 +45,26 @@ pub fn lanzaboote_image(
         s(".kernelp", kernel_path_file, kernel_path_offs),
     ];
 
-    wrap_in_pe(&lanzaboote_stub, sections)
+    wrap_in_pe(&target_dir, "lanzaboote-stub.efi", &lanzaboote_stub, sections)
 }
 
-pub fn wrap_initrd(initrd_stub: &Path, initrd: &Path) -> Result<PathBuf> {
+pub fn wrap_initrd(target_dir: &TempDir, initrd_stub: &Path, initrd: &Path) -> Result<PathBuf> {
     let initrd_offs = stub_offset(initrd_stub)?;
     let sections = vec![s(".initrd", initrd, initrd_offs)];
-    wrap_in_pe(initrd_stub, sections)
+    wrap_in_pe(target_dir, "wrapped-initrd.exe", initrd_stub, sections)
 }
 
-fn wrap_in_pe(stub: &Path, sections: Vec<Section>) -> Result<PathBuf> {
-    let image = NamedTempFile::new().context("Failed to generate named temp file")?;
+fn wrap_in_pe(target_dir: &TempDir, filename: &str, stub: &Path, sections: Vec<Section>) -> Result<PathBuf> {
+    let image_path = target_dir.path().join(filename);
+    let _ = fs::OpenOptions::new()
+        .create(true)
+        .write(true)
+        .mode(0o600)
+        .open(&image_path)
+        .context("Failed to generate named temp file")?;
 
     let mut args: Vec<String> = sections.iter().flat_map(Section::to_objcopy).collect();
-    let extra_args = vec![utils::path_to_string(stub), utils::path_to_string(&image)];
+    let extra_args = vec![utils::path_to_string(stub), utils::path_to_string(&image_path)];
     args.extend(extra_args);
 
     let status = Command::new("objcopy")
@@ -60,13 +75,7 @@ fn wrap_in_pe(stub: &Path, sections: Vec<Section>) -> Result<PathBuf> {
         return Err(anyhow::anyhow!("Failed to wrap in pe with args `{:?}`", &args).into());
     }
 
-    let (_, persistent_image) = image.keep().with_context(|| {
-        format!(
-            "Failed to persist image with stub: {} from temporary file",
-            stub.display()
-        )
-    })?;
-    Ok(persistent_image)
+    Ok(image_path)
 }
 
 struct Section {
@@ -94,12 +103,17 @@ fn s(name: &'static str, file_path: impl AsRef<Path>, offset: u64) -> Section {
     }
 }
 
-fn write_to_tmp(contents: impl AsRef<[u8]>) -> Result<PathBuf> {
-    let mut tmpfile = NamedTempFile::new().context("Failed to create tempfile")?;
+fn write_to_tmp(secure_temp: &TempDir, filename: &str, contents: impl AsRef<[u8]>) -> Result<(PathBuf, fs::File)> {
+    let mut tmpfile = fs::OpenOptions::new()
+        .create(true)
+        .write(true)
+        .mode(0o600)
+        .open(secure_temp.path().join(filename))
+        .context("Failed to create tempfile")?;
     tmpfile
         .write_all(contents.as_ref())
         .context("Failed to write to tempfile")?;
-    Ok(tmpfile.keep()?.1)
+    Ok((secure_temp.path().join(filename), tmpfile))
 }
 
 fn esp_relative_path_string(esp: &Path, path: &Path) -> String {