diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 739e1dc59..6871aa810 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -164,6 +164,7 @@ jobs: release_name: "v${{ steps.releaseNote.outputs.version }}" body: | ${{ steps.releaseNote.outputs.note }} + prerelease: true # Upload release assets - name: Upload Release Asset (win-x64) diff --git a/releaseNote.md b/releaseNote.md index cef16c589..9c2e8f87c 100644 --- a/releaseNote.md +++ b/releaseNote.md @@ -2,7 +2,7 @@ ## Bugs -- Fixed an issue where ephemeral runners did not restart after upgrading (#1396) +- Fixed an issue where container environment variables names or values could escape the docker command (#2108) ## Misc 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 0907eaed2..03dec7aab 100644 --- a/src/Runner.Worker/Handlers/StepHost.cs +++ b/src/Runner.Worker/Handlers/StepHost.cs @@ -188,7 +188,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 990835884..8c6292c39 100644 --- a/src/runnerversion +++ b/src/runnerversion @@ -1 +1 @@ -2.283.3 +2.283.4