From df6b1b07f7467358aa7b99617432259bd641f4ce Mon Sep 17 00:00:00 2001 From: nikstur Date: Tue, 14 Feb 2023 01:49:10 +0100 Subject: [PATCH] tool: use random names for secure tempfiles Using random names for tempfiles makes handling them easier. It reduces the amount of noise in the code because no custom name needs to be provided for each tempfile. The names were not really useful in any case. It also does not burden the developer with ensuring uniqueness of names. This is relevant when files for multiple generations need to be stored in the same directory (e.g. because they need to be accessed after handling one generation). Out of an abundance of caution, 32 random alphanumeric characters are chosen for each filename. The tempfile crate, in comparison, only chooses 8. 32 characters should be enough to avoid collisions, even if the PRNG is not of cryptographic quality. --- rust/tool/Cargo.lock | 1 + rust/tool/Cargo.toml | 3 +++ rust/tool/src/install.rs | 9 +++++---- rust/tool/src/pe.rs | 19 +++++++------------ rust/tool/src/utils.rs | 35 ++++++++++++++++++++++++++--------- 5 files changed, 42 insertions(+), 25 deletions(-) diff --git a/rust/tool/Cargo.lock b/rust/tool/Cargo.lock index 213b9f7..272be21 100644 --- a/rust/tool/Cargo.lock +++ b/rust/tool/Cargo.lock @@ -282,6 +282,7 @@ dependencies = [ "bootspec", "clap", "expect-test", + "fastrand", "filetime", "goblin", "nix", diff --git a/rust/tool/Cargo.toml b/rust/tool/Cargo.toml index 864accf..1403b0b 100644 --- a/rust/tool/Cargo.toml +++ b/rust/tool/Cargo.toml @@ -22,6 +22,9 @@ bootspec = { git = "https://github.com/DeterminateSystems/bootspec" } walkdir = "2.3.2" time = "0.3.17" sha2 = "0.10.6" +# Keep the fastrand version aligned with the one from tempfile to avoid two +# different versions. +fastrand = "1.6.0" [dev-dependencies] assert_cmd = "2.0.7" diff --git a/rust/tool/src/install.rs b/rust/tool/src/install.rs index 4a9eaab..2e6ff95 100644 --- a/rust/tool/src/install.rs +++ b/rust/tool/src/install.rs @@ -140,19 +140,20 @@ impl Installer { let os_release = OsRelease::from_generation(generation) .context("Failed to build OsRelease from generation.")?; let os_release_path = tempdir - .write_secure_file("os-release", os_release.to_string().as_bytes()) + .write_secure_file(os_release.to_string().as_bytes()) .context("Failed to write os-release file.")?; println!("Appending secrets to initrd..."); - let initrd_location = tempdir.path().join("initrd"); - fs::copy( + let initrd_content = fs::read( bootspec .initrd .as_ref() .context("Lanzaboote does not support missing initrd yet")?, - &initrd_location, )?; + let initrd_location = tempdir + .write_secure_file(initrd_content) + .context("Failed to copy initrd to tempfile.")?; if let Some(initrd_secrets_script) = &bootspec.initrd_secrets { append_initrd_secrets(initrd_secrets_script, &initrd_location)?; } diff --git a/rust/tool/src/pe.rs b/rust/tool/src/pe.rs index 99f184f..f31cf0d 100644 --- a/rust/tool/src/pe.rs +++ b/rust/tool/src/pe.rs @@ -8,7 +8,7 @@ use anyhow::{Context, Result}; use goblin::pe::PE; use sha2::{Digest, Sha256}; -use crate::utils::SecureTempDirExt; +use crate::utils::{tmpname, SecureTempDirExt}; type Hash = sha2::digest::Output; @@ -30,18 +30,13 @@ 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 = - tempdir.write_secure_file("kernel-cmdline", kernel_cmdline.join(" "))?; + let kernel_cmdline_file = tempdir.write_secure_file(kernel_cmdline.join(" "))?; - 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 kernel_path_file = tempdir.write_secure_file(esp_relative_uefi_path(esp, kernel_path)?)?; + let kernel_hash_file = tempdir.write_secure_file(file_hash(kernel_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 initrd_path_file = tempdir.write_secure_file(esp_relative_uefi_path(esp, initrd_path)?)?; + let initrd_hash_file = tempdir.write_secure_file(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)?; @@ -59,7 +54,7 @@ pub fn lanzaboote_image( s(".kernelh", kernel_hash_file, kernel_hash_offs), ]; - let image_path = tempdir.path().join("lanzaboote-stub.efi"); + let image_path = tempdir.path().join(tmpname()); wrap_in_pe(lanzaboote_stub, sections, &image_path)?; Ok(image_path) } diff --git a/rust/tool/src/utils.rs b/rust/tool/src/utils.rs index 951290a..2597150 100644 --- a/rust/tool/src/utils.rs +++ b/rust/tool/src/utils.rs @@ -1,15 +1,20 @@ +use std::ffi::OsString; use std::fs; use std::io::Write; +use std::iter::repeat_with; use std::os::unix::fs::OpenOptionsExt; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use anyhow::{Context, Result}; use tempfile::TempDir; +/// The number of random alphanumeric characters in the tempfiles. +const TEMPFILE_RANDOM_LENGTH: usize = 32; + /// 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; + fn create_secure_file(&self, path: &Path) -> Result; + fn write_secure_file(&self, contents: impl AsRef<[u8]>) -> Result; } /// This implementation has three useful properties: @@ -25,20 +30,19 @@ pub trait SecureTempDirExt { /// 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); + fn create_secure_file(&self, path: &Path) -> Result { fs::OpenOptions::new() .create(true) .write(true) .mode(0o600) - .open(&path) + .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)?; + fn write_secure_file(&self, contents: impl AsRef<[u8]>) -> Result { + let path = self.path().join(tmpname()); + let mut tmpfile = self.create_secure_file(&path)?; tmpfile .write_all(contents.as_ref()) @@ -47,3 +51,16 @@ impl SecureTempDirExt for TempDir { Ok(path) } } + +/// Generate a random (but not cryptographically secure) name for a temporary file. +/// +/// This is heavily inspired by the way temporary names are generated in the `tempfile` crate. +/// Since the `tempfile` crate does not expose this functionality, we have to recreate it here. +pub fn tmpname() -> OsString { + let mut buf = OsString::with_capacity(TEMPFILE_RANDOM_LENGTH); + let mut char_buf = [0u8; 4]; + for c in repeat_with(fastrand::alphanumeric).take(TEMPFILE_RANDOM_LENGTH) { + buf.push(c.encode_utf8(&mut char_buf)); + } + buf +}