diff --git a/docs/bootstrap/packages/brew.md b/docs/bootstrap/packages/brew.md index 3ab2fc59bc3..2a183658f0a 100644 --- a/docs/bootstrap/packages/brew.md +++ b/docs/bootstrap/packages/brew.md @@ -67,6 +67,27 @@ installs app bundles into `/Applications` while recording the version under "brew-cask:homebrew/cask/visual-studio-code" = "latest" ``` +### Overriding the application directory + +By default `app` artifacts are installed into `/Applications`, matching +Homebrew. Set the `MISE_BREW_CASK_OPT_APPDIR` environment variable to install +them somewhere else — for example a user-writable `~/Applications` that does not +require elevation: + +```sh +MISE_BREW_CASK_OPT_APPDIR="$HOME/Applications" mise bootstrap packages apply brew-cask:firefox +``` + +The value must be an absolute path, must not contain `..`, and must not resolve +to the filesystem root. It is resolved to a real path (symlinks are followed) +before use, so it acts as a fixed containment boundary for the app links mise +creates. An empty value is ignored and falls back to `/Applications`. When the +override is set, a cask that targets the default `/Applications` (as most do) is +relocated into the override directory, preserving any subdirectories the cask +requests; targets that a cask anchors under `$HOMEBREW_PREFIX/Applications` are +left in the Homebrew prefix and are never relocated. This mirrors Homebrew's own +`--appdir` install option. + On Linux, initial cask support is limited to font-only casks without lifecycle hooks or structured `preflight_steps` or `postflight_steps` — concepts from Homebrew's cask DSL, documented in the diff --git a/e2e/cli/test_system_install_brew_cask_appdir_slow b/e2e/cli/test_system_install_brew_cask_appdir_slow new file mode 100644 index 00000000000..573dca52be4 --- /dev/null +++ b/e2e/cli/test_system_install_brew_cask_appdir_slow @@ -0,0 +1,49 @@ +#!/usr/bin/env bash + +# Verifies MISE_BREW_CASK_OPT_APPDIR relocates a cask's app artifact out of the +# default /Applications and into an override directory. Needs a macOS CI runner +# with network, and is kept out of local runs so it never touches a developer's +# installed applications. +if [[ "$(uname)" != "Darwin" ]] || [[ ${CI:-} != "true" ]]; then + exit 0 +fi + +# The hiddenbar cask hardcodes an absolute `/Applications/Hidden Bar.app` +# target, which is the exact shape the override has to relocate. +default_app="/Applications/Hidden Bar.app" +caskroom="/opt/homebrew/Caskroom/hiddenbar" +appdir="$PWD/Applications" +override_app="$appdir/Hidden Bar.app" + +# Never run if it would disturb a real installation of this cask. +if [[ -e $default_app ]] || [[ -e $caskroom ]]; then + exit 0 +fi + +cleanup() { + shopt -s nullglob + local app_work_paths=( + "$appdir"/Hidden\ Bar.mise-old-* + "$appdir"/Hidden\ Bar.mise-tmp-* + ) + chmod -RN "$override_app" "${app_work_paths[@]}" 2>/dev/null || true + rm -rf "$override_app" "$caskroom" "$appdir" "${app_work_paths[@]}" +} +trap cleanup EXIT + +export MISE_BREW_CASK_OPT_APPDIR="$appdir" + +cat <mise.toml +[bootstrap.packages] +"brew-cask:hiddenbar" = "latest" +EOF + +assert_contains "mise bootstrap packages status" "missing" +mise bootstrap packages apply --yes + +# The app must land under the override directory, and must NOT be created in the +# default /Applications location. +assert_directory_exists "$override_app" +assert_directory_exists "$caskroom" +assert_fail "test -e '$default_app'" +assert_contains "mise bootstrap packages status" "installed" diff --git a/src/system/packages/brew/cask.rs b/src/system/packages/brew/cask.rs index b694e3e2393..3e64c7a057c 100644 --- a/src/system/packages/brew/cask.rs +++ b/src/system/packages/brew/cask.rs @@ -2,7 +2,6 @@ use std::io::{Read, Seek, SeekFrom}; #[cfg(unix)] use std::os::unix::ffi::OsStrExt; use std::path::{Component, Path, PathBuf}; -use std::process::Stdio; use std::{ borrow::Cow, collections::{BTreeMap, BTreeSet}, @@ -34,6 +33,12 @@ use crate::ui::progress_report::{ProgressIcon, SingleReport}; const API_BASE: &str = "https://formulae.brew.sh/api"; const HOMEBREW_CASK_RAW: &str = "https://raw.githubusercontent.com/Homebrew/homebrew-cask"; const CASK_SHIM_RB: &str = include_str!("cask_shim.rb"); +/// where `app` artifacts are linked when [`APP_DIR_ENV`] is unset +const DEFAULT_APP_DIR: &str = "/Applications"; +/// user-facing override for the `app` artifact destination, mirroring +/// `brew install --appdir`; see [`target_app_dir`] and +/// docs/bootstrap/packages/brew.md +const APP_DIR_ENV: &str = "MISE_BREW_CASK_OPT_APPDIR"; pub struct BrewCaskManager {} @@ -663,7 +668,7 @@ impl CommandWrapperArtifact { } fn target_path(&self) -> Result { - binary_target_path(&self.target_name()?, Path::new("/Applications")) + binary_target_path(&self.target_name()?, &target_app_dir()?) } fn caskroom_path(&self, caskroom: &Path) -> PathBuf { @@ -1188,67 +1193,111 @@ fn install_app(stage: &Path, caskroom: &Path, app: &AppArtifact) -> Result<()> { let caskroom_app = caskroom.join(app_bundle_name(app.target_name())?); file::remove_all(&caskroom_app)?; ditto(&source, &caskroom_app)?; - let target = app_target_path(app.target_name())?; - if let Some(parent) = target.parent() { - file::create_dir_all(parent)?; - } - let tmp_target = target.with_extension(format!( - "mise-tmp-{}", - crate::hash::hash_to_str(&target.display().to_string()) - )); - file::remove_all(&tmp_target)?; - ditto(&caskroom_app, &tmp_target)?; - swap_app(&target, &tmp_target)?; + let logical_target = app_target_path(app.target_name())?; + // Hold the verified appdir open for the whole mutation and address the app + // only by name relative to that descriptor. Nothing below resolves a + // pathname for the application directory, so a post-validation replacement + // of any appdir component — even by the same uid — cannot redirect the + // copy, rename, removal, permission repair, or quarantine steps. + let parent = ensure_trusted_appdir( + logical_target + .parent() + .ok_or_else(|| eyre!("brew-cask: app target has no parent directory"))?, + )?; + let name = logical_target + .file_name() + .ok_or_else(|| eyre!("brew-cask: app target has no filename"))? + .to_owned(); + // Suffix hashes stay derived from the logical path so temporary and backup + // names are stable across runs. + let name_hash = crate::hash::hash_to_str(&logical_target.display().to_string()); + let tmp_name = replace_bundle_extension(&name, &format!("mise-tmp-{name_hash}")); + let old_name = replace_bundle_extension(&name, &format!("mise-old-{name_hash}")); + remove_all_at(&parent.fd, &tmp_name)?; + ditto_into(&caskroom_app, &parent.fd, &tmp_name)?; + swap_app_at(&parent, &name, &tmp_name, &old_name)?; // Remove macOS quarantine attribute so Gatekeeper doesn't block the app. - let _ = std::process::Command::new("xattr") - .args(["-r", "-d", "com.apple.quarantine"]) - .arg(&target) - .stdout(std::process::Stdio::null()) - .stderr(std::process::Stdio::null()) - .status(); + let relative = Path::new(".").join(&name); + let _ = run_in_trusted_dir( + "xattr", + &[ + std::ffi::OsStr::new("-r"), + std::ffi::OsStr::new("-d"), + std::ffi::OsStr::new("com.apple.quarantine"), + relative.as_os_str(), + ], + &parent.fd, + ); Ok(()) } -/// Atomically replace an app, restoring the previous bundle if activation fails. -fn swap_app(target: &Path, tmp_target: &Path) -> Result<()> { - // Atomic swap: rename existing target aside before putting the new one in place so that - // a failure during rename leaves the old app intact rather than leaving nothing. - let old_target = target.with_extension(format!( - "mise-old-{}", - crate::hash::hash_to_str(&target.display().to_string()) - )); - remove_app(&old_target)?; - if target.exists() { - file::rename(target, &old_target)?; +/// Replace an app bundle's extension for a bare file name +/// (`Firefox.app` + `mise-tmp-ab12` -> `Firefox.mise-tmp-ab12`). +fn replace_bundle_extension(name: &std::ffi::OsStr, extension: &str) -> std::ffi::OsString { + Path::new(name).with_extension(extension).into_os_string() +} + +/// Atomically replace an app inside `parent`, restoring the previous bundle if +/// activation fails. All operations are `*at`-relative to the verified +/// descriptor, so no application directory pathname is ever re-resolved. +#[cfg(unix)] +fn swap_app_at( + parent: &TrustedOperationParent, + name: &std::ffi::OsStr, + tmp_name: &std::ffi::OsStr, + old_name: &std::ffi::OsStr, +) -> Result<()> { + // Atomic swap: rename the existing target aside before putting the new one + // in place so a failure leaves the old app intact rather than nothing. + remove_app_at(parent, old_name)?; + if exists_at(&parent.fd, name)? { + nix::fcntl::renameat(&parent.fd, name, &parent.fd, old_name)?; } - if let Err(e) = file::rename(tmp_target, target) { + if let Err(e) = nix::fcntl::renameat(&parent.fd, tmp_name, &parent.fd, name) { // Restore the old app if the swap failed. - if old_target.exists() { - let _ = file::rename(&old_target, target); + if exists_at(&parent.fd, old_name)? { + let _ = nix::fcntl::renameat(&parent.fd, old_name, &parent.fd, name); } - return Err(e); + return Err(e).wrap_err_with(|| { + format!( + "brew-cask: failed to activate {}", + Path::new(name).display() + ) + }); } // The replacement is already live. A cleanup failure must not report the - // install as failed or prevent install_app from removing quarantine. - if let Err(err) = remove_app(&old_target) { + // install as failed or prevent removing quarantine. + if let Err(err) = remove_app_at(parent, old_name) { warn!( "brew-cask: failed to remove old app backup {}: {err:#}", - old_target.display() + Path::new(old_name).display() ); } Ok(()) } -/// Remove an app bundle, repairing protected contents before escalating ownership. -fn remove_app(path: &Path) -> Result<()> { - match file::remove_all(path) { +/// Whether `name` exists in `dir`, without following a final symlink. +#[cfg(unix)] +fn exists_at(dir: Fd, name: &std::ffi::OsStr) -> Result { + match nix::sys::stat::fstatat(dir, name, nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW) { + Ok(_) => Ok(true), + Err(nix::errno::Errno::ENOENT) => Ok(false), + Err(err) => Err(err.into()), + } +} + +/// Remove an app bundle inside `parent`, repairing protected contents before +/// escalating ownership. Removal and repair are bound to the descriptor. +#[cfg(unix)] +fn remove_app_at(parent: &TrustedOperationParent, name: &std::ffi::OsStr) -> Result<()> { + match remove_all_at(&parent.fd, name) { Ok(()) => return Ok(()), Err(err) if !is_permission_denied(&err) => return Err(err), Err(_) => {} } - repair_app_permissions(path); - match file::remove_all(path) { + repair_app_permissions_at(parent, name); + match remove_all_at(&parent.fd, name) { Ok(()) => return Ok(()), Err(err) if !is_permission_denied(&err) => return Err(err), Err(_) => {} @@ -1257,36 +1306,69 @@ fn remove_app(path: &Path) -> Result<()> { let user = nix::unistd::User::from_uid(nix::unistd::geteuid())? .map(|user| user.name) .ok_or_else(|| eyre!("brew-cask: could not determine current user"))?; - // Match Homebrew's final ownership-recovery step. sudo::run applies the - // system_packages.sudo setting and refuses to prompt without a TTY. - sudo::run( + // Match Homebrew's final ownership-recovery step. Both the unprivileged and + // the elevated attempt run with their working directory bound to the + // verified appdir descriptor and address the bundle by a relative name, so + // the recursive chown cannot be redirected outside the validated directory + // by a replacement of any appdir component. sudo::run_in_dir applies the + // same system_packages.sudo policy as sudo::run and refuses to prompt + // without a TTY. + // + // `-h -P` keep the recursion inside the bundle: cask bundles legitimately + // contain symlinks, and without these the walk would dereference one that + // points outward and change ownership of the referent outside the verified + // application directory. + let relative = Path::new(".").join(name); + let status = run_in_trusted_dir( "chown", &[ - "-R".to_string(), - "--".to_string(), - user, - path.display().to_string(), + std::ffi::OsStr::new("-R"), + std::ffi::OsStr::new("-h"), + std::ffi::OsStr::new("-P"), + std::ffi::OsStr::new("--"), + std::ffi::OsStr::new(&user), + relative.as_os_str(), ], - &[], - )?; - repair_app_permissions(path); - file::remove_all(path) + &parent.fd, + ); + if !matches!(status, Ok(status) if status.success()) { + sudo::run_in_dir( + "chown", + &[ + "-R".to_string(), + "-h".to_string(), + "-P".to_string(), + "--".to_string(), + user, + relative.display().to_string(), + ], + &parent.fd, + )?; + } + repair_app_permissions_at(parent, name); + remove_all_at(&parent.fd, name) } -/// Clear flags, restore owner permissions, and remove ACLs from an app bundle. -fn repair_app_permissions(path: &Path) { +/// Clear flags, restore owner permissions, and remove ACLs from an app bundle, +/// resolving it relative to the verified descriptor. +/// +/// Every command is recursive, so `-P` is mandatory: cask bundles legitimately +/// contain symlinks, and without it a bundle symlink pointing outside the +/// application directory would have the flags or permissions of its referent +/// changed instead. macOS `chmod`/`chflags` reject `-R` together with `-h`, but +/// `-P` alone keeps the recursion from dereferencing symlink entries (verified: +/// an outward symlink's referent keeps its original mode and flags). +#[cfg(unix)] +fn repair_app_permissions_at(parent: &TrustedOperationParent, name: &std::ffi::OsStr) { + let relative = Path::new(".").join(name); let run = |program: &str, args: &[&str]| { - let _ = std::process::Command::new(program) - .args(args) - .arg(path) - .stdin(Stdio::null()) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status(); + let mut argv: Vec<&std::ffi::OsStr> = args.iter().map(std::ffi::OsStr::new).collect(); + argv.push(relative.as_os_str()); + let _ = run_in_trusted_dir(program, &argv, &parent.fd); }; - run("/usr/bin/chflags", &["-R", "--", "000"]); - run("/bin/chmod", &["-R", "--", "u+rwx"]); - run("/bin/chmod", &["-R", "-N"]); + run("/usr/bin/chflags", &["-R", "-P", "--", "000"]); + run("/bin/chmod", &["-R", "-P", "--", "u+rwx"]); + run("/bin/chmod", &["-R", "-P", "-N"]); } /// Return whether an eyre chain originated from an I/O permission error. @@ -1956,7 +2038,7 @@ impl TrustedOperationParent { { let mut path = PathBuf::new(); nix::fcntl::fcntl(&self.fd, nix::fcntl::FcntlArg::F_GETPATH(&mut path))?; - return Ok(path); + Ok(path) } #[cfg(not(any(target_os = "linux", target_os = "macos", target_os = "ios")))] Ok(Path::new("/dev/fd").join(std::os::fd::AsRawFd::as_raw_fd(&self.fd).to_string())) @@ -2024,20 +2106,45 @@ fn open_trusted_operation_parent( allow_current_user: bool, create_missing: bool, ) -> Result { - use nix::fcntl::{OFlag, open, openat}; - use nix::sys::stat::{Mode, SFlag, fstat}; - let prefix = prefix::prefix(); let parent = target .parent() .ok_or_else(|| eyre!("brew-cask: generic artifact target has no parent"))?; let relative_parent = parent.strip_prefix(&prefix)?; let resolved_prefix = file::desymlink_path(&prefix); + open_trusted_directory( + &resolved_prefix, + relative_parent, + allow_current_user, + create_missing, + ) +} + +/// Open (and optionally create) `resolved_root`/`relative` one component at a +/// time with `O_NOFOLLOW`, verifying at every step that the component is a real +/// directory owned by root or the current user and not writable by an untrusted +/// party. `resolved_root` must already be a real (symlink-free) absolute path. +/// +/// Walking component-by-component with `O_NOFOLLOW` binds the operation to the +/// exact directory chain that existed when the walk ran: a symlink cannot be +/// interposed for any component, and because every ancestor is verified as +/// non-untrusted-writable an unprivileged attacker cannot plant one to begin +/// with. Callers then mutate relative to the returned directory fd. +#[cfg(unix)] +fn open_trusted_directory( + resolved_root: &Path, + relative: &Path, + allow_current_user: bool, + create_missing: bool, +) -> Result { + use nix::fcntl::{OFlag, open, openat}; + use nix::sys::stat::{Mode, SFlag, fstat}; + let flags = OFlag::O_RDONLY | OFlag::O_DIRECTORY | OFlag::O_NOFOLLOW; - let mut fd = open(&resolved_prefix, flags, Mode::empty()).wrap_err_with(|| { + let mut fd = open(resolved_root, flags, Mode::empty()).wrap_err_with(|| { format!( "brew-cask: cannot open operation directory {}", - resolved_prefix.display() + resolved_root.display() ) })?; let current_uid = nix::unistd::geteuid().as_raw(); @@ -2065,9 +2172,9 @@ fn open_trusted_operation_parent( } Ok(()) }; - let mut directory = resolved_prefix; + let mut directory = resolved_root.to_path_buf(); verify(&fd, &directory)?; - for component in relative_parent.components() { + for component in relative.components() { let Component::Normal(name) = component else { bail!("brew-cask: invalid generic artifact parent"); }; @@ -2111,6 +2218,49 @@ fn open_trusted_operation_parent( Ok(TrustedOperationParent { fd }) } +/// Create the appdir (if missing) using symlink-safe, trust-verified directory +/// operations and return the bound descriptor. +/// +/// Every component is opened with `openat`/`O_NOFOLLOW` and trust-checked, and +/// missing components are created with `mkdirat`, so a symlink interposed on any +/// component — including a not-yet-existing tail — is rejected rather than +/// followed. +/// +/// The descriptor is returned rather than dropped so the caller can address the +/// app through it. Releasing it here would reintroduce the race: a replacement +/// of an accepted (user-owned) component between validation and mutation would +/// otherwise redirect the subsequent copy, rename, removal, and ownership +/// recovery, all of which resolve pathnames again. +#[cfg(unix)] +fn ensure_trusted_appdir(appdir: &Path) -> Result { + // Anchor the walk at `/` and verify every component from there. `/` is the + // only directory that cannot be renamed or replaced, so it is the one safe + // pathname to open; each component below it is opened with `openat` and + // `O_NOFOLLOW` and trust-checked before descending. + // + // Scanning for the deepest existing ancestor and re-opening *that* by + // pathname would reintroduce a race: between the scan and the open, a + // same-uid process could replace the scanned directory with another + // same-uid-owned real directory, which would pass the ownership checks and + // become the retained descriptor. Starting from an unreplaceable root and + // descending only through verified descriptors removes that window. + // The path is walked exactly as given — it is NOT canonicalized here. + // Canonicalizing would follow a symlink planted on the appdir or its tail, + // which is precisely the substitution this walk must reject. Configured + // overrides are already resolved by `target_app_dir`, so production paths + // are symlink-free and the walk succeeds; a planted symlink hits + // `O_NOFOLLOW` and fails. + let relative = appdir.strip_prefix(Path::new("/")).map_err(|_| { + eyre!( + "brew-cask: app directory '{}' must be an absolute path", + appdir.display() + ) + })?; + // `allow_current_user` is true because a per-user appdir such as + // `~/Applications` is legitimately owned by the invoking user. + open_trusted_directory(Path::new("/"), relative, true, true) +} + fn run_installer_artifact( stage: &Path, installer: &InstallerArtifact, @@ -2325,6 +2475,104 @@ fn ditto(from: &Path, to: &Path) -> Result<()> { Ok(()) } +/// Run a helper with its working directory bound to `dir` via `fchdir`, so +/// relative arguments resolve from that exact directory inode. +/// +/// Passing a pathname to a subprocess would let it re-resolve every component, +/// which a same-uid replacement can redirect. `fchdir` in the child pins +/// resolution to the descriptor mise already verified, so relative names cannot +/// escape the validated application directory. +#[cfg(unix)] +fn run_in_trusted_dir( + program: &str, + args: &[&std::ffi::OsStr], + dir: Fd, +) -> Result { + use std::os::fd::AsRawFd; + use std::os::unix::process::CommandExt; + + let raw = dir.as_fd().as_raw_fd(); + let mut cmd = std::process::Command::new(program); + cmd.args(args); + // SAFETY: `fchdir` is async-signal-safe and only alters the child's working + // directory. `raw` stays open in the parent for the duration of the spawn, + // and CLOEXEC (if set) only takes effect at exec, after pre_exec runs. + unsafe { + cmd.pre_exec(move || { + if nix::libc::fchdir(raw) == -1 { + return Err(std::io::Error::last_os_error()); + } + Ok(()) + }); + } + cmd.status() + .wrap_err_with(|| format!("failed to run {program}")) +} + +/// Copy the bundle `from` into a freshly created directory `name` inside `dir`, +/// without ever letting the copy resolve `name` as a pathname. +/// +/// `name` is created with `mkdirat` and then opened with `openat`/`O_NOFOLLOW`, +/// so the destination is guaranteed to be a real directory this call created. +/// `ditto` is then pointed at that descriptor's working directory, so it writes +/// into the bound inode rather than resolving a relative name. Without this, a +/// same-uid process could create the predictable temporary name as a symlink +/// after the preceding removal and `ditto` would follow it, copying the +/// application outside the verified application directory. +/// +/// A racing creation of `name` surfaces as `EEXIST` and fails closed rather than +/// being followed. +#[cfg(unix)] +fn ditto_into(from: &Path, dir: Fd, name: &std::ffi::OsStr) -> Result<()> { + nix::sys::stat::mkdirat(&dir, name, nix::sys::stat::Mode::S_IRWXU).wrap_err_with(|| { + format!( + "brew-cask: cannot create staging directory {}", + Path::new(name).display() + ) + })?; + let destination = nix::fcntl::openat( + &dir, + name, + nix::fcntl::OFlag::O_RDONLY + | nix::fcntl::OFlag::O_DIRECTORY + | nix::fcntl::OFlag::O_NOFOLLOW, + nix::sys::stat::Mode::empty(), + ) + .wrap_err_with(|| { + format!( + "brew-cask: cannot open staging directory {}", + Path::new(name).display() + ) + })?; + let stat = nix::sys::stat::fstat(&destination)?; + if stat.st_uid != nix::unistd::geteuid().as_raw() { + bail!("brew-cask: staging directory is not owned by the current user"); + } + // `ditto src dst` copies the *contents* of src into dst, so pointing it at + // the bound directory reproduces the bundle in place. + let status = run_in_trusted_dir( + "ditto", + &[from.as_os_str(), std::ffi::OsStr::new(".")], + &destination, + )?; + if !status.success() { + bail!( + "ditto failed copying {} to {}", + from.display(), + Path::new(name).display() + ); + } + // Restore the bundle's own permissions, which the private staging mode hid. + if let Ok(metadata) = from.symlink_metadata() { + use std::os::unix::fs::PermissionsExt; + let mode = nix::sys::stat::Mode::from_bits_truncate( + metadata.permissions().mode() as nix::libc::mode_t + ); + nix::sys::stat::fchmod(&destination, mode)?; + } + Ok(()) +} + fn install_pkg(stage: &Path, pkg: &PkgArtifact) -> Result<()> { let source = find_file_artifact(stage, &pkg.source) .ok_or_else(|| eyre!("brew-cask: pkg artifact '{}' was not found", pkg.source))?; @@ -4411,7 +4659,7 @@ fn cask_appdir(apps: &[AppArtifact]) -> Result { return Ok(prefix_app_dir); } } - Ok(PathBuf::from("/Applications")) + target_app_dir() } fn link_binary(caskroom: &Path, appdir: &Path, binary: &BinaryArtifact) -> Result<()> { @@ -4464,7 +4712,7 @@ fn caskroom_binary_path( ) -> Result { let target = binary.target_path(appdir)?; let roots = if is_appdir_binary_target(&binary.target_name()?) { - let mut roots = allowed_appdir_roots(); + let mut roots = allowed_appdir_roots()?; roots.extend(allowed_binary_target_roots()); roots } else { @@ -5681,6 +5929,7 @@ fn path_ends_with_ignore_ascii_case(path: &Path, suffix: &Path) -> bool { } fn app_target_path(target_name: &str) -> Result { + let app_dir = target_app_dir()?; if target_name.contains('\0') { bail!("brew-cask: app target contains NUL"); } @@ -5695,14 +5944,94 @@ fn app_target_path(target_name: &str) -> Result { } if path.is_absolute() { let prefix_app_dir = prefix::prefix().join("Applications"); - if path.starts_with("/Applications") || path.starts_with(&prefix_app_dir) { + if path.starts_with(&app_dir) || path.starts_with(&prefix_app_dir) { return Ok(path); } - bail!("brew-cask: app target '{target_name}' must be under /Applications"); + // Casks routinely hardcode an absolute `/Applications/Foo.app` + // target. When an override appdir is configured, relocate such a + // target into it (preserving any subdirectories) rather than + // rejecting it. `$HOMEBREW_PREFIX`-anchored targets are handled by + // the check above and are never relocated. + if app_dir != Path::new(DEFAULT_APP_DIR) + && let Ok(rest) = path.strip_prefix(DEFAULT_APP_DIR) + { + return Ok(app_dir.join(rest)); + } + bail!( + "brew-cask: app target '{target_name}' must be under {}", + app_dir.display() + ); } bail!("brew-cask: app target '{target_name}' must be an absolute path"); } - Ok(PathBuf::from("/Applications").join(target_name)) + Ok(app_dir.join(target_name)) +} + +/// The directory `app` artifacts are linked into: `/Applications` unless +/// [`APP_DIR_ENV`] overrides it. +/// +/// The override is validated here rather than at the point of use because +/// `app_target_path` treats the result as a containment boundary for symlinks +/// that may be created with elevated privileges. An empty value falls back to +/// the default so that exporting `MISE_BREW_CASK_OPT_APPDIR=` cannot disable +/// that boundary: `Path::starts_with("")` is true for every path. +fn target_app_dir() -> Result { + let Ok(dir) = crate::env::var(APP_DIR_ENV) else { + return Ok(PathBuf::from(DEFAULT_APP_DIR)); + }; + if dir.is_empty() { + return Ok(PathBuf::from(DEFAULT_APP_DIR)); + } + let dir = PathBuf::from(dir); + if !dir.is_absolute() { + bail!( + "brew-cask: {APP_DIR_ENV} '{}' must be an absolute path", + dir.display() + ); + } + if dir + .components() + .any(|component| matches!(component, Component::ParentDir)) + { + bail!( + "brew-cask: {APP_DIR_ENV} '{}' must not contain '..'", + dir.display() + ); + } + // Resolve the override to a real absolute path: canonicalize its longest + // existing prefix and re-append the components that do not exist yet. This + // makes the appdir a symlink-free containment boundary — privileged cask + // mutations then operate on resolved paths and cannot be redirected through + // a symlinked component — and it collapses every spelling of the filesystem + // root (`/`, `//`, `/.`, a symlink to `/`, ...) to `/` so they can all be + // rejected together. + let resolved = resolve_appdir(&dir); + if !resolved + .components() + .any(|component| matches!(component, Component::Normal(_))) + { + bail!( + "brew-cask: {APP_DIR_ENV} '{}' must not resolve to the filesystem root", + dir.display() + ); + } + Ok(resolved) +} + +/// Resolve `dir` by canonicalizing its longest existing ancestor and +/// re-appending the not-yet-existing tail. Symlinks in the existing portion are +/// followed, so the result is a real path the caller can safely use as a +/// containment boundary. Falls back to `dir` unchanged if nothing along the +/// path can be canonicalized (not expected for an absolute path, where `/` +/// always resolves). +fn resolve_appdir(dir: &Path) -> PathBuf { + for ancestor in dir.ancestors() { + if let Ok(real) = ancestor.canonicalize() { + let tail = dir.strip_prefix(ancestor).unwrap_or(Path::new("")); + return real.join(tail); + } + } + dir.to_path_buf() } fn app_bundle_name(target_name: &str) -> Result<&str> { @@ -5729,11 +6058,14 @@ fn allowed_binary_target_roots() -> Vec { roots } -fn allowed_appdir_roots() -> Vec { - vec![ - PathBuf::from("/Applications"), - prefix::prefix().join("Applications"), - ] +fn allowed_appdir_roots() -> Result> { + let mut roots = vec![PathBuf::from(DEFAULT_APP_DIR)]; + for root in [target_app_dir()?, prefix::prefix().join("Applications")] { + if !roots.contains(&root) { + roots.push(root); + } + } + Ok(roots) } fn is_appdir_binary_target(target_name: &str) -> bool { @@ -5761,7 +6093,7 @@ fn binary_target_path(target_name: &str, appdir: &Path) -> Result { { bail!("brew-cask: binary $APPDIR target '{target_name}' must stay below Applications"); } - if !allowed_appdir_roots().iter().any(|root| root == appdir) { + if !allowed_appdir_roots()?.iter().any(|root| root == appdir) { bail!("brew-cask: invalid appdir '{}'", appdir.display()); } return Ok(appdir.join(relative)); @@ -6662,7 +6994,7 @@ fn validate_cask_prune_candidate(candidate: &CaskPruneCandidate) -> Result<()> { .get(path) .ok_or_else(|| eyre!("missing app target record"))?; if record.fingerprint.kind != CaskTargetKind::Directory - || !allowed_appdir_roots() + || !allowed_appdir_roots()? .iter() .any(|root| path_is_below(path, root)) || !path.file_name().is_some_and(|name| { @@ -6976,6 +7308,8 @@ mod tests { use std::os::unix::fs::PermissionsExt; use std::sync::Mutex; + use crate::test::EnvVarGuard; + static ENV_LOCK: Mutex<()> = Mutex::new(()); struct BrewPrefixGuard { @@ -6999,6 +7333,22 @@ mod tests { } } + /// A temporary directory whose ancestors pass `ensure_trusted_appdir`'s + /// trust checks. + /// + /// The system temp directory is unusable for these tests on Linux because + /// `/tmp` is world-writable (mode 1777) and the trusted walk correctly + /// refuses to operate through it. Real application directories + /// (`/Applications`, `~/Applications`) are never world-writable, so anchor + /// the fixture under the test home instead. + fn trusted_tempdir() -> Result { + let base = &*crate::env::HOME; + file::create_dir_all(base)?; + Ok(tempfile::Builder::new() + .prefix(".mise-cask-appdir-") + .tempdir_in(base)?) + } + fn run_cask_shim( ruby: &Path, shim: &Path, @@ -12102,11 +12452,526 @@ end Ok(()) } + #[test] + fn app_target_path_defaults_to_applications() -> Result<()> { + let _lock = ENV_LOCK.lock().unwrap(); + let mut _guard = EnvVarGuard::new(); + _guard.remove(APP_DIR_ENV); + assert_eq!( + app_target_path("Firefox.app")?, + PathBuf::from("/Applications/Firefox.app") + ); + Ok(()) + } + + #[test] + fn parse_app_artifact_target_without_slash_is_preserved() { + // The Homebrew API commonly renders `app` targets as a bare bundle + // name. Parsing must keep it verbatim and must not consult the override. + let _lock = ENV_LOCK.lock().unwrap(); + let mut _guard = EnvVarGuard::new(); + _guard.set(APP_DIR_ENV, "/tmp/should-not-be-used"); + let value: Value = + serde_json::json!({"app": ["Firefox.app", {"target": "Firefox Nightly.app"}]}); + assert_eq!( + parse_app_artifact(&value), + Some(AppArtifact { + source: "Firefox.app".to_string(), + target: Some("Firefox Nightly.app".to_string()), + }) + ); + } + + #[test] + fn parse_app_artifact_preserves_prefix_target() { + // A `$HOMEBREW_PREFIX`-anchored target must survive parsing so that + // `cask_appdir`/`app_target_path` can route it into the prefix. + let _lock = ENV_LOCK.lock().unwrap(); + let mut _guard = EnvVarGuard::new(); + _guard.set(APP_DIR_ENV, "/tmp/should-not-be-used"); + let value: Value = serde_json::json!({ + "app": ["Example.app", {"target": "$HOMEBREW_PREFIX/Applications/Example.app"}] + }); + assert_eq!( + parse_app_artifact(&value), + Some(AppArtifact { + source: "Example.app".to_string(), + target: Some("$HOMEBREW_PREFIX/Applications/Example.app".to_string()), + }) + ); + } + + #[test] + fn app_target_path_honours_appdir_override() -> Result<()> { + let _lock = ENV_LOCK.lock().unwrap(); + let tmp = tempfile::tempdir()?; + // target_app_dir canonicalizes the override, so compare against the + // resolved base (macOS tempdirs live under the `/var` symlink). + let base = tmp.path().canonicalize()?; + let mut _guard = EnvVarGuard::new(); + _guard.set(APP_DIR_ENV, &base); + assert_eq!(app_target_path("Firefox.app")?, base.join("Firefox.app")); + Ok(()) + } + + #[test] + fn app_target_path_accepts_absolute_target_under_override() -> Result<()> { + let _lock = ENV_LOCK.lock().unwrap(); + let tmp = tempfile::tempdir()?; + let base = tmp.path().canonicalize()?; + let mut _guard = EnvVarGuard::new(); + _guard.set(APP_DIR_ENV, &base); + let target = base.join("Firefox.app"); + assert_eq!(app_target_path(&target.to_string_lossy())?, target); + Ok(()) + } + + #[test] + fn app_target_path_relocates_default_applications_target() -> Result<()> { + // The Homebrew API frequently hardcodes an absolute + // `/Applications/Foo.app` target (e.g. the firefox cask). With an + // override configured this must be relocated into it. + let _lock = ENV_LOCK.lock().unwrap(); + let tmp = tempfile::tempdir()?; + let base = tmp.path().canonicalize()?; + let mut _guard = EnvVarGuard::new(); + _guard.set(APP_DIR_ENV, &base); + assert_eq!( + app_target_path("/Applications/Firefox.app")?, + base.join("Firefox.app") + ); + Ok(()) + } + + #[test] + fn app_target_path_relocation_preserves_subdirectories() -> Result<()> { + let _lock = ENV_LOCK.lock().unwrap(); + let tmp = tempfile::tempdir()?; + let base = tmp.path().canonicalize()?; + let mut _guard = EnvVarGuard::new(); + _guard.set(APP_DIR_ENV, &base); + assert_eq!( + app_target_path("/Applications/JetBrains/IDEA.app")?, + base.join("JetBrains/IDEA.app") + ); + Ok(()) + } + + #[test] + fn app_target_path_defaults_keep_absolute_applications_target() -> Result<()> { + // Without an override, an absolute `/Applications` target is accepted + // as-is (no relocation). + let _lock = ENV_LOCK.lock().unwrap(); + let mut _guard = EnvVarGuard::new(); + _guard.remove(APP_DIR_ENV); + assert_eq!( + app_target_path("/Applications/Firefox.app")?, + PathBuf::from("/Applications/Firefox.app") + ); + Ok(()) + } + + #[test] + fn app_target_path_rejects_target_outside_override() -> Result<()> { + let _lock = ENV_LOCK.lock().unwrap(); + let tmp = tempfile::tempdir()?; + let base = tmp.path().canonicalize()?; + let mut _guard = EnvVarGuard::new(); + _guard.set(APP_DIR_ENV, &base); + let err = app_target_path("/Users/someone/Evil.app") + .unwrap_err() + .to_string(); + assert!(err.contains(&base.to_string_lossy().to_string()), "{err}"); + assert!(!err.contains("/Applications"), "{err}"); + Ok(()) + } + + #[test] + fn cask_appdir_uses_override() -> Result<()> { + let _lock = ENV_LOCK.lock().unwrap(); + let tmp = tempfile::tempdir()?; + let base = tmp.path().canonicalize()?; + let _prefix = BrewPrefixGuard::set(&base.join("prefix")); + let appdir = base.join("appdir"); + let mut _guard = EnvVarGuard::new(); + _guard.set(APP_DIR_ENV, &appdir); + let app = AppArtifact { + source: "Example.app".to_string(), + target: None, + }; + assert_eq!(cask_appdir(&[app])?, appdir); + Ok(()) + } + + #[test] + fn command_wrapper_target_path_uses_override() -> Result<()> { + let _lock = ENV_LOCK.lock().unwrap(); + let tmp = tempfile::tempdir()?; + let base = tmp.path().canonicalize()?; + let appdir = base.join("appdir"); + let mut _guard = EnvVarGuard::new(); + _guard.set(APP_DIR_ENV, &appdir); + // Only `$APPDIR`-anchored targets resolve into the appdir; a bare name + // lands under the prefix's bin, so anchor the target to exercise the + // override path. + let wrapper = CommandWrapperArtifact { + name: "gimp".to_string(), + target: Some("$APPDIR/GIMP.app/Contents/MacOS/gimp".to_string()), + content: None, + executable: None, + args: Vec::new(), + env: BTreeMap::new(), + }; + assert_eq!( + wrapper.target_path()?, + appdir.join("GIMP.app/Contents/MacOS/gimp"), + ); + Ok(()) + } + + #[test] + fn allowed_appdir_roots_has_no_duplicates() -> Result<()> { + let _lock = ENV_LOCK.lock().unwrap(); + let tmp = tempfile::tempdir()?; + let base = tmp.path().canonicalize()?; + let _prefix = BrewPrefixGuard::set(&base.join("prefix")); + + let mut _guard = EnvVarGuard::new(); + _guard.remove(APP_DIR_ENV); + let roots = allowed_appdir_roots()?; + let unique: BTreeSet<_> = roots.iter().collect(); + assert_eq!(unique.len(), roots.len(), "{roots:?}"); + assert!(roots.contains(&PathBuf::from("/Applications"))); + + let appdir = base.join("appdir"); + _guard.set(APP_DIR_ENV, &appdir); + let roots = allowed_appdir_roots()?; + let unique: BTreeSet<_> = roots.iter().collect(); + assert_eq!(unique.len(), roots.len(), "{roots:?}"); + assert!(roots.contains(&appdir)); + Ok(()) + } + + #[test] + fn binary_target_path_accepts_override_appdir() -> Result<()> { + let _lock = ENV_LOCK.lock().unwrap(); + let tmp = tempfile::tempdir()?; + let base = tmp.path().canonicalize()?; + let appdir = base.join("appdir"); + let mut _guard = EnvVarGuard::new(); + _guard.set(APP_DIR_ENV, &appdir); + assert_eq!( + binary_target_path("$APPDIR/Foo.app/Contents/MacOS/foo", &appdir)?, + appdir.join("Foo.app/Contents/MacOS/foo"), + ); + Ok(()) + } + + #[test] + fn ensure_trusted_appdir_refuses_world_writable_ancestor() -> Result<()> { + // Regression guard for the CI failure: a world-writable ancestor (as + // `/tmp` is, mode 1777) must be refused, because any local user could + // substitute components beneath it. Real application directories are + // never world-writable. + let tmp = trusted_tempdir()?; + let base = tmp.path().canonicalize()?; + let shared = base.join("shared"); + file::create_dir_all(&shared)?; + let mode = std::fs::Permissions::from_mode(0o1777); + std::fs::set_permissions(&shared, mode)?; + let err = match ensure_trusted_appdir(&shared.join("Applications")) { + Ok(_) => panic!("expected world-writable ancestor to be refused"), + Err(err) => err.to_string(), + }; + assert!(err.contains("untrusted directory"), "{err}"); + Ok(()) + } + + #[test] + fn ensure_trusted_appdir_creates_missing_tail() -> Result<()> { + let tmp = trusted_tempdir()?; + let base = tmp.path().canonicalize()?; + let appdir = base.join("Applications"); + ensure_trusted_appdir(&appdir)?; + assert!(appdir.symlink_metadata()?.file_type().is_dir()); + // Idempotent when the directory already exists. + ensure_trusted_appdir(&appdir)?; + Ok(()) + } + + #[test] + fn ensure_trusted_appdir_rejects_symlinked_tail() -> Result<()> { + // Simulate a symlink planted on the not-yet-existing appdir tail + // between validation and mutation: it must be rejected, not followed. + let tmp = trusted_tempdir()?; + let base = tmp.path().canonicalize()?; + let elsewhere = base.join("elsewhere"); + file::create_dir_all(&elsewhere)?; + let appdir = base.join("Applications"); + std::os::unix::fs::symlink(&elsewhere, &appdir)?; + let err = match ensure_trusted_appdir(&appdir) { + Ok(_) => panic!("expected symlinked appdir tail to be rejected"), + Err(err) => err.to_string(), + }; + // Must fail because the tail is a symlink, not because an ancestor was + // untrusted (which is a different guard). + assert!(err.contains("cannot open operation directory"), "{err}"); + assert!(!err.contains("untrusted directory"), "{err}"); + Ok(()) + } + + #[test] + fn ensure_trusted_appdir_stays_bound_after_same_uid_replacement() -> Result<()> { + // The reviewer's scenario: after validation, a same-uid process swaps + // the accepted appdir for a different directory (or symlink). Because + // the descriptor is retained and mutations are addressed through it, + // writes still land in the originally validated directory. + let tmp = trusted_tempdir()?; + let base = tmp.path().canonicalize()?; + let appdir = base.join("Applications"); + let parent = ensure_trusted_appdir(&appdir)?; + + // Swap the validated directory aside and put an attacker-controlled + // path in its place. + let stashed = base.join("stashed"); + std::fs::rename(&appdir, &stashed)?; + let attacker = base.join("attacker"); + file::create_dir_all(&attacker)?; + std::os::unix::fs::symlink(&attacker, &appdir)?; + + // Writing through the bound descriptor path must reach the original + // directory (now at `stashed`), never the attacker's directory. + let bound = parent.path()?; + crate::file::write(bound.join("canary"), "bound")?; + assert!(stashed.join("canary").is_file()); + assert!(!attacker.join("canary").exists()); + Ok(()) + } + + // `ditto` only exists on macOS, and app artifacts are macOS-only in + // practice (Linux cask support is font-only). #[cfg(target_os = "macos")] #[test] - fn upgrades_app_with_protected_existing_contents() -> Result<()> { + fn ditto_into_stays_bound_after_directory_replacement() -> Result<()> { + // Bind the appdir, then have a same-uid replacement swap the directory + // pathname for an attacker-controlled one. The fd-bound copy must still + // land in the originally validated directory. + let tmp = trusted_tempdir()?; + let base = tmp.path().canonicalize()?; + let appdir = base.join("Applications"); + let parent = ensure_trusted_appdir(&appdir)?; + + let source = base.join("payload"); + file::create_dir_all(&source)?; + crate::file::write(source.join("marker"), "payload")?; + + let stashed = base.join("stashed"); + std::fs::rename(&appdir, &stashed)?; + let attacker = base.join("attacker"); + file::create_dir_all(&attacker)?; + std::os::unix::fs::symlink(&attacker, &appdir)?; + + ditto_into(&source, &parent.fd, std::ffi::OsStr::new("Copied.app"))?; + assert!(stashed.join("Copied.app/marker").is_file()); + assert!(!attacker.join("Copied.app").exists()); + Ok(()) + } + + #[test] + fn ensure_trusted_appdir_walks_from_unreplaceable_root() -> Result<()> { + // The appdir is never re-opened via a scanned ancestor pathname: the + // walk starts at `/` and descends only through verified descriptors. + // Swapping an intermediate component for another same-uid-owned real + // directory before the call therefore cannot be reached through a + // previously-resolved root, and a symlink swap is rejected outright. + let tmp = trusted_tempdir()?; + let base = tmp.path().canonicalize()?; + let middle = base.join("middle"); + file::create_dir_all(&middle)?; + let appdir = middle.join("Applications"); + let parent = ensure_trusted_appdir(&appdir)?; + assert!(appdir.symlink_metadata()?.file_type().is_dir()); + + // Replace the intermediate component with a same-uid symlink: the next + // walk must refuse it rather than following it. + let attacker = base.join("attacker"); + file::create_dir_all(&attacker)?; + std::fs::remove_dir_all(&middle)?; + std::os::unix::fs::symlink(&attacker, &middle)?; + assert!(ensure_trusted_appdir(&appdir).is_err()); + // Nothing was created inside the attacker's directory. + assert!(!attacker.join("Applications").exists()); + drop(parent); + Ok(()) + } + + #[test] + fn ditto_into_rejects_preplanted_symlink_destination() -> Result<()> { + // A same-uid process creates the predictable temporary name as a + // symlink before the copy. The copy must fail closed rather than follow + // it, so nothing is written outside the verified directory. + let tmp = trusted_tempdir()?; + let base = tmp.path().canonicalize()?; + let appdir = base.join("Applications"); + let parent = ensure_trusted_appdir(&appdir)?; + + let source = base.join("payload"); + file::create_dir_all(&source)?; + crate::file::write(source.join("marker"), "payload")?; + + let attacker = base.join("attacker"); + file::create_dir_all(&attacker)?; + let tmp_name = std::ffi::OsStr::new("Foo.mise-tmp-abc"); + std::os::unix::fs::symlink(&attacker, appdir.join(tmp_name))?; + + // Fails at `mkdirat` (EEXIST) before `ditto` is ever spawned, so this + // holds on platforms without `ditto` too. + let err = match ditto_into(&source, &parent.fd, tmp_name) { + Ok(()) => panic!("expected pre-planted symlink destination to be refused"), + Err(err) => err.to_string(), + }; + assert!(err.contains("cannot create staging directory"), "{err}"); + assert!(!attacker.join("marker").exists()); + Ok(()) + } + + #[cfg(target_os = "macos")] + #[test] + fn repair_app_permissions_does_not_traverse_bundle_symlinks() -> Result<()> { + // A cask bundle may contain a symlink pointing outside the application + // directory. The recursive flag/permission repair must not follow it and + // change the referent. + let tmp = trusted_tempdir()?; + let base = tmp.path().canonicalize()?; + let appdir = base.join("Applications"); + let parent = ensure_trusted_appdir(&appdir)?; + + let outside = base.join("outside.txt"); + crate::file::write(&outside, "keep")?; + let status = std::process::Command::new("/bin/chmod") + .args(["644"]) + .arg(&outside) + .status()?; + assert!(status.success()); + + let bundle = appdir.join("Victim.app"); + file::create_dir_all(&bundle)?; + std::os::unix::fs::symlink(&outside, bundle.join("link"))?; + + repair_app_permissions_at(&parent, std::ffi::OsStr::new("Victim.app")); + + // The referent keeps its mode and gains no flags. + let mode = std::process::Command::new("/usr/bin/stat") + .args(["-f", "%Sp"]) + .arg(&outside) + .output()?; + let mode = String::from_utf8_lossy(&mode.stdout).trim().to_string(); + assert_eq!(mode, "-rw-r--r--", "referent mode changed: {mode}"); + let flags = std::process::Command::new("/usr/bin/stat") + .args(["-f", "%Sf"]) + .arg(&outside) + .output()?; + let flags = String::from_utf8_lossy(&flags.stdout).trim().to_string(); + assert!( + flags.is_empty() || flags == "-", + "referent flags set: {flags}" + ); + assert_eq!(crate::file::read_to_string(&outside)?, "keep"); + Ok(()) + } + + #[test] + fn empty_appdir_override_falls_back_to_applications() -> Result<()> { + let _lock = ENV_LOCK.lock().unwrap(); + let mut _guard = EnvVarGuard::new(); + _guard.set(APP_DIR_ENV, ""); + assert_eq!( + app_target_path("Firefox.app")?, + PathBuf::from("/Applications/Firefox.app") + ); + assert!(app_target_path("/etc/passwd").is_err()); + Ok(()) + } + + #[test] + fn relative_appdir_override_is_rejected() -> Result<()> { + let _lock = ENV_LOCK.lock().unwrap(); + let mut _guard = EnvVarGuard::new(); + _guard.set(APP_DIR_ENV, "relative/apps"); + assert!(app_target_path("Firefox.app").is_err()); + Ok(()) + } + + #[test] + fn appdir_override_with_parent_dir_is_rejected() -> Result<()> { + let _lock = ENV_LOCK.lock().unwrap(); + let mut _guard = EnvVarGuard::new(); + _guard.set(APP_DIR_ENV, "/Applications/../etc"); + assert!(app_target_path("Firefox.app").is_err()); + Ok(()) + } + + #[test] + fn appdir_override_root_alias_is_rejected() -> Result<()> { + // Alternate spellings of the filesystem root must not become the + // containment boundary. + let _lock = ENV_LOCK.lock().unwrap(); + for alias in ["/.", "//", "/./."] { + let mut _guard = EnvVarGuard::new(); + _guard.set(APP_DIR_ENV, alias); + assert!( + app_target_path("Firefox.app").is_err(), + "expected {alias} to be rejected" + ); + } + Ok(()) + } + + #[test] + fn appdir_override_with_symlink_to_root_is_rejected() -> Result<()> { + let _lock = ENV_LOCK.lock().unwrap(); let tmp = tempfile::tempdir()?; - let target = tmp.path().join("Docker.app"); + // An override that resolves to the filesystem root would make `/` the + // containment boundary for privileged mutations, so it must be + // rejected — including when reached through a symlink. + let link = tmp.path().join("link-to-root"); + std::os::unix::fs::symlink("/", &link)?; + let mut _guard = EnvVarGuard::new(); + _guard.set(APP_DIR_ENV, &link); + let err = app_target_path("Firefox.app").unwrap_err().to_string(); + assert!( + err.contains("must not resolve to the filesystem root"), + "{err}" + ); + Ok(()) + } + + #[test] + fn appdir_override_with_benign_symlink_is_resolved() -> Result<()> { + let _lock = ENV_LOCK.lock().unwrap(); + let tmp = tempfile::tempdir()?; + // A symlink whose target is an ordinary directory (not root) is + // accepted, but the boundary is the resolved real path so privileged + // mutations cannot be redirected through the link. + let real = tmp.path().join("real"); + file::create_dir_all(&real)?; + let real = real.canonicalize()?; + let link = tmp.path().join("link"); + std::os::unix::fs::symlink(&real, &link)?; + let mut _guard = EnvVarGuard::new(); + _guard.set(APP_DIR_ENV, &link); + assert_eq!(app_target_path("Firefox.app")?, real.join("Firefox.app")); + Ok(()) + } + + #[cfg(target_os = "macos")] + #[test] + fn upgrades_app_with_protected_existing_contents() -> Result<()> { + let tmp = trusted_tempdir()?; + // ensure_trusted_appdir walks the appdir with O_NOFOLLOW, so use a + // canonical base (macOS tempdirs sit under the `/var` symlink). + let base = tmp.path().canonicalize()?; + let target = base.join("Docker.app"); let protected_dir = target.join("Contents/Resources"); file::create_dir_all(&protected_dir)?; crate::file::write(protected_dir.join("docker"), "old")?; @@ -12116,17 +12981,23 @@ end .status()?; assert!(status.success()); - let tmp_target = tmp.path().join("Docker.mise-tmp-test"); + let tmp_target = base.join("Docker.mise-tmp-test"); file::create_dir_all(&tmp_target)?; crate::file::write(tmp_target.join("version"), "new")?; - let result = swap_app(&target, &tmp_target); + // swap_app_at addresses entries by name relative to the verified appdir + // descriptor, so open the containing directory and use bare names. + let parent = ensure_trusted_appdir(&base)?; + let old_name = std::ffi::OsString::from("Docker.mise-old-test"); + let old_target = base.join(&old_name); + let result = swap_app_at( + &parent, + std::ffi::OsStr::new("Docker.app"), + std::ffi::OsStr::new("Docker.mise-tmp-test"), + &old_name, + ); // Remove the ACL so tempfile can clean up even when the repro fails. - let old_target = target.with_extension(format!( - "mise-old-{}", - crate::hash::hash_to_str(&target.display().to_string()) - )); if old_target.exists() { let status = std::process::Command::new("/bin/chmod") .arg("-RN") diff --git a/src/system/sudo.rs b/src/system/sudo.rs index 68e2ba810f0..0ff9f82131d 100644 --- a/src/system/sudo.rs +++ b/src/system/sudo.rs @@ -105,6 +105,60 @@ pub(crate) fn run(program: &str, args: &[String], envs: &[(String, String)]) -> cmd.raw(true).execute() } +/// Run `program args...` elevated, with the child's working directory bound to +/// `dir` via `fchdir` so relative arguments resolve from that exact directory +/// inode. +/// +/// Elevation policy is identical to [`run`]. The difference is that callers can +/// pass *relative* names which cannot be redirected: a pathname handed to an +/// elevated recursive operation would be re-resolved by the child, so a +/// same-uid replacement of a component between validation and execution could +/// point it somewhere else. Binding the child's cwd to a descriptor the caller +/// already verified removes that window. +/// +/// stdio is inherited so a sudo password prompt still reaches the terminal. +#[cfg(unix)] +pub(crate) fn run_in_dir( + program: &str, + args: &[String], + dir: Fd, +) -> Result<()> { + use std::os::fd::AsRawFd; + use std::os::unix::process::CommandExt; + + use eyre::WrapErr; + + let argv = argv_with_env(program, args, &[]); + let manual_cmd = std::iter::once("sudo".to_string()) + .chain(std::iter::once(program.to_string())) + .chain(args.iter().cloned()) + .collect::>() + .join(" "); + ensure_elevation_available(&manual_cmd)?; + info!("$ {}", argv.join(" ")); + let raw = dir.as_fd().as_raw_fd(); + let mut cmd = Command::new(&argv[0]); + cmd.args(&argv[1..]); + // SAFETY: `fchdir` is async-signal-safe and only alters the child's working + // directory. `raw` stays open in the parent across the spawn, and CLOEXEC + // (if set) only takes effect at exec, after pre_exec has run. + unsafe { + cmd.pre_exec(move || { + if nix::libc::fchdir(raw) == -1 { + return Err(std::io::Error::last_os_error()); + } + Ok(()) + }); + } + let status = cmd + .status() + .wrap_err_with(|| format!("failed to run {}", argv.join(" ")))?; + if !status.success() { + bail!("{} failed", argv.join(" ")); + } + Ok(()) +} + /// Run an elevated command and capture its output. /// /// Interactive callers authenticate with an inherited `sudo -v` first so a diff --git a/src/test.rs b/src/test.rs index 89c597a50f7..ec2c1a33ac1 100644 --- a/src/test.rs +++ b/src/test.rs @@ -128,6 +128,17 @@ impl EnvVarGuard { env::set_var(&key, value); self } + + /// Removes an environment variable for the duration of the guard, + /// restoring any previous value on drop. Useful for asserting default + /// behavior even when the variable happens to be set in the caller's + /// environment. + pub fn remove>(&mut self, key: K) -> &mut Self { + let key = key.as_ref().to_os_string(); + self.prev.push((key.clone(), env::var_os(&key))); + env::remove_var(&key); + self + } } #[cfg(unix)]