Merge pull request #69 from nix-community/refactor-tmpdir

Refactor tempdir: Add SecureTempDirExt

rust/tool/src/install.rs

@@ -5,7 +5,6 @@ use std::process::Command;
 
 use anyhow::{Context, Result};
 use nix::unistd::sync;
-use tempfile::tempdir;
 
 use crate::esp::EspPaths;
 use crate::gc::Roots;
@@ -13,6 +12,7 @@ use crate::generation::{Generation, GenerationLink};
 use crate::os_release::OsRelease;
 use crate::pe;
 use crate::signature::KeyPair;
+use crate::utils::SecureTempDirExt;
 
 pub struct Installer {
     gc_roots: Roots,
@@ -121,23 +121,18 @@ impl Installer {
         let kernel_cmdline =
             assemble_kernel_cmdline(&bootspec.init, bootspec.kernel_params.clone());
 
-        // prepare a secure temporary directory
+        // This tempdir must live for the entire lifetime of the current function.
-        // permission bits are not set, because when files below
+        let tempdir = tempfile::tempdir()?;
-        // 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 os_release = OsRelease::from_generation(generation)
             .context("Failed to build OsRelease from generation.")?;
-        let os_release_path = secure_temp_dir.path().join("os-release");
+        let os_release_path = tempdir
-        fs::write(&os_release_path, os_release.to_string().as_bytes())
+            .write_secure_file("os-release", os_release.to_string().as_bytes())
-            .with_context(|| format!("Failed to write os-release file: {:?}", os_release_path))?;
+            .context("Failed to write os-release file.")?;
 
         println!("Appending secrets to initrd...");
 
-        let initrd_location = secure_temp_dir.path().join("initrd");
+        let initrd_location = tempdir.path().join("initrd");
         copy(
             bootspec
                 .initrd
@@ -168,7 +163,7 @@ impl Installer {
         install(&initrd_location, &esp_paths.initrd).context("Failed to install initrd to ESP")?;
 
         let lanzaboote_image = pe::lanzaboote_image(
-            &secure_temp_dir,
+            &tempdir,
             &self.lanzaboote_stub,
             &os_release_path,
             &kernel_cmdline,

rust/tool/src/main.rs

@@ -6,6 +6,7 @@ mod install;
 mod os_release;
 mod pe;
 mod signature;
+mod utils;
 
 use anyhow::Result;
 use clap::Parser;

rust/tool/src/pe.rs

@@ -1,8 +1,6 @@
 use std::ffi::OsString;
 use std::fs;
-use std::io::Write;
 use std::os::unix::fs::MetadataExt;
-use std::os::unix::prelude::OpenOptionsExt;
 use std::path::{Path, PathBuf};
 use std::process::Command;
 
@@ -10,7 +8,7 @@ use anyhow::{Context, Result};
 use goblin::pe::PE;
 use sha2::{Digest, Sha256};
 
-use tempfile::TempDir;
+use crate::utils::SecureTempDirExt;
 
 type Hash = sha2::digest::Output<Sha256>;
 
@@ -20,7 +18,9 @@ type Hash = sha2::digest::Output<Sha256>;
 /// be present in the ESP. This is required, because we need to read
 /// them to compute hashes.
 pub fn lanzaboote_image(
-    target_dir: &TempDir,
+    // Because the returned path of this function is inside the tempdir as well, the tempdir must
+    // live longer than the function. This is why it cannot be created inside the function.
+    tempdir: &tempfile::TempDir,
     lanzaboote_stub: &Path,
     os_release: &Path,
     kernel_cmdline: &[String],
@@ -30,29 +30,18 @@ pub fn lanzaboote_image(
 ) -> Result<PathBuf> {
     // objcopy can only copy 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(target_dir, "kernel-cmdline", kernel_cmdline.join(" "))?;
+    let kernel_cmdline_file =
+        tempdir.write_secure_file("kernel-cmdline", kernel_cmdline.join(" "))?;
 
-    let kernel_path_file = write_to_tmp(
+    let kernel_path_file =
-        target_dir,
+        tempdir.write_secure_file("kernel-path", esp_relative_uefi_path(esp, kernel_path)?)?;
-        "kernel-esp-path",
+    let kernel_hash_file =
-        esp_relative_uefi_path(esp, kernel_path)?,
+        tempdir.write_secure_file("kernel-hash", file_hash(kernel_path)?.as_slice())?;
-    )?;
-    let kernel_hash_file = write_to_tmp(
-        target_dir,
-        "kernel-hash",
-        file_hash(kernel_path)?.as_slice(),
-    )?;
 
-    let initrd_path_file = write_to_tmp(
+    let initrd_path_file =
-        target_dir,
+        tempdir.write_secure_file("initrd-path", esp_relative_uefi_path(esp, initrd_path)?)?;
-        "initrd-esp-path",
+    let initrd_hash_file =
-        esp_relative_uefi_path(esp, initrd_path)?,
+        tempdir.write_secure_file("initrd-hash", file_hash(initrd_path)?.as_slice())?;
-    )?;
-    let initrd_hash_file = write_to_tmp(
-        target_dir,
-        "initrd-hash",
-        file_hash(initrd_path)?.as_slice(),
-    )?;
 
     let os_release_offs = stub_offset(lanzaboote_stub)?;
     let kernel_cmdline_offs = os_release_offs + file_size(os_release)?;
@@ -70,7 +59,9 @@ pub fn lanzaboote_image(
         s(".kernelh", kernel_hash_file, kernel_hash_offs),
     ];
 
-    wrap_in_pe(target_dir, "lanzaboote-stub.efi", lanzaboote_stub, sections)
+    let image_path = tempdir.path().join("lanzaboote-stub.efi");
+    wrap_in_pe(lanzaboote_stub, sections, &image_path)?;
+    Ok(image_path)
 }
 
 /// Compute the SHA 256 hash of a file.
@@ -80,25 +71,11 @@ fn file_hash(file: &Path) -> Result<Hash> {
 
 /// Take a PE binary stub and attach sections to it.
 ///
-/// The result is then written to a new file. Returns the filename of
+/// The resulting binary is then written to a newly created file at the provided output path.
-/// the generated file.
+fn wrap_in_pe(stub: &Path, sections: Vec<Section>, output: &Path) -> Result<()> {
-fn wrap_in_pe(
-    target_dir: &TempDir,
-    output_filename: &str,
-    stub: &Path,
-    sections: Vec<Section>,
-) -> Result<PathBuf> {
-    let image_path = target_dir.path().join(output_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<OsString> = sections.iter().flat_map(Section::to_objcopy).collect();
 
-    [stub.as_os_str(), image_path.as_os_str()]
+    [stub.as_os_str(), output.as_os_str()]
         .iter()
         .for_each(|a| args.push(a.into()));
 
@@ -113,7 +90,7 @@ fn wrap_in_pe(
         ));
     }
 
-    Ok(image_path)
+    Ok(())
 }
 
 struct Section {
@@ -148,28 +125,6 @@ fn s(name: &'static str, file_path: impl AsRef<Path>, offset: u64) -> Section {
     }
 }
 
-/// Write a `u8` slice to a temporary file.
-fn write_to_tmp(
-    secure_temp: &TempDir,
-    filename: &str,
-    contents: impl AsRef<[u8]>,
-) -> Result<PathBuf> {
-    let path = secure_temp.path().join(filename);
-
-    let mut tmpfile = fs::OpenOptions::new()
-        .create(true)
-        .write(true)
-        .mode(0o600)
-        .open(&path)
-        .context("Failed to create tempfile")?;
-
-    tmpfile
-        .write_all(contents.as_ref())
-        .context("Failed to write to tempfile")?;
-
-    Ok(path)
-}
-
 /// Convert a path to an UEFI path relative to the specified ESP.
 fn esp_relative_uefi_path(esp: &Path, path: &Path) -> Result<String> {
     let relative_path = path

rust/tool/src/utils.rs

@@ -0,0 +1,49 @@
+use std::fs;
+use std::io::Write;
+use std::os::unix::fs::OpenOptionsExt;
+use std::path::PathBuf;
+
+use anyhow::{Context, Result};
+use tempfile::TempDir;
+
+/// Extension for a temporary directory that enables creating secure temporary files in it.
+pub trait SecureTempDirExt {
+    fn create_secure_file(&self, file_name: &str) -> Result<fs::File>;
+    fn write_secure_file(&self, file_name: &str, contents: impl AsRef<[u8]>) -> Result<PathBuf>;
+}
+
+/// This implementation has three useful properties:
+///
+/// - Files are created with mode 0o600, so that they are only accessible by the current user.
+/// - Files are named and not ephemeral (unlike a real temporary file).
+/// - The directory and its children are cleaned up (i.e. deleted) when the variable that holds the
+///   directory goes out of scope.
+///
+/// This protects against an attacker _without_ root access from modifying files undetected. It
+/// provides no prection against an attacker _with_ root access. Additionally, because the files
+/// have named paths, they can be passed to external programs while still being securely deleted
+/// after they are not needed anymore.
+impl SecureTempDirExt for TempDir {
+    /// Create a temporary file that can only be accessed by the current Linux user.
+    fn create_secure_file(&self, file_name: &str) -> Result<fs::File> {
+        let path = self.path().join(file_name);
+        fs::OpenOptions::new()
+            .create(true)
+            .write(true)
+            .mode(0o600)
+            .open(&path)
+            .with_context(|| format!("Failed to create tempfile: {path:?}"))
+    }
+
+    /// Create a temporary file and write a `u8` slice to it.
+    fn write_secure_file(&self, file_name: &str, contents: impl AsRef<[u8]>) -> Result<PathBuf> {
+        let path = self.path().join(file_name);
+        let mut tmpfile = self.create_secure_file(file_name)?;
+
+        tmpfile
+            .write_all(contents.as_ref())
+            .with_context(|| format!("Failed to write to tempfile {path:?}"))?;
+
+        Ok(path)
+    }
+}