Skip to content

Commit a2fb048

Browse files
nikola-jokicnikola-jokic
committed
Escaping key and quoting it to avoid key based command injection (actions#2062)
* escaping key and quoting it to avoid key based command injection * extracted creation of flags to DockerUtil, with testing included
1 parent 5731c0e commit a2fb048

4 files changed

Lines changed: 83 additions & 6 deletions

File tree

‎src/Runner.Worker/Container/DockerCommandManager.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -131,11 +131,11 @@ public async Task<string> DockerCreate(IExecutionContext context, ContainerInfo
131131
{
132132
if (String.IsNullOrEmpty(env.Value))
133133
{
134-
dockerOptions.Add($"-e \"{env.Key}\"");
134+
dockerOptions.Add(DockerUtil.CreateEscapedOption("-e", env.Key));
135135
}
136136
else
137137
{
138-
dockerOptions.Add($"-e \"{env.Key}={env.Value.Replace("\"", "\\\"")}\"");
138+
dockerOptions.Add(DockerUtil.CreateEscapedOption("-e", env.Key, env.Value));
139139
}
140140
}
141141

@@ -202,7 +202,7 @@ public async Task<int> DockerRun(IExecutionContext context, ContainerInfo contai
202202
{
203203
// e.g. -e MY_SECRET maps the value into the exec'ed process without exposing
204204
// the value directly in the command
205-
dockerOptions.Add($"-e {env.Key}");
205+
dockerOptions.Add(DockerUtil.CreateEscapedOption("-e", env.Key));
206206
}
207207

208208
// Watermark for GitHub Action environment

‎src/Runner.Worker/Container/DockerUtil.cs‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ public static List<PortMapping> ParseDockerPort(IList<string> portMappingLines)
1717
string pattern = $"^(?<{targetPort}>\\d+)/(?<{proto}>\\w+) -> (?<{host}>.+):(?<{hostPort}>\\d+)$";
1818

1919
List<PortMapping> portMappings = new List<PortMapping>();
20-
foreach(var line in portMappingLines)
20+
foreach (var line in portMappingLines)
2121
{
2222
Match m = Regex.Match(line, pattern, RegexOptions.None, TimeSpan.FromSeconds(1));
2323
if (m.Success)
@@ -61,5 +61,28 @@ public static string ParseRegistryHostnameFromImageName(string name)
6161
}
6262
return "";
6363
}
64+
65+
public static string CreateEscapedOption(string flag, string key)
66+
{
67+
if (String.IsNullOrEmpty(key))
68+
{
69+
return "";
70+
}
71+
return $"{flag} \"{EscapeString(key)}\"";
72+
}
73+
74+
public static string CreateEscapedOption(string flag, string key, string value)
75+
{
76+
if (String.IsNullOrEmpty(key))
77+
{
78+
return "";
79+
}
80+
return $"{flag} \"{EscapeString(key)}={EscapeString(value)}\"";
81+
}
82+
83+
private static string EscapeString(string value)
84+
{
85+
return value.Replace("\\", "\\\\").Replace("\"", "\\\"");
86+
}
6487
}
6588
}

‎src/Runner.Worker/Handlers/StepHost.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ public async Task<int> ExecuteAsync(IExecutionContext context,
193193
TranslateToContainerPath(environment);
194194
await containerHookManager.RunScriptStepAsync(context,
195195
Container,
196-
workingDirectory,
196+
workingDirectory,
197197
fileName,
198198
arguments,
199199
environment,
@@ -216,7 +216,7 @@ await containerHookManager.RunScriptStepAsync(context,
216216
{
217217
// e.g. -e MY_SECRET maps the value into the exec'ed process without exposing
218218
// the value directly in the command
219-
dockerCommandArgs.Add($"-e {env.Key}");
219+
dockerCommandArgs.Add(DockerUtil.CreateEscapedOption("-e", env.Key));
220220
}
221221
if (!string.IsNullOrEmpty(PrependPath))
222222
{

‎src/Test/L0/Container/DockerUtilL0.cs‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,5 +144,59 @@ public void ParseRegistryHostnameFromImageName(string input, string expected)
144144
var actual = DockerUtil.ParseRegistryHostnameFromImageName(input);
145145
Assert.Equal(expected, actual);
146146
}
147+
148+
[Theory]
149+
[Trait("Level", "L0")]
150+
[Trait("Category", "Worker")]
151+
[InlineData("", "")]
152+
[InlineData("HOME alpine:3.8 sh -c id #", "HOME alpine:3.8 sh -c id #")]
153+
[InlineData("HOME \"alpine:3.8 sh -c id #", "HOME \\\"alpine:3.8 sh -c id #")]
154+
[InlineData("HOME \\\"alpine:3.8 sh -c id #", "HOME \\\\\\\"alpine:3.8 sh -c id #")]
155+
[InlineData("HOME \\\\\"alpine:3.8 sh -c id #", "HOME \\\\\\\\\\\"alpine:3.8 sh -c id #")]
156+
[InlineData("HOME \"\"alpine:3.8 sh -c id #", "HOME \\\"\\\"alpine:3.8 sh -c id #")]
157+
[InlineData("HOME \\\"\"alpine:3.8 sh -c id #", "HOME \\\\\\\"\\\"alpine:3.8 sh -c id #")]
158+
[InlineData("HOME \"\\\"alpine:3.8 sh -c id #", "HOME \\\"\\\\\\\"alpine:3.8 sh -c id #")]
159+
public void CreateEscapedOption_keyOnly(string input, string escaped)
160+
{
161+
var flag = "--example";
162+
var actual = DockerUtil.CreateEscapedOption(flag, input);
163+
string expected;
164+
if (String.IsNullOrEmpty(input))
165+
{
166+
expected = "";
167+
}
168+
else
169+
{
170+
expected = $"{flag} \"{escaped}\"";
171+
}
172+
Assert.Equal(expected, actual);
173+
}
174+
175+
[Theory]
176+
[Trait("Level", "L0")]
177+
[Trait("Category", "Worker")]
178+
[InlineData("HOME", "", "HOME", "")]
179+
[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 #")]
180+
[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 #")]
181+
[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 #")]
182+
[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 #")]
183+
[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 #")]
184+
[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 #")]
185+
[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 #")]
186+
public void CreateEscapedOption_keyValue(string keyInput, string valueInput, string escapedKey, string escapedValue)
187+
{
188+
var flag = "--example";
189+
var actual = DockerUtil.CreateEscapedOption(flag, keyInput, valueInput);
190+
string expected;
191+
if (String.IsNullOrEmpty(keyInput))
192+
{
193+
expected = "";
194+
}
195+
else
196+
{
197+
expected = $"{flag} \"{escapedKey}={escapedValue}\"";
198+
}
199+
Assert.Equal(expected, actual);
200+
}
147201
}
148202
}

0 commit comments

Comments
 (0)
⚡