From e8885b2024dceeb8ac67f42830e4ef40733e50f4 Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Wed, 17 Aug 2022 16:10:38 +0200 Subject: [PATCH 1/2] escaping key and quoting it to avoid key based command injection --- src/Runner.Worker/Container/DockerCommandManager.cs | 6 +++--- src/Runner.Worker/Handlers/StepHost.cs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Runner.Worker/Container/DockerCommandManager.cs b/src/Runner.Worker/Container/DockerCommandManager.cs index 86cf0eeedf2..bf1678d743a 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($"-e \"{env.Key.Replace("\\", "\\\\").Replace("\"", "\\\"")}\""); } else { - dockerOptions.Add($"-e \"{env.Key}={env.Value.Replace("\"", "\\\"")}\""); + dockerOptions.Add($"-e \"{env.Key.Replace("\\", "\\\\").Replace("\"", "\\\"")}={env.Value.Replace("\\", "\\\\").Replace("\"", "\\\"")}\""); } } @@ -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($"-e \"{env.Key.Replace("\\", "\\\\").Replace("\"", "\\\"")}\""); } // Watermark for GitHub Action environment diff --git a/src/Runner.Worker/Handlers/StepHost.cs b/src/Runner.Worker/Handlers/StepHost.cs index 95feaca9aef..38e82df482d 100644 --- a/src/Runner.Worker/Handlers/StepHost.cs +++ b/src/Runner.Worker/Handlers/StepHost.cs @@ -193,7 +193,7 @@ public async Task ExecuteAsync(IExecutionContext context, TranslateToContainerPath(environment); await containerHookManager.RunScriptStepAsync(context, Container, - workingDirectory, + workingDirectory, fileName, arguments, environment, @@ -216,7 +216,7 @@ await containerHookManager.RunScriptStepAsync(context, { // 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($"-e \"{env.Key.Replace("\\", "\\\\").Replace("\"", "\\\"")}\""); } if (!string.IsNullOrEmpty(PrependPath)) { From f078f80ec53f118c71a48d5d54d9b75d2019fe83 Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Fri, 19 Aug 2022 15:26:45 +0200 Subject: [PATCH 2/2] 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 | 2 +- src/Test/L0/Container/DockerUtilL0.cs | 54 +++++++++++++++++++ 4 files changed, 82 insertions(+), 5 deletions(-) diff --git a/src/Runner.Worker/Container/DockerCommandManager.cs b/src/Runner.Worker/Container/DockerCommandManager.cs index bf1678d743a..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.Replace("\\", "\\\\").Replace("\"", "\\\"")}\""); + dockerOptions.Add(DockerUtil.CreateEscapedOption("-e", env.Key)); } else { - dockerOptions.Add($"-e \"{env.Key.Replace("\\", "\\\\").Replace("\"", "\\\"")}={env.Value.Replace("\\", "\\\\").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.Replace("\\", "\\\\").Replace("\"", "\\\"")}\""); + 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..bde59f5b641 100644 --- a/src/Runner.Worker/Container/DockerUtil.cs +++ b/src/Runner.Worker/Container/DockerUtil.cs @@ -17,7 +17,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 +61,28 @@ 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 ""; + } + 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 38e82df482d..1270dd90e6f 100644 --- a/src/Runner.Worker/Handlers/StepHost.cs +++ b/src/Runner.Worker/Handlers/StepHost.cs @@ -216,7 +216,7 @@ await containerHookManager.RunScriptStepAsync(context, { // 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.Replace("\\", "\\\\").Replace("\"", "\\\"")}\""); + 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..e5ae05fda72 100644 --- a/src/Test/L0/Container/DockerUtilL0.cs +++ b/src/Test/L0/Container/DockerUtilL0.cs @@ -144,5 +144,59 @@ 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("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); + } } }