From 208171b92004e799eb2b4cfea12c92bbaec373a3 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 05:36:05 -0500 Subject: [PATCH] update: stage replacements exclusively and refuse oversized tar metadata (#563) * update: stage replacements exclusively and refuse oversized tar metadata - self_update: stage_copy creates the staged file beside the target with an unpredictable name (tempfile, O_EXCL) and writes bytes and mode through the handle, for both the AppImage and the tarball standalone replacement; a link at the old ..update names is never followed. - install::tree::unpack: check_metadata walks the tar headers first and refuses PAX or GNU long-name records over 1 MiB, which the tar crate buffers whole before the entry loop sees a size; both passes read at most MAX_TAR_BYTES of decompressed stream. * address review: refuse size-truncated update archives, fixed staging prefix --- crates/update/src/install.rs | 75 ++++++++++++++++++++++++++++++-- crates/update/src/self_update.rs | 55 +++++++++++++++-------- 2 files changed, 107 insertions(+), 23 deletions(-) diff --git a/crates/update/src/install.rs b/crates/update/src/install.rs index fae711a9b..070e035be 100644 --- a/crates/update/src/install.rs +++ b/crates/update/src/install.rs @@ -1939,18 +1939,61 @@ pub mod macos { #[cfg(unix)] #[allow(dead_code)] pub(crate) mod tree { + use std::io::Read; + use super::{App, Path, PathBuf, single_top_folder}; + // Bound entry count and cumulative unpacked bytes to prevent decompression bombs (CWE-409). + const MAX_ENTRIES: usize = 50_000; + const MAX_UNPACKED_BYTES: u64 = 2 * 1024 * 1024 * 1024; + /// The most decompressed tar stream either pass reads: the unpacked bytes plus room for + /// headers, padding and metadata records. + const MAX_TAR_BYTES: u64 = MAX_UNPACKED_BYTES + (1 << 30); + /// The most a PAX or GNU long-name record may hold. The tar crate buffers such a record + /// whole while the iterator advances, before the loop below sees any entry size. + const MAX_METADATA_BYTES: u64 = 1 << 20; + + /// Walks the headers of the tarball `file` without unpacking anything, and refuses a + /// metadata record (PAX `x`/`g`, GNU `L`/`K`) past [`MAX_METADATA_BYTES`]. A damaged + /// archive passes: unpacking reports it. + fn check_metadata(file: &Path) -> Result<(), String> { + let gz = std::fs::File::open(file).map_err(|e| e.to_string())?; + let mut tar = flate2::read::GzDecoder::new(gz).take(MAX_TAR_BYTES); + let mut header = [0u8; 512]; + while tar.read_exact(&mut header).is_ok() && header.iter().any(|b| *b != 0) { + let size = header_size(&header[124..136]).ok_or("invalid tar header")?; + if matches!(header[156], b'x' | b'g' | b'L' | b'K') && size > MAX_METADATA_BYTES { + return Err("archive has an oversized metadata record".into()); + } + let padded = size.div_ceil(512).saturating_mul(512); + if std::io::copy(&mut (&mut tar).take(padded), &mut std::io::sink()).map_err(|e| e.to_string())? < padded { + break; + } + } + Ok(()) + } + + /// A tar header's size field: octal digits, or big-endian base-256 after a set top bit. + fn header_size(field: &[u8]) -> Option { + if field.first()? & 0x80 != 0 { + return field + .iter() + .enumerate() + .try_fold(0u64, |n, (i, b)| n.checked_mul(256)?.checked_add(u64::from(if i == 0 { b & 0x7f } else { *b }))); + } + let digits = std::str::from_utf8(field).ok()?.trim_matches(|c: char| c == '\0' || c == ' '); + if digits.is_empty() { Some(0) } else { u64::from_str_radix(digits, 8).ok() } + } + /// Unpacks `file` and returns a staging folder laid out like `/usr/local`. pub fn unpack(app: &App, file: &Path, staging: &Path) -> Result { + check_metadata(file)?; let _ = std::fs::remove_dir_all(staging); std::fs::create_dir_all(staging).map_err(|e| format!("couldn't create {} ({e})", staging.display()))?; let gz = std::fs::File::open(file).map_err(|e| e.to_string())?; - let mut archive = tar::Archive::new(flate2::read::GzDecoder::new(gz)); + // Never past what `check_metadata` read. + let mut archive = tar::Archive::new(flate2::read::GzDecoder::new(gz).take(MAX_TAR_BYTES)); archive.set_preserve_permissions(true); - // Bound entry count and cumulative unpacked bytes to prevent decompression bombs (CWE-409). - const MAX_ENTRIES: usize = 50_000; - const MAX_UNPACKED_BYTES: u64 = 2 * 1024 * 1024 * 1024; let mut entry_count = 0; let mut total_unpacked = 0u64; let mut directories = Vec::new(); @@ -1977,6 +2020,10 @@ pub(crate) mod tree { for mut dir in directories { dir.unpack_in(staging).map_err(|e| format!("couldn't unpack archive directory ({e})"))?; } + // tar reads a limit-truncated stream as a clean end of archive, so an exhausted limit is refused here. + if archive.into_inner().limit() == 0 { + return Err("archive exceeds maximum size".into()); + } let top = single_top_folder(staging)?; if top.join("bin").join(app.id).is_file() { return Ok(top); @@ -2739,6 +2786,26 @@ mod tests { assert!(tree::unpack(app, &bad, &dir.path().join("staging")).is_err()); } + #[cfg(unix)] + #[test] + fn an_oversized_pax_record_is_refused_before_tar_buffers_it() { + use std::io::Write; + let dir = tempfile::tempdir().unwrap(); + // A PAX header claiming 1 MiB + 1 byte of records, compressed to almost nothing. + let mut header = tar::Header::new_ustar(); + header.set_entry_type(tar::EntryType::XHeader); + header.set_size((1 << 20) + 1); + header.set_path("pax").unwrap(); + header.set_cksum(); + let file = dir.path().join("pax.tar.gz"); + let mut gz = flate2::write::GzEncoder::new(std::fs::File::create(&file).unwrap(), flate2::Compression::best()); + gz.write_all(header.as_bytes()).unwrap(); + gz.write_all(&vec![b'a'; (1 << 20) + 512]).unwrap(); + gz.finish().unwrap(); + let err = tree::unpack(App::by_id("wordcraft").unwrap(), &file, &dir.path().join("staging")).unwrap_err(); + assert!(err.contains("oversized metadata"), "{err}"); + } + #[cfg(unix)] #[test] fn tree_files_does_not_traverse_directory_symlinks() { diff --git a/crates/update/src/self_update.rs b/crates/update/src/self_update.rs index 780590db2..87ae16d9b 100644 --- a/crates/update/src/self_update.rs +++ b/crates/update/src/self_update.rs @@ -1068,12 +1068,9 @@ fn unmark_unless_scheduled(paths: &Paths, scheduled: Result) -> Re /// mounted from its own (now unlinked) file, so it keeps working until it exits. #[cfg(target_os = "linux")] fn replace_appimage(new: &Path, path: &Path) -> Result<(), String> { - use std::os::unix::fs::PermissionsExt; - let dir = path.parent().ok_or("the AppImage isn't in a folder")?; - let staged = dir.join(format!(".{}.update", path.file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_default())); - std::fs::copy(new, &staged).map_err(|e| format!("couldn't write next to {} ({e})", path.display()))?; + path.parent().ok_or("the AppImage isn't in a folder")?; // The replacement AppImage must be executable; 0o755 is not group/world-writable. - std::fs::set_permissions(&staged, std::fs::Permissions::from_mode(0o755)).map_err(|e| e.to_string())?; // NOSONAR + let staged = stage_copy(new, path, 0o755)?; // NOSONAR std::fs::rename(&staged, path).map_err(|e| { let _ = std::fs::remove_file(&staged); format!("couldn't replace {} ({e})", path.display()) @@ -1185,20 +1182,24 @@ fn stage(from: &Path, to: &Path, executable: bool) -> Result { use std::os::unix::fs::PermissionsExt; let dir = to.parent().ok_or_else(|| format!("{} isn't in a folder", to.display()))?; std::fs::create_dir_all(dir).map_err(|e| format!("couldn't create {} ({e})", dir.display()))?; - let name = to.file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_default(); - let staged = dir.join(format!(".{name}.{}.update", std::process::id())); - let done = std::fs::copy(from, &staged).map_err(|e| format!("couldn't write next to {} ({e})", to.display())).and_then(|_| { - let mode = std::fs::metadata(from).map(|m| m.permissions().mode()).unwrap_or(0o644); - let mode = if executable { 0o755 } else { mode & 0o755 }; - std::fs::set_permissions(&staged, std::fs::Permissions::from_mode(mode)).map_err(|e| e.to_string()) - }); - match done { - Ok(()) => Ok(staged), - Err(e) => { - let _ = std::fs::remove_file(&staged); - Err(e) - } - } + let mode = std::fs::metadata(from).map(|m| m.permissions().mode()).unwrap_or(0o644); + stage_copy(from, to, if executable { 0o755 } else { mode & 0o755 }) +} + +/// Copies `from` to a new file beside `to` and returns its path. The file is created exclusively +/// under an unpredictable name, so a link someone left at a guessable name is never followed, +/// and its bytes and `mode` go through the open handle. Removed again when any step fails. +#[cfg(unix)] +fn stage_copy(from: &Path, to: &Path, mode: u32) -> Result { + use std::os::unix::fs::PermissionsExt; + let dir = to.parent().ok_or_else(|| format!("{} isn't in a folder", to.display()))?; + let write_error = |e: std::io::Error| format!("couldn't write next to {} ({e})", to.display()); + // fixed prefix: the temp name's length must not grow with the destination's basename + let mut staged = tempfile::Builder::new().prefix(".staged-").suffix(".update").tempfile_in(dir).map_err(write_error)?; + let mut source = std::fs::File::open(from).map_err(|e| format!("couldn't read {} ({e})", from.display()))?; + std::io::copy(&mut source, staged.as_file_mut()).map_err(write_error)?; + staged.as_file().set_permissions(std::fs::Permissions::from_mode(mode)).map_err(|e| e.to_string())?; + staged.into_temp_path().keep().map_err(|e| e.to_string()) } /// A regular file, not a link to one. @@ -1730,6 +1731,22 @@ fn step_after_wait(app: &App, reference: &Path, pid: u32, exe: &Path, step: &Fin mod tests { use super::*; + #[cfg(unix)] + #[test] + fn staging_never_writes_through_a_link_at_the_old_staging_name() { + let dir = tempfile::tempdir().unwrap(); + let (new, exe, victim) = (dir.path().join("new"), dir.path().join("app"), dir.path().join("victim")); + std::fs::write(&new, b"replacement").unwrap(); + std::fs::write(&exe, b"old").unwrap(); + std::fs::write(&victim, b"keep me").unwrap(); + // The names staging used to take: `.app.update` and `.app..update`. + std::os::unix::fs::symlink(&victim, dir.path().join(".app.update")).unwrap(); + std::os::unix::fs::symlink(&victim, dir.path().join(format!(".app.{}.update", std::process::id()))).unwrap(); + let staged = stage_copy(&new, &exe, 0o755).unwrap(); + assert_eq!(std::fs::read(&staged).unwrap(), b"replacement"); + assert!(std::fs::symlink_metadata(&staged).unwrap().file_type().is_file()); + assert_eq!(std::fs::read(&victim).unwrap(), b"keep me"); + } #[test] fn macos_package_manager_folders_are_recognised() { for owned in [