Skip to content

fix: build BrowserStackLocal argv as discrete elements; repair no-op access-key strip #61

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
07souravkunda wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

fix: build BrowserStackLocal argv as discrete elements; repair no-op access-key strip #61

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
40f1011
fix: pass BrowserStackLocal arguments as discrete argv elements (CWE-88)
07souravkunda Aug 7, 2026
a18e503
revert: drop the option-key format check
07souravkunda Aug 12, 2026
200c4e7
test: regression for chmod shell-metacharacter injection (CWE-78)
07souravkunda Aug 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 67 additions & 4 deletions BrowserStackLocal/BrowserStackLocal Unit Tests/BrowserStackTunnelTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

using NUnit.Framework;
using BrowserStack;
using System.Collections.Generic;
using System.Text;
using System.IO;

Expand Down Expand Up @@ -91,15 +92,15 @@ public void TestBinaryPathOnNoMoreFallback()
public void TestBinaryArguments()
{
tunnel = new TunnelClass();
tunnel.addBinaryArguments("dummyArguments");
Assert.AreEqual(tunnel.getBinaryArguments(), "dummyArguments");
tunnel.addBinaryArguments(new List<string> { "-dummyFlag", "dummyValue" });
CollectionAssert.AreEqual(new List<string> { "-dummyFlag", "dummyValue" }, tunnel.getBinaryArguments());
}
[TestMethod]
public void TestBinaryArgumentsAreEmptyOnNull()
{
tunnel = new TunnelClass();
tunnel.addBinaryArguments(null);
Assert.AreEqual(tunnel.getBinaryArguments(), "");
Assert.IsEmpty(tunnel.getBinaryArguments());
}


Expand Down Expand Up @@ -130,6 +131,64 @@ public void testFallbackException()
{
tunnel.fallbackPaths();
}

// Regression for the chmod shell-metacharacter injection (F-001): binaryAbsolute must
// reach chmod as a single argument, never interpolated into a shell command line. On
// pre-fix code (`bash -c "chmod 0755 <path>"`) the payload below runs `touch <marker>`
// and never chmods the real file, so BOTH asserts fail; the fix (`/bin/chmod` +
// ArgumentList) creates no marker and chmods the real path. Unix-only: on Windows
// modifyBinaryPermission takes the ACL branch, not chmod.
[TestMethod]
public void TestModifyBinaryPermissionDoesNotInterpretShellMetacharacters()
{
if (os.Platform.ToString() != "Unix")
{
Assert.Ignore("Unix-only: Windows takes the ACL branch in modifyBinaryPermission, not chmod");
return;
}

string prevCwd = Directory.GetCurrentDirectory();
// Space-free working dir so the injected `touch pwned` (if it runs) lands here deterministically.
string work = Path.Combine(Path.GetTempPath(), "bsloc" + Guid.NewGuid().ToString("N"));
Directory.CreateDirectory(work);
Directory.SetCurrentDirectory(work);
try
{
// Filename carries a space AND a shell-injection payload. A filename cannot contain '/',
// so the injected command targets the (deterministic) CWD, not an absolute path.
string binaryPath = Path.Combine(work, "bs local; touch pwned; #");
File.WriteAllText(binaryPath, "#!/bin/sh\n"); // default perms ~0644 (not executable)

tunnel = new TunnelClass();
((TunnelClass)tunnel).setBinaryAbsolute(binaryPath);
tunnel.modifyBinaryPermission();

Assert.IsFalse(File.Exists(Path.Combine(work, "pwned")),
"shell metacharacters in binaryAbsolute were interpreted - OS command injection");
Assert.IsTrue(IsExecutable(binaryPath),
"chmod 0755 was not applied to the real binary path (the path was mangled by the shell)");
}
finally
{
Directory.SetCurrentDirectory(prevCwd);
try { Directory.Delete(work, true); } catch { }
}
}

