diff --git a/doc/ReleaseNotes.md b/doc/ReleaseNotes.md index 4b81aabb85..e5be826943 100644 --- a/doc/ReleaseNotes.md +++ b/doc/ReleaseNotes.md @@ -6,3 +6,4 @@ Nothing yet. * Updated NUnit to v4 * Fixed a crash (`0x8000ffff`) when using `--disable-interactivity` with the Resume experimental feature enabled during install operations. +* Fixed relative path handling for rooted paths. diff --git a/src/AppInstallerCLICore/ExecutionContext.h b/src/AppInstallerCLICore/ExecutionContext.h index a966cc2cd6..d4cfd96fe7 100644 --- a/src/AppInstallerCLICore/ExecutionContext.h +++ b/src/AppInstallerCLICore/ExecutionContext.h @@ -45,6 +45,9 @@ // Returns if the context is terminated. #define AICLI_RETURN_IF_TERMINATED(_context_) if ((_context_).IsTerminated()) { return; } +// Returns the specified value if the context is terminated. +#define AICLI_RETURN_VALUE_IF_TERMINATED(_context_,_ret_) if ((_context_).IsTerminated()) { return _ret_; } + namespace AppInstaller::CLI { struct Command; diff --git a/src/AppInstallerCLICore/Workflows/PortableFlow.cpp b/src/AppInstallerCLICore/Workflows/PortableFlow.cpp index 7d58b77110..c14cada640 100644 --- a/src/AppInstallerCLICore/Workflows/PortableFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/PortableFlow.cpp @@ -7,6 +7,7 @@ #include #include #include +#include using namespace AppInstaller::Manifest; using namespace AppInstaller::Repository; @@ -68,6 +69,16 @@ namespace AppInstaller::CLI::Workflow AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_PORTABLE_REPARSE_POINT_NOT_SUPPORTED); } } + + void EnsurePathIsRelative(Execution::Context& context, const Manifest::string_t& path, std::string_view field, Resource::StringId errorStringId) + { + if (Filesystem::PathEscapesBaseDirectory(path)) + { + AICLI_LOG(CLI, Error, << "File path for [" << field << "] points to a location outside of its base directory: " << path); + context.Reporter.Error() << errorStringId << std::endl; + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); + } + } } void VerifyPackageAndSourceMatch(Execution::Context& context) @@ -206,6 +217,12 @@ namespace AppInstaller::CLI::Workflow for (const auto& nestedInstallerFile : nestedInstallerFiles) { + EnsurePathIsRelative(context, nestedInstallerFile.RelativeFilePath, "RelativeFilePath", ManifestError::RelativeFilePathEscapesDirectory); + AICLI_RETURN_VALUE_IF_TERMINATED(context, {}); + + EnsurePathIsRelative(context, nestedInstallerFile.PortableCommandAlias, "PortableCommandAlias", ManifestError::PortableCommandAliasEscapesDirectory); + AICLI_RETURN_VALUE_IF_TERMINATED(context, {}); + const std::filesystem::path& targetPath = targetInstallDirectory / ConvertToUTF16(nestedInstallerFile.RelativeFilePath); std::filesystem::path commandAlias; @@ -230,6 +247,9 @@ namespace AppInstaller::CLI::Workflow if (!commands.empty()) { + EnsurePathIsRelative(context, commands[0], "CommandAlias", ManifestError::PortableCommandAliasEscapesDirectory); + AICLI_RETURN_VALUE_IF_TERMINATED(context, {}); + commandAlias = ConvertToUTF16(commands[0]); } @@ -257,6 +277,7 @@ namespace AppInstaller::CLI::Workflow context.Reporter.Info() << Resource::String::InstallFlowStartingPackageInstall << std::endl; std::vector desiredState = GetDesiredStateForPortableInstall(context); + AICLI_RETURN_IF_TERMINATED(context); portableInstaller.SetDesiredState(desiredState); diff --git a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj index 093eb5b235..7bd2310985 100644 --- a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj +++ b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj @@ -570,6 +570,9 @@ true + + true + true diff --git a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters index 29debf6a13..5fc2e79a90 100644 --- a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters +++ b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters @@ -522,6 +522,9 @@ TestData + + TestData + TestData diff --git a/src/AppInstallerCLITests/Filesystem.cpp b/src/AppInstallerCLITests/Filesystem.cpp index 12684f19d5..95ea62c493 100644 --- a/src/AppInstallerCLITests/Filesystem.cpp +++ b/src/AppInstallerCLITests/Filesystem.cpp @@ -12,15 +12,69 @@ using namespace TestCommon; TEST_CASE("PathEscapesDirectory", "[filesystem]") { - std::string badRelativePath = "../../target.exe"; - std::string badRelativePath2 = "test/../../target.exe"; - std::string goodRelativePath = "target.exe"; - std::string goodRelativePath2 = "test/../test1/target.exe"; - - REQUIRE(PathEscapesBaseDirectory(badRelativePath)); - REQUIRE(PathEscapesBaseDirectory(badRelativePath2)); - REQUIRE_FALSE(PathEscapesBaseDirectory(goodRelativePath)); - REQUIRE_FALSE(PathEscapesBaseDirectory(goodRelativePath2)); + SECTION("Simple relative paths stay within the base directory") + { + REQUIRE_FALSE(PathEscapesBaseDirectory("target.exe")); + REQUIRE_FALSE(PathEscapesBaseDirectory("test\\target.exe")); + REQUIRE_FALSE(PathEscapesBaseDirectory("test/subdir/target.exe")); + } + + SECTION("Relative paths whose '..' components resolve back inside do not escape") + { + REQUIRE_FALSE(PathEscapesBaseDirectory("test/../test1/target.exe")); + REQUIRE_FALSE(PathEscapesBaseDirectory("./target.exe")); + REQUIRE_FALSE(PathEscapesBaseDirectory("a/b/../../c.exe")); + } + + SECTION("Paths that resolve to the base directory itself do not escape") + { + REQUIRE_FALSE(PathEscapesBaseDirectory(".")); + REQUIRE_FALSE(PathEscapesBaseDirectory("test/..")); + } + + SECTION("An empty path refers to the base directory itself and does not escape") + { + REQUIRE_FALSE(PathEscapesBaseDirectory("")); + } + + SECTION("Relative paths that traverse above the base directory escape") + { + REQUIRE(PathEscapesBaseDirectory("../../target.exe")); + REQUIRE(PathEscapesBaseDirectory("test/../../target.exe")); + REQUIRE(PathEscapesBaseDirectory("../target.exe")); + REQUIRE(PathEscapesBaseDirectory("..")); + REQUIRE(PathEscapesBaseDirectory("a/../../b.exe")); + + // Mixed separators are still normalized correctly. + REQUIRE(PathEscapesBaseDirectory("test\\../..\\target.exe")); + } + + SECTION("Absolute paths escape the base directory") + { + REQUIRE(PathEscapesBaseDirectory("C:\\Windows\\target.exe")); + REQUIRE(PathEscapesBaseDirectory("C:/Windows/target.exe")); + } + + SECTION("UNC paths in their various forms escape the base directory") + { + REQUIRE(PathEscapesBaseDirectory("\\\\server\\share\\target.exe")); + REQUIRE(PathEscapesBaseDirectory("//server/share/target.exe")); + + // Extended-length prefix. + REQUIRE(PathEscapesBaseDirectory("\\\\?\\C:\\target.exe")); + } + + SECTION("Root-relative paths (no drive) resolve to the root of the base directory's drive") + { + REQUIRE(PathEscapesBaseDirectory("\\Windows\\target.exe")); + REQUIRE(PathEscapesBaseDirectory("/Windows/target.exe")); + } + + SECTION("Drive-relative paths resolve against the current directory of the given drive") + { + REQUIRE(PathEscapesBaseDirectory("C:target.exe")); + REQUIRE(PathEscapesBaseDirectory("C:")); + } } TEST_CASE("VerifySymlink", "[filesystem]") diff --git a/src/AppInstallerCLITests/InstallFlow.cpp b/src/AppInstallerCLITests/InstallFlow.cpp index 64fb767bbc..ece6d8ffb1 100644 --- a/src/AppInstallerCLITests/InstallFlow.cpp +++ b/src/AppInstallerCLITests/InstallFlow.cpp @@ -18,7 +18,9 @@ #include #include #include +#include #include +#include using namespace winrt::Windows::Foundation; using namespace TestCommon; @@ -726,6 +728,52 @@ TEST_CASE("InstallFlow_Portable", "[InstallFlow][workflow]") REQUIRE(std::filesystem::exists(portableInstallResultPath.GetPath())); } +TEST_CASE("PortableInstallFlow_RejectsEscapingPathsAtPointOfUse", "[InstallFlow][workflow]") +{ + TestCommon::TempDirectory targetDirectory("TestPortableInstallRoot", false); + TestCommon::TempDirectory extractedDirectory("TestPortableExtractedRoot", true); + TestCommon::TempFile installerFile("TestPortableInstaller.exe"); + + std::ostringstream installOutput; + TestContext context{ installOutput, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + + ManifestInstaller installer; + std::filesystem::path installerPath = installerFile.GetPath(); + + SECTION("Command alias") + { + installer.BaseInstallerType = InstallerTypeEnum::Portable; + installer.Commands = { "C:\\escape" }; + } + + SECTION("Nested installer relative path") + { + installer.BaseInstallerType = InstallerTypeEnum::Zip; + installer.NestedInstallerFiles = { { "C:\\escape", {} } }; + installerPath = extractedDirectory.GetPath(); + } + + SECTION("Nested installer command alias") + { + installer.BaseInstallerType = InstallerTypeEnum::Zip; + installer.NestedInstallerFiles = { { "installer.exe", "C:\\escape" } }; + installerPath = extractedDirectory.GetPath(); + } + + AppInstaller::CLI::Portable::PortableInstaller portableInstaller{ + ScopeEnum::User, Architecture::X64, "TestProductCode" }; + portableInstaller.TargetInstallLocation = targetDirectory.GetPath(); + + context.Add(installer); + context.Add(installerPath); + context.Add(std::move(portableInstaller)); + + PortableInstallImpl(context); + + REQUIRE_TERMINATED_WITH(context, APPINSTALLER_CLI_ERROR_INVALID_MANIFEST); +} + TEST_CASE("InstallFlow_Portable_SymlinkCreationFail", "[InstallFlow][workflow]") { TestCommon::TempDirectory tempDirectory("TestPortableInstallRoot", false); diff --git a/src/AppInstallerCLITests/TestData/Manifest-Bad-InstallerTypePortable-InvalidCommandAlias.yaml b/src/AppInstallerCLITests/TestData/Manifest-Bad-InstallerTypePortable-InvalidCommandAlias.yaml new file mode 100644 index 0000000000..d8d3104a62 --- /dev/null +++ b/src/AppInstallerCLITests/TestData/Manifest-Bad-InstallerTypePortable-InvalidCommandAlias.yaml @@ -0,0 +1,20 @@ +# Bad manifest. A portable installer must not have a command alias outside the base directory. +# yaml-language-server: $schema=https://aka.ms/winget-manifest.singleton.1.2.0.schema.json + +PackageIdentifier: TestInstaller.WithLicenseAgreement +PackageVersion: 1.0.0.0 +PackageLocale: en-US +PackageName: AppInstaller Test Installer +Publisher: Microsoft Corporation +Moniker: AICLITestExe +License: Test +ShortDescription: Test installer for portable with a command alias outside the base directory +Commands: + - ../command-alias +Installers: + - Architecture: x64 + InstallerUrl: https://ThisIsNotUsed + InstallerType: portable + InstallerSha256: 65DB2F2AC2686C7F2FD69D4A4C6683B888DC55BFA20A0E32CA9F838B51689A3B +ManifestType: singleton +ManifestVersion: 1.2.0 diff --git a/src/AppInstallerCLITests/YamlManifest.cpp b/src/AppInstallerCLITests/YamlManifest.cpp index 60f997e782..796c4d15fa 100644 --- a/src/AppInstallerCLITests/YamlManifest.cpp +++ b/src/AppInstallerCLITests/YamlManifest.cpp @@ -878,6 +878,7 @@ TEST_CASE("ReadBadManifests", "[ManifestValidation]") { "Manifest-Bad-InstallerTypeInvalid.yaml", "Invalid field value. [InstallerType]" }, { "Manifest-Bad-InstallerTypeMissing.yaml", "Invalid field value. [InstallerType]" }, { "Manifest-Bad-InstallerTypePortable-InvalidAppsAndFeatures.yaml", "Only zero or one entry for Apps and Features may be specified for InstallerType portable." }, + { "Manifest-Bad-InstallerTypePortable-InvalidCommandAlias.yaml", "Portable command alias must not point to a location outside of base directory." }, { "Manifest-Bad-InstallerTypePortable-InvalidCommands.yaml", "Only zero or one value for Commands may be specified for InstallerType portable." }, { "Manifest-Bad-InstallerTypePortable-InvalidScope.yaml", "Scope is not supported for InstallerType portable." }, { "Manifest-Bad-InstallerTypeZip-DuplicateCommandAlias.yaml", "Duplicate portable command alias found." }, diff --git a/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp b/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp index 57573d151d..bfe866f0ed 100644 --- a/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp +++ b/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp @@ -306,6 +306,14 @@ namespace AppInstaller::Manifest { resultErrors.emplace_back(ManifestError::ExceededCommandsLimit); } + + if (!installer.Commands.empty()) + { + if (AppInstaller::Filesystem::PathEscapesBaseDirectory(installer.Commands[0])) + { + resultErrors.emplace_back(ManifestError::PortableCommandAliasEscapesDirectory, "Commands"); + } + } } if (installer.EffectiveInstallerType() == InstallerTypeEnum::Portable) diff --git a/src/AppInstallerSharedLib/Filesystem.cpp b/src/AppInstallerSharedLib/Filesystem.cpp index 85acbf09a3..90394c6824 100644 --- a/src/AppInstallerSharedLib/Filesystem.cpp +++ b/src/AppInstallerSharedLib/Filesystem.cpp @@ -372,9 +372,25 @@ namespace AppInstaller::Filesystem bool PathEscapesBaseDirectory(std::string_view relativePath) { - // Normalize the path, then check if the first part is ".." - auto resolvedPath = std::filesystem::path{ relativePath }.lexically_normal(); - return !resolvedPath.empty() && *resolvedPath.begin() == ".."; + std::filesystem::path path{ relativePath }; + + // Reject any path that has a root component. This covers absolute paths (e.g. "C:\foo"), + // drive-relative paths (e.g. "C:foo") and root-relative paths (e.g. "\foo"). + if (path.has_root_path()) + { + return true; + } + + // Resolve any "." and ".." components lexically (i.e. without touching the filesystem). + auto resolvedPath = path.lexically_normal(); + + // If the normalized path still begins with "..", it points to a parent of the base directory. + if (!resolvedPath.empty() && *resolvedPath.begin() == "..") + { + return true; + } + + return false; } // Complicated rename algorithm due to somewhat arbitrary failures.