diff --git a/packages/app-lib/src/api/curseforge.rs b/packages/app-lib/src/api/curseforge.rs index 4e8e333..bd9e147 100644 --- a/packages/app-lib/src/api/curseforge.rs +++ b/packages/app-lib/src/api/curseforge.rs @@ -5197,19 +5197,9 @@ fn modpack_zip_error(error: zip::result::ZipError) -> crate::Error { } fn safe_archive_relative_path(value: &str) -> crate::Result { - let path = Path::new(value); - if value.is_empty() - || path.is_absolute() - || path - .components() - .any(|component| !matches!(component, Component::Normal(_))) - { - return Err(ErrorKind::InputError( - "CurseForge modpack contains an invalid override path".to_string(), - ) - .into()); - } - Ok(path.to_string_lossy().replace('\\', "/")) + let normalized = value.replace('\\', "/"); + crate::util::io::safe_relative_join(Path::new(""), &normalized)?; + Ok(normalized) } pub(crate) fn loader_family(loader_id: &str) -> &str { @@ -10933,6 +10923,21 @@ mod tests { ); assert!(safe_archive_relative_path("../options.txt").is_err()); assert!(safe_archive_relative_path("/options.txt").is_err()); + for path in [ + r"..\options.txt", + "config/.. /outside", + "config/file:stream", + "C:/outside", + "C:outside", + "config/NUL", + "config/file.", + ] { + assert!(safe_archive_relative_path(path).is_err(), "{path}"); + } + assert_eq!( + safe_archive_relative_path(r"config\example.toml").unwrap(), + "config/example.toml" + ); } #[test] diff --git a/packages/app-lib/src/api/pack/archive_util.rs b/packages/app-lib/src/api/pack/archive_util.rs index 254a11b..5c74296 100644 --- a/packages/app-lib/src/api/pack/archive_util.rs +++ b/packages/app-lib/src/api/pack/archive_util.rs @@ -1,6 +1,6 @@ //! Shared helpers for extracting content from local modpack archives. -use std::path::{Component, Path, PathBuf}; +use std::path::{Path, PathBuf}; use super::detect::decode_zip_entry_name; use crate::util::io; @@ -13,22 +13,6 @@ fn archive_error(error: zip::result::ZipError) -> crate::Error { .into() } -pub(crate) fn safe_relative_path(value: &str) -> crate::Result { - let path = Path::new(value); - if value.is_empty() - || path.is_absolute() - || path - .components() - .any(|component| !matches!(component, Component::Normal(_))) - { - return Err(crate::ErrorKind::InputError( - "Modpack archive contains an invalid file path".to_string(), - ) - .into()); - } - Ok(path.to_string_lossy().replace('\\', "/")) -} - /// Extracts every file under `prefix` in the archive into `target_dir`, /// preserving the directory structure below the prefix. Returns the number of /// files written. @@ -114,7 +98,8 @@ fn extract_archive_subdir_sync( ) .into()); } - let target = target_dir.join(safe_relative_path(relative)?); + let target = + io::safe_relative_join(target_dir, &relative.replace('\\', "/"))?; if let Some(parent) = target.parent() { std::fs::create_dir_all(parent) .map_err(|error| io::IOError::with_path(error, parent))?; @@ -252,6 +237,87 @@ pub(crate) async fn create_import_scratch_dir( #[cfg(test)] mod tests { use super::*; + use std::io::Write; + + #[test] + fn unsafe_override_paths_fail_without_writing_outside_target() { + let root = tempfile::tempdir().unwrap(); + let archive_path = root.path().join("pack.zip"); + let target_dir = root.path().join("server"); + std::fs::create_dir(&target_dir).unwrap(); + let outside = root.path().join("outside.txt"); + std::fs::write(&outside, b"unchanged").unwrap(); + for prefix in ["overrides/", "server/overrides/"] { + for relative in [ + "../outside.txt", + r"..\outside.txt", + "config/../../outside.txt", + "/outside.txt", + "C:/outside.txt", + "C:outside.txt", + r"\\server\share\outside.txt", + "config/.. /outside.txt", + "config/mod.jar:payload", + "config/NUL", + "config/secret.", + ] { + let mut archive = zip::ZipWriter::new( + std::fs::File::create(&archive_path).unwrap(), + ); + archive + .start_file( + format!("{prefix}{relative}"), + zip::write::SimpleFileOptions::default(), + ) + .unwrap(); + archive.write_all(b"malicious").unwrap(); + archive.finish().unwrap(); + assert!( + extract_archive_subdir_sync( + &archive_path, + prefix, + &target_dir, + None, + ) + .is_err(), + "{prefix}{relative}" + ); + assert_eq!(std::fs::read(&outside).unwrap(), b"unchanged"); + assert_eq!(std::fs::read_dir(&target_dir).unwrap().count(), 0); + } + } + } + + #[test] + fn override_paths_with_backslashes_are_normalized() { + let root = tempfile::tempdir().unwrap(); + let archive_path = root.path().join("pack.zip"); + let target_dir = root.path().join("server"); + let mut archive = + zip::ZipWriter::new(std::fs::File::create(&archive_path).unwrap()); + archive + .start_file( + r"overrides/config\example.toml", + zip::write::SimpleFileOptions::default(), + ) + .unwrap(); + archive.write_all(b"valid").unwrap(); + archive.finish().unwrap(); + assert_eq!( + extract_archive_subdir_sync( + &archive_path, + "overrides/", + &target_dir, + None, + ) + .unwrap(), + 1 + ); + assert_eq!( + std::fs::read(target_dir.join("config/example.toml")).unwrap(), + b"valid" + ); + } struct CancelOnRead { cancellation: CancellationToken, diff --git a/packages/app-lib/src/api/pack/install_from.rs b/packages/app-lib/src/api/pack/install_from.rs index 53c812b..245ca05 100644 --- a/packages/app-lib/src/api/pack/install_from.rs +++ b/packages/app-lib/src/api/pack/install_from.rs @@ -20,7 +20,7 @@ use serde::{Deserialize, Serialize}; use std::collections::HashMap; use std::future::Future; -use std::path::{Component, Path, PathBuf}; +use std::path::{Path, PathBuf}; use std::pin::Pin; use zip::ZipArchive; @@ -320,23 +320,13 @@ pub(crate) async fn generate_pack_from_version_id_with_reporter( "Specified version has no files".to_string(), ) })?; - let file_name = Path::new(&pack_file.filename); - if file_name.components().count() != 1 - || !matches!(file_name.components().next(), Some(Component::Normal(_))) - { - return Err(crate::ErrorKind::InputError( - "Modrinth returned an invalid modpack file name".to_string(), - ) - .into()); - } let hash = pack_file.hashes.get("sha1"); - let pack_path = state - .directories - .caches_dir() - .join("modpacks") - .join(&project_id) - .join(&version_id) - .join(file_name); + let pack_path = modpack_cache_path( + &state.directories.caches_dir().join("modpacks"), + &project_id, + &version_id, + &pack_file.filename, + )?; let details = InstallPhaseDetails::Modpack { project_id: Some(project_id.clone()), @@ -517,8 +507,70 @@ pub(crate) async fn generate_pack_from_version_id_with_reporter( }) } -#[tracing::instrument] +fn modpack_cache_path( + root: &Path, + project_id: &str, + version_id: &str, + filename: &str, +) -> crate::Result { + let mut path = root.to_path_buf(); + for segment in [project_id, version_id, filename] { + if segment.contains('/') { + return Err(crate::ErrorKind::InputError( + "Invalid modpack cache path component".to_string(), + ) + .into()); + } + path = crate::util::io::safe_relative_join(&path, segment)?; + } + Ok(path) +} +#[cfg(test)] +mod cache_path_tests { + use super::modpack_cache_path; + use std::path::Path; + + #[test] + fn cache_components_cannot_escape_their_directories() { + let root = Path::new("modpacks"); + assert_eq!( + modpack_cache_path(root, "project", "version", "pack.mrpack") + .unwrap(), + root.join("project/version/pack.mrpack") + ); + for malicious in [ + "../outside", + "nested/name", + r"..\outside", + "C:/outside", + "C:outside", + r"\\server\share", + ".", + "", + "NUL", + "pack.mrpack:stream", + ".. ", + ] { + for position in 0..3 { + let mut components = ["project", "version", "pack.mrpack"]; + components[position] = malicious; + assert!( + modpack_cache_path( + root, + components[0], + components[1], + components[2] + ) + .is_err(), + "{position}: {malicious:?}" + ); + } + } + } +} + +#[tracing::instrument] pub async fn generate_pack_from_file( path: PathBuf, instance_id: String, diff --git a/packages/app-lib/src/api/pack/install_mrpack.rs b/packages/app-lib/src/api/pack/install_mrpack.rs index 7de173b..d5127a5 100644 --- a/packages/app-lib/src/api/pack/install_mrpack.rs +++ b/packages/app-lib/src/api/pack/install_mrpack.rs @@ -891,6 +891,12 @@ pub(crate) async fn install_zipped_mrpack_files_with_reporter( num_files, content_total_bytes, }; + for file in &pack.files { + io::join_within_root( + &content_context.instance_full_path, + &content_context.resolve_install_path(file), + )?; + } let skipped_missing_content_paths = Arc::new( reporter .current_state() @@ -1039,8 +1045,14 @@ pub(crate) async fn install_zipped_mrpack_files_with_reporter( let project_size = project.file_size as u64; let project_path = content_context.resolve_install_path(&project); - let target_path = - content_context.instance_full_path.join(&project_path); + let target_path = io::join_within_root( + &content_context.instance_full_path, + &project_path, + ).map_err(|error| RequiredFileFailure::new( + manifest_index, + &project_path, + &error.to_string(), + ))?; let context = InstallErrorContext::new("download modpack content file") .maybe_project_id( @@ -1451,7 +1463,10 @@ pub(crate) async fn install_zipped_mrpack_files_with_reporter( entry_name: filename.to_string(), crc32: file.crc32(), uncompressed_size: file.uncompressed_size(), - target_path: instance_full_path.join(&relative_path), + target_path: io::safe_relative_join( + &instance_full_path, + &relative_path, + )?, relative_path, }) }) @@ -1739,6 +1754,7 @@ fn override_relative_path( "Invalid modpack override path: {relative}" )) })?; + io::safe_relative_join(Path::new(""), relative.as_str())?; Ok(relative.as_str().to_string()) } @@ -1885,7 +1901,11 @@ pub async fn remove_all_related_files( // Iterate over all Modrinth project file paths in the json, and remove them // (There should be few, but this removes any files the .mrpack intended as Modrinth projects but were unrecognized) for file in pack.files { - match io::remove_file(instance_full_path.join(file.path.as_str())).await + match io::remove_file(io::safe_relative_join( + &instance_full_path, + file.path.as_str(), + )?) + .await { Ok(_) => (), Err(err) if err.kind() == ErrorKind::NotFound => (), @@ -1904,23 +1924,14 @@ pub async fn remove_all_related_files( for file in override_file_entries { let entry_name = file.filename().as_str().unwrap(); - let entry_name = - entry_name.strip_prefix(&archive_base).unwrap_or(entry_name); let relative_override_file_path = - SafeRelativeUtf8UnixPathBuf::try_from(entry_name.to_string())?; - let relative_override_file_path = relative_override_file_path - .strip_prefix("overrides") - .or_else(|_| relative_override_file_path.strip_prefix("client-overrides")) - .map_err(|_| { - crate::Error::from(crate::ErrorKind::OtherError( - format!("Failed to strip override prefix from override file path: {relative_override_file_path}") - )) - })?; + override_relative_path(entry_name, &archive_base)?; // Remove this file if a corresponding one exists in the filesystem - match io::remove_file( - instance_full_path.join(relative_override_file_path.as_str()), - ) + match io::remove_file(io::safe_relative_join( + &instance_full_path, + &relative_override_file_path, + )?) .await { Ok(_) => (), @@ -1939,6 +1950,35 @@ mod tests { use async_zip::{Compression, ZipEntryBuilder}; use std::sync::Mutex as StdMutex; + #[test] + fn override_paths_reject_portable_traversal_and_windows_aliases() { + for prefix in ["overrides/", "client-overrides/"] { + assert_eq!( + override_relative_path( + &format!("{prefix}config/example.toml"), + "" + ) + .unwrap(), + "config/example.toml" + ); + for relative in [ + "../outside", + "config/.. /outside", + "config/file:stream", + "C:/outside", + "C:outside", + "config/NUL", + "config/file.", + ] { + assert!( + override_relative_path(&format!("{prefix}{relative}"), "") + .is_err(), + "{prefix}{relative}" + ); + } + } + } + fn override_spec(index: usize, target: &str) -> OverrideExtractionSpec { OverrideExtractionSpec { index, diff --git a/packages/app-lib/src/api/servers/files.rs b/packages/app-lib/src/api/servers/files.rs index 6007c65..6580a7c 100644 --- a/packages/app-lib/src/api/servers/files.rs +++ b/packages/app-lib/src/api/servers/files.rs @@ -12,6 +12,8 @@ use crate::{ErrorKind, Result}; use super::manifest::server_path; +pub(super) use crate::util::io::safe_relative_join; + const DOWNLOAD_PROGRESS_STEP: u64 = 512 * 1024; pub async fn read_file(server_id: &str, file: &str) -> Result { @@ -124,44 +126,7 @@ pub(super) async fn download_to_dir( async fn resolve_server_file(server_id: &str, file: &str) -> Result { let dir = server_path(server_id).await?; - safe_join(&dir, file) -} - -fn safe_join(dir: &Path, file: &str) -> Result { - if file.is_empty() - || file.contains('\\') - || file.starts_with('/') - || file - .split('/') - .any(|segment| segment == ".." || segment.is_empty()) - { - return Err(ErrorKind::InputError(format!( - "Invalid file name: {file}" - )) - .as_error()); - } - Ok(dir.join(file)) -} - -/// Joins a relative path that may contain `/` separators but never escapes the -/// server directory (no `..`, empty segments, or absolute/Windows roots). -pub(super) fn safe_relative_join(dir: &Path, rel: &str) -> Result { - let is_windows_drive_root = - rel.as_bytes().get(1).is_some_and(|byte| *byte == b':') - && rel.as_bytes().first().is_some_and(u8::is_ascii_alphabetic); - if rel.is_empty() - || Path::new(rel).is_absolute() - || is_windows_drive_root - || rel.starts_with('/') - || rel.starts_with('\\') - || rel.split(['/', '\\']).any(|segment| { - segment.is_empty() || segment == ".." || segment == "." - }) - { - return Err(ErrorKind::InputError(format!("Invalid file path: {rel}")) - .as_error()); - } - Ok(dir.join(rel)) + safe_relative_join(&dir, file) } #[cfg(test)] @@ -169,13 +134,34 @@ mod tests { use super::*; #[test] - fn safe_join_rejects_traversal() { + fn safe_relative_join_rejects_unsafe_paths() { let dir = Path::new("/tmp/servers/a"); - assert!(safe_join(dir, "server.properties").is_ok()); - assert!(safe_join(dir, "../secret").is_err()); - assert!(safe_join(dir, "/etc/passwd").is_err()); - assert!(safe_join(dir, "a//b").is_err()); - assert!(safe_join(dir, "").is_err()); + for path in [ + "../secret", + "mods/../../secret", + r"..\secret", + r"mods/..\../secret", + "/etc/passwd", + "C:/windows/system32", + r"C:\windows\system32", + "C:secret", + r"\\server\share\secret", + r"\secret", + r"\\?\C:\secret", + "a//b", + "a/./b", + ".", + "", + "mods/.. /secret", + "mods/secret.", + "mods/secret ", + "mods/mod.jar:payload", + "NUL/secret", + "mods/CON.jar", + "mods/mod\0.jar", + ] { + assert!(safe_relative_join(dir, path).is_err(), "{path:?}"); + } } #[test] diff --git a/packages/app-lib/src/api/servers/forge.rs b/packages/app-lib/src/api/servers/forge.rs index 0d1f4a3..efb8b44 100644 --- a/packages/app-lib/src/api/servers/forge.rs +++ b/packages/app-lib/src/api/servers/forge.rs @@ -14,7 +14,7 @@ use crate::event::emit::emit_server; use crate::util::io::IOError; use crate::{ErrorKind, Result}; -use super::files::download_to_dir; +use super::files::{download_to_dir, safe_relative_join}; use super::manifest::{ InstallState, read_manifest, server_path, write_manifest, }; @@ -41,6 +41,7 @@ pub async fn install_forge( drop(manifest); let installer_name = format!("forge-{mc_version}-{build}-installer.jar"); + let installer_path = safe_relative_join(&dir, &installer_name)?; let installer_url = format!("{FORGE_MAVEN}/{mc_version}-{build}/{installer_name}"); @@ -52,7 +53,6 @@ pub async fn install_forge( download_to_dir(server_id, &dir, &installer_url, &installer_name, None) .await?; - let installer_path = dir.join(&installer_name); let java = java_path.clone().unwrap_or_else(|| "java".to_string()); log(server_id, "Running Forge installer (this may take a while)").await?; diff --git a/packages/app-lib/src/api/servers/lifecycle.rs b/packages/app-lib/src/api/servers/lifecycle.rs index f259b76..45814d2 100644 --- a/packages/app-lib/src/api/servers/lifecycle.rs +++ b/packages/app-lib/src/api/servers/lifecycle.rs @@ -18,6 +18,7 @@ use crate::state::{clear_log_buffer, get_log_buffer, push_log_line}; use crate::util::io::IOError; use crate::{ErrorKind, Result}; +use super::files::safe_relative_join; use super::logs::{ analyze_exit_reason, stream_server_output, stream_server_pty_output, tail_server_log_file, @@ -131,7 +132,7 @@ async fn start_inner( forge_launch_args(&dir)? } else { let jar_name = resolve_jar_name(&manifest); - let jar_path = dir.join(&jar_name); + let jar_path = safe_relative_join(&dir, &jar_name)?; if !jar_path.exists() { return Err(ErrorKind::LauncherError(format!( "Server jar not found: {jar_name}. Download the server files first." @@ -275,9 +276,11 @@ async fn start_inner( // so these lines stop the console from looking frozen. let loader_first_run = matches!(manifest.server_type.as_str(), "fabric" | "quilt") - && !dir - .join(format!("{}-server-launch.jar", manifest.server_type)) - .exists(); + && !safe_relative_join( + &dir, + &format!("{}-server-launch.jar", manifest.server_type), + )? + .exists(); log_server_step( server_id, diff --git a/packages/app-lib/src/api/servers/manage.rs b/packages/app-lib/src/api/servers/manage.rs index f6b517c..5890c59 100644 --- a/packages/app-lib/src/api/servers/manage.rs +++ b/packages/app-lib/src/api/servers/manage.rs @@ -7,6 +7,7 @@ use crate::state::remove_log_buffer; use crate::util::io::{self, IOError}; use crate::{ErrorKind, Result, State}; +use super::files::safe_relative_join; use super::lifecycle::is_running; use super::manifest::{ ServerInfo, ServerManifest, build_server_info, read_manifest, @@ -72,7 +73,7 @@ pub async fn create( let state = State::get().await?; let id = Uuid::new_v4().to_string(); let dir_name = format!("{}-{}", sanitize_folder_name(name), &id[..8]); - let dir = state.directories.servers_dir().join(&dir_name); + let dir = safe_relative_join(&state.directories.servers_dir(), &dir_name)?; io::create_dir_all(&dir).await?; let manifest = ServerManifest { diff --git a/packages/app-lib/src/api/servers/manifest.rs b/packages/app-lib/src/api/servers/manifest.rs index 8d2d464..1985d9d 100644 --- a/packages/app-lib/src/api/servers/manifest.rs +++ b/packages/app-lib/src/api/servers/manifest.rs @@ -118,7 +118,7 @@ pub(super) async fn server_path(server_id: &str) -> Result { .as_error()); } let state = State::get().await?; - let path = state.directories.server_dir(server_id); + let path = state.directories.server_dir(server_id)?; if !path.is_dir() { return Err(ErrorKind::InputError(format!( "Unknown server: {server_id}" diff --git a/packages/app-lib/src/api/servers/modpack.rs b/packages/app-lib/src/api/servers/modpack.rs index cf73c93..7e0d252 100644 --- a/packages/app-lib/src/api/servers/modpack.rs +++ b/packages/app-lib/src/api/servers/modpack.rs @@ -33,7 +33,7 @@ use crate::util::fetch::{ use crate::util::io::IOError; use crate::{ErrorKind, Result}; -use super::files::download_to_dir; +use super::files::{download_to_dir, safe_relative_join}; use super::manifest::{ InstallState, ModpackInfo, read_manifest, server_path, write_manifest, }; @@ -92,10 +92,52 @@ struct MrpackFile { env: Option, #[serde(default)] downloads: Vec, - #[serde(default)] + #[serde(default, rename = "fileSize")] file_size: Option, } +impl MrpackFile { + fn integrity(&self) -> Result { + let sha1 = self.hashes.get("sha1").cloned(); + let sha512 = self.hashes.get("sha512").cloned(); + if sha1.is_none() && sha512.is_none() { + return Err(ErrorKind::InputError(format!( + "Modpack file {} is missing a SHA-1 or SHA-512 checksum", + self.path + )) + .as_error()); + } + for (hash, length) in [(&sha1, 40), (&sha512, 128)] { + if let Some(hash) = hash + && (hash.len() != length + || !hash.bytes().all(|byte| byte.is_ascii_hexdigit())) + { + return Err(ErrorKind::InputError(format!( + "Modpack file {} has an invalid checksum", + self.path + )) + .as_error()); + } + } + Ok(Integrity { + sha1, + sha512, + size: self.file_size, + ..Integrity::default() + }) + } +} + +/// Validate the entire index before starting concurrent downloads, including +/// entries excluded from server installation but fetched for metadata. +fn validate_index(index: &MrpackIndex, dir: &Path) -> Result<()> { + for file in &index.files { + safe_relative_join(dir, &file.path)?; + file.integrity()?; + } + Ok(()) +} + #[derive(Deserialize, Clone)] struct MrpackEnv { #[serde(default)] @@ -272,7 +314,7 @@ async fn run_modpack_install( &state, mrpack_url, &dir.join(MRPACK_FILENAME), - mrpack_sha1.map(str::to_string), + mrpack_sha1.map(Integrity::sha1).unwrap_or_default(), ResourceClass::Modpack, AggregateProgress::new(0), ) @@ -282,6 +324,7 @@ async fn run_modpack_install( let manifest_entry = find_manifest_entry(&archive_path).await?; let base_folder = base_folder(&manifest_entry); let index = parse_index(&archive_path, &manifest_entry).await?; + validate_index(&index, dir)?; let (installable_files, excluded_files): ( Vec<&MrpackFile>, @@ -310,19 +353,19 @@ async fn run_modpack_install( ) .await?; - let files: Vec<(String, String, Option, u64)> = installable_files + let files: Vec<(String, String, Integrity, u64)> = installable_files .iter() .filter_map(|file| { file.downloads.first().map(|url| { - ( + Ok(( file.path.clone(), url.clone(), - file.hashes.get("sha1").cloned(), + file.integrity()?, file.file_size.unwrap_or(0), - ) + )) }) }) - .collect(); + .collect::>()?; let progress = AggregateProgress::new(total_bytes); let state_ref = state.clone(); @@ -336,14 +379,14 @@ async fn run_modpack_install( 0.0, num_files, None, - move |(path, url, sha1, size)| { + move |(path, url, integrity, size)| { let server_id = server_id_ref.clone(); let dir = dir_ref.clone(); let state = state_ref.clone(); let progress = progress.clone(); async move { log(&server_id, &format!("Downloading {path}")).await?; - let destination = dir.join(&path); + let destination = safe_relative_join(&dir, &path)?; if let Some(parent) = destination.parent() { crate::util::io::create_dir_all(parent).await?; } @@ -352,7 +395,7 @@ async fn run_modpack_install( &state, &url, &destination, - sha1, + integrity, ResourceClass::Modpack, progress.clone(), ) @@ -446,7 +489,7 @@ async fn download_icon(dir: &Path, icon_url: &str) -> Result { ) }) .unwrap_or_else(|| "png".to_string()); - let destination = dir.join(format!("icon.{extension}")); + let destination = safe_relative_join(dir, &format!("icon.{extension}"))?; let client = reqwest::Client::builder() .user_agent(crate::launcher_user_agent()) @@ -476,15 +519,13 @@ async fn download_with_engine( state: &State, url: &str, destination: &Path, - sha1: Option, + integrity: Integrity, resource: ResourceClass, progress: AggregateProgress, ) -> Result<()> { - let mut request = DownloadRequest::new(url, resource); - if let Some(sha1) = &sha1 { - request = request.with_integrity(Integrity::sha1(sha1.clone())); - } - request = request.with_segmented_download(true); + let request = DownloadRequest::new(url, resource) + .with_integrity(integrity) + .with_segmented_download(true); let server_id = server_id.to_string(); let progress = progress.clone(); @@ -579,7 +620,7 @@ async fn fetch_excluded_mod_ids( state: &State, excluded_files: &[&MrpackFile], ) -> Result> { - let candidates: Vec<(String, String, Option)> = excluded_files + let candidates: Vec<(String, String, Integrity)> = excluded_files .iter() .filter_map(|file| { let path = file.path.replace('\\', "/"); @@ -589,14 +630,14 @@ async fn fetch_excluded_mod_ids( return None; } file.downloads.first().map(|url| { - ( + Ok(( path.rsplit('/').next().unwrap_or(&path).to_string(), url.clone(), - file.hashes.get("sha1").cloned(), - ) + file.integrity()?, + )) }) }) - .collect(); + .collect::>()?; if candidates.is_empty() { return Ok(HashSet::new()); } @@ -611,9 +652,10 @@ async fn fetch_excluded_mod_ids( ErrorKind::FSError(format!("Failed to create temporary directory: {e}")) })?; let mut unavailable = HashSet::new(); - for (filename, url, sha1) in candidates { - let destination = temp_dir.path().join(&filename); - match download_metadata_jar(state, &url, &destination, sha1).await { + for (filename, url, integrity) in candidates { + let destination = safe_relative_join(temp_dir.path(), &filename)?; + match download_metadata_jar(state, &url, &destination, integrity).await + { Ok(()) => match read_mod_metadata(&destination) { Some(metadata) => { unavailable @@ -647,12 +689,10 @@ async fn download_metadata_jar( state: &State, url: &str, destination: &Path, - sha1: Option, + integrity: Integrity, ) -> Result<()> { - let mut request = DownloadRequest::new(url, ResourceClass::Modpack); - if let Some(sha1) = &sha1 { - request = request.with_integrity(Integrity::sha1(sha1.clone())); - } + let request = DownloadRequest::new(url, ResourceClass::Modpack) + .with_integrity(integrity); download_to_path( request, destination, @@ -908,6 +948,122 @@ mod tests { use super::*; use std::io::Write; + fn index_file(path: &str) -> serde_json::Value { + serde_json::json!({ + "path": path, + "hashes": { "sha1": "0".repeat(40) }, + "downloads": ["https://example.com/mod.jar"], + "fileSize": 10 + }) + } + + #[tokio::test] + async fn malicious_index_paths_fail_before_materializing_files() { + let temp = tempfile::tempdir().unwrap(); + let server = temp.path().join("server"); + std::fs::create_dir(&server).unwrap(); + let outside = temp.path().join("outside.txt"); + std::fs::write(&outside, "unchanged").unwrap(); + let archive = temp.path().join("pack.mrpack"); + for path in [ + "../outside.txt", + "mods/../../outside.txt", + r"..\outside.txt", + r"mods/..\../outside.txt", + "/tmp/outside.txt", + "C:/outside.txt", + r"C:\outside.txt", + "C:outside.txt", + r"\\server\share\outside.txt", + r"\outside.txt", + r"\\?\C:\outside.txt", + "mods//mod.jar", + "mods/./mod.jar", + "mods/.. /outside.txt", + "mods/mod.jar:payload", + "NUL/outside.txt", + "mods/CON.jar", + "", + ] { + for environment in ["required", "unsupported"] { + let mut malicious = index_file(path); + // Filtered entries must also fail before any downloads. + malicious["env"] = serde_json::json!({"server": environment}); + let contents = serde_json::json!({ + "files": [index_file("mods/valid.jar"), malicious] + }) + .to_string(); + create_jar(&archive, &[(MRPACK_MANIFEST_ENTRY, &contents)]); + let index = + parse_index(&archive, MRPACK_MANIFEST_ENTRY).await.unwrap(); + assert!(validate_index(&index, &server).is_err(), "{path:?}"); + assert_eq!( + std::fs::read_to_string(&outside).unwrap(), + "unchanged" + ); + assert_eq!(std::fs::read_dir(&server).unwrap().count(), 0); + } + } + } + + #[test] + fn index_requires_valid_supported_checksums() { + for hashes in [ + serde_json::json!({}), + serde_json::json!({"md5": "0".repeat(32)}), + serde_json::json!({"sha1": ""}), + serde_json::json!({"sha1": "0".repeat(39)}), + serde_json::json!({"sha512": "g".repeat(128)}), + serde_json::json!({"sha1": "invalid", "sha512": "0".repeat(128)}), + ] { + let mut file = index_file("mods/mod.jar"); + file["hashes"] = hashes; + let index: MrpackIndex = + serde_json::from_value(serde_json::json!({"files": [file]})) + .unwrap(); + assert!(validate_index(&index, Path::new("server")).is_err()); + } + let mut file = index_file("mods/mod.jar"); + file.as_object_mut().unwrap().remove("hashes"); + let index: MrpackIndex = + serde_json::from_value(serde_json::json!({"files": [file]})) + .unwrap(); + assert!(validate_index(&index, Path::new("server")).is_err()); + } + + #[tokio::test] + async fn index_checksums_verify_file_contents() { + use crate::util::fetch::verify_file; + use sha2::{Digest, Sha512}; + + let temp = tempfile::tempdir().unwrap(); + let target = temp.path().join("mod.jar"); + let contents = b"verified mod"; + std::fs::write(&target, contents).unwrap(); + for hashes in [ + serde_json::json!({"sha1": sha1_smol::Sha1::from(contents).digest().to_string()}), + serde_json::json!({"sha512": format!("{:x}", Sha512::digest(contents))}), + serde_json::json!({ + "sha1": sha1_smol::Sha1::from(contents).digest().to_string(), + "sha512": format!("{:x}", Sha512::digest(contents)) + }), + ] { + let mut file = index_file("mods/mod.jar"); + file["hashes"] = hashes; + file["fileSize"] = serde_json::json!(contents.len()); + let index: MrpackIndex = + serde_json::from_value(serde_json::json!({"files": [file]})) + .unwrap(); + validate_index(&index, temp.path()).unwrap(); + let integrity = index.files[0].integrity().unwrap(); + assert_eq!(integrity.size, Some(contents.len() as u64)); + verify_file(&target, &integrity).await.unwrap(); + std::fs::write(&target, b"tampered mod").unwrap(); + assert!(verify_file(&target, &integrity).await.is_err()); + std::fs::write(&target, contents).unwrap(); + } + } + #[test] fn server_only_files_are_installable() { let installable = MrpackFile { diff --git a/packages/app-lib/src/state/dirs.rs b/packages/app-lib/src/state/dirs.rs index 4d48b76..c29bf92 100644 --- a/packages/app-lib/src/state/dirs.rs +++ b/packages/app-lib/src/state/dirs.rs @@ -171,8 +171,8 @@ impl DirectoryInfo { /// Gets the directory of a managed dedicated server by id #[inline] - pub fn server_dir(&self, server_id: &str) -> PathBuf { - self.servers_dir().join(server_id) + pub fn server_dir(&self, server_id: &str) -> crate::Result { + crate::util::io::safe_relative_join(&self.servers_dir(), server_id) } #[inline] @@ -708,6 +708,28 @@ mod resolve_game_dir_tests { } } + #[test] + fn server_directory_rejects_absolute_and_windows_alias_paths() { + let dirs = dirs(); + assert_eq!( + dirs.server_dir("server-1234").unwrap(), + dirs.servers_dir().join("server-1234") + ); + for id in [ + "../outside", + "C:outside", + "C:/outside", + r"\outside", + r"\\server\share", + ".", + "NUL", + "server:stream", + "server.", + ] { + assert!(dirs.server_dir(id).is_err(), "{id:?}"); + } + } + #[test] fn override_used_when_set_and_absolute() { let dirs = dirs(); diff --git a/packages/app-lib/src/util/io.rs b/packages/app-lib/src/util/io.rs index 6b6fa57..1ac23d0 100644 --- a/packages/app-lib/src/util/io.rs +++ b/packages/app-lib/src/util/io.rs @@ -60,6 +60,34 @@ pub(crate) fn io_error_with_lock_info( io_error_with_lock_info_for_paths(source, path, &[path]) } +/// Joins an untrusted Unix-style relative file path, rejecting traversal and +/// Windows aliases on every platform. This is a lexical check, not a symlink +/// sandbox. +pub(crate) fn safe_relative_join( + root: &Path, + relative: &str, +) -> crate::Result { + path_util::SafeRelativeUtf8UnixPathBuf::try_from(relative.to_string()) + .map_err(|error| { + crate::ErrorKind::InputError(format!( + "Invalid file path {relative}: {error}" + )) + })?; + if relative.split('/').any(|segment| { + segment.is_empty() + || segment == "." + || segment.ends_with(['.', ' ']) + || segment.contains(':') + || segment.chars().any(char::is_control) + }) { + return Err(crate::ErrorKind::InputError(format!( + "Invalid file path: {relative}" + )) + .into()); + } + Ok(root.join(relative)) +} + /// Join a user-provided relative path onto `root`, rejecting traversal. pub(crate) fn join_within_root( root: &Path, @@ -95,7 +123,7 @@ pub(crate) fn join_within_root( ) .into()); } - Ok(root.join(safe)) + safe_relative_join(root, &safe.to_string_lossy().replace('\\', "/")) } pub(crate) fn io_error_with_lock_info_for_paths( @@ -895,6 +923,39 @@ mod windows_sharing_violation_tests { } } +#[cfg(test)] +mod path_join_tests { + use super::*; + + #[test] + fn join_within_root_uses_portable_path_validation() { + let root = Path::new("root"); + assert_eq!( + join_within_root(root, r"config\example.toml").unwrap(), + root.join("config/example.toml") + ); + assert_eq!( + join_within_root(root, "./config/./example.toml").unwrap(), + root.join("config/example.toml") + ); + for relative in [ + "../outside", + r"..\outside", + "/outside", + "C:/outside", + "C:outside", + r"\\server\share", + "config/.. /outside", + "config/file:stream", + "config/NUL", + "config/file.", + "config/file\0", + ] { + assert!(join_within_root(root, relative).is_err(), "{relative:?}"); + } + } +} + #[cfg(test)] mod copy_dir_tests { use super::*;