// Returns true iff `path` has the execute bit set. Uses sh's `$0` positional so the
// path (which contains a space + metacharacters) is passed safely, not re-parsed.
private static bool IsExecutable(string path)
{
var psi = new System.Diagnostics.ProcessStartInfo("/bin/sh") { UseShellExecute = false };
psi.ArgumentList.Add("-c");
psi.ArgumentList.Add("test -x \"$0\"");
psi.ArgumentList.Add(path);
using (var p = System.Diagnostics.Process.Start(psi))
{
p.WaitForExit();
return p.ExitCode == 0;
}
}
public class TunnelClass : BrowserStackTunnel
{
public TunnelClass() : base("test-user-agent") {}
Expand All @@ -141,10 +200,14 @@ public string getBinaryAbsolute()
{
return binaryAbsolute;
}
public string getBinaryArguments()
public List<string> getBinaryArguments()
{
return binaryArguments;
}
public void setBinaryAbsolute(string path)
{
binaryAbsolute = path;
}
}
}
}
178 changes: 167 additions & 11 deletions BrowserStackLocal/BrowserStackLocal Unit Tests/LocalTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ public void TestWorksWithAccessKeyInOptions()
local.setTunnel(tunnelMock.Object);
Assert.DoesNotThrow(new TestDelegate(startWithOptions),
"BROWSERSTACK_ACCESS_KEY cannot be empty. Specify one by adding key to options or adding to the environment variable BROWSERSTACK_ACCESS_KEY.");
tunnelMock.Verify(mock => mock.addBinaryArguments(It.IsRegex("-logFile \"" + logAbsolute + "\" " + "--source \"c-sharp:.*")), Times.Once());
tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
InOrder(a, "-logFile", logAbsolute, "--source") && StartsWithAny(a, "c-sharp:"))), Times.Once());
tunnelMock.Verify(mock => mock.Run("dummyKey", "", logAbsolute, "start"), Times.Once());
local.stop();
}
Expand All @@ -73,7 +74,8 @@ public void TestWorksWithAccessKeyNotInOptions()
local.setTunnel(tunnelMock.Object);
Assert.DoesNotThrow(new TestDelegate(startWithOptions),
"BROWSERSTACK_ACCESS_KEY cannot be empty. Specify one by adding key to options or adding to the environment variable BROWSERSTACK_ACCESS_KEY.");
tunnelMock.Verify(mock => mock.addBinaryArguments(It.IsRegex("-logFile \"" + logAbsolute + "\" .*")), Times.Once());
tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
InOrder(a, "-logFile", logAbsolute))), Times.Once());
tunnelMock.Verify(mock => mock.Run("envDummyKey", "", logAbsolute, "start"), Times.Once());
local.stop();
}
Expand All @@ -90,7 +92,8 @@ public void TestWorksForFolderTesting()
tunnelMock.Setup(mock => mock.Run("dummyKey", "dummyFolderPath", logAbsolute, "start"));
local.setTunnel(tunnelMock.Object);
local.start(options);
tunnelMock.Verify(mock => mock.addBinaryArguments(It.IsRegex("-logFile \"" + logAbsolute + "\" .*")), Times.Once());
tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
InOrder(a, "-logFile", logAbsolute))), Times.Once());
tunnelMock.Verify(mock => mock.Run("dummyKey", "dummyFolderPath", logAbsolute, "start"), Times.Once());
local.stop();
}
Expand All @@ -108,7 +111,8 @@ public void TestWorksForBinaryPath()
local.setTunnel(tunnelMock.Object);
local.start(options);
tunnelMock.Verify(mock => mock.addBinaryPath("dummyPath", "", It.IsAny<bool>(), It.IsAny<Exception>()), Times.Once);
tunnelMock.Verify(mock => mock.addBinaryArguments(It.IsRegex("-logFile \"" + logAbsolute + "\" .*")), Times.Once());
tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
InOrder(a, "-logFile", logAbsolute))), Times.Once());
tunnelMock.Verify(mock => mock.Run("dummyKey", "", logAbsolute, "start"), Times.Once());
local.stop();
}
Expand All @@ -130,7 +134,8 @@ public void TestWorksWithBooleanOptions()
local.setTunnel(tunnelMock.Object);
local.start(options);
tunnelMock.Verify(mock => mock.addBinaryPath("", "", It.IsAny<bool>(), It.IsAny<Exception>()), Times.Once);
tunnelMock.Verify(mock => mock.addBinaryArguments(It.IsRegex("-vvv.*-force.*-forcelocal.*-forceproxy.*-onlyAutomate.*")), Times.Once());
tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
InOrder(a, "-vvv", "-force", "-forcelocal", "-forceproxy", "-onlyAutomate"))), Times.Once());
tunnelMock.Verify(mock => mock.Run("dummyKey", "", logAbsolute, "start"), Times.Once());
local.stop();
}
Expand All @@ -153,8 +158,9 @@ public void TestWorksWithValueOptions()
local.setTunnel(tunnelMock.Object);
local.start(options);
tunnelMock.Verify(mock => mock.addBinaryPath("", "", It.IsAny<bool>(), It.IsAny<Exception>()), Times.Once);
tunnelMock.Verify(mock => mock.addBinaryArguments(
It.IsRegex("-localIdentifier.*dummyIdentifier.*dummyHost.*-proxyHost.*dummyHost.*-proxyPort.*dummyPort.*-proxyUser.*dummyUser.*-proxyPass.*dummyPass.*")
tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
InOrder(a, "-localIdentifier", "dummyIdentifier", "dummyHost", "-proxyHost", "dummyHost",
"-proxyPort", "dummyPort", "-proxyUser", "dummyUser", "-proxyPass", "dummyPass"))
), Times.Once());
tunnelMock.Verify(mock => mock.Run("dummyKey", "", logAbsolute, "start"), Times.Once());
local.stop();
Expand All @@ -176,8 +182,9 @@ public void TestWorksWithCustomOptions()
local.setTunnel(tunnelMock.Object);
local.start(options);
tunnelMock.Verify(mock => mock.addBinaryPath("", "", It.IsAny<bool>(), It.IsAny<Exception>()), Times.Once);
tunnelMock.Verify(mock => mock.addBinaryArguments(
It.IsRegex("-customBoolKey1.*-customBoolKey2.*-customKey1.*customValue1.*-customKey2.*customValue2.*")
tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
InOrder(a, "-customBoolKey1", "-customBoolKey2", "-customKey1", "customValue1",
"-customKey2", "customValue2"))
), Times.Once());
tunnelMock.Verify(mock => mock.Run("dummyKey", "", logAbsolute, "start"), Times.Once());
local.stop();
Expand All @@ -201,7 +208,8 @@ public void TestCallsFallbackOnFailure()
local.setTunnel(tunnelMock.Object);
local.start(options);
tunnelMock.Verify(mock => mock.addBinaryPath("", "", It.IsAny<bool>(), It.IsAny<Exception>()), Times.Once);
tunnelMock.Verify(mock => mock.addBinaryArguments(It.IsRegex("-logFile \"" + logAbsolute + "\" .*")), Times.Once());
tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
InOrder(a, "-logFile", logAbsolute))), Times.Once());
tunnelMock.Verify(mock => mock.Run("dummyKey", "", logAbsolute, "start"), Times.Exactly(2));
tunnelMock.Verify(mock => mock.fallbackPaths(), Times.Once());
local.stop();
Expand All @@ -220,7 +228,8 @@ public void TestKillsTunnel()
local.start(options);
local.stop();
tunnelMock.Verify(mock => mock.addBinaryPath("", "", It.IsAny<bool>(), It.IsAny<Exception>()), Times.Once);
tunnelMock.Verify(mock => mock.addBinaryArguments(It.IsRegex("-logFile \"" + logAbsolute + "\" .*")), Times.Once());
tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
InOrder(a, "-logFile", logAbsolute))), Times.Once());
tunnelMock.Verify(mock => mock.Run("dummyKey", "", logAbsolute, "start"), Times.Once());
}

