Fix potential unsafety of joining path and building and parsing GAV

Author: theorzr

portablemc/src/base/mod.rs

@@ -608,14 +608,22 @@ impl Installer {
                     // If we find a arch replacement pattern, we must replace it with
                     // the target architecture bit-ness (32, 64).
                     const ARCH_REPLACEMENT_PATTERN: &str = "${arch}";
+                    let new_gav;
                     if let Some(pattern_idx) = classifier.find(ARCH_REPLACEMENT_PATTERN) {
                         let mut classifier = classifier.clone();
                         classifier.replace_range(pattern_idx..pattern_idx + ARCH_REPLACEMENT_PATTERN.len(), os_bits);
-                        lib_gav.set_classifier(Some(&classifier));
+                        new_gav = lib_gav.with_classifier(Some(&classifier));
                     } else {
-                        lib_gav.set_classifier(Some(&classifier));
+                        new_gav = lib_gav.with_classifier(Some(&classifier));
                     }
 
+                    // Ignore that new GAV if the gav cannot be constructed!
+                    let Some(new_gav) = new_gav else {
+                        continue;
+                    };
+
+                    lib_gav = new_gav;
+
                 }
 
                 // Start by applying rules before the actual parsing. Important, we do
@@ -629,8 +637,9 @@ impl Installer {
 
                 // Clone the spec with wildcard for version because we shouldn't override
                 // if any of the group/artifact/classifier/extension are matching.
-                let mut lib_gav_wildcard = lib_gav.clone();
-                lib_gav_wildcard.set_version("*");
+                // Unwrapping because it's a single character version, which could not 
+                // exceed the limits checked by the previous version.
+                let lib_gav_wildcard = lib_gav.with_version("*").unwrap();
                 if !libraries_set.insert(lib_gav_wildcard) {
                     continue;
                 }
@@ -705,8 +714,7 @@ impl Installer {
 
             // Construct the library path depending on its presence.
             let lib_file = if let Some(lib_rel_path) = lib.path.as_deref() {
-                // FIXME: Insecure path joining.
-                self.libraries_dir.join(lib_rel_path)
+                self.libraries_dir.join(check_path_relative_and_safe(lib_rel_path)?)
             } else {
                 lib.name.file(&self.libraries_dir)
             };
@@ -2270,6 +2278,16 @@ struct MojangJvmLink {
     target_file: Box<Path>,
 }
 
+/// Check that the given path does not contain any root or parent directory component.
+pub(crate) fn check_path_relative_and_safe<P: AsRef<Path> + ?Sized>(path: &P) -> Result<&Path> {
+    let path = path.as_ref();
+    if path.is_relative_and_safe() {
+        Ok(path)
+    } else {
+        Err(Error::new_io_file(io::Error::new(io::ErrorKind::InvalidInput, "the is not relative or contains unsafe components"), path))
+    }
+}
+
 /// Check if a file at a given path has the corresponding properties (size and/or SHA-1), 
 /// returning true if it is valid, so false is returned anyway if the file doesn't exists.
 pub(crate) fn check_file(file: &Path, size: Option<u32>, sha1: Option<&[u8; 20]>) -> Result<bool> {

portablemc/src/base/serde.rs

@@ -5,6 +5,7 @@
 
 use std::collections::HashMap;
 use std::fmt;
+use std::path::PathBuf;
 
 use chrono::{DateTime, FixedOffset};
 
@@ -152,7 +153,7 @@ impl VersionLibraryDownloads {
 #[derive(serde::Deserialize, serde::Serialize, Debug, Clone)]
 #[serde(rename_all = "camelCase")]
 pub struct VersionLibraryDownload {
-    pub path: Option<String>,
+    pub path: Option<PathBuf>,
     #[serde(flatten)]
     pub download: Download,
 }

portablemc/src/forge/mod.rs

@@ -157,11 +157,11 @@ impl Installer {
                     let libs_dir = mojang.base().libraries_dir();
 
                     // Start by checking patched client and universal client.
-                    if !check_exists(&config.name.with_classifier(Some("client")).file(libs_dir)) {
+                    if let Some(client_gav) = config.name.with_classifier(Some("client")) && !check_exists(&client_gav.file(libs_dir)) {
                         break InstallReason::MissingPatchedClient;
                     }
 
-                    if !check_exists(&config.name.with_classifier(Some("universal")).file(libs_dir)) {
+                    if let Some(universal_gav) = config.name.with_classifier(Some("universal")) && !check_exists(&universal_gav.file(libs_dir)) {
                         break InstallReason::MissingUniversalClient;
                     }
 
@@ -704,7 +704,7 @@ impl InstallConfig {
 
         Some(Self {
             default_prefix: "forge",
-            name: Gav::new("net.minecraftforge", "forge", name, None, None),
+            name: Gav::new("net.minecraftforge", "forge", name, None, None)?,
             repo_url: "https://maven.minecraftforge.net",
             game_version: if game_version == "1.7.10_pre4" {
                 "1.7.10-pre4".to_string()  // The only pre-release supported.
@@ -746,10 +746,10 @@ impl InstallConfig {
         let game_version;
 
         if name == "47.1.82" || name.starts_with("1.20.1-") {
-            gav = Gav::new("net.neoforged", "forge", name, None, None);
+            gav = Gav::new("net.neoforged", "forge", name, None, None)?;
             game_version = "1.20.1".to_string();
         } else {
-            gav = Gav::new("net.neoforged", "neoforge", name, None, None);
+            gav = Gav::new("net.neoforged", "neoforge", name, None, None)?;
             game_version = match parse_generic_version::<2, 2>(name)? {
                 [major, 0] => format!("1.{major}"),
                 [major, minor] => format!("1.{major}.{minor}"),
@@ -788,7 +788,13 @@ fn try_install(
     // simply no installer for this version!
     handler.on_event(Event::FetchInstaller { version: config.name.version() });
 
-    let installer_gav = config.name.with_classifier(Some("installer"));
+    // Handle the case where the next installer gav could not be constructed, too long?
+    let Some(installer_gav) = config.name.with_classifier(Some("installer")) else {
+        return Err(Error::InstallerNotFound { 
+            version: config.name.version().to_string(),
+        });
+    };
+
     let installer_url = format!("{}/{}", config.repo_url, installer_gav.url());
 
     // Download and check result in case installer is just not found.
@@ -904,8 +910,7 @@ fn try_install(
                 let lib_dl = &lib.downloads.artifact;
 
                 let lib_file = if let Some(lib_path) = &lib_dl.path {
-                    // FIXME: Insecure joining!
-                    libraries_dir.join(lib_path)
+                    libraries_dir.join(base::check_path_relative_and_safe(lib_path)?)
                 } else {
                     lib.name.file(&libraries_dir)
                 };
@@ -949,9 +954,8 @@ fn try_install(
                     }
                     _ => {
                         // This is a file that we should extract to the temp directory.
-                        // FIXME: Insecure joining.
                         let entry = entry.strip_prefix('/').unwrap_or(entry);
-                        let tmp_file = tmp_dir.join(entry);
+                        let tmp_file = tmp_dir.join(base::check_path_relative_and_safe(entry)?);
                         extract_installer_file(installer_file, &mut installer_zip, entry, &tmp_file)?;
                         InstallDataTypedEntry::File(tmp_file)
                     }