From 150191724e6e77d359b6ded92b786721770d49f7 Mon Sep 17 00:00:00 2001 From: Thomas Boop <52323235+thboop@users.noreply.github.com> Date: Tue, 20 Sep 2022 12:30:33 -0400 Subject: [PATCH] 293 Backport for Container Escaping fix (#2133) * Fix escaping of docker envs backport * create as prerelease * add release number Co-authored-by: Nikola Jokic <97525037+nikola-jokic@users.noreply.github.com> --- .github/workflows/release.yml | 5 +- releaseNote.md | 5 +- .../Container/DockerCommandManager.cs | 6 +-- src/Runner.Worker/Container/DockerUtil.cs | 44 ++++++++++++++++- src/Runner.Worker/Handlers/StepHost.cs | 4 +- src/Test/L0/Container/DockerUtilL0.cs | 49 +++++++++++++++++++ src/runnerversion | 2 +- 7 files changed, 102 insertions(+), 13 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index a3ec84623..a753b3eff 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -89,7 +89,7 @@ jobs: - runtime: osx-x64 os: macOS-latest devScript: ./dev.sh - + - runtime: osx-arm64 os: macOS-latest devScript: ./dev.sh @@ -158,7 +158,7 @@ jobs: id: sha_noruntime_noexternals name: Compute SHA256 working-directory: _package_trims/trim_runtime_externals - + - name: Create trimmedpackages.json for ${{ matrix.runtime }} if: matrix.runtime == 'win-x64' uses: actions/github-script@0.3.0 @@ -288,6 +288,7 @@ jobs: release_name: "v${{ steps.releaseNote.outputs.version }}" body: | ${{ steps.releaseNote.outputs.note }} + prerelease: true # Upload release assets (full runner packages) - name: Upload Release Asset (win-x64) diff --git a/releaseNote.md b/releaseNote.md index 0150eddee..b420db971 100644 --- a/releaseNote.md +++ b/releaseNote.md @@ -1,11 +1,8 @@ ## Features -- Allow self-hosted runner admins to fail jobs that don't have a job container (#1895) -- Experimental: Self-hosted runner admins can now use scripts to customize the container invocation in the runner (#1853) ## Bugs -- Fixed an issue where a Job Hook would fail to execute if the shell path contains a space on Windows (#1826) +- Fixed an issue where container environment variables names or values could escape the docker command (#2108) ## Misc -- Handle new `HostedRunnerShutdownMessage` to shutdown hosted runners faster (#1922) ## Windows x64 We recommend configuring the runner in a root folder of the Windows drive (e.g. "C:\actions-runner"). This will help avoid issues related to service identity folder permissions and long file path restrictions on Windows. diff --git a/src/Runner.Worker/Container/DockerCommandManager.cs b/src/Runner.Worker/Container/DockerCommandManager.cs index 86cf0eeed..a0c158bdf 100644 --- a/src/Runner.Worker/Container/DockerCommandManager.cs +++ b/src/Runner.Worker/Container/DockerCommandManager.cs @@ -131,11 +131,11 @@ namespace GitHub.Runner.Worker.Container { if (String.IsNullOrEmpty(env.Value)) { - dockerOptions.Add($"-e \"{env.Key}\""); + dockerOptions.Add(DockerUtil.CreateEscapedOption("-e", env.Key)); } else { - dockerOptions.Add($"-e \"{env.Key}={env.Value.Replace("\"", "\\\"")}\""); + dockerOptions.Add(DockerUtil.CreateEscapedOption("-e", env.Key, env.Value)); } } @@ -202,7 +202,7 @@ namespace GitHub.Runner.Worker.Container { // e.g. -e MY_SECRET maps the value into the exec'ed process without exposing // the value directly in the command - dockerOptions.Add($"-e {env.Key}"); + dockerOptions.Add(DockerUtil.CreateEscapedOption("-e", env.Key)); } // Watermark for GitHub Action environment diff --git a/src/Runner.Worker/Container/DockerUtil.cs b/src/Runner.Worker/Container/DockerUtil.cs index 02c2ece5b..97a53759b 100644 --- a/src/Runner.Worker/Container/DockerUtil.cs +++ b/src/Runner.Worker/Container/DockerUtil.cs @@ -6,6 +6,9 @@ namespace GitHub.Runner.Worker.Container { public class DockerUtil { + private static readonly Regex QuoteEscape = new Regex(@"(\\*)" + "\"", RegexOptions.Compiled); + private static readonly Regex EndOfStringEscape = new Regex(@"(\\+)$", RegexOptions.Compiled); + public static List ParseDockerPort(IList portMappingLines) { const string targetPort = "targetPort"; @@ -17,7 +20,7 @@ namespace GitHub.Runner.Worker.Container string pattern = $"^(?<{targetPort}>\\d+)/(?<{proto}>\\w+) -> (?<{host}>.+):(?<{hostPort}>\\d+)$"; List portMappings = new List(); - foreach(var line in portMappingLines) + foreach (var line in portMappingLines) { Match m = Regex.Match(line, pattern, RegexOptions.None, TimeSpan.FromSeconds(1)); if (m.Success) @@ -61,5 +64,44 @@ namespace GitHub.Runner.Worker.Container } return ""; } + + public static string CreateEscapedOption(string flag, string key) + { + if (String.IsNullOrEmpty(key)) + { + return ""; + } + return $"{flag} {EscapeString(key)}"; + } + + public static string CreateEscapedOption(string flag, string key, string value) + { + if (String.IsNullOrEmpty(key)) + { + return ""; + } + var escapedString = EscapeString($"{key}={value}"); + return $"{flag} {escapedString}"; + } + + private static string EscapeString(string value) + { + if (String.IsNullOrEmpty(value)) + { + return ""; + } + // Dotnet escaping rules are weird here, we can only escape \ if it precedes a " + // If a double quotation mark follows two or an even number of backslashes, each proceeding backslash pair is replaced with one backslash and the double quotation mark is removed. + // If a double quotation mark follows an odd number of backslashes, including just one, each preceding pair is replaced with one backslash and the remaining backslash is removed; however, in this case the double quotation mark is not removed. + // https://docs.microsoft.com/en-us/dotnet/api/system.environment.getcommandlineargs?redirectedfrom=MSDN&view=net-6.0#remarks + + // First, find any \ followed by a " and double the number of \ + 1. + value = QuoteEscape.Replace(value, @"$1$1\" + "\""); + // Next, what if it ends in `\`, it would escape the end quote. So, we need to detect that at the end of the string and perform the same escape + // Luckily, we can just use the $ character with detects the end of string in regex + value = EndOfStringEscape.Replace(value, @"$1$1"); + // Finally, wrap it in quotes + return $"\"{value}\""; + } } } diff --git a/src/Runner.Worker/Handlers/StepHost.cs b/src/Runner.Worker/Handlers/StepHost.cs index 95feaca9a..1270dd90e 100644 --- a/src/Runner.Worker/Handlers/StepHost.cs +++ b/src/Runner.Worker/Handlers/StepHost.cs @@ -193,7 +193,7 @@ namespace GitHub.Runner.Worker.Handlers TranslateToContainerPath(environment); await containerHookManager.RunScriptStepAsync(context, Container, - workingDirectory, + workingDirectory, fileName, arguments, environment, @@ -216,7 +216,7 @@ namespace GitHub.Runner.Worker.Handlers { // e.g. -e MY_SECRET maps the value into the exec'ed process without exposing // the value directly in the command - dockerCommandArgs.Add($"-e {env.Key}"); + dockerCommandArgs.Add(DockerUtil.CreateEscapedOption("-e", env.Key)); } if (!string.IsNullOrEmpty(PrependPath)) { diff --git a/src/Test/L0/Container/DockerUtilL0.cs b/src/Test/L0/Container/DockerUtilL0.cs index c069255a8..1e45d598e 100644 --- a/src/Test/L0/Container/DockerUtilL0.cs +++ b/src/Test/L0/Container/DockerUtilL0.cs @@ -144,5 +144,54 @@ namespace GitHub.Runner.Common.Tests.Worker.Container var actual = DockerUtil.ParseRegistryHostnameFromImageName(input); Assert.Equal(expected, actual); } + + [Theory] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + [InlineData("", "")] + [InlineData("foo", "foo")] + [InlineData("foo \\ bar", "foo \\ bar")] + [InlineData("foo \\", "foo \\\\")] + [InlineData("foo \\\\", "foo \\\\\\\\")] + [InlineData("foo \\\" bar", "foo \\\\\\\" bar")] + [InlineData("foo \\\\\" bar", "foo \\\\\\\\\\\" bar")] + public void CreateEscapedOption_keyOnly(string input, string escaped) + { + var flag = "--example"; + var actual = DockerUtil.CreateEscapedOption(flag, input); + string expected; + if (String.IsNullOrEmpty(input)) + { + expected = ""; + } + else + { + expected = $"{flag} \"{escaped}\""; + } + Assert.Equal(expected, actual); + } + + [Theory] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + [InlineData("foo", "bar", "foo=bar")] + [InlineData("foo\\", "bar", "foo\\=bar")] + [InlineData("foo\\", "bar\\", "foo\\=bar\\\\")] + [InlineData("foo \\","bar \\", "foo \\=bar \\\\")] + public void CreateEscapedOption_keyValue(string keyInput, string valueInput, string escapedString) + { + var flag = "--example"; + var actual = DockerUtil.CreateEscapedOption(flag, keyInput, valueInput); + string expected; + if (String.IsNullOrEmpty(keyInput)) + { + expected = ""; + } + else + { + expected = $"{flag} \"{escapedString}\""; + } + Assert.Equal(expected, actual); + } } } diff --git a/src/runnerversion b/src/runnerversion index d5c616980..05eb53826 100644 --- a/src/runnerversion +++ b/src/runnerversion @@ -1 +1 @@ -2.293.0 +2.293.1