Skip to content

Commit 2fda4e2

Browse files
07souravkunda07souravkunda
authored
Merge pull request #61 from browserstack/security/argv-injection-cwe-88
fix: build BrowserStackLocal argv as discrete elements; repair no-op access-key strip
2 parents d833550 + 200c4e7 commit 2fda4e2

4 files changed

Lines changed: 289 additions & 37 deletions

File tree

‎BrowserStackLocal/BrowserStackLocal Unit Tests/BrowserStackTunnelTests.cs‎

Lines changed: 67 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
using NUnit.Framework;
77
using BrowserStack;
8+
using System.Collections.Generic;
89
using System.Text;
910
using System.IO;
1011

@@ -91,15 +92,15 @@ public void TestBinaryPathOnNoMoreFallback()
9192
public void TestBinaryArguments()
9293
{
9394
tunnel = new TunnelClass();
94-
tunnel.addBinaryArguments("dummyArguments");
95-
Assert.AreEqual(tunnel.getBinaryArguments(), "dummyArguments");
95+
tunnel.addBinaryArguments(new List<string> { "-dummyFlag", "dummyValue" });
96+
CollectionAssert.AreEqual(new List<string> { "-dummyFlag", "dummyValue" }, tunnel.getBinaryArguments());
9697
}
9798
[TestMethod]
9899
public void TestBinaryArgumentsAreEmptyOnNull()
99100
{
100101
tunnel = new TunnelClass();
101102
tunnel.addBinaryArguments(null);
102-
Assert.AreEqual(tunnel.getBinaryArguments(), "");
103+
Assert.IsEmpty(tunnel.getBinaryArguments());
103104
}
104105

105106

@@ -130,6 +131,64 @@ public void testFallbackException()
130131
{
131132
tunnel.fallbackPaths();
132133
}
134+
135+
// Regression for the chmod shell-metacharacter injection (F-001): binaryAbsolute must
136+
// reach chmod as a single argument, never interpolated into a shell command line. On
137+
// pre-fix code (`bash -c "chmod 0755 <path>"`) the payload below runs `touch <marker>`
138+
// and never chmods the real file, so BOTH asserts fail; the fix (`/bin/chmod` +
139+
// ArgumentList) creates no marker and chmods the real path. Unix-only: on Windows
140+
// modifyBinaryPermission takes the ACL branch, not chmod.
141+
[TestMethod]
142+
public void TestModifyBinaryPermissionDoesNotInterpretShellMetacharacters()
143+
{
144+
if (os.Platform.ToString() != "Unix")
145+
{
146+
Assert.Ignore("Unix-only: Windows takes the ACL branch in modifyBinaryPermission, not chmod");
147+
return;
148+
}
149+
150+
string prevCwd = Directory.GetCurrentDirectory();
151+
// Space-free working dir so the injected `touch pwned` (if it runs) lands here deterministically.
152+
string work = Path.Combine(Path.GetTempPath(), "bsloc" + Guid.NewGuid().ToString("N"));
153+
Directory.CreateDirectory(work);
154+
Directory.SetCurrentDirectory(work);
155+
try
156+
{
157+
// Filename carries a space AND a shell-injection payload. A filename cannot contain '/',
158+
// so the injected command targets the (deterministic) CWD, not an absolute path.
159+
string binaryPath = Path.Combine(work, "bs local; touch pwned; #");
160+
File.WriteAllText(binaryPath, "#!/bin/sh\n"); // default perms ~0644 (not executable)
161+
162+
tunnel = new TunnelClass();
163+
((TunnelClass)tunnel).setBinaryAbsolute(binaryPath);
164+
tunnel.modifyBinaryPermission();
165+
166+
Assert.IsFalse(File.Exists(Path.Combine(work, "pwned")),
167+
"shell metacharacters in binaryAbsolute were interpreted - OS command injection");
168+
Assert.IsTrue(IsExecutable(binaryPath),
169+
"chmod 0755 was not applied to the real binary path (the path was mangled by the shell)");
170+
}
171+
finally
172+
{
173+
Directory.SetCurrentDirectory(prevCwd);
174+
try { Directory.Delete(work, true); } catch { }
175+
}
176+
}
177+
178+
// Returns true iff `path` has the execute bit set. Uses sh's `$0` positional so the
179+
// path (which contains a space + metacharacters) is passed safely, not re-parsed.
180+
private static bool IsExecutable(string path)
181+
{
182+
var psi = new System.Diagnostics.ProcessStartInfo("/bin/sh") { UseShellExecute = false };
183+
psi.ArgumentList.Add("-c");
184+
psi.ArgumentList.Add("test -x \"$0\"");
185+
psi.ArgumentList.Add(path);
186+
using (var p = System.Diagnostics.Process.Start(psi))
187+
{
188+
p.WaitForExit();
189+
return p.ExitCode == 0;
190+
}
191+
}
133192
public class TunnelClass : BrowserStackTunnel
134193
{
135194
public TunnelClass() : base("test-user-agent") {}
@@ -141,10 +200,14 @@ public string getBinaryAbsolute()
141200
{
142201
return binaryAbsolute;
143202
}
144-
public string getBinaryArguments()
203+
public List<string> getBinaryArguments()
145204
{
146205
return binaryArguments;
147206
}
207+
public void setBinaryAbsolute(string path)
208+
{
209+
binaryAbsolute = path;
210+
}
148211
}
149212
}
150213
}

