Ensure relative paths are rooted

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.

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;

src/AppInstallerCLICore/Workflows/PortableFlow.cpp

@@ -7,6 +7,7 @@
 #include <winget/Filesystem.h>
 #include <winget/PortableFileEntry.h>
 #include <winget/PortableIndex.h>
+#include <winget/ManifestValidation.h>
 
 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<AppInstaller::Portable::PortableFileEntry> desiredState = GetDesiredStateForPortableInstall(context);
+            AICLI_RETURN_IF_TERMINATED(context);
 
             portableInstaller.SetDesiredState(desiredState);
 

src/AppInstallerCLITests/AppInstallerCLITests.vcxproj

@@ -570,6 +570,9 @@
     <CopyFileToFolders Include="TestData\Manifest-Bad-InstallerTypePortable-InvalidAppsAndFeatures.yaml">
       <DeploymentContent>true</DeploymentContent>
     </CopyFileToFolders>
+    <CopyFileToFolders Include="TestData\Manifest-Bad-InstallerTypePortable-InvalidCommandAlias.yaml">
+      <DeploymentContent>true</DeploymentContent>
+    </CopyFileToFolders>
     <CopyFileToFolders Include="TestData\Manifest-Bad-InstallerTypePortable-InvalidCommands.yaml">
       <DeploymentContent>true</DeploymentContent>
     </CopyFileToFolders>

src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters

@@ -522,6 +522,9 @@
     <CopyFileToFolders Include="TestData\Manifest-Bad-InstallerTypePortable-InvalidAppsAndFeatures.yaml">
       <Filter>TestData</Filter>
     </CopyFileToFolders>
+    <CopyFileToFolders Include="TestData\Manifest-Bad-InstallerTypePortable-InvalidCommandAlias.yaml">
+      <Filter>TestData</Filter>
+    </CopyFileToFolders>
     <CopyFileToFolders Include="TestData\Manifest-Bad-InstallerTypePortable-InvalidCommands.yaml">
       <Filter>TestData</Filter>
     </CopyFileToFolders>

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]")

src/AppInstallerCLITests/InstallFlow.cpp

@@ -18,7 +18,9 @@
 #include <Workflows/ArchiveFlow.h>
 #include <Workflows/DownloadFlow.h>
 #include <Workflows/MsiInstallFlow.h>
+#include <Workflows/PortableFlow.h>
 #include <Workflows/ShellExecuteInstallerHandler.h>
+#include <PortableInstaller.h>
 
 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<Execution::Data::Installer>(installer);
+    context.Add<Execution::Data::InstallerPath>(installerPath);
+    context.Add<Execution::Data::PortableInstaller>(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);

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

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." },

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)

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.