From 6e452b50dfedfd4beacbf085942fb730c4ff3584 Mon Sep 17 00:00:00 2001 From: nikstur Date: Fri, 13 Jan 2023 03:10:58 +0100 Subject: [PATCH] tool: add SecureTempDirExt Add an extension to TempDir that allows to create secure tempfiles. This way, everything related to creating secure tempfiles is bundled in a single place and can easily be reused. --- rust/tool/src/install.rs | 21 ++++------ rust/tool/src/main.rs | 1 + rust/tool/src/pe.rs | 87 ++++++++++------------------------------ rust/tool/src/utils.rs | 49 ++++++++++++++++++++++ 4 files changed, 79 insertions(+), 79 deletions(-) create mode 100644 rust/tool/src/utils.rs diff --git a/rust/tool/src/install.rs b/rust/tool/src/install.rs index 1621fc4..e7e48ff 100644 --- a/rust/tool/src/install.rs +++ b/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 - // 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()?; + // This tempdir must live for the entire lifetime of the current function. + let tempdir = tempfile::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"); - fs::write(&os_release_path, os_release.to_string().as_bytes()) - .with_context(|| format!("Failed to write os-release file: {:?}", os_release_path))?; + let os_release_path = tempdir + .write_secure_file("os-release", os_release.to_string().as_bytes()) + .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, diff --git a/rust/tool/src/main.rs b/rust/tool/src/main.rs index e502d9d..d886bba 100644 --- a/rust/tool/src/main.rs +++ b/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; diff --git a/rust/tool/src/pe.rs b/rust/tool/src/pe.rs index f6c47d1..7f32d01 100644 --- a/rust/tool/src/pe.rs +++ b/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; @@ -20,7 +18,9 @@ type Hash = sha2::digest::Output; /// 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 { // 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( - target_dir, - "kernel-esp-path", - esp_relative_uefi_path(esp, kernel_path)?, - )?; - let kernel_hash_file = write_to_tmp( - target_dir, - "kernel-hash", - file_hash(kernel_path)?.as_slice(), - )?; + let kernel_path_file = + tempdir.write_secure_file("kernel-path", esp_relative_uefi_path(esp, kernel_path)?)?; + let kernel_hash_file = + tempdir.write_secure_file("kernel-hash", file_hash(kernel_path)?.as_slice())?; - let initrd_path_file = write_to_tmp( - target_dir, - "initrd-esp-path", - esp_relative_uefi_path(esp, initrd_path)?, - )?; - let initrd_hash_file = write_to_tmp( - target_dir, - "initrd-hash", - file_hash(initrd_path)?.as_slice(), - )?; + let initrd_path_file = + tempdir.write_secure_file("initrd-path", esp_relative_uefi_path(esp, initrd_path)?)?; + let initrd_hash_file = + tempdir.write_secure_file("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 { /// Take a PE binary stub and attach sections to it. /// -/// The result is then written to a new file. Returns the filename of -/// the generated file. -fn wrap_in_pe( - target_dir: &TempDir, - output_filename: &str, - stub: &Path, - sections: Vec
, -) -> Result { - 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")?; - +/// The resulting binary is then written to a newly created file at the provided output path. +fn wrap_in_pe(stub: &Path, sections: Vec
, output: &Path) -> Result<()> { let mut args: Vec = 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, 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 { - 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 { let relative_path = path diff --git a/rust/tool/src/utils.rs b/rust/tool/src/utils.rs new file mode 100644 index 0000000..951290a --- /dev/null +++ b/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; + fn write_secure_file(&self, file_name: &str, contents: impl AsRef<[u8]>) -> Result; +} + +/// 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 { + 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 { + 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) + } +}