From 01fd04464d931699c6989f7c366a5ff3e155fe56 Mon Sep 17 00:00:00 2001 From: Nikola Jokic <97525037+nikola-jokic@users.noreply.github.com> Date: Tue, 23 Aug 2022 07:42:29 -0700 Subject: [PATCH] Escaping key and quoting it to avoid key based command injection (#2062) * escaping key and quoting it to avoid key based command injection * extracted creation of flags to DockerUtil, with testing included --- .../Container/DockerCommandManager.cs | 6 +-- src/Runner.Worker/Container/DockerUtil.cs | 25 ++++++++- src/Runner.Worker/Handlers/StepHost.cs | 4 +- src/Test/L0/Container/DockerUtilL0.cs | 54 +++++++++++++++++++ 4 files changed, 83 insertions(+), 6 deletions(-) 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..bde59f5b6 100644 --- a/src/Runner.Worker/Container/DockerUtil.cs +++ b/src/Runner.Worker/Container/DockerUtil.cs @@ -17,7 +17,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 +61,28 @@ 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 ""; + } + return $"{flag} \"{EscapeString(key)}={EscapeString(value)}\""; + } + + private static string EscapeString(string value) + { + return value.Replace("\\", "\\\\").Replace("\"", "\\\""); + } } } 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..e5ae05fda 100644 --- a/src/Test/L0/Container/DockerUtilL0.cs +++ b/src/Test/L0/Container/DockerUtilL0.cs @@ -144,5 +144,59 @@ 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("HOME alpine:3.8 sh -c id #", "HOME alpine:3.8 sh -c id #")] + [InlineData("HOME \"alpine:3.8 sh -c id #", "HOME \\\"alpine:3.8 sh -c id #")] + [InlineData("HOME \\\"alpine:3.8 sh -c id #", "HOME \\\\\\\"alpine:3.8 sh -c id #")] + [InlineData("HOME \\\\\"alpine:3.8 sh -c id #", "HOME \\\\\\\\\\\"alpine:3.8 sh -c id #")] + [InlineData("HOME \"\"alpine:3.8 sh -c id #", "HOME \\\"\\\"alpine:3.8 sh -c id #")] + [InlineData("HOME \\\"\"alpine:3.8 sh -c id #", "HOME \\\\\\\"\\\"alpine:3.8 sh -c id #")] + [InlineData("HOME \"\\\"alpine:3.8 sh -c id #", "HOME \\\"\\\\\\\"alpine:3.8 sh -c id #")] + 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("HOME", "", "HOME", "")] + [InlineData("HOME alpine:3.8 sh -c id #", "HOME alpine:3.8 sh -c id #", "HOME alpine:3.8 sh -c id #", "HOME alpine:3.8 sh -c id #")] + [InlineData("HOME \"alpine:3.8 sh -c id #", "HOME \"alpine:3.8 sh -c id #", "HOME \\\"alpine:3.8 sh -c id #", "HOME \\\"alpine:3.8 sh -c id #")] + [InlineData("HOME \\\"alpine:3.8 sh -c id #", "HOME \\\"alpine:3.8 sh -c id #", "HOME \\\\\\\"alpine:3.8 sh -c id #", "HOME \\\\\\\"alpine:3.8 sh -c id #")] + [InlineData("HOME \\\\\"alpine:3.8 sh -c id #", "HOME \\\\\"alpine:3.8 sh -c id #", "HOME \\\\\\\\\\\"alpine:3.8 sh -c id #", "HOME \\\\\\\\\\\"alpine:3.8 sh -c id #")] + [InlineData("HOME \"\"alpine:3.8 sh -c id #", "HOME \"\"alpine:3.8 sh -c id #", "HOME \\\"\\\"alpine:3.8 sh -c id #", "HOME \\\"\\\"alpine:3.8 sh -c id #")] + [InlineData("HOME \\\"\"alpine:3.8 sh -c id #", "HOME \\\"\"alpine:3.8 sh -c id #", "HOME \\\\\\\"\\\"alpine:3.8 sh -c id #", "HOME \\\\\\\"\\\"alpine:3.8 sh -c id #")] + [InlineData("HOME \"\\\"alpine:3.8 sh -c id #", "HOME \"\\\"alpine:3.8 sh -c id #", "HOME \\\"\\\\\\\"alpine:3.8 sh -c id #", "HOME \\\"\\\\\\\"alpine:3.8 sh -c id #")] + public void CreateEscapedOption_keyValue(string keyInput, string valueInput, string escapedKey, string escapedValue) + { + var flag = "--example"; + var actual = DockerUtil.CreateEscapedOption(flag, keyInput, valueInput); + string expected; + if (String.IsNullOrEmpty(keyInput)) + { + expected = ""; + } + else + { + expected = $"{flag} \"{escapedKey}={escapedValue}\""; + } + Assert.Equal(expected, actual); + } } }