From bbcf9f214575c96698b4a513ee4ce6e2cec5bc3c Mon Sep 17 00:00:00 2001 From: Thomas Boop <52323235+thboop@users.noreply.github.com> Date: Tue, 20 Sep 2022 12:53:27 -0400 Subject: [PATCH] M289 hotfix for container escaping (#2135) * Fix escaping of docker envs backport * create as prerelease * 2.289.4 release notes Co-authored-by: Nikola Jokic <97525037+nikola-jokic@users.noreply.github.com> --- .github/workflows/release.yml | 3 +- releaseNote.md | 2 +- .../Container/DockerCommandManager.cs | 6 +-- src/Runner.Worker/Container/DockerUtil.cs | 44 ++++++++++++++++- src/Runner.Worker/Handlers/StepHost.cs | 2 +- src/Test/L0/Container/DockerUtilL0.cs | 49 +++++++++++++++++++ src/runnerversion | 2 +- 7 files changed, 100 insertions(+), 8 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index a7cd9f440da..d34b1ac3c27 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -157,7 +157,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 @@ -282,6 +282,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 d1ae63d2ffe..7f1e2e539bd 100644 --- a/releaseNote.md +++ b/releaseNote.md @@ -1,7 +1,7 @@ ## Features ## Bugs -- Fixed an issue where websockets failed to successfully close when posting log lines (#1790) +- Fixed an issue where container environment variables names or values could escape the docker command (#2108) ## Windows x64 diff --git a/src/Runner.Worker/Container/DockerCommandManager.cs b/src/Runner.Worker/Container/DockerCommandManager.cs index 86cf0eeedf2..a0c158bdf68 100644 --- a/src/Runner.Worker/Container/DockerCommandManager.cs +++ b/src/Runner.Worker/Container/DockerCommandManager.cs @@ -131,11 +131,11 @@ public async Task DockerCreate(IExecutionContext context, ContainerInfo { 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 @@ public async Task DockerRun(IExecutionContext context, ContainerInfo contai { // 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 02c2ece5b79..97a53759bc7 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 @@ public static List ParseDockerPort(IList portMappingLines) 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 @@ public static string ParseRegistryHostnameFromImageName(string name) } 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 8741c22bbe5..f65a0f53305 100644 --- a/src/Runner.Worker/Handlers/StepHost.cs +++ b/src/Runner.Worker/Handlers/StepHost.cs @@ -188,7 +188,7 @@ public async Task ExecuteAsync(string workingDirectory, { // 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 c069255a85c..1e45d598e89 100644 --- a/src/Test/L0/Container/DockerUtilL0.cs +++ b/src/Test/L0/Container/DockerUtilL0.cs @@ -144,5 +144,54 @@ public void ParseRegistryHostnameFromImageName(string input, string expected) 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 02c94ddc52b..2b89a8478ac 100644 --- a/src/runnerversion +++ b/src/runnerversion @@ -1 +1 @@ -2.289.3 +2.289.4