From d7369e73e916305c3cde577aa0e1073c3eaf607f Mon Sep 17 00:00:00 2001 From: Andrew Gunnerson Date: Sun, 10 Aug 2025 19:50:43 -0400 Subject: [PATCH] Remove cap-std and cap-tempfile dependencies There is not much benefit for our use case to have kernel-level openat-style sandboxing of paths. We already check all untrusted paths for safety and the sandboxing prevented the use of symlinks that point outside of the parent directory of specified paths. This commit also moves the path safety checks to the util module to avoid having multiple implementations spread out everywhere. Signed-off-by: Andrew Gunnerson --- Cargo.lock | 195 ------------------------------------- avbroot/Cargo.toml | 2 - avbroot/src/cli/avb.rs | 73 +++++--------- avbroot/src/cli/cpio.rs | 40 +++----- avbroot/src/cli/lp.rs | 40 +++----- avbroot/src/cli/ota.rs | 77 ++++++--------- avbroot/src/cli/payload.rs | 33 ++----- avbroot/src/patch/boot.rs | 22 +++-- avbroot/src/util.rs | 65 ++++++++++++- e2e/src/main.rs | 7 +- 10 files changed, 173 insertions(+), 381 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1a85011..8b5bef8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -28,12 +28,6 @@ dependencies = [ "memchr", ] -[[package]] -name = "ambient-authority" -version = "0.0.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e9d4ee0d472d1cd2e28c97dfa124b3d8d992e10eb0a035f33f5d12e3a177ba3b" - [[package]] name = "anstream" version = "0.6.20" @@ -121,8 +115,6 @@ dependencies = [ "bitflags", "bstr", "bzip2", - "cap-std", - "cap-tempfile", "clap", "clap_complete", "cms", @@ -250,49 +242,6 @@ dependencies = [ "libbz2-rs-sys", ] -[[package]] -name = "cap-primitives" -version = "3.4.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0a1e394ed14f39f8bc26f59d4c0c010dbe7f0a1b9bafff451b1f98b67c8af62a" -dependencies = [ - "ambient-authority", - "fs-set-times", - "io-extras", - "io-lifetimes", - "ipnet", - "maybe-owned", - "rustix", - "rustix-linux-procfs", - "windows-sys 0.59.0", - "winx", -] - -[[package]] -name = "cap-std" -version = "3.4.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "07c0355ca583dd58f176c3c12489d684163861ede3c9efa6fd8bba314c984189" -dependencies = [ - "cap-primitives", - "io-extras", - "io-lifetimes", - "rustix", -] - -[[package]] -name = "cap-tempfile" -version = "3.4.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9bdc50d18ee6c3551b30eb7ad5c4628d7c73ed9e1696b63c432a55602d634d7d" -dependencies = [ - "cap-std", - "rand", - "rustix", - "rustix-linux-procfs", - "uuid", -] - [[package]] name = "cbc" version = "0.1.2" @@ -662,17 +611,6 @@ version = "1.0.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3f9eec918d3f24069decb9af1554cad7c880e2da24a9afd88aca000531ab82c1" -[[package]] -name = "fs-set-times" -version = "0.20.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "94e7099f6313ecacbe1256e8ff9d617b75d1bcb16a6fddef94866d225a01a14a" -dependencies = [ - "io-lifetimes", - "rustix", - "windows-sys 0.59.0", -] - [[package]] name = "fuzz" version = "3.19.0" @@ -806,28 +744,6 @@ dependencies = [ "generic-array", ] -[[package]] -name = "io-extras" -version = "0.18.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2285ddfe3054097ef4b2fe909ef8c3bcd1ea52a8f0d274416caebeef39f04a65" -dependencies = [ - "io-lifetimes", - "windows-sys 0.59.0", -] - -[[package]] -name = "io-lifetimes" -version = "2.0.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "06432fb54d3be7964ecd3649233cddf80db2832f47fec34c01f65b3d9d774983" - -[[package]] -name = "ipnet" -version = "2.11.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "469fb0b9cefa57e3ef31275ee7cacb78f2fdca44e4765491884a2b119d4eb130" - [[package]] name = "is_terminal_polyfill" version = "1.70.1" @@ -843,16 +759,6 @@ dependencies = [ "either", ] -[[package]] -name = "js-sys" -version = "0.3.77" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1cfaf33c695fc6e08064efbc1f72ec937429614f25eef83af942d0e227c3a28f" -dependencies = [ - "once_cell", - "wasm-bindgen", -] - [[package]] name = "lazy_static" version = "1.5.0" @@ -954,12 +860,6 @@ dependencies = [ "sha2", ] -[[package]] -name = "maybe-owned" -version = "0.3.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4facc753ae494aeb6e3c22f839b158aebd4f9270f55cd3c79906c45476c47ab4" - [[package]] name = "memchr" version = "2.7.5" @@ -1496,22 +1396,6 @@ dependencies = [ "windows-sys 0.60.2", ] -[[package]] -name = "rustix-linux-procfs" -version = "0.1.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2fc84bf7e9aa16c4f2c758f27412dc9841341e16aa682d9c7ac308fe3ee12056" -dependencies = [ - "once_cell", - "rustix", -] - -[[package]] -name = "rustversion" -version = "1.0.21" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8a0d197bd2c9dc6e53b84da9556a69ba4cdfab8619eb41a8bd1cc2027a0f6b1d" - [[package]] name = "salsa20" version = "0.10.2" @@ -1889,17 +1773,6 @@ version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "06abde3611657adf66d383f00b093d7faecc7fa57071cce2578660c9f1010821" -[[package]] -name = "uuid" -version = "1.17.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3cf4199d1e5d15ddd86a694e4d0dffa9c323ce759fea589f00fef9d81cc1931d" -dependencies = [ - "getrandom 0.3.3", - "js-sys", - "wasm-bindgen", -] - [[package]] name = "valuable" version = "0.1.1" @@ -1927,64 +1800,6 @@ dependencies = [ "wit-bindgen-rt", ] -[[package]] -name = "wasm-bindgen" -version = "0.2.100" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1edc8929d7499fc4e8f0be2262a241556cfc54a0bea223790e71446f2aab1ef5" -dependencies = [ - "cfg-if", - "once_cell", - "rustversion", - "wasm-bindgen-macro", -] - -[[package]] -name = "wasm-bindgen-backend" -version = "0.2.100" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2f0a0651a5c2bc21487bde11ee802ccaf4c51935d0d3d42a6101f98161700bc6" -dependencies = [ - "bumpalo", - "log", - "proc-macro2", - "quote", - "syn 2.0.104", - "wasm-bindgen-shared", -] - -[[package]] -name = "wasm-bindgen-macro" -version = "0.2.100" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7fe63fc6d09ed3792bd0897b314f53de8e16568c2b3f7982f468c0bf9bd0b407" -dependencies = [ - "quote", - "wasm-bindgen-macro-support", -] - -[[package]] -name = "wasm-bindgen-macro-support" -version = "0.2.100" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8ae87ea40c9f689fc23f209965b6fb8a99ad69aeeb0231408be24920604395de" -dependencies = [ - "proc-macro2", - "quote", - "syn 2.0.104", - "wasm-bindgen-backend", - "wasm-bindgen-shared", -] - -[[package]] -name = "wasm-bindgen-shared" -version = "0.2.100" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1a05d73b933a847d6cccdda8f838a22ff101ad9bf93e33684f39c1f5f0eece3d" -dependencies = [ - "unicode-ident", -] - [[package]] name = "winapi" version = "0.3.9" @@ -2178,16 +1993,6 @@ dependencies = [ "memchr", ] -[[package]] -name = "winx" -version = "0.36.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3f3fd376f71958b862e7afb20cfe5a22830e1963462f3a17f49d82a6c1d1f42d" -dependencies = [ - "bitflags", - "windows-sys 0.59.0", -] - [[package]] name = "wit-bindgen-rt" version = "0.39.0" diff --git a/avbroot/Cargo.toml b/avbroot/Cargo.toml index 46d0872..c603ebb 100644 --- a/avbroot/Cargo.toml +++ b/avbroot/Cargo.toml @@ -14,8 +14,6 @@ base64 = "0.22.1" bitflags = { version = "2.4.1", features = ["serde"] } bstr = "1.6.2" bzip2 = "0.6.0" -cap-std = "3.0.0" -cap-tempfile = "3.0.0" clap = { version = "4.4.1", features = ["derive"] } clap_complete = "4.4.0" cms = { version = "0.2.2", features = ["std"] } diff --git a/avbroot/src/cli/avb.rs b/avbroot/src/cli/avb.rs index 12be852..3ccfee3 100644 --- a/avbroot/src/cli/avb.rs +++ b/avbroot/src/cli/avb.rs @@ -1,20 +1,16 @@ -// SPDX-FileCopyrightText: 2023-2024 Andrew Gunnerson +// SPDX-FileCopyrightText: 2023-2025 Andrew Gunnerson // SPDX-License-Identifier: GPL-3.0-only use std::{ collections::{HashMap, HashSet}, - ffi::{OsStr, OsString}, - fs::{self, File}, + ffi::OsString, + fs::{self, File, OpenOptions}, io::{self, BufReader, BufWriter, Cursor, Seek, SeekFrom, Write}, path::{Path, PathBuf}, sync::atomic::AtomicBool, }; use anyhow::{Context, Result, anyhow, bail}; -use cap_std::{ - ambient_authority, - fs::{Dir, OpenOptions}, -}; use clap::{Args, Parser, Subcommand}; use rayon::prelude::{IntoParallelRefIterator, ParallelIterator}; use rsa::RsaPublicKey; @@ -410,20 +406,11 @@ fn display_info(display: &DisplayGroup, info: &AvbInfo) { } } -/// Ensure that the partition name won't cause directory traversals. -fn ensure_name_is_safe(name: &str) -> Result<()> { - if Path::new(name).file_name() != Some(OsStr::new(name)) { - bail!("Unsafe partition name: {name}"); - } - - Ok(()) -} - /// Recursively verify an image's vbmeta header and all of the chained images. /// `seen` is used to prevent cycles. `descriptors` will contain all of the hash /// and hash tree descriptors that need to be verified. pub fn verify_headers( - directory: &Dir, + directory: &Path, name: &str, expected_key: Option<&RsaPublicKey>, seen: &mut HashSet, @@ -433,12 +420,9 @@ pub fn verify_headers( return Ok(()); } - ensure_name_is_safe(name)?; - - let path = format!("{name}.img"); - let raw_reader = directory - .open(&path) - .with_context(|| format!("Failed to open for reading: {path:?}"))?; + let path = util::path_join_single(directory, format!("{name}.img"))?; + let raw_reader = + File::open(&path).with_context(|| format!("Failed to open for reading: {path:?}"))?; let (header, _, _) = avb::load_image(BufReader::new(raw_reader)) .with_context(|| format!("Failed to load vbmeta structures: {path:?}"))?; @@ -545,7 +529,7 @@ fn verify_and_repair( /// Verify hash and hash tree descriptor digests and FEC data against their /// corresponding input files. pub fn verify_descriptors( - directory: &Dir, + directory: &Path, descriptors: &HashMap, repair: bool, cancel_signal: &AtomicBool, @@ -557,10 +541,12 @@ pub fn verify_descriptors( .map(|(name, descriptor)| { let _span = parent_span.enter(); - let path = format!("{name}.img"); - let file = match directory - .open_with(&path, OpenOptions::new().read(true).write(repair)) - .map(|f| PSeekFile::new(f.into_std())) + let path = util::path_join_single(directory, format!("{name}.img"))?; + let file = match OpenOptions::new() + .read(true) + .write(repair) + .open(&path) + .map(PSeekFile::new) { Ok(f) => f, // Some devices, like bluejay, have vbmeta descriptors that @@ -587,7 +573,7 @@ pub fn verify_descriptors( } fn compute_digest_recursive( - directory: &Dir, + directory: &Path, name: &str, context: &mut ring::digest::Context, max_depth: u8, @@ -602,11 +588,8 @@ fn compute_digest_recursive( seen.insert(name.to_owned()); - ensure_name_is_safe(name)?; - - let path = format!("{name}.img"); - let mut raw_reader = directory - .open(&path) + let path = util::path_join_single(directory, format!("{name}.img"))?; + let mut raw_reader = File::open(&path) .map(BufReader::new) .with_context(|| format!("Failed to open for reading: {path:?}"))?; let (header, footer, _) = avb::load_image(&mut raw_reader) @@ -658,7 +641,11 @@ fn compute_digest_recursive( /// the root vbmeta image, followed by the headers in the immediate chained /// partitions. This digest is not defined to be recursive, so headers of /// chained partitions more than one level deep are ignored. -pub fn compute_digest(directory: &Dir, name: &str, cancel_signal: &AtomicBool) -> Result<[u8; 32]> { +pub fn compute_digest( + directory: &Path, + name: &str, + cancel_signal: &AtomicBool, +) -> Result<[u8; 32]> { let mut seen = HashSet::::new(); let mut context = ring::digest::Context::new(&ring::digest::SHA256); @@ -776,10 +763,7 @@ fn verify_subcommand(cli: &VerifyCli, cancel_signal: &AtomicBool) -> Result<()> None }; - let authority = ambient_authority(); - let parent_path = util::parent_path(&cli.input); - let directory = Dir::open_ambient_dir(parent_path, authority) - .with_context(|| format!("Failed to open directory: {parent_path:?}"))?; + let directory = util::parent_path(&cli.input); let name = cli .input .file_stem() @@ -791,13 +775,13 @@ fn verify_subcommand(cli: &VerifyCli, cancel_signal: &AtomicBool) -> Result<()> let mut descriptors = HashMap::::new(); verify_headers( - &directory, + directory, name, public_key.as_ref(), &mut seen, &mut descriptors, )?; - verify_descriptors(&directory, &descriptors, cli.repair, cancel_signal)?; + verify_descriptors(directory, &descriptors, cli.repair, cancel_signal)?; info!("Successfully verified all vbmeta signatures and hashes"); @@ -805,10 +789,7 @@ fn verify_subcommand(cli: &VerifyCli, cancel_signal: &AtomicBool) -> Result<()> } fn digest_subcommand(cli: &DigestCli, cancel_signal: &AtomicBool) -> Result<()> { - let authority = ambient_authority(); - let parent_path = util::parent_path(&cli.input); - let directory = Dir::open_ambient_dir(parent_path, authority) - .with_context(|| format!("Failed to open directory: {parent_path:?}"))?; + let directory = util::parent_path(&cli.input); let name = cli .input .file_stem() @@ -816,7 +797,7 @@ fn digest_subcommand(cli: &DigestCli, cancel_signal: &AtomicBool) -> Result<()> .to_str() .ok_or_else(|| anyhow!("Invalid UTF-8: {:?}", cli.input))?; - let digest = compute_digest(&directory, name, cancel_signal)?; + let digest = compute_digest(directory, name, cancel_signal)?; println!("{}", hex::encode(digest)); diff --git a/avbroot/src/cli/cpio.rs b/avbroot/src/cli/cpio.rs index 35118a6..6dcb7e5 100644 --- a/avbroot/src/cli/cpio.rs +++ b/avbroot/src/cli/cpio.rs @@ -1,4 +1,4 @@ -// SPDX-FileCopyrightText: 2023 Andrew Gunnerson +// SPDX-FileCopyrightText: 2023-2025 Andrew Gunnerson // SPDX-License-Identifier: GPL-3.0-only use std::{ @@ -11,7 +11,6 @@ use std::{ use anyhow::{Context, Result, anyhow}; use bstr::ByteSlice; -use cap_std::{ambient_authority, fs::Dir}; use clap::{Parser, Subcommand}; use num_traits::ToPrimitive; use serde::{Deserialize, Serialize}; @@ -91,17 +90,17 @@ fn write_info(path: &Path, info: &CpioInfo) -> Result<()> { /// Open reader to the corresponding file inside the tree if the entry is a /// regular file. Unsafe paths will result in an error. -fn open_tree_file(tree: &Dir, entry: &CpioEntry) -> Result, u32)>> { +fn open_tree_file(tree: &Path, entry: &CpioEntry) -> Result, u32)>> { if entry.file_type == CpioEntryType::Regular { - let path = entry + let sub_path = entry .path .as_bstr() .to_path() .with_context(|| format!("Invalid entry path: {:?}", entry.path.as_bstr()))?; + let path = util::path_join(tree, sub_path)?; - let mut reader = tree - .open(path) - .map(|f| BufReader::new(f.into_std())) + let mut reader = File::open(&path) + .map(BufReader::new) .with_context(|| format!("Failed to open for reading: {path:?}"))?; let file_size = reader @@ -122,21 +121,21 @@ fn open_tree_file(tree: &Dir, entry: &CpioEntry) -> Result Result>> { +fn create_tree_file(tree: &Path, entry: &CpioEntry) -> Result>> { if entry.file_type == CpioEntryType::Regular { - let path = entry + let sub_path = entry .path .as_bstr() .to_path() .with_context(|| format!("Invalid entry path: {:?}", entry.path.as_bstr()))?; - let parent = util::parent_path(path); + let path = util::path_join(tree, sub_path)?; + let parent = util::parent_path(&path); - tree.create_dir_all(parent) + fs::create_dir_all(parent) .with_context(|| format!("Failed to create directory: {parent:?}"))?; - let writer = tree - .create(path) - .map(|f| BufWriter::new(f.into_std())) + let writer = File::create(&path) + .map(BufWriter::new) .with_context(|| format!("Failed to open for writing: {path:?}"))?; Ok(Some(writer)) @@ -171,16 +170,13 @@ fn unpack_subcommand( display_format(cpio_cli, format); - let authority = ambient_authority(); - Dir::create_ambient_dir_all(&cli.output_tree, authority) + fs::create_dir_all(&cli.output_tree) .with_context(|| format!("Failed to create directory: {:?}", cli.output_tree))?; - let tree = Dir::open_ambient_dir(&cli.output_tree, authority) - .with_context(|| format!("Failed to open directory: {:?}", cli.output_tree))?; while let Some(entry) = reader.next_entry().context("Failed to read cpio entry")? { display_entry(cpio_cli, &entry); - if let Some(mut writer) = create_tree_file(&tree, &entry)? { + if let Some(mut writer) = create_tree_file(&cli.output_tree, &entry)? { let file_size = entry.data.size()?; stream::copy_n(&mut reader, &mut writer, file_size.into(), cancel_signal) @@ -209,12 +205,8 @@ fn pack_subcommand(cpio_cli: &CpioCli, cli: &PackCli, cancel_signal: &AtomicBool cpio::assign_inodes(&mut info.entries, true)?; - let authority = ambient_authority(); - let tree = Dir::open_ambient_dir(&cli.input_tree, authority) - .with_context(|| format!("Failed to open directory: {:?}", cli.input_tree))?; - for entry in &mut info.entries { - let out = open_tree_file(&tree, entry)?; + let out = open_tree_file(&cli.input_tree, entry)?; if let Some((_, file_size)) = &out { entry.data = CpioEntryData::Size(*file_size); diff --git a/avbroot/src/cli/lp.rs b/avbroot/src/cli/lp.rs index deec13b..0d62aa5 100644 --- a/avbroot/src/cli/lp.rs +++ b/avbroot/src/cli/lp.rs @@ -1,8 +1,7 @@ -// SPDX-FileCopyrightText: 2024 Andrew Gunnerson +// SPDX-FileCopyrightText: 2024-2025 Andrew Gunnerson // SPDX-License-Identifier: GPL-3.0-only use std::{ - ffi::OsStr, fs::{self, File}, io::{Seek, SeekFrom}, path::{Path, PathBuf}, @@ -10,7 +9,6 @@ use std::{ }; use anyhow::{Context, Result, bail}; -use cap_std::{ambient_authority, fs::Dir}; use clap::{CommandFactory, Parser, Subcommand}; use rayon::iter::{ IndexedParallelIterator, IntoParallelIterator, IntoParallelRefIterator, ParallelIterator, @@ -19,6 +17,7 @@ use rayon::iter::{ use crate::{ format::lp::{Extent, ExtentType, ImageType, Metadata, SECTOR_SIZE}, stream::{self, FromReader, PSeekFile, Reopen, ToWriter}, + util, }; fn open_lp_inputs(paths: &[impl AsRef]) -> Result<(Vec, Metadata)> { @@ -190,11 +189,8 @@ fn unpack_subcommand(lp_cli: &LpCli, cli: &UnpackCli, cancel_signal: &AtomicBool return Ok(()); } - let authority = ambient_authority(); - Dir::create_ambient_dir_all(&cli.output_images, authority) + fs::create_dir_all(&cli.output_images) .with_context(|| format!("Failed to create directory: {:?}", cli.output_images))?; - let directory = Dir::open_ambient_dir(&cli.output_images, authority) - .with_context(|| format!("Failed to open directory: {:?}", cli.output_images))?; let slot = &metadata.slots[0]; @@ -216,11 +212,11 @@ fn unpack_subcommand(lp_cli: &LpCli, cli: &UnpackCli, cancel_signal: &AtomicBool for partition in &group.partitions { // A partition name with unsafe characters fails during parsing. - let path = format!("{}.img", partition.name); + let path = + util::path_join_single(&cli.output_images, format!("{}.img", partition.name))?; - let file = directory - .create(&path) - .map(|f| PSeekFile::new(f.into_std())) + let file = File::create(&path) + .map(PSeekFile::new) .with_context(|| format!("Failed to open for writing: {path:?}"))?; file.set_len(partition.size()?) @@ -303,35 +299,21 @@ fn pack_subcommand(lp_cli: &LpCli, cli: &PackCli, cancel_signal: &AtomicBool) -> } } - for group in &slot.groups { - for partition in &group.partitions { - let name = &partition.name; - - if Path::new(name).file_name() != Some(OsStr::new(name)) { - bail!("Unsafe partition name: {name}"); - } - } - } - // Preopen all image input files. let mut paths = vec![]; let mut files = vec![]; if metadata.image_type == ImageType::Normal { - let authority = ambient_authority(); - let directory = Dir::open_ambient_dir(&cli.input_images, authority) - .with_context(|| format!("Failed to open directory: {:?}", cli.input_images))?; - for group in &mut slot.groups { let mut group_paths = vec![]; let mut group_files = vec![]; for partition in &mut group.partitions { - let path = format!("{}.img", partition.name); + let path = + util::path_join_single(&cli.input_images, format!("{}.img", partition.name))?; - let mut file = directory - .open(&path) - .map(|f| PSeekFile::new(f.into_std())) + let mut file = File::open(&path) + .map(PSeekFile::new) .with_context(|| format!("Failed to open for reading: {path:?}"))?; let size = file diff --git a/avbroot/src/cli/ota.rs b/avbroot/src/cli/ota.rs index f5cf29e..e926ec6 100644 --- a/avbroot/src/cli/ota.rs +++ b/avbroot/src/cli/ota.rs @@ -15,11 +15,9 @@ use std::{ use anyhow::{Context, Result, anyhow, bail}; use bitflags::bitflags; -use cap_std::{ambient_authority, fs::Dir}; -use cap_tempfile::TempDir; use clap::{ArgAction, Args, Parser, Subcommand, value_parser}; use rayon::{iter::IntoParallelRefIterator, prelude::ParallelIterator}; -use tempfile::NamedTempFile; +use tempfile::{NamedTempFile, TempDir}; use topological_sort::TopologicalSort; use tracing::{debug_span, error, info, warn}; use x509_cert::Certificate; @@ -1293,29 +1291,22 @@ fn patch_ota_zip( pub fn extract_payload( raw_reader: &PSeekFile, - directory: &Dir, + directory: &Path, payload_offset: u64, payload_size: u64, header: &PayloadHeader, images: &BTreeSet, cancel_signal: &AtomicBool, ) -> Result<()> { - for name in images { - if Path::new(name).file_name() != Some(OsStr::new(name)) { - bail!("Unsafe partition name: {name}"); - } - } - info!("Extracting from the payload: {}", util::join(images, ", ")); // Pre-open all output files. let output_files = images .iter() .map(|name| { - let path = format!("{name}.img"); - let file = directory - .create(&path) - .map(|f| PSeekFile::new(f.into_std())) + let path = util::path_join_single(directory, format!("{name}.img"))?; + let file = File::create(&path) + .map(PSeekFile::new) .with_context(|| format!("Failed to open for writing: {path:?}"))?; Ok((name.as_str(), file)) }) @@ -1345,7 +1336,7 @@ pub fn extract_payload( } fn verify_partition_hashes( - directory: &Dir, + directory: &Path, header: &PayloadHeader, images: &BTreeSet, cancel_signal: &AtomicBool, @@ -1365,9 +1356,8 @@ fn verify_partition_hashes( .and_then(|info| info.hash.as_ref()) .ok_or_else(|| anyhow!("Hash not found for partition: {name}"))?; - let path = format!("{name}.img"); - let file = directory - .open(&path) + let path = util::path_join_single(directory, format!("{name}.img"))?; + let file = File::open(&path) .with_context(|| format!("Failed to open for reading: {path:?}"))?; let mut writer = HashingWriter::new( @@ -1702,15 +1692,12 @@ pub fn extract_subcommand(cli: &ExtractCli, cancel_signal: &AtomicBool) -> Resul return Ok(()); } - let authority = ambient_authority(); - Dir::create_ambient_dir_all(&cli.directory, authority) + fs::create_dir_all(&cli.directory) .with_context(|| format!("Failed to create directory: {:?}", cli.directory))?; - let directory = Dir::open_ambient_dir(&cli.directory, authority) - .with_context(|| format!("Failed to open directory: {:?}", cli.directory))?; extract_payload( &raw_reader, - &directory, + &cli.directory, payload_offset, payload_size, &header, @@ -1742,9 +1729,9 @@ pub fn extract_subcommand(cli: &ExtractCli, cancel_signal: &AtomicBool) -> Resul .and_then(|p| p.device.first()) .ok_or_else(|| anyhow!("Device codename not found in OTA metadata"))?; - directory - .write(ANDROID_INFO, format!("require board={device}\n")) - .with_context(|| format!("Failed to write file: {ANDROID_INFO}"))?; + let android_info_path = util::path_join_single(&cli.directory, ANDROID_INFO)?; + fs::write(&android_info_path, format!("require board={device}\n")) + .with_context(|| format!("Failed to write file: {android_info_path:?}"))?; // Find out which images can be flashed with fastboot. The bootloader // (and potentially modem) partitions need to be flashed as a whole and @@ -1753,9 +1740,9 @@ pub fn extract_subcommand(cli: &ExtractCli, cancel_signal: &AtomicBool) -> Resul let mut flashable_images = BTreeSet::new(); for name in &unique_images { - let file = directory - .open(format!("{name}.img")) - .with_context(|| format!("Failed to open image for reading: {name}"))?; + let path = util::path_join_single(&cli.directory, format!("{name}.img"))?; + let file = File::open(&path) + .with_context(|| format!("Failed to open image for reading: {path:?}"))?; match avb::load_image(file) { Ok(_) => { @@ -1763,7 +1750,7 @@ pub fn extract_subcommand(cli: &ExtractCli, cancel_signal: &AtomicBool) -> Resul } // Treat images without AVB metadata as bootloader partitions. Err(avb::Error::InvalidHeaderMagic(_)) => continue, - Err(e) => return Err(e).with_context(|| format!("Failed to load image: {name}")), + Err(e) => return Err(e).with_context(|| format!("Failed to load image: {path:?}")), } } @@ -1851,9 +1838,9 @@ pub fn extract_subcommand(cli: &ExtractCli, cancel_signal: &AtomicBool) -> Resul fastboot_info.push_str("if-wipe erase userdata\n"); fastboot_info.push_str("if-wipe erase metadata\n"); - directory - .write(FASTBOOT_INFO, fastboot_info) - .with_context(|| format!("Failed to write file: {FASTBOOT_INFO}"))?; + let fastboot_info_path = util::path_join_single(&cli.directory, FASTBOOT_INFO)?; + fs::write(&fastboot_info_path, fastboot_info) + .with_context(|| format!("Failed to write file: {fastboot_info_path:?}"))?; } Ok(()) @@ -1934,8 +1921,7 @@ pub fn verify_subcommand(cli: &VerifyCli, cancel_signal: &AtomicBool) -> Result< info!("Extracting partition images to temporary directory"); - let authority = ambient_authority(); - let temp_dir = TempDir::new(authority).context("Failed to create temporary directory")?; + let temp_dir = TempDir::new().context("Failed to create temporary directory")?; let raw_reader = reader.into_inner(); let unique_images = header .manifest @@ -1947,7 +1933,7 @@ pub fn verify_subcommand(cli: &VerifyCli, cancel_signal: &AtomicBool) -> Result< extract_payload( &raw_reader, - &temp_dir, + temp_dir.path(), pf_payload.offset, pf_payload.size, &header, @@ -1957,7 +1943,8 @@ pub fn verify_subcommand(cli: &VerifyCli, cancel_signal: &AtomicBool) -> Result< info!("Verifying partition hashes"); - if let Err(e) = verify_partition_hashes(&temp_dir, &header, &unique_images, cancel_signal) { + if let Err(e) = verify_partition_hashes(temp_dir.path(), &header, &unique_images, cancel_signal) + { fail_later!("{e:?}"); } @@ -1977,7 +1964,7 @@ pub fn verify_subcommand(cli: &VerifyCli, cancel_signal: &AtomicBool) -> Result< let mut descriptors = HashMap::::new(); if let Err(e) = cli::avb::verify_headers( - &temp_dir, + temp_dir.path(), "vbmeta", public_key.as_ref(), &mut seen, @@ -1988,8 +1975,9 @@ pub fn verify_subcommand(cli: &VerifyCli, cancel_signal: &AtomicBool) -> Result< fail_later!("{e:?}"); } - if let Err(e) = cli::avb::verify_descriptors(&temp_dir, &descriptors, false, cancel_signal) - .context("Failed to verify images against AVB descriptors") + if let Err(e) = + cli::avb::verify_descriptors(temp_dir.path(), &descriptors, false, cancel_signal) + .context("Failed to verify images against AVB descriptors") { fail_later!("{e:?}"); } @@ -2003,11 +1991,10 @@ pub fn verify_subcommand(cli: &VerifyCli, cancel_signal: &AtomicBool) -> Result< .map(|(name, _)| name.as_str()) .collect::>(); let boot_images = boot::load_boot_images(&boot_image_names, |name| { - Ok(Box::new( - temp_dir - .open(format!("{name}.img")) - .map(|f| PSeekFile::new(f.into_std()))?, - )) + let path = util::path_join_single(temp_dir.path(), format!("{name}.img")) + .map_err(|e| io::Error::new(io::ErrorKind::InvalidInput, e))?; + + Ok(Box::new(File::open(path).map(PSeekFile::new)?)) }) .context("Failed to load all boot images")?; let targets = OtaCertPatcher::new(ota_cert.clone()) diff --git a/avbroot/src/cli/payload.rs b/avbroot/src/cli/payload.rs index afa568a..60ed329 100644 --- a/avbroot/src/cli/payload.rs +++ b/avbroot/src/cli/payload.rs @@ -1,9 +1,9 @@ -// SPDX-FileCopyrightText: 2024 Andrew Gunnerson +// SPDX-FileCopyrightText: 2024-2025 Andrew Gunnerson // SPDX-License-Identifier: GPL-3.0-only use std::{ collections::HashMap, - ffi::{OsStr, OsString}, + ffi::OsString, fs::{self, File}, io::{BufReader, BufWriter, Seek, SeekFrom}, path::{Path, PathBuf}, @@ -11,7 +11,6 @@ use std::{ }; use anyhow::{Context, Result, anyhow, bail}; -use cap_std::{ambient_authority, fs::Dir}; use clap::{Args, Parser, Subcommand}; use tracing::info; @@ -20,6 +19,7 @@ use crate::{ crypto::{self, PassphraseSource, RsaSigningKey}, format::payload::{PayloadHeader, PayloadWriter}, stream::{self, FromReader, PSeekFile}, + util, }; fn open_reader(path: &Path, allow_delta: bool) -> Result<(BufReader, PayloadHeader)> { @@ -113,15 +113,12 @@ fn unpack_subcommand( write_info(&cli.output_info, &header)?; - let authority = ambient_authority(); - Dir::create_ambient_dir_all(&cli.output_images, authority) + fs::create_dir_all(&cli.output_images) .with_context(|| format!("Failed to create directory: {:?}", cli.output_images))?; - let directory = Dir::open_ambient_dir(&cli.output_images, authority) - .with_context(|| format!("Failed to open directory: {:?}", cli.output_images))?; ota::extract_payload( &PSeekFile::new(reader.into_inner()), - &directory, + &cli.output_images, 0, payload_size, &header, @@ -147,28 +144,16 @@ fn pack_subcommand( let mut header = read_info(&cli.input_info)?; - let authority = ambient_authority(); - let directory = Dir::open_ambient_dir(&cli.input_images, authority) - .with_context(|| format!("Failed to open directory: {:?}", cli.input_images))?; - - for p in &header.manifest.partitions { - let name = &p.partition_name; - - if Path::new(name).file_name() != Some(OsStr::new(name)) { - bail!("Unsafe partition name: {name}"); - } - } - // Pre-open all of the image files. let input_files = header .manifest .partitions .iter() .map(|p| { - let path = format!("{}.img", p.partition_name); - let file = directory - .open(&path) - .map(|f| PSeekFile::new(f.into_std())) + let path = + util::path_join_single(&cli.input_images, format!("{}.img", p.partition_name))?; + let file = File::open(&path) + .map(PSeekFile::new) .with_context(|| format!("Failed to open file: {path:?}"))?; Ok((p.partition_name.clone(), file)) diff --git a/avbroot/src/patch/boot.rs b/avbroot/src/patch/boot.rs index ab5a615..f30f481 100644 --- a/avbroot/src/patch/boot.rs +++ b/avbroot/src/patch/boot.rs @@ -35,6 +35,7 @@ use crate::{ }, patch::otacert::{self, OtaCertBuildFlags}, stream::{self, FromReader, HashingWriter, ReadSeek, SectionReader, ToWriter, WriteSeek}, + util, }; #[derive(Debug, Error)] @@ -929,16 +930,17 @@ impl PrepatchedImagePatcher { return Ok(None); }; - let kmi_version = captures - .iter() - // Capture #0 is the entire match. - .skip(1) - .flatten() - .map(|c| c.as_bytes()) - // Our regex only matches ASCII bytes. - .map(|c| std::str::from_utf8(c).unwrap()) - .collect::>() - .join("-"); + let kmi_version = util::join( + captures + .iter() + // Capture #0 is the entire match. + .skip(1) + .flatten() + .map(|c| c.as_bytes()) + // Our regex only matches ASCII bytes. + .map(|c| std::str::from_utf8(c).unwrap()), + "-", + ); Ok(Some(kmi_version)) } diff --git a/avbroot/src/util.rs b/avbroot/src/util.rs index a08639d..e409da7 100644 --- a/avbroot/src/util.rs +++ b/avbroot/src/util.rs @@ -8,7 +8,7 @@ use std::{ ops::{ Bound, Range, RangeBounds, RangeFrom, RangeFull, RangeInclusive, RangeTo, RangeToInclusive, }, - path::Path, + path::{Component, Path, PathBuf}, }; use num_traits::{NumCast, PrimInt}; @@ -313,6 +313,67 @@ pub fn parent_path(path: &Path) -> &Path { Path::new(".") } +/// Path safety-related errors. +#[derive(Clone, Debug, Error)] +pub enum PathSafetyError { + #[error("Path contains multiple components: {0:?}")] + NotSingle(PathBuf), + #[error("Path contains '..': {0:?}")] + HasDotDot(PathBuf), +} + +/// Join `parent` with `child`, disallowing unsafe child paths. Absolute child +/// paths are converted into relative paths and `..` components result in an +/// error. +pub fn path_join( + parent: impl AsRef, + child: impl AsRef, +) -> Result { + fn inner(parent: &Path, child: &Path) -> Result { + let mut result = parent.to_owned(); + + for component in child.components() { + match component { + // Make absolute paths relative. + Component::Prefix(_) | Component::RootDir | Component::CurDir => continue, + // Unsafe path. We block this even if it wouldn't escape the parent. + Component::ParentDir => { + return Err(PathSafetyError::HasDotDot(child.to_path_buf())); + } + Component::Normal(os_str) => result.push(os_str), + } + } + + Ok(result) + } + + inner(parent.as_ref(), child.as_ref()) +} + +/// Ensure that the path has no directory components. +pub fn path_ensure_single(name: impl AsRef) -> Result<(), PathSafetyError> { + fn inner(name: &Path) -> Result<(), PathSafetyError> { + // Path::file_name() already checks for `.` and `..`. + if name.file_name() != Some(name.as_os_str()) { + return Err(PathSafetyError::NotSingle(name.to_path_buf())); + } + + Ok(()) + } + + inner(name.as_ref()) +} + +/// Like [`path_join`], but ensures that the child path contains no directory +/// components with [`path_ensure_single`]. +pub fn path_join_single( + parent: impl AsRef, + child: impl AsRef, +) -> Result { + path_ensure_single(child.as_ref())?; + path_join(parent.as_ref(), child.as_ref()) +} + /// Sort and merge overlapping intervals. pub fn merge_overlapping(sections: &[Range]) -> Vec> where @@ -379,6 +440,7 @@ where .is_ok() } +/// Join arbitrary displayable items with a separator. pub fn join(into_iter: impl IntoIterator, sep: &str) -> String { use std::fmt::Write; @@ -395,6 +457,7 @@ pub fn join(into_iter: impl IntoIterator, sep: &str) -> Str result } +/// Sort arbitrary sequence of sortable items. pub fn sort(iter: impl Iterator) -> Vec { let mut items = iter.collect::>(); items.sort(); diff --git a/e2e/src/main.rs b/e2e/src/main.rs index 697513e..9fe4826 100644 --- a/e2e/src/main.rs +++ b/e2e/src/main.rs @@ -47,6 +47,7 @@ use avbroot::{ }, }, stream::{self, CountingWriter, FromReader, HashingReader, PSeekFile, Reopen, ToWriter}, + util, }; use clap::Parser; use rsa::{BigUint, rand_core::OsRng, traits::PublicKeyParts}; @@ -1222,10 +1223,6 @@ fn test_subcommand(cli: &TestCli, cancel_signal: &AtomicBool) -> Result<()> { ]; for name in profiles { - if Path::new(name).file_name() != Some(OsStr::new(name)) { - bail!("Unsafe profile name: {name}"); - } - let profile = &config.profile[name]; for (zip_mode, hashes) in [ @@ -1235,7 +1232,7 @@ fn test_subcommand(cli: &TestCli, cancel_signal: &AtomicBool) -> Result<()> { let _span = info_span!("profile", name, %zip_mode).entered(); // Can't use NamedTempFile because avbroot does atomic replaces. - let mut profile_dir = work_dir.join(name); + let mut profile_dir = util::path_join_single(work_dir, name)?; profile_dir.push(zip_mode.to_string()); let out_original = profile_dir.join("ota.zip"); let out_magisk = profile_dir.join("ota_magisk.zip");