diff --git a/CHANGELOG.md b/CHANGELOG.md index 221c1eb..474e4d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,15 @@ # Changelog +## 0.9.9 - 2026-04-05 + +### Fixed + +- Potential path traversal attack in extract subcommand +- Empty MPQ archive name when directory supplied with trailing slash +- Bug where MPQ internal files were added with create command +- Signature being added automatically when not requested +- Potential DWORD overflow in `AddFiles` + ## 0.9.8 - 2026-03-22 ### Added diff --git a/CMakeLists.txt b/CMakeLists.txt index 5145361..650f5c5 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,6 +1,6 @@ cmake_minimum_required(VERSION 3.10) -project(MPQCLI VERSION 0.9.8) +project(MPQCLI VERSION 0.9.9) # Options option(BUILD_MPQCLI "Build the mpqcli CLI app" ON) diff --git a/README.md b/README.md index cd1f439..cdd6736 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ ![Release Version](https://img.shields.io/github/v/release/TheGrayDot/mpqcli?style=flat) -![Release downloads](https://img.shields.io/github/downloads/thegraydot/mpqcli/total?label=release_downloads) ![Package downloads](https://img.shields.io/badge/package_downloads-262-green) +![Release downloads](https://img.shields.io/github/downloads/thegraydot/mpqcli/total?label=release_downloads) ![Package downloads](https://img.shields.io/badge/package_downloads-353-green) A command-line tool to create, add, remove, list, extract, read, and verify MPQ archives using the [StormLib library](https://github.com/ladislav-zezula/StormLib). diff --git a/extern/CLI11 b/extern/CLI11 index fe3772d..74d72dd 160000 --- a/extern/CLI11 +++ b/extern/CLI11 @@ -1 +1 @@ -Subproject commit fe3772d3c2969330ed0e4f32351ad066e8d375c5 +Subproject commit 74d72dd38f7ccb122c9439c79d6c2c294d10f3bc diff --git a/extern/StormLib b/extern/StormLib index 3b74456..570734e 160000 --- a/extern/StormLib +++ b/extern/StormLib @@ -1 +1 @@ -Subproject commit 3b744568033588d1e7b395e80fac86459f75f3e6 +Subproject commit 570734e0ac4b1389361f1be3d1ec98f7235beea2 diff --git a/src/gamerules.h b/src/gamerules.h index d4976cf..55b6dc8 100644 --- a/src/gamerules.h +++ b/src/gamerules.h @@ -86,7 +86,7 @@ struct MpqCreateSettings { streamFlags(STREAM_PROVIDER_FLAT | BASE_PROVIDER_FILE), fileFlags1(MPQ_FILE_DEFAULT_INTERNAL), fileFlags2(0), - fileFlags3(MPQ_FILE_DEFAULT_INTERNAL), + fileFlags3(0), attrFlags(0), sectorSize(0x1000), rawChunkSize(0) {} diff --git a/src/main.cpp b/src/main.cpp index 591e77f..e97d72b 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -245,6 +245,11 @@ int main(int argc, char **argv) { outputFilePath = fs::absolute(baseOutput); } else { outputFilePath = fs::path(baseTarget); + // If the path ends with a separator (e.g. "dir/"), strip the + // trailing separator first so we get "dir.mpq" + if (outputFilePath.filename().empty()) { + outputFilePath = outputFilePath.parent_path(); + } outputFilePath.replace_extension(".mpq"); } std::string outputFile = outputFilePath.u8string(); diff --git a/src/mpq.cpp b/src/mpq.cpp index 01f9cbc..9b69222 100644 --- a/src/mpq.cpp +++ b/src/mpq.cpp @@ -15,6 +15,12 @@ namespace fs = std::filesystem; +static const std::vector kSpecialMpqFiles = { + "(listfile)", + "(signature)", + "(attributes)" +}; + int OpenMpqArchive(const std::string &filename, HANDLE *hArchive, int32_t flags) { if (!SFileOpenArchive(filename.c_str(), 0, flags, hArchive)) { std::cerr << "[!] Failed to open: " << filename << std::endl; @@ -113,6 +119,18 @@ int ExtractFile(HANDLE hArchive, const std::string& output, const std::string& f // Ensure sub-directories for folder-nested files exist fs::path outputFilePathName = outputPathBase / fileNameString; + + // Guard against path traversal attacks: resolve any ".." components and verify + // the output path is a descendant of the intended base directory + fs::path resolvedOutput = fs::weakly_canonical(outputFilePathName); + if (std::mismatch(outputPathBase.begin(), outputPathBase.end(), + resolvedOutput.begin(), resolvedOutput.end()).first + != outputPathBase.end()) { + std::cerr << "[!] Blocked: path traversal attempt detected: " + << fileNameString << std::endl; + return 1; + } + std::string outputFileName{outputFilePathName.u8string()}; std::filesystem::create_directories(outputFilePathName.parent_path()); @@ -185,6 +203,12 @@ int AddFiles(HANDLE hArchive, const std::string& inputPath, LCID locale, const G // Normalise path for MPQ std::string archiveFilePath = WindowsifyFilePath(inputFilePath.u8string()); + // Skip special MPQ files that StormLib manages automatically + if (std::find(kSpecialMpqFiles.begin(), kSpecialMpqFiles.end(), archiveFilePath) != kSpecialMpqFiles.end()) { + std::cout << "[*] Skipping special MPQ file: " << archiveFilePath << std::endl; + continue; + } + AddFile(hArchive, entry.path().u8string(), archiveFilePath, locale, gameRules, overrides, false); } } @@ -239,7 +263,13 @@ int AddFile( } // Get file size for rule matching - const auto fileSize = static_cast(fs::file_size(localFile)); + const std::uintmax_t rawFileSize = fs::file_size(localFile); + if (rawFileSize > std::numeric_limits::max()) { + std::cerr << "[!] Warning: file exceeds 4GB, size-based compression rules may not apply correctly: " + << localFile << std::endl; + } + const DWORD fileSize = static_cast( + std::min(rawFileSize, static_cast(std::numeric_limits::max()))); // Get game-specific rules auto [flags, compressionFirst, compressionNext] = @@ -334,19 +364,11 @@ int ListFiles(HANDLE hArchive, const std::string& listfileName, bool listAll, bo listDetailed = true; // If the user specified properties, we need to print the detailed output } - // "Special" files are base files used by MPQ file format - // These are skipped, unless "-a" or "--all" are specified - std::vector specialFiles = { - "(listfile)", - "(signature)", - "(attributes)" - }; - std::set seenFileNames; // Used to prevent printing the same file name multiple times // Loop through all files in the MPQ archive do { // Skip special files unless user wants to list all (like ls -a) - if (!listAll && std::find(specialFiles.begin(), specialFiles.end(), findData.cFileName) != specialFiles.end()) { + if (!listAll && std::find(kSpecialMpqFiles.begin(), kSpecialMpqFiles.end(), findData.cFileName) != kSpecialMpqFiles.end()) { continue; } diff --git a/test/conftest.py b/test/conftest.py index 322c221..d68f000 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -167,6 +167,44 @@ def generate_mpq_without_internal_listfile(binary_path): assert result.returncode == 0, f"mpqcli failed with error: {result.stderr}" +@pytest.fixture(scope="function") +def generate_path_traversal_mpq(binary_path): + script_dir = Path(__file__).parent + + data_dir = script_dir / "data" + data_dir.mkdir(parents=True, exist_ok=True) + + traversal_files_dir = data_dir / "traversal_files" + shutil.rmtree(traversal_files_dir, ignore_errors=True) + traversal_files_dir.mkdir(parents=True, exist_ok=True) + + mpq_file = data_dir / "mpq_with_path_traversal.mpq" + mpq_file.unlink(missing_ok=True) + + safe_file = traversal_files_dir / "safe.txt" + safe_file.write_text("This is a safe file.\n", newline="\n") + + # Create a base MPQ containing the safe file + result = subprocess.run( + [str(binary_path), "create", "-o", str(mpq_file), str(traversal_files_dir)], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True + ) + assert result.returncode == 0, f"mpqcli failed with error: {result.stderr}" + + # Embed a second entry whose archive path traverses above the output directory + result = subprocess.run( + [str(binary_path), "add", str(safe_file), str(mpq_file), "-p", "../../sneaky.txt"], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True + ) + assert result.returncode == 0, f"mpqcli failed with error: {result.stderr}" + + yield mpq_file + + @pytest.fixture(scope="session") def download_test_files(): script_dir = Path(__file__).parent diff --git a/test/test_create.py b/test/test_create.py index 9674314..e75528f 100644 --- a/test/test_create.py +++ b/test/test_create.py @@ -536,6 +536,137 @@ def test_deletion_marker_only_for_zero_size_files(binary_path): output_file.unlink(missing_ok=True) +def test_create_mpq_no_sign_flag_has_no_signature(binary_path, generate_test_files): + """ + Test that a newly created archive without --sign does not have a weak signature slot. + + This test checks: + - If the MPQ archive is created successfully. + - If the signature type is None (no signature slot pre-allocated). + """ + _ = generate_test_files + script_dir = Path(__file__).parent + target_dir = script_dir / "data" / "files" + + output_file = script_dir / "data" / "mpq_no_sign.mpq" + output_file.unlink(missing_ok=True) + + result = subprocess.run( + [str(binary_path), "create", str(target_dir), "-o", str(output_file)], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True + ) + + assert result.returncode == 0, f"mpqcli failed with error: {result.stderr}" + assert output_file.exists(), "MPQ file was not created" + + info = subprocess.run( + [str(binary_path), "info", "-p", "signature-type", str(output_file)], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True + ) + + assert info.returncode == 0, f"mpqcli info failed with error: {info.stderr}" + assert info.stdout.strip() == "None", f"Expected signature type 'None', got: {info.stdout.strip()!r}" + + output_file.unlink(missing_ok=True) + + +def test_create_mpq_directory_with_trailing_slash(binary_path, generate_test_files): + """ + Test MPQ archive creation when the target directory has a trailing slash. + + Regression test for: mpqcli create dir/ producing "dir/.mpq" instead of "dir.mpq". + + This test checks: + - The archive is named after the directory, not "/.mpq". + - The archive is created successfully and is non-empty. + """ + _ = generate_test_files + script_dir = Path(__file__).parent + # Construct the path string with an explicit trailing slash + target_dir_str = str(script_dir / "data" / "files") + "/" + + expected_output = script_dir / "data" / "files.mpq" + malformed_output = script_dir / "data" / "files" / ".mpq" + + # Clean up from any previous run + expected_output.unlink(missing_ok=True) + malformed_output.unlink(missing_ok=True) + + result = subprocess.run( + [str(binary_path), "create", target_dir_str], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True + ) + + assert result.returncode == 0, f"mpqcli failed with error: {result.stderr}" + assert expected_output.exists(), ( + f"Expected archive '{expected_output}' was not created. " + f"Malformed path exists: {malformed_output.exists()}" + ) + assert expected_output.stat().st_size > 0, "MPQ file is empty" + assert not malformed_output.exists(), ( + f"Malformed archive path '{malformed_output}' was created — trailing slash bug is present" + ) + + expected_output.unlink(missing_ok=True) + + +def test_create_mpq_skips_special_files(binary_path, tmp_path): + """ + Test that special MPQ files in the source directory are skipped during archive creation. + + This test checks: + - That (listfile), (signature), and (attributes) are not added as regular files. + - That the signature type is None after creation (no false signature from (signature) file). + - That the special files do not appear in the archive file listing. + """ + source_dir = tmp_path / "src" + source_dir.mkdir() + (source_dir / "readme.txt").write_text("hello") + + # Plant all three special files in the source directory + for name in ["(listfile)", "(signature)", "(attributes)"]: + (source_dir / name).write_bytes(b"fake content") + + output_file = tmp_path / "output.mpq" + + result = subprocess.run( + [str(binary_path), "create", str(source_dir), "-o", str(output_file)], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True + ) + + assert result.returncode == 0, f"mpqcli failed with error: {result.stderr}" + assert output_file.exists(), "MPQ file was not created" + + # Signature type must be None — (signature) must not have been ingested + info = subprocess.run( + [str(binary_path), "info", "-p", "signature-type", str(output_file)], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True + ) + assert info.returncode == 0, f"mpqcli info failed with error: {info.stderr}" + assert info.stdout.strip() == "None", f"Expected signature type 'None', got: {info.stdout.strip()!r}" + + # Special files must not appear in the regular file listing + listing = subprocess.run( + [str(binary_path), "list", str(output_file)], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True + ) + assert listing.returncode == 0, f"mpqcli list failed with error: {listing.stderr}" + for name in ["(listfile)", "(signature)", "(attributes)"]: + assert name not in listing.stdout, f"Special file {name!r} unexpectedly found in archive listing" + + def verify_archive_file_content(binary_path, test_file, expected_output): result = subprocess.run( [str(binary_path), "list", str(test_file), "-d", "-p", "locale"], diff --git a/test/test_extract.py b/test/test_extract.py index 0dafdde..497d165 100644 --- a/test/test_extract.py +++ b/test/test_extract.py @@ -1,3 +1,4 @@ +import os import shutil import subprocess from pathlib import Path @@ -412,7 +413,6 @@ def test_extract_all_files_from_mpq_without_providing_listfile(binary_path, gene expected_output = { "File00000000.xxx", - "File00000003.xxx", } result = subprocess.run( @@ -459,7 +459,6 @@ def test_extract_all_files_from_mpq_without_providing_listfile_and_with_given_lo expected_output = { "File00000000.xxx", "File00000002.xxx", - "File00000003.xxx", } result = subprocess.run( @@ -550,7 +549,6 @@ def test_extract_all_files_from_mpq_providing_partial_external_listfile(binary_p expected_output = { - "(signature)", "capybaras.txt", } @@ -599,7 +597,6 @@ def test_extract_all_files_from_mpq_providing_partial_external_listfile_and_with expected_output = { "File00000000.xxx", - "(signature)", "cats.txt", } @@ -647,7 +644,6 @@ def test_extract_all_files_from_mpq_providing_complete_external_listfile(binary_ expected_output = { - "(signature)", "capybaras.txt", } @@ -695,7 +691,6 @@ def test_extract_all_files_from_mpq_providing_complete_external_listfile_and_wit expected_output = { - "(signature)", "dogs.txt", "capybaras.txt", } @@ -722,3 +717,47 @@ def test_extract_all_files_from_mpq_providing_complete_external_listfile_and_wit assert output_lines == expected_lines, f"Unexpected output: {output_lines}" assert output_file.exists(), "Output directory was not created" assert output_files == expected_output, f"Unexpected files: {output_files}" + + +def test_extract_path_traversal_is_blocked(binary_path, generate_path_traversal_mpq): + """ + Test that extracting an MPQ containing a path traversal filename is blocked (zip-slip). + + This test checks: + - That the traversal entry is not written outside the output directory. + - That the extraction of the traversal entry is reported as blocked on stderr. + - That safe files in the same archive are still extracted correctly. + - That the overall exit code is non-zero. + """ + test_file = generate_path_traversal_mpq + script_dir = Path(__file__).parent + output_dir = script_dir / "data" / "extracted_traversal" + if output_dir.exists(): + shutil.rmtree(output_dir) + + result = subprocess.run( + [str(binary_path), "extract", "-o", str(output_dir), str(test_file)], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True + ) + + expected_stdout = { + "[*] Extracted: safe.txt", + "[*] Extracted: (listfile)", + } + expected_stderr = { + "[!] Blocked: path traversal attempt detected: " + os.path.normpath("../../sneaky.txt"), + "", + "[!] Failed to extract all files.", + } + + stdout_lines = set(result.stdout.splitlines()) + stderr_lines = set(result.stderr.splitlines()) + + assert result.returncode == 1, f"Expected non-zero exit for traversal attempt, got: {result.returncode}" + assert stdout_lines == expected_stdout, f"Unexpected stdout: {stdout_lines}" + assert stderr_lines == expected_stderr, f"Unexpected stderr: {stderr_lines}" + + # Confirm no file escaped outside the intended output directory + assert not (output_dir.parent / "sneaky.txt").exists(), "Path traversal was not blocked: sneaky.txt escaped" diff --git a/test/test_list.py b/test/test_list.py index c563517..8d3fd1c 100644 --- a/test/test_list.py +++ b/test/test_list.py @@ -157,7 +157,6 @@ def test_list_mpq_without_providing_listfile(binary_path, generate_mpq_without_i "File00000000.xxx", "File00000001.xxx", "File00000002.xxx", - "File00000003.xxx", } result = subprocess.run( [str(binary_path), "list", str(test_file)], @@ -175,7 +174,6 @@ def test_list_mpq_without_providing_listfile(binary_path, generate_mpq_without_i "File00000000.xxx", "File00000001.xxx", "File00000002.xxx", - "File00000003.xxx", } result = subprocess.run( [str(binary_path), "list", "-a", str(test_file)], @@ -194,14 +192,12 @@ def test_list_mpq_without_providing_listfile(binary_path, generate_mpq_without_i " 31 enUS File00000000.xxx", " 33 deDE File00000001.xxx", " 27 041D File00000002.xxx", - " 72 enUS File00000003.xxx", } else: expected_output = { " 31 enUS File00000000.xxx", " 32 deDE File00000001.xxx", " 26 041D File00000002.xxx", - " 72 enUS File00000003.xxx", } result = subprocess.run( @@ -253,7 +249,6 @@ def test_list_mpq_providing_partial_external_listfile(binary_path, generate_mpq_ "File00000000.xxx", "File00000002.xxx", "cats.txt", - "(signature)", } result = subprocess.run( [str(binary_path), "list", "-a", str(test_file), "--listfile", str(listfile)], @@ -272,14 +267,12 @@ def test_list_mpq_providing_partial_external_listfile(binary_path, generate_mpq_ " 31 enUS File00000000.xxx", " 27 041D File00000002.xxx", " 33 deDE cats.txt", - " 72 enUS (signature)", } else: expected_output = { " 31 enUS File00000000.xxx", " 26 041D File00000002.xxx", " 32 deDE cats.txt", - " 72 enUS (signature)", } result = subprocess.run( [str(binary_path), "list", "-ad", str(test_file), "--listfile", str(listfile)], @@ -329,7 +322,6 @@ def test_list_mpq_providing_complete_external_listfile(binary_path, generate_mpq "dogs.txt", "cats.txt", "capybaras.txt", - "(signature)", } result = subprocess.run( [str(binary_path), "list", "-a", str(test_file), "--listfile", str(listfile)], @@ -348,14 +340,12 @@ def test_list_mpq_providing_complete_external_listfile(binary_path, generate_mpq " 31 enUS capybaras.txt", " 33 deDE cats.txt", " 27 041D dogs.txt", - " 72 enUS (signature)", } else: expected_output = { " 31 enUS capybaras.txt", " 32 deDE cats.txt", " 26 041D dogs.txt", - " 72 enUS (signature)", } result = subprocess.run(