‎BrowserStackLocal/BrowserStackLocal Unit Tests/LocalTests.cs‎

Lines changed: 167 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,8 @@ public void TestWorksWithAccessKeyInOptions()
5757
local.setTunnel(tunnelMock.Object);
5858
Assert.DoesNotThrow(new TestDelegate(startWithOptions),
5959
"BROWSERSTACK_ACCESS_KEY cannot be empty. Specify one by adding key to options or adding to the environment variable BROWSERSTACK_ACCESS_KEY.");
60-
tunnelMock.Verify(mock => mock.addBinaryArguments(It.IsRegex("-logFile \"" + logAbsolute + "\" " + "--source \"c-sharp:.*")), Times.Once());
60+
tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
61+
InOrder(a, "-logFile", logAbsolute, "--source") && StartsWithAny(a, "c-sharp:"))), Times.Once());
6162
tunnelMock.Verify(mock => mock.Run("dummyKey", "", logAbsolute, "start"), Times.Once());
6263
local.stop();
6364
}
@@ -73,7 +74,8 @@ public void TestWorksWithAccessKeyNotInOptions()
7374
local.setTunnel(tunnelMock.Object);
7475
Assert.DoesNotThrow(new TestDelegate(startWithOptions),
7576
"BROWSERSTACK_ACCESS_KEY cannot be empty. Specify one by adding key to options or adding to the environment variable BROWSERSTACK_ACCESS_KEY.");
76-
tunnelMock.Verify(mock => mock.addBinaryArguments(It.IsRegex("-logFile \"" + logAbsolute + "\" .*")), Times.Once());
77+
tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
78+
InOrder(a, "-logFile", logAbsolute))), Times.Once());
7779
tunnelMock.Verify(mock => mock.Run("envDummyKey", "", logAbsolute, "start"), Times.Once());
7880
local.stop();
7981
}
@@ -90,7 +92,8 @@ public void TestWorksForFolderTesting()
9092
tunnelMock.Setup(mock => mock.Run("dummyKey", "dummyFolderPath", logAbsolute, "start"));
9193
local.setTunnel(tunnelMock.Object);
9294
local.start(options);
93-
tunnelMock.Verify(mock => mock.addBinaryArguments(It.IsRegex("-logFile \"" + logAbsolute + "\" .*")), Times.Once());
95+
tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
96+
InOrder(a, "-logFile", logAbsolute))), Times.Once());
9497
tunnelMock.Verify(mock => mock.Run("dummyKey", "dummyFolderPath", logAbsolute, "start"), Times.Once());
9598
local.stop();
9699
}
@@ -108,7 +111,8 @@ public void TestWorksForBinaryPath()
108111
local.setTunnel(tunnelMock.Object);
109112
local.start(options);
110113
tunnelMock.Verify(mock => mock.addBinaryPath("dummyPath", "", It.IsAny<bool>(), It.IsAny<Exception>()), Times.Once);
111-
tunnelMock.Verify(mock => mock.addBinaryArguments(It.IsRegex("-logFile \"" + logAbsolute + "\" .*")), Times.Once());
114+
tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
115+
InOrder(a, "-logFile", logAbsolute))), Times.Once());
112116
tunnelMock.Verify(mock => mock.Run("dummyKey", "", logAbsolute, "start"), Times.Once());
113117
local.stop();
114118
}
@@ -130,7 +134,8 @@ public void TestWorksWithBooleanOptions()
130134
local.setTunnel(tunnelMock.Object);
131135
local.start(options);
132136
tunnelMock.Verify(mock => mock.addBinaryPath("", "", It.IsAny<bool>(), It.IsAny<Exception>()), Times.Once);
133-
tunnelMock.Verify(mock => mock.addBinaryArguments(It.IsRegex("-vvv.*-force.*-forcelocal.*-forceproxy.*-onlyAutomate.*")), Times.Once());
137+
tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
138+
InOrder(a, "-vvv", "-force", "-forcelocal", "-forceproxy", "-onlyAutomate"))), Times.Once());
134139
tunnelMock.Verify(mock => mock.Run("dummyKey", "", logAbsolute, "start"), Times.Once());
135140
local.stop();
136141
}
@@ -153,8 +158,9 @@ public void TestWorksWithValueOptions()
153158
local.setTunnel(tunnelMock.Object);
154159
local.start(options);
155160
tunnelMock.Verify(mock => mock.addBinaryPath("", "", It.IsAny<bool>(), It.IsAny<Exception>()), Times.Once);
156-
tunnelMock.Verify(mock => mock.addBinaryArguments(
157-
It.IsRegex("-localIdentifier.*dummyIdentifier.*dummyHost.*-proxyHost.*dummyHost.*-proxyPort.*dummyPort.*-proxyUser.*dummyUser.*-proxyPass.*dummyPass.*")
161+
tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
162+
InOrder(a, "-localIdentifier", "dummyIdentifier", "dummyHost", "-proxyHost", "dummyHost",
163+
"-proxyPort", "dummyPort", "-proxyUser", "dummyUser", "-proxyPass", "dummyPass"))
158164
), Times.Once());
159165
tunnelMock.Verify(mock => mock.Run("dummyKey", "", logAbsolute, "start"), Times.Once());
160166
local.stop();
@@ -176,8 +182,9 @@ public void TestWorksWithCustomOptions()
176182
local.setTunnel(tunnelMock.Object);
177183
local.start(options);
178184
tunnelMock.Verify(mock => mock.addBinaryPath("", "", It.IsAny<bool>(), It.IsAny<Exception>()), Times.Once);
179-
tunnelMock.Verify(mock => mock.addBinaryArguments(
180-
It.IsRegex("-customBoolKey1.*-customBoolKey2.*-customKey1.*customValue1.*-customKey2.*customValue2.*")
185+
tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
186+
InOrder(a, "-customBoolKey1", "-customBoolKey2", "-customKey1", "customValue1",
187+
"-customKey2", "customValue2"))
181188
), Times.Once());
182189
tunnelMock.Verify(mock => mock.Run("dummyKey", "", logAbsolute, "start"), Times.Once());
183190
local.stop();
@@ -201,7 +208,8 @@ public void TestCallsFallbackOnFailure()
201208
local.setTunnel(tunnelMock.Object);
202209
local.start(options);
203210
tunnelMock.Verify(mock => mock.addBinaryPath("", "", It.IsAny<bool>(), It.IsAny<Exception>()), Times.Once);
204-
tunnelMock.Verify(mock => mock.addBinaryArguments(It.IsRegex("-logFile \"" + logAbsolute + "\" .*")), Times.Once());
211+
tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
212+
InOrder(a, "-logFile", logAbsolute))), Times.Once());
205213
tunnelMock.Verify(mock => mock.Run("dummyKey", "", logAbsolute, "start"), Times.Exactly(2));
206214
tunnelMock.Verify(mock => mock.fallbackPaths(), Times.Once());
207215
local.stop();
@@ -220,7 +228,8 @@ public void TestKillsTunnel()
220228
local.start(options);
221229
local.stop();
222230
tunnelMock.Verify(mock => mock.addBinaryPath("", "", It.IsAny<bool>(), It.IsAny<Exception>()), Times.Once);
223-
tunnelMock.Verify(mock => mock.addBinaryArguments(It.IsRegex("-logFile \"" + logAbsolute + "\" .*")), Times.Once());
231+
tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
232+
InOrder(a, "-logFile", logAbsolute))), Times.Once());
224233
tunnelMock.Verify(mock => mock.Run("dummyKey", "", logAbsolute, "start"), Times.Once());
225234
}
226235

