Escaping key and quoting it to avoid key based command injection (act…

…ions#2062) * escaping key and quoting it to avoid key based command injection * extracted creation of flags to DockerUtil, with testing included
lyst · Aug 23, 2022 · 01fd044 · 01fd044
1 parent 1cb1779
commit 01fd044
Show file tree

Hide file tree

Showing 4 changed files with 83 additions and 6 deletions.
diff --git a/src/Runner.Worker/Container/DockerCommandManager.cs b/src/Runner.Worker/Container/DockerCommandManager.cs
@@ -131,11 +131,11 @@ public async Task<string> 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<int> 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
@@ -17,7 +17,7 @@ public static List<PortMapping> ParseDockerPort(IList<string> portMappingLines)
             string pattern = $"^(?<{targetPort}>\\d+)/(?<{proto}>\\w+) -> (?<{host}>.+):(?<{hostPort}>\\d+)$";
 
             List<PortMapping> portMappings = new List<PortMapping>();
-            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
@@ -193,7 +193,7 @@ public async Task<int> 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(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
@@ -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);
+        }
     }
 }