From 2c5e9155ab07e6fc7868db66b82cea8506cdb2d2 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 4 Nov 2026 15:24:27 +0000 Subject: [PATCH] Never resolve a bare name to a batch file on the overlay PATH [patch] Since #137, a bare name with no extension on Windows was tried with every PATHEXT extension, so `npm` resolved to `npm.cmd`. CreateProcess starts a batch file through cmd.exe, which re-parses the command line, and the argument-vector escaping follows CommandLineToArgvW rather than cmd.exe's rules, so `&`, `|` or `%` in an argument ran as commands (the BatBadBut class, CVE-2024-24576). A name with no extension is now tried with `.exe` only, which is all CreateProcess adds, restoring the pre-#137 set of files a bare name can start. A batch file named with its extension is still the caller's choice. The README says so and warns against passing it untrusted arguments. The PATHEXT test is replaced by one that a bare name resolves to tool.exe and one that it never resolves to tool.cmd, tool.bat or tool.com while an explicit tool.cmd still does. Fixes #138 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017vfJW5DyciGEV4wZvik3pQ --- README.md | 2 +- RunCommand.Test/RunCommandTests.cs | 36 +++++++++++++++++++++--------- RunCommand/RunCommand.cs | 31 ++++++++++++------------- 3 files changed, 40 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index d86b895..54d8ae8 100644 --- a/README.md +++ b/README.md @@ -235,7 +235,7 @@ EnvironmentVariables = new Dictionary Environment variables are the only control surface some tools expose, so this covers behaviour with no command-line equivalent — `GIT_TERMINAL_PROMPT=0` to make an authenticating `git fetch` fail rather than block forever on a prompt no terminal will answer, `GIT_ASKPASS`/`SSH_ASKPASS` to supply credentials without putting them on a command line where any process listing can read them, and `LC_ALL=C` to force stable, machine-parseable output rather than whatever the host locale produces. -When the overlay sets `PATH`, a bare executable name such as `node` or `dotnet` is looked up on that `PATH`, so an overlay can pin which toolchain runs. The first directory holding the name wins. Relative `PATH` entries are skipped. On Windows, a name with no extension is tried with each `PATHEXT` extension. On other platforms the file must have an execute bit. A name with a directory part is a path rather than a name to search for, so it is never looked up. If the overlay `PATH` holds no match, or the overlay leaves `PATH` alone or removes it with `null`, the name is found the usual way, on the calling process's `PATH`. +When the overlay sets `PATH`, a bare executable name such as `node` or `dotnet` is looked up on that `PATH`, so an overlay can pin which toolchain runs. The first directory holding the name wins. Relative `PATH` entries are skipped. On Windows, a name with no extension is tried with `.exe` only, the one extension `CreateProcess` adds, so a bare name such as `npm` never starts `npm.cmd`. A batch file runs through `cmd.exe`, which re-parses the command line and would run `&`, `|` or `%` in an argument as commands; name the batch file with its extension if you mean to start one, and do not pass it untrusted arguments. On other platforms the file must have an execute bit. A name with a directory part is a path rather than a name to search for, so it is never looked up. If the overlay `PATH` holds no match, or the overlay leaves `PATH` alone or removes it with `null`, the name is found the usual way, on the calling process's `PATH`. > **_NOTE:_** _`EnvironmentVariables` cannot be combined with `Elevation.Elevated` on Windows. Elevation requires `UseShellExecute`, which offers nowhere to pass an environment, so the call throws `ArgumentException` rather than silently dropping the variables._ diff --git a/RunCommand.Test/RunCommandTests.cs b/RunCommand.Test/RunCommandTests.cs index 3a202a5..4f19da8 100644 --- a/RunCommand.Test/RunCommandTests.cs +++ b/RunCommand.Test/RunCommandTests.cs @@ -960,16 +960,30 @@ public void FindOverlayPathShouldMatchTheKeyIgnoringCaseOnlyOnWindows() } [TestMethod] - public void ResolveOnSearchPathShouldApplyPathExtOnWindows() + public void ResolveOnSearchPathShouldAddExeToANameWithNoExtensionOnWindows() { string directory = CreateDirectoryForTest(); - string name = $"tool-{nameof(ResolveOnSearchPathShouldApplyPathExtOnWindows)}"; - WriteScript(directory, $"{name}.CMD", "exit 0", executable: true); - Dictionary environment = new() { ["PATHEXT"] = ".EXE;.CMD" }; + string name = $"tool-{nameof(ResolveOnSearchPathShouldAddExeToANameWithNoExtensionOnWindows)}"; + WriteScript(directory, $"{name}.exe", "exit 0", executable: true); - string? resolved = RunCommand.ResolveOnSearchPath(name, directory, environment, isWindows: true); + string? resolved = RunCommand.ResolveOnSearchPath(name, directory, isWindows: true); - Assert.AreEqual(Path.Join(directory, $"{name}.CMD"), resolved); + Assert.AreEqual(Path.Join(directory, $"{name}.exe"), resolved); + } + + [TestMethod] + public void ResolveOnSearchPathShouldNeverResolveABareNameToABatchFileOnWindows() + { + // cmd.exe re-parses a batch file's command line, so a bare name that reached tool.cmd would let & or | in an + // argument run a second command (ktsu-dev/RunCommand#138). CreateProcess only ever adds .exe to such a name. + string directory = CreateDirectoryForTest(); + string name = $"tool-{nameof(ResolveOnSearchPathShouldNeverResolveABareNameToABatchFileOnWindows)}"; + WriteScript(directory, $"{name}.cmd", "exit 0", executable: true); + WriteScript(directory, $"{name}.bat", "exit 0", executable: true); + WriteScript(directory, $"{name}.com", "exit 0", executable: true); + + Assert.IsNull(RunCommand.ResolveOnSearchPath(name, directory, isWindows: true)); + Assert.AreEqual(Path.Join(directory, $"{name}.cmd"), RunCommand.ResolveOnSearchPath($"{name}.cmd", directory, isWindows: true), "A batch file named with its extension is the caller's explicit choice."); } [TestMethod] @@ -978,9 +992,9 @@ public void ResolveOnSearchPathShouldLeaveANameWithADirectoryPartAlone() string directory = CreateDirectoryForTest(); WriteScript(directory, "tool", "exit 0", executable: true); - Assert.IsNull(RunCommand.ResolveOnSearchPath("./tool", directory, new Dictionary(), isWindows: false)); - Assert.IsNull(RunCommand.ResolveOnSearchPath(@"bin\tool", directory, new Dictionary(), isWindows: true)); - Assert.IsNull(RunCommand.ResolveOnSearchPath("C:tool", directory, new Dictionary(), isWindows: true)); + Assert.IsNull(RunCommand.ResolveOnSearchPath("./tool", directory, isWindows: false)); + Assert.IsNull(RunCommand.ResolveOnSearchPath(@"bin\tool", directory, isWindows: true)); + Assert.IsNull(RunCommand.ResolveOnSearchPath("C:tool", directory, isWindows: true)); } [TestMethod] @@ -991,8 +1005,8 @@ public void ResolveOnSearchPathShouldSkipRelativeAndMissingEntries() WriteScript(directory, name, "exit 0", executable: true); string searchPath = string.Join(Path.PathSeparator, "relative", Path.Join(directory, "missing"), "", directory); - Assert.AreEqual(Path.Join(directory, name), RunCommand.ResolveOnSearchPath(name, searchPath, new Dictionary(), isWindows: false)); - Assert.IsNull(RunCommand.ResolveOnSearchPath(name, "relative", new Dictionary(), isWindows: false)); + Assert.AreEqual(Path.Join(directory, name), RunCommand.ResolveOnSearchPath(name, searchPath, isWindows: false)); + Assert.IsNull(RunCommand.ResolveOnSearchPath(name, "relative", isWindows: false)); } private static void SkipOnWindows() diff --git a/RunCommand/RunCommand.cs b/RunCommand/RunCommand.cs index 2e538d5..f22f210 100644 --- a/RunCommand/RunCommand.cs +++ b/RunCommand/RunCommand.cs @@ -413,7 +413,7 @@ private static ProcessStartInfo CreateStartInfo(string fileName, OutputHandler o string? overlayPath = FindOverlayPath(options.EnvironmentVariables, isWindows); if (overlayPath is not null) { - startInfo.FileName = ResolveOnSearchPath(startInfo.FileName, overlayPath, startInfo.Environment, isWindows) ?? startInfo.FileName; + startInfo.FileName = ResolveOnSearchPath(startInfo.FileName, overlayPath, isWindows) ?? startInfo.FileName; } } @@ -456,8 +456,7 @@ private static ProcessStartInfo CreateStartInfo(string fileName, OutputHandler o /// /// The executable as the caller gave it. /// The overlay's PATH. - /// The command's environment, which supplies PATHEXT on Windows. - /// Whether to try the PATHEXT extensions and accept a backslash as a separator. + /// Whether to add .exe to a name with no extension and accept a backslash as a separator. /// The full path of the first match, or to keep the usual lookup. /// /// With off, Process.Start searches the calling process's @@ -465,8 +464,15 @@ private static ProcessStartInfo CreateStartInfo(string fileName, OutputHandler o /// PATH at a pinned toolchain either failed to start a tool only it holds or started a same-named tool from /// the caller's PATH. A name with a directory part is a path rather than a name to search for, so it is /// left alone, and so are relative PATH entries, which would otherwise depend on the caller's directory. + /// + /// On Windows a name with no extension is tried with .exe only, which is all CreateProcess appends, + /// rather than with every PATHEXT extension. A bare name therefore never resolves to a .bat or + /// .cmd file: CreateProcess runs those through cmd.exe, which re-parses the command line, so + /// &, | or % in an argument would run as commands although the argument vector is escaped + /// for CommandLineToArgvW. A caller that wants a batch file names it with its extension. + /// /// - internal static string? ResolveOnSearchPath(string fileName, string searchPath, IDictionary environment, bool isWindows) + internal static string? ResolveOnSearchPath(string fileName, string searchPath, bool isWindows) { bool hasDirectoryPart = fileName.Contains('/') || (isWindows && (fileName.Contains('\\') || fileName.Contains(':'))); if (fileName.Length == 0 || hasDirectoryPart) @@ -474,23 +480,14 @@ private static ProcessStartInfo CreateStartInfo(string fileName, OutputHandler o return null; } - string[] candidates = [fileName]; - if (isWindows && !Path.HasExtension(fileName)) - { - _ = environment.TryGetValue("PATHEXT", out string? pathExt); - string[] extensions = (pathExt ?? ".COM;.EXE;.BAT;.CMD").Split([';'], StringSplitOptions.RemoveEmptyEntries); - candidates = Array.ConvertAll(extensions, extension => fileName + extension.Trim()); - } + string candidate = isWindows && !Path.HasExtension(fileName) ? fileName + ".exe" : fileName; foreach (string directory in searchPath.Split([Path.PathSeparator], StringSplitOptions.RemoveEmptyEntries)) { - foreach (string candidate in candidates) + string? match = TryExecutable(directory.Trim().Trim('"'), candidate); + if (match is not null) { - string? match = TryExecutable(directory.Trim().Trim('"'), candidate); - if (match is not null) - { - return match; - } + return match; } }