@@ -273,6 +282,153 @@ public void TestSetProxyIgnoresInvalidPort()
273282
local.stop();
274283
}
275284

285+
// ---- argv helpers -------------------------------------------------------
286+
// Arguments are now discrete argv elements rather than one concatenated string,
287+
// so assertions match elements in order instead of matching a regex.
288+
private static bool InOrder(List<string> actual, params string[] expected)
289+
{
290+
int idx = 0;
291+
foreach (string e in expected)
292+
{
293+
idx = actual.IndexOf(e, idx);
294+
if (idx < 0) return false;
295+
idx++;
296+
}
297+
return true;
298+
}
299+
300+
private static bool StartsWithAny(List<string> actual, string prefix)
301+
{
302+
return actual.Exists(a => a != null && a.StartsWith(prefix));
303+
}
304+
305+
// ---- regression tests: CWE-88 argument injection ------------------------
306+
// Each of these fails on the pre-fix code, where every value was concatenated
307+
// into one string that Process.Start then re-tokenised on whitespace.
308+
309+
[TestMethod]
310+
public void TestOptionValueWithSpacesStaysOneArgument()
311+
{
312+
options = new List<KeyValuePair<string, string>>();
313+
options.Add(new KeyValuePair<string, string>("key", "dummyKey"));
314+
options.Add(new KeyValuePair<string, string>("proxyPass", "p@ss --proxy evil.example.com"));
315+
316+
local = new LocalClass();
317+
Mock<BrowserStackTunnel> tunnelMock = new Mock<BrowserStackTunnel>("test-user-agent");
318+
local.setTunnel(tunnelMock.Object);
319+
local.start(options);
320+
321+
tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
322+
InOrder(a, "-proxyPass", "p@ss --proxy evil.example.com")
323+
&& !a.Contains("--proxy"))), Times.Once());
324+
local.stop();
325+
}
326+
327+
[TestMethod]
328+
public void TestUnknownOptionValueWithSpacesStaysOneArgument()
329+
{
330+
options = new List<KeyValuePair<string, string>>();
331+
options.Add(new KeyValuePair<string, string>("key", "dummyKey"));
332+
options.Add(new KeyValuePair<string, string>("customKey", "legit --config /tmp/attacker.cfg"));
333+
334+
local = new LocalClass();
335+
Mock<BrowserStackTunnel> tunnelMock = new Mock<BrowserStackTunnel>("test-user-agent");
336+
local.setTunnel(tunnelMock.Object);
337+
local.start(options);
338+
339+
tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
340+
InOrder(a, "-customKey", "legit --config /tmp/attacker.cfg")
341+
&& !a.Contains("--config"))), Times.Once());
342+
local.stop();
343+
}
344+
345+
[TestMethod]
346+
public void TestLogFilePathWithQuoteStaysOneArgument()
347+
{
348+
options = new List<KeyValuePair<string, string>>();
349+
options.Add(new KeyValuePair<string, string>("key", "dummyKey"));
350+
options.Add(new KeyValuePair<string, string>("logfile", "/tmp/x\" --proxy evil.example.com \""));
351+
352+
local = new LocalClass();
353+
Mock<BrowserStackTunnel> tunnelMock = new Mock<BrowserStackTunnel>("test-user-agent");
354+
local.setTunnel(tunnelMock.Object);
355+
local.start(options);
356+
357+
tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
358+
InOrder(a, "-logFile", "/tmp/x\" --proxy evil.example.com \"")
359+
&& !a.Contains("--proxy"))), Times.Once());
360+
local.stop();
361+
}
362+
363+
[TestMethod]
364+
public void TestAccessKeyWhitespaceIsStrippedFromOptions()
365+
{
366+
options = new List<KeyValuePair<string, string>>();
367+
options.Add(new KeyValuePair<string, string>("key", " dummy Key --proxy evil.example.com "));
368+
369+
local = new LocalClass();
370+
Mock<BrowserStackTunnel> tunnelMock = new Mock<BrowserStackTunnel>("test-user-agent");
371+
local.setTunnel(tunnelMock.Object);
372+
local.start(options);
373+
374+
// Whitespace removed, so no "--proxy" token can split out of the key.
375+
tunnelMock.Verify(mock => mock.Run("dummyKey--proxyevil.example.com", "", logAbsolute, "start"),
376+
Times.Once());
377+
local.stop();
378+
}
379+
380+
[TestMethod]
381+
public void TestAccessKeyWhitespaceIsStrippedFromEnvironmentVariable()
382+
{
383+
Environment.SetEnvironmentVariable("BROWSERSTACK_ACCESS_KEY", "env Dummy\tKey");
384+
options = new List<KeyValuePair<string, string>>();
385+
386+
local = new LocalClass();
387+
Mock<BrowserStackTunnel> tunnelMock = new Mock<BrowserStackTunnel>("test-user-agent");
388+
local.setTunnel(tunnelMock.Object);
389+
local.start(options);
390+
391+
tunnelMock.Verify(mock => mock.Run("envDummyKey", "", logAbsolute, "start"), Times.Once());
392+
local.stop();
393+
}
394+
395+
[TestMethod]
396+
public void TestFolderPathWithSpacesIsPreserved()
397+
{
398+
options = new List<KeyValuePair<string, string>>();
399+
options.Add(new KeyValuePair<string, string>("key", "dummyKey"));
400+
options.Add(new KeyValuePair<string, string>("f", "/my/awesome folder"));
401+
402+
local = new LocalClass();
403+
Mock<BrowserStackTunnel> tunnelMock = new Mock<BrowserStackTunnel>("test-user-agent");
404+
local.setTunnel(tunnelMock.Object);
405+
local.start(options);
406+
407+
tunnelMock.Verify(mock => mock.Run("dummyKey", "/my/awesome folder", logAbsolute, "start"),
408+
Times.Once());
409+
local.stop();
410+
}
411+
412+
[TestMethod]
413+
public void TestDocumentedPassThroughOptionsStillWork()
414+
{
415+
options = new List<KeyValuePair<string, string>>();
416+
options.Add(new KeyValuePair<string, string>("key", "dummyKey"));
417+
options.Add(new KeyValuePair<string, string>("localProxyHost", "127.0.0.1"));
418+
options.Add(new KeyValuePair<string, string>("localProxyPort", "8000"));
419+
options.Add(new KeyValuePair<string, string>("-pac-file", "/tmp/my proxy.pac"));
420+
421+
local = new LocalClass();
422+
Mock<BrowserStackTunnel> tunnelMock = new Mock<BrowserStackTunnel>("test-user-agent");
423+
local.setTunnel(tunnelMock.Object);
424+
local.start(options);
425+
426+
tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
427+
InOrder(a, "-localProxyHost", "127.0.0.1", "-localProxyPort", "8000",
428+
"--pac-file", "/tmp/my proxy.pac"))), Times.Once());
429+
local.stop();
430+
}
431+
276432
public void startWithOptions()
277433
{
278434
local.start(options);

0 commit comments

Comments
 (0)