fix(peer): reject unsafe game IDs in state marker paths
Scanner finding #12 ("state marker path escape"). The per-game state helpers in `state_paths.rs` joined a raw game ID below `<state_dir>/games/`. The public `setup_done_path` was therefore usable with an absolute or parent-containing ID by an embedding caller, and the legacy migration discovered IDs from directory names in the user's games folder and joined them unconditionally. Every shipping caller today validates its ID or takes it from the catalog, so this was a footgun rather than an exploited hole, but the fix is small and removes the reliance on every future caller remembering the rule. Add `validate_game_state_id`, which rejects separators and NUL and then delegates to `lanspread_db::content_manifest::validate_portable_component` (the catalog's own rules: no `.`/`..`, no trailing dot or space, no control or Windows-reserved characters, no Windows device names). Reusing the catalog validator rather than a private copy guarantees that any ID the catalog can publish is accepted here and that the two cannot drift apart. `setup_done_path` now returns `eyre::Result<PathBuf>`; it is the only state path the embedding application calls with an ID that may originate from UI input. `launch_settings_applied_path` leaves the public API and becomes `pub(crate)`; the two public launch-settings entry points (`apply_launch_settings_once`, `mark_launch_settings_applied`) validate the ID before any filesystem work. `game_state_dir` carries a `debug_assert!` documenting the contract for internal callers without turning a bad ID into a release-build panic; the migration test suite exercises that assertion in debug builds. Behaviour changes: - Legacy migration logs a warning, counts a failure and leaves the legacy marker in place for a games-folder directory whose name is not a portable game ID, instead of creating state below it. A new test covers a trailing-dot directory name. - The Windows launcher ignores a run request whose ID `setup_done_path` rejects, with a warning, mirroring the existing invalid-ID early return. Tests cover catalog-valid IDs that must remain accepted (embedded dots, spaces, `console.txt`, `com10`, non-ASCII) and unsafe IDs that must be rejected (empty, `.`, `..`, separators, NUL, trailing dot or space, device names, `a:b`). This ports the fallible API from the parallel security branch (lanspread2 commits 4146a0e and 9f26c63) onto the validator this branch already exports from `lanspread-db`. Test plan: - `cargo test -p lanspread-peer --lib`: 491 passed. - `just clippy`: clean. - On Windows, launch a game with a valid ID and confirm the setup marker is still written under `<app-data>/games/<id>/setup_done`. Claude-Session: https://claude.ai/code/session_01QRkCv4a4GqkajyamxmbSuA
This commit is contained in:
@@ -233,7 +233,11 @@ Reserved per-game paths:
|
|||||||
- `.local.backup/` holds the previous install while an update or uninstall is in
|
- `.local.backup/` holds the previous install while an update or uninstall is in
|
||||||
flight.
|
flight.
|
||||||
- `games/<game_id>/install_intent.json` in the configured state directory is the
|
- `games/<game_id>/install_intent.json` in the configured state directory is the
|
||||||
atomic per-game intent log.
|
atomic per-game intent log. Every `games/<game_id>/` path is derived from a
|
||||||
|
game ID that satisfies the catalog's portable component rules; the public
|
||||||
|
`setup_done_path` and the launch-settings entry points validate the ID before
|
||||||
|
joining it, and legacy migration refuses a games-folder directory whose name
|
||||||
|
would not pass.
|
||||||
- `games/<game_id>/download_ownership/v1-<root_digest>/record.json` in that
|
- `games/<game_id>/download_ownership/v1-<root_digest>/record.json` in that
|
||||||
state directory records the last committed and any pending downloader-owned
|
state directory records the last committed and any pending downloader-owned
|
||||||
regular-file set and its exact catalog content ID. The namespace is derived
|
regular-file set and its exact catalog content ID. The namespace is derived
|
||||||
|
|||||||
@@ -3280,7 +3280,7 @@ mod tests {
|
|||||||
fn streamed_install_marks_settings_only_after_successful_promotion() {
|
fn streamed_install_marks_settings_only_after_successful_promotion() {
|
||||||
let games = TempDir::new("lanspread-handler-stream-promotion");
|
let games = TempDir::new("lanspread-handler-stream-promotion");
|
||||||
let state = TempDir::new("lanspread-handler-stream-promotion-state");
|
let state = TempDir::new("lanspread-handler-stream-promotion-state");
|
||||||
let marker = crate::launch_settings_applied_path(state.path(), "game");
|
let marker = crate::state_paths::launch_settings_applied_path(state.path(), "game");
|
||||||
let transaction = install::begin_streamed_install(&games.game_root(), state.path(), "game")
|
let transaction = install::begin_streamed_install(&games.game_root(), state.path(), "game")
|
||||||
.expect("streamed install transaction should begin");
|
.expect("streamed install transaction should begin");
|
||||||
write_file(&transaction.staging_dir().join("payload.txt"), b"installed");
|
write_file(&transaction.staging_dir().join("payload.txt"), b"installed");
|
||||||
|
|||||||
@@ -24,7 +24,7 @@ use eyre::WrapErr;
|
|||||||
use crate::{
|
use crate::{
|
||||||
game_paths::LOCAL_DIR,
|
game_paths::LOCAL_DIR,
|
||||||
scoped_blocking::scoped_blocking,
|
scoped_blocking::scoped_blocking,
|
||||||
state_paths::launch_settings_applied_path,
|
state_paths::{launch_settings_applied_path, validate_game_state_id},
|
||||||
};
|
};
|
||||||
|
|
||||||
const ACCOUNT_NAME_FILE: &str = "account_name.txt";
|
const ACCOUNT_NAME_FILE: &str = "account_name.txt";
|
||||||
@@ -141,6 +141,7 @@ pub fn apply_launch_settings_once(
|
|||||||
account_name: Option<&str>,
|
account_name: Option<&str>,
|
||||||
language: Option<&str>,
|
language: Option<&str>,
|
||||||
) -> eyre::Result<LaunchSettingsOutcome> {
|
) -> eyre::Result<LaunchSettingsOutcome> {
|
||||||
|
validate_game_state_id(game_id)?;
|
||||||
scoped_blocking(|| {
|
scoped_blocking(|| {
|
||||||
apply_launch_settings_once_blocking(state_dir, game_root, game_id, account_name, language)
|
apply_launch_settings_once_blocking(state_dir, game_root, game_id, account_name, language)
|
||||||
})
|
})
|
||||||
@@ -172,6 +173,7 @@ pub fn apply_launch_settings_to_verified_tree(
|
|||||||
/// recoverable: leaving it absent makes the existing first-play path retry the
|
/// recoverable: leaving it absent makes the existing first-play path retry the
|
||||||
/// rewrite.
|
/// rewrite.
|
||||||
pub fn mark_launch_settings_applied(state_dir: &Path, game_id: &str) -> eyre::Result<()> {
|
pub fn mark_launch_settings_applied(state_dir: &Path, game_id: &str) -> eyre::Result<()> {
|
||||||
|
validate_game_state_id(game_id)?;
|
||||||
scoped_blocking(|| mark_applied(&launch_settings_applied_path(state_dir, game_id)))
|
scoped_blocking(|| mark_applied(&launch_settings_applied_path(state_dir, game_id)))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -125,7 +125,7 @@ pub use crate::{
|
|||||||
mark_launch_settings_applied,
|
mark_launch_settings_applied,
|
||||||
},
|
},
|
||||||
startup::PeerRuntimeHandle,
|
startup::PeerRuntimeHandle,
|
||||||
state_paths::{launch_settings_applied_path, setup_done_path},
|
state_paths::setup_done_path,
|
||||||
stream_install::{
|
stream_install::{
|
||||||
ExternalUnrarStreamProvider,
|
ExternalUnrarStreamProvider,
|
||||||
NoopStreamInstallProvider,
|
NoopStreamInstallProvider,
|
||||||
|
|||||||
@@ -206,7 +206,17 @@ fn note_legacy_install_intent(root: &Path) -> MigrationReport {
|
|||||||
fn migrate_setup_marker(state_dir: &Path, id: &str, root: &Path) -> MigrationReport {
|
fn migrate_setup_marker(state_dir: &Path, id: &str, root: &Path) -> MigrationReport {
|
||||||
let mut report = MigrationReport::default();
|
let mut report = MigrationReport::default();
|
||||||
let legacy_path = root.join("local").join(LEGACY_FIRST_START_DONE_FILE);
|
let legacy_path = root.join("local").join(LEGACY_FIRST_START_DONE_FILE);
|
||||||
let target_path = setup_done_path(state_dir, id);
|
// Roots are discovered from directory names in the games folder, so an
|
||||||
|
// arbitrary name must be refused here rather than joined below the state
|
||||||
|
// directory.
|
||||||
|
let target_path = match setup_done_path(state_dir, id) {
|
||||||
|
Ok(path) => path,
|
||||||
|
Err(error) => {
|
||||||
|
log::warn!("Refusing setup marker migration for unsafe game ID {id:?}: {error}");
|
||||||
|
report.failures += 1;
|
||||||
|
return report;
|
||||||
|
}
|
||||||
|
};
|
||||||
|
|
||||||
match scoped_blocking(|| migrate_empty_marker(&legacy_path, &target_path)) {
|
match scoped_blocking(|| migrate_empty_marker(&legacy_path, &target_path)) {
|
||||||
Ok(MigrationOutcome::Migrated) => {
|
Ok(MigrationOutcome::Migrated) => {
|
||||||
@@ -521,7 +531,11 @@ mod tests {
|
|||||||
assert_eq!(report.install_intents_migrated, 0);
|
assert_eq!(report.install_intents_migrated, 0);
|
||||||
assert_eq!(report.failures, 2);
|
assert_eq!(report.failures, 2);
|
||||||
assert_eq!(report.setup_markers_migrated, 1);
|
assert_eq!(report.setup_markers_migrated, 1);
|
||||||
assert!(setup_done_path(state.path(), "game").is_file());
|
assert!(
|
||||||
|
setup_done_path(state.path(), "game")
|
||||||
|
.expect("setup marker path should be valid")
|
||||||
|
.is_file()
|
||||||
|
);
|
||||||
assert!(legacy_intent.exists());
|
assert!(legacy_intent.exists());
|
||||||
assert!(legacy_tmp.exists());
|
assert!(legacy_tmp.exists());
|
||||||
assert!(!legacy_setup.exists());
|
assert!(!legacy_setup.exists());
|
||||||
@@ -555,6 +569,32 @@ mod tests {
|
|||||||
assert_eq!(second.failures, 0);
|
assert_eq!(second.failures, 0);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn unsafe_root_name_is_refused_without_panicking_or_touching_state() {
|
||||||
|
let games = TempDir::new("lanspread-migration-games");
|
||||||
|
let state = TempDir::new("lanspread-migration-state");
|
||||||
|
// A trailing dot is a legal directory name on Unix but is not a
|
||||||
|
// portable game ID; joining it below the state directory must be
|
||||||
|
// refused rather than attempted.
|
||||||
|
let legacy_marker = games
|
||||||
|
.path()
|
||||||
|
.join("game.")
|
||||||
|
.join("local")
|
||||||
|
.join(LEGACY_FIRST_START_DONE_FILE);
|
||||||
|
write_file(&legacy_marker, b"");
|
||||||
|
|
||||||
|
let report = migrate_legacy_state(games.path(), state.path()).await;
|
||||||
|
|
||||||
|
assert_eq!(report.games_checked, 1);
|
||||||
|
assert_eq!(report.failures, 1);
|
||||||
|
assert_eq!(report.setup_markers_migrated, 0);
|
||||||
|
assert!(legacy_marker.exists(), "legacy marker must be kept");
|
||||||
|
assert!(
|
||||||
|
!state.path().join("games").exists(),
|
||||||
|
"no per-game state may be created for an unsafe ID"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn app_state_wins_over_legacy_per_game_state() {
|
async fn app_state_wins_over_legacy_per_game_state() {
|
||||||
let games = TempDir::new("lanspread-migration-games");
|
let games = TempDir::new("lanspread-migration-games");
|
||||||
@@ -571,7 +611,10 @@ mod tests {
|
|||||||
&legacy_intent_path,
|
&legacy_intent_path,
|
||||||
br#"{"schema_version":1,"state":"Installing"}"#,
|
br#"{"schema_version":1,"state":"Installing"}"#,
|
||||||
);
|
);
|
||||||
write_file(&setup_done_path(state.path(), "game"), b"");
|
write_file(
|
||||||
|
&setup_done_path(state.path(), "game").expect("setup marker path should be valid"),
|
||||||
|
b"",
|
||||||
|
);
|
||||||
write_file(&legacy_setup, b"");
|
write_file(&legacy_setup, b"");
|
||||||
|
|
||||||
let report = migrate_legacy_state(games.path(), state.path()).await;
|
let report = migrate_legacy_state(games.path(), state.path()).await;
|
||||||
|
|||||||
@@ -1,5 +1,7 @@
|
|||||||
use std::path::{Path, PathBuf};
|
use std::path::{Path, PathBuf};
|
||||||
|
|
||||||
|
use lanspread_db::content_manifest::validate_portable_component;
|
||||||
|
|
||||||
const PEER_IDENTITY_FILE: &str = "peer-identity-v1.json";
|
const PEER_IDENTITY_FILE: &str = "peer-identity-v1.json";
|
||||||
const LOCAL_LIBRARY_DIR: &str = "local_library";
|
const LOCAL_LIBRARY_DIR: &str = "local_library";
|
||||||
const LOCAL_LIBRARY_INDEX_FILE: &str = "index.json";
|
const LOCAL_LIBRARY_INDEX_FILE: &str = "index.json";
|
||||||
@@ -55,21 +57,56 @@ pub(crate) fn local_library_index_path(state_dir: &Path) -> PathBuf {
|
|||||||
.join(LOCAL_LIBRARY_INDEX_FILE)
|
.join(LOCAL_LIBRARY_INDEX_FILE)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Joins a per-game state directory below `<state_dir>/games`.
|
||||||
|
///
|
||||||
|
/// Callers inside this crate pass IDs that come from the catalog or have
|
||||||
|
/// already passed [`validate_game_state_id`]; the debug assertion documents
|
||||||
|
/// that contract without turning a bad ID into a release-build panic.
|
||||||
pub(crate) fn game_state_dir(state_dir: &Path, game_id: &str) -> PathBuf {
|
pub(crate) fn game_state_dir(state_dir: &Path, game_id: &str) -> PathBuf {
|
||||||
|
debug_assert!(
|
||||||
|
validate_game_state_id(game_id).is_ok(),
|
||||||
|
"game_state_dir called with an unvalidated game ID: {game_id:?}"
|
||||||
|
);
|
||||||
games_state_dir(state_dir).join(game_id)
|
games_state_dir(state_dir).join(game_id)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Rejects a game ID that could escape or alias its per-game state
|
||||||
|
/// directory: separators, NUL, `.`/`..`, trailing dots or spaces, control or
|
||||||
|
/// Windows-reserved characters and Windows device names.
|
||||||
|
///
|
||||||
|
/// The rules are the catalog's own portable component rules, so every ID a
|
||||||
|
/// catalog can publish is accepted and the check cannot drift from the
|
||||||
|
/// validator that admits game IDs in the first place.
|
||||||
|
///
|
||||||
|
/// # Errors
|
||||||
|
///
|
||||||
|
/// Returns the first violated rule.
|
||||||
|
pub(crate) fn validate_game_state_id(game_id: &str) -> eyre::Result<()> {
|
||||||
|
if game_id.contains(['/', '\\', '\0']) {
|
||||||
|
eyre::bail!("game ID must be one path component: {game_id:?}");
|
||||||
|
}
|
||||||
|
validate_portable_component(game_id)
|
||||||
|
}
|
||||||
|
|
||||||
pub(crate) fn games_state_dir(state_dir: &Path) -> PathBuf {
|
pub(crate) fn games_state_dir(state_dir: &Path) -> PathBuf {
|
||||||
state_dir.join(GAMES_DIR)
|
state_dir.join(GAMES_DIR)
|
||||||
}
|
}
|
||||||
|
|
||||||
#[must_use]
|
/// Path of the marker that records a completed one-time `game_setup` run.
|
||||||
pub fn setup_done_path(state_dir: &Path, game_id: &str) -> PathBuf {
|
///
|
||||||
game_state_dir(state_dir, game_id).join(SETUP_DONE_FILE)
|
/// This is the only state path exposed to embedding applications, whose game
|
||||||
|
/// IDs may originate from UI input rather than the catalog, so it validates
|
||||||
|
/// the ID before joining it below the state directory.
|
||||||
|
///
|
||||||
|
/// # Errors
|
||||||
|
///
|
||||||
|
/// Returns an error when `game_id` is not a single portable path component.
|
||||||
|
pub fn setup_done_path(state_dir: &Path, game_id: &str) -> eyre::Result<PathBuf> {
|
||||||
|
validate_game_state_id(game_id)?;
|
||||||
|
Ok(game_state_dir(state_dir, game_id).join(SETUP_DONE_FILE))
|
||||||
}
|
}
|
||||||
|
|
||||||
#[must_use]
|
pub(crate) fn launch_settings_applied_path(state_dir: &Path, game_id: &str) -> PathBuf {
|
||||||
pub fn launch_settings_applied_path(state_dir: &Path, game_id: &str) -> PathBuf {
|
|
||||||
game_state_dir(state_dir, game_id).join(LAUNCH_SETTINGS_APPLIED_FILE)
|
game_state_dir(state_dir, game_id).join(LAUNCH_SETTINGS_APPLIED_FILE)
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -188,4 +225,44 @@ mod tests {
|
|||||||
explicit
|
explicit
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn setup_done_path_accepts_catalog_style_game_ids() {
|
||||||
|
let state_dir = Path::new("/state");
|
||||||
|
assert_eq!(
|
||||||
|
setup_done_path(state_dir, "game").expect("plain ID should be accepted"),
|
||||||
|
Path::new("/state/games/game/setup_done")
|
||||||
|
);
|
||||||
|
for game_id in ["game..v1 (final)", "console.txt", "com10", "Jörg"] {
|
||||||
|
assert!(
|
||||||
|
setup_done_path(state_dir, game_id).is_ok(),
|
||||||
|
"rejected catalog-valid game ID {game_id:?}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn setup_done_path_rejects_escaping_game_ids() {
|
||||||
|
let state_dir = Path::new("/state");
|
||||||
|
for game_id in [
|
||||||
|
"",
|
||||||
|
".",
|
||||||
|
"..",
|
||||||
|
"../outside",
|
||||||
|
"/outside",
|
||||||
|
r"nested\game",
|
||||||
|
"game/child",
|
||||||
|
"game\0",
|
||||||
|
"game.",
|
||||||
|
"game ",
|
||||||
|
"NUL",
|
||||||
|
"con.txt",
|
||||||
|
"a:b",
|
||||||
|
] {
|
||||||
|
assert!(
|
||||||
|
setup_done_path(state_dir, game_id).is_err(),
|
||||||
|
"accepted unsafe game ID {game_id:?}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1907,7 +1907,13 @@ async fn run_game_windows(
|
|||||||
return Ok(());
|
return Ok(());
|
||||||
};
|
};
|
||||||
|
|
||||||
let setup_done_file = lanspread_peer::setup_done_path(&state_dir, &id);
|
let setup_done_file = match lanspread_peer::setup_done_path(&state_dir, &id) {
|
||||||
|
Ok(path) => path,
|
||||||
|
Err(error) => {
|
||||||
|
log::warn!("Ignoring run request for unsafe game id {id}: {error}");
|
||||||
|
return Ok(());
|
||||||
|
}
|
||||||
|
};
|
||||||
if !setup_done_file.exists() && game_setup_bin.exists() {
|
if !setup_done_file.exists() && game_setup_bin.exists() {
|
||||||
if !local_install_is_present(&game_path) {
|
if !local_install_is_present(&game_path) {
|
||||||
log::warn!(
|
log::warn!(
|
||||||
|
|||||||
Reference in New Issue
Block a user