fix(install): skip and reject links when extracting .eti archives
Security audit findings EXP2-SEC-01, SEC-IPC-04 and Codex #3 (symlink redirection through externally extracted archives). The ordinary install path hands every root `.eti` archive to an external `unrar x` process and promotes the resulting staging directory to `local/` as soon as the extractor exits 0. Nothing inspected what unrar materialised. A symbolic link (or, on Windows, a junction) inside an archive would survive promotion and later redirect the launch-time settings rewrite in `apply_launch_settings_once`, the uninstall path, or any script the game ships. The archives themselves are BLAKE3 verified against the bundled catalog before they can be installed, so a hostile link would have to be published by the catalog operator; this is defense in depth rather than a live remote exploit. Two independent layers now guard promotion: - Both `unrar` invocations (Tauri sidecar and peer-cli external unpacker) pass `-ol-`, which makes unrar 7.x skip symbolic-link entries entirely. The bundled 7.10 sidecar was checked against a fixture archive. - `install_inner`/`update_inner` walk the staging tree without following links and refuse to promote it if any entry is a symlink or (on Windows) carries the reparse-point attribute. The normal rollback then removes staging and clears the install intent. The audit's `-sl-` suggestion does not exist in unrar; `-sl<size>` is a size filter. Post-extraction digest verification of every extracted file remains out of scope for the ordinary path; Stream Install already verifies each output entry. Test plan: `just test` (new unix test installs with a fake unpacker that plants a symlink and asserts install fails, `local/` is absent and the intent is cleared; the peer-cli controlled-unrar test checks the new argument position). Manual: install a fixture game via peer-cli. Claude-Session: https://claude.ai/code/session_017C3Nbgwpdm3YNwZhhFLHwg
This commit is contained in:
@@ -347,6 +347,9 @@ impl Unpacker for ExternalUnrarUnpacker {
|
||||
std::ffi::OsString::from("x"),
|
||||
std::ffi::OsString::from("-o+"),
|
||||
std::ffi::OsString::from("-p-"),
|
||||
// Skip symbolic links inside the archive; the install
|
||||
// transaction also audits the extracted tree for links.
|
||||
std::ffi::OsString::from("-ol-"),
|
||||
archive.as_os_str().to_owned(),
|
||||
dest.as_os_str().to_owned(),
|
||||
],
|
||||
@@ -649,7 +652,8 @@ mod tests {
|
||||
r#"#!/bin/sh
|
||||
set -eu
|
||||
[ "$3" = "-p-" ]
|
||||
dest=$5
|
||||
[ "$4" = "-ol-" ]
|
||||
dest=$6
|
||||
printf '%s' "$$" > "$dest/child.pid"
|
||||
printf 'started' > "$dest/started"
|
||||
while [ ! -e "$dest/release" ]; do :; done
|
||||
|
||||
@@ -462,6 +462,7 @@ async fn install_inner(
|
||||
prepare_owned_empty_dir(&staging)?;
|
||||
root_capability.sync_game_root()?;
|
||||
unpack_archives(game_root, &staging, unpacker, cancel_token).await?;
|
||||
reject_links_in_staging(&staging)?;
|
||||
rename_path(&staging, &local)
|
||||
.wrap_err_with(|| format!("failed to promote install for {id}"))?;
|
||||
root_capability.sync_game_root()?;
|
||||
@@ -490,6 +491,7 @@ async fn update_inner(
|
||||
prepare_owned_empty_dir(&staging)?;
|
||||
root_capability.sync_game_root()?;
|
||||
unpack_archives(game_root, &staging, unpacker, cancel_token).await?;
|
||||
reject_links_in_staging(&staging)?;
|
||||
rename_path(&staging, &local).wrap_err_with(|| format!("failed to promote update for {id}"))?;
|
||||
root_capability.sync_game_root()?;
|
||||
Ok(())
|
||||
@@ -543,6 +545,53 @@ async fn unpack_archives(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Refuses to promote an extracted tree that contains symbolic links or, on
|
||||
/// Windows, any reparse point (junctions, mount points).
|
||||
///
|
||||
/// Extraction is delegated to an external `unrar` process whose output is
|
||||
/// otherwise trusted verbatim. The archives themselves are BLAKE3-verified
|
||||
/// against the bundled catalog, so a link would have to originate from the
|
||||
/// catalog publisher; this audit is defense in depth so that a link can never
|
||||
/// redirect later launch-time rewrites (`apply_launch_settings_once`) or an
|
||||
/// uninstall outside the game directory. `unrar` is additionally invoked
|
||||
/// with `-ol-` so links are skipped at extraction time.
|
||||
fn reject_links_in_staging(staging: &Path) -> eyre::Result<()> {
|
||||
let staging = staging.to_path_buf();
|
||||
scoped_blocking(move || {
|
||||
for entry in walkdir::WalkDir::new(&staging).follow_links(false) {
|
||||
let entry = entry.wrap_err_with(|| {
|
||||
format!("failed to audit extracted tree {}", staging.display())
|
||||
})?;
|
||||
let metadata = fs::symlink_metadata(entry.path()).wrap_err_with(|| {
|
||||
format!(
|
||||
"failed to inspect extracted entry {}",
|
||||
entry.path().display()
|
||||
)
|
||||
})?;
|
||||
if is_link_or_reparse_point(&metadata) {
|
||||
eyre::bail!(
|
||||
"extracted archive contains a link at {}; refusing to install it",
|
||||
entry.path().display()
|
||||
);
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
})
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
fn is_link_or_reparse_point(metadata: &fs::Metadata) -> bool {
|
||||
use std::os::windows::fs::MetadataExt as _;
|
||||
const FILE_ATTRIBUTE_REPARSE_POINT: u32 = 0x400;
|
||||
metadata.file_type().is_symlink()
|
||||
|| metadata.file_attributes() & FILE_ATTRIBUTE_REPARSE_POINT != 0
|
||||
}
|
||||
|
||||
#[cfg(not(windows))]
|
||||
fn is_link_or_reparse_point(metadata: &fs::Metadata) -> bool {
|
||||
metadata.file_type().is_symlink()
|
||||
}
|
||||
|
||||
pub(crate) fn root_eti_archives(game_root: &Path) -> eyre::Result<Vec<PathBuf>> {
|
||||
scoped_blocking(|| {
|
||||
let mut archives = Vec::new();
|
||||
@@ -1149,6 +1198,7 @@ mod tests {
|
||||
struct FakeUnpacker {
|
||||
fail: bool,
|
||||
create_commit_conflict: bool,
|
||||
create_symlink: bool,
|
||||
archives: Mutex<Vec<PathBuf>>,
|
||||
}
|
||||
|
||||
@@ -1156,16 +1206,22 @@ mod tests {
|
||||
fn failing() -> Self {
|
||||
Self {
|
||||
fail: true,
|
||||
create_commit_conflict: false,
|
||||
archives: Mutex::new(Vec::new()),
|
||||
..Self::default()
|
||||
}
|
||||
}
|
||||
|
||||
fn commit_conflict() -> Self {
|
||||
Self {
|
||||
fail: false,
|
||||
create_commit_conflict: true,
|
||||
archives: Mutex::new(Vec::new()),
|
||||
..Self::default()
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
fn symlinking() -> Self {
|
||||
Self {
|
||||
create_symlink: true,
|
||||
..Self::default()
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1187,6 +1243,11 @@ mod tests {
|
||||
}
|
||||
scoped_blocking(|| {
|
||||
fs::write(dest.join("payload.txt"), b"installed")?;
|
||||
#[cfg(unix)]
|
||||
if self.create_symlink {
|
||||
fs::create_dir_all(dest.join("nested"))?;
|
||||
std::os::unix::fs::symlink("/", dest.join("nested").join("escape"))?;
|
||||
}
|
||||
if self.create_commit_conflict {
|
||||
let game_root = dest
|
||||
.parent()
|
||||
@@ -1695,6 +1756,31 @@ mod tests {
|
||||
assert_intent_missing(state.path(), &root, "game");
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn install_refuses_extracted_trees_that_contain_links() {
|
||||
let temp = TempDir::new("lanspread-install");
|
||||
let state = test_state();
|
||||
let root = temp.game_root();
|
||||
write_file(&root.join("game.eti"), b"archive");
|
||||
write_file(&root.join("version.ini"), b"20250101");
|
||||
|
||||
let err = install(
|
||||
&root,
|
||||
state.path(),
|
||||
"game",
|
||||
Arc::new(FakeUnpacker::symlinking()),
|
||||
CancellationToken::new(),
|
||||
)
|
||||
.await
|
||||
.expect_err("install should refuse a link in the extracted tree");
|
||||
|
||||
assert!(err.to_string().contains("contains a link"), "{err:#}");
|
||||
assert!(!root.join("local").exists());
|
||||
assert!(!root.join(".local.installing").exists());
|
||||
assert_intent_missing(state.path(), &root, "game");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn update_commit_conflict_preserves_every_ambiguous_directory() {
|
||||
let temp = TempDir::new("lanspread-install");
|
||||
|
||||
@@ -3201,6 +3201,10 @@ async fn run_unrar_sidecar(
|
||||
[
|
||||
std::ffi::OsString::from("x"),
|
||||
std::ffi::OsString::from("-p-"),
|
||||
// Skip symbolic links inside the archive entirely. The install
|
||||
// transaction additionally refuses to promote a tree containing
|
||||
// links, so a link can never redirect later file operations.
|
||||
std::ffi::OsString::from("-ol-"),
|
||||
paths.archive.as_os_str().to_owned(),
|
||||
std::ffi::OsString::from("-y"),
|
||||
std::ffi::OsString::from("-o"),
|
||||
|
||||
Reference in New Issue
Block a user