Expand Down Expand Up @@ -273,6 +282,153 @@ public void TestSetProxyIgnoresInvalidPort()
local.stop();
}

// ---- argv helpers -------------------------------------------------------
// Arguments are now discrete argv elements rather than one concatenated string,
// so assertions match elements in order instead of matching a regex.
private static bool InOrder(List<string> actual, params string[] expected)
{
int idx = 0;
foreach (string e in expected)
{
idx = actual.IndexOf(e, idx);
if (idx < 0) return false;
idx++;
}
return true;
}

private static bool StartsWithAny(List<string> actual, string prefix)
{
return actual.Exists(a => a != null && a.StartsWith(prefix));
}

// ---- regression tests: CWE-88 argument injection ------------------------

@07souravkunda 07souravkunda Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[for-human] Precision point on what the end-to-end evidence covers, worth knowing before sign-off.

The tunnel-session proof cited for this branch (184ff50f0876cdc79c09194dd4ffe606216a3573, verified independently: done, CLIENT_STOPPED_SESSION, local capability active, build locsec-csharp-argv) was produced against 40f1011. The current head is a18e503 — the 12 Aug revert that dropped the option-key format check — and no session has been run against it. The suite count also moved with that commit: eight regression tests at 40f1011, seven here, since the reverted one was optionKeyPattern's.

I do not think this is a real risk. The revert only removes a code path that threw on oddly-shaped option keys; it cannot break input that previously worked, and it touches neither argv construction nor modifyBinaryPermission. So the argv and chmod guarantees the session exercised are unchanged. But the tracker comments attest "PR #61 was verified by the sibling session" without noting that the head moved afterwards, so a reader would reasonably assume the verified commit is the one being merged.

Cheapest resolution is the merge-time ask already on this PR: run CI via workflow_dispatch on windows-latest at a18e503. That covers both this and the per-platform ArgumentList quoting concern in one go, since the only executed run so far was on macOS.

// Each of these fails on the pre-fix code, where every value was concatenated
// into one string that Process.Start then re-tokenised on whitespace.

