fix(security): backport upstream modpack path traversal fixes
Backport Mystic-Stars/Axolotl b2f81391e584aebf3ef07e8db2fc9cdad2ffb4c7. Adapt validation to this fork and protect CurseForge archive paths and MRPack uninstall cleanup.
This commit is contained in:
1 parent
598fb5c679
commit
159f9e3f38
12 files changed
+545
-153
No files matched your search
@@ -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<String> {
|
||||
@@ -124,44 +126,7 @@ pub(super) async fn download_to_dir(
|
||||
|
||||
async fn resolve_server_file(server_id: &str, file: &str) -> Result<PathBuf> {
|
||||
let dir = server_path(server_id).await?;
|
||||
safe_join(&dir, file)
|
||||
}
|
||||
|
||||
fn safe_join(dir: &Path, file: &str) -> Result<PathBuf> {
|
||||
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<PathBuf> {
|
||||
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]
|
||||
|
||||
@@ -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?;
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -118,7 +118,7 @@ pub(super) async fn server_path(server_id: &str) -> Result<PathBuf> {
|
||||
.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}"
|
||||
|
||||
@@ -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<MrpackEnv>,
|
||||
#[serde(default)]
|
||||
downloads: Vec<String>,
|
||||
#[serde(default)]
|
||||
#[serde(default, rename = "fileSize")]
|
||||
file_size: Option<u64>,
|
||||
}
|
||||
|
||||
impl MrpackFile {
|
||||
fn integrity(&self) -> Result<Integrity> {
|
||||
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<String>, 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::<Result<_>>()?;
|
||||
|
||||
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<PathBuf> {
|
||||
)
|
||||
})
|
||||
.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<String>,
|
||||
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<HashSet<String>> {
|
||||
let candidates: Vec<(String, String, Option<String>)> = 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::<Result<_>>()?;
|
||||
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<String>,
|
||||
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 {
|
||||
|
||||
Reference in new issue
Block a user