[TestMethod]
public void TestOptionValueWithSpacesStaysOneArgument()
{
options = new List<KeyValuePair<string, string>>();
options.Add(new KeyValuePair<string, string>("key", "dummyKey"));
options.Add(new KeyValuePair<string, string>("proxyPass", "p@ss --proxy evil.example.com"));

local = new LocalClass();
Mock<BrowserStackTunnel> tunnelMock = new Mock<BrowserStackTunnel>("test-user-agent");
local.setTunnel(tunnelMock.Object);
local.start(options);

tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
InOrder(a, "-proxyPass", "p@ss --proxy evil.example.com")
&& !a.Contains("--proxy"))), Times.Once());
local.stop();
}

[TestMethod]
public void TestUnknownOptionValueWithSpacesStaysOneArgument()
{
options = new List<KeyValuePair<string, string>>();
options.Add(new KeyValuePair<string, string>("key", "dummyKey"));
options.Add(new KeyValuePair<string, string>("customKey", "legit --config /tmp/attacker.cfg"));

local = new LocalClass();
Mock<BrowserStackTunnel> tunnelMock = new Mock<BrowserStackTunnel>("test-user-agent");
local.setTunnel(tunnelMock.Object);
local.start(options);

tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
InOrder(a, "-customKey", "legit --config /tmp/attacker.cfg")
&& !a.Contains("--config"))), Times.Once());
local.stop();
}

[TestMethod]
public void TestLogFilePathWithQuoteStaysOneArgument()
{
options = new List<KeyValuePair<string, string>>();
options.Add(new KeyValuePair<string, string>("key", "dummyKey"));
options.Add(new KeyValuePair<string, string>("logfile", "/tmp/x\" --proxy evil.example.com \""));

local = new LocalClass();
Mock<BrowserStackTunnel> tunnelMock = new Mock<BrowserStackTunnel>("test-user-agent");
local.setTunnel(tunnelMock.Object);
local.start(options);

tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
InOrder(a, "-logFile", "/tmp/x\" --proxy evil.example.com \"")
&& !a.Contains("--proxy"))), Times.Once());
local.stop();
}

[TestMethod]
public void TestAccessKeyWhitespaceIsStrippedFromOptions()
{
options = new List<KeyValuePair<string, string>>();
options.Add(new KeyValuePair<string, string>("key", " dummy Key --proxy evil.example.com "));

local = new LocalClass();
Mock<BrowserStackTunnel> tunnelMock = new Mock<BrowserStackTunnel>("test-user-agent");
local.setTunnel(tunnelMock.Object);
local.start(options);

// Whitespace removed, so no "--proxy" token can split out of the key.
tunnelMock.Verify(mock => mock.Run("dummyKey--proxyevil.example.com", "", logAbsolute, "start"),
Times.Once());
local.stop();
}

[TestMethod]
public void TestAccessKeyWhitespaceIsStrippedFromEnvironmentVariable()
{
Environment.SetEnvironmentVariable("BROWSERSTACK_ACCESS_KEY", "env Dummy\tKey");
options = new List<KeyValuePair<string, string>>();

local = new LocalClass();
Mock<BrowserStackTunnel> tunnelMock = new Mock<BrowserStackTunnel>("test-user-agent");
local.setTunnel(tunnelMock.Object);
local.start(options);

tunnelMock.Verify(mock => mock.Run("envDummyKey", "", logAbsolute, "start"), Times.Once());
local.stop();
}

[TestMethod]
public void TestFolderPathWithSpacesIsPreserved()
{
options = new List<KeyValuePair<string, string>>();
options.Add(new KeyValuePair<string, string>("key", "dummyKey"));
options.Add(new KeyValuePair<string, string>("f", "/my/awesome folder"));

local = new LocalClass();
Mock<BrowserStackTunnel> tunnelMock = new Mock<BrowserStackTunnel>("test-user-agent");
local.setTunnel(tunnelMock.Object);
local.start(options);

tunnelMock.Verify(mock => mock.Run("dummyKey", "/my/awesome folder", logAbsolute, "start"),
Times.Once());
local.stop();
}

[TestMethod]
public void TestDocumentedPassThroughOptionsStillWork()
{
options = new List<KeyValuePair<string, string>>();
options.Add(new KeyValuePair<string, string>("key", "dummyKey"));
options.Add(new KeyValuePair<string, string>("localProxyHost", "127.0.0.1"));
options.Add(new KeyValuePair<string, string>("localProxyPort", "8000"));
options.Add(new KeyValuePair<string, string>("-pac-file", "/tmp/my proxy.pac"));

local = new LocalClass();
Mock<BrowserStackTunnel> tunnelMock = new Mock<BrowserStackTunnel>("test-user-agent");
local.setTunnel(tunnelMock.Object);
local.start(options);

tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is<List<string>>(a =>
InOrder(a, "-localProxyHost", "127.0.0.1", "-localProxyPort", "8000",
"--pac-file", "/tmp/my proxy.pac"))), Times.Once());
local.stop();
}

public void startWithOptions()
{
local.start(options);
Expand Down
Loading
Loading