Skip to content

fix: pass BrowserStackLocal arguments as discrete argv elements (CWE-88) #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 1 commit into
base: master
Choose a base branch
from
Draft

fix: pass BrowserStackLocal arguments as discrete argv elements (CWE-88) #61

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 5 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 @@ -141,7 +142,7 @@ public string getBinaryAbsolute()
{
return binaryAbsolute;
}
public string getBinaryArguments()
public List<string> getBinaryArguments()
{
return binaryArguments;
}
Expand Down
192 changes: 181 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,167 @@ 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 ------------------------
// 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 TestOptionKeyWithWhitespaceIsRejected()
{
options = new List<KeyValuePair<string, string>>();
options.Add(new KeyValuePair<string, string>("key", "dummyKey"));
options.Add(new KeyValuePair<string, string>("foo --proxy evil.example.com", "bar"));

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

Assert.Throws(typeof(ArgumentException), new TestDelegate(startWithOptions));
}

[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
40 changes: 27 additions & 13 deletions BrowserStackLocal/BrowserStackLocal/BrowserStackTunnel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public class BrowserStackTunnel : IDisposable

public int basePathsIndex = -1;
protected string binaryAbsolute = "";
protected string binaryArguments = "";
protected List<string> binaryArguments = new List<string>();

protected StringBuilder output;
public LocalState localState;
Expand Down Expand Up @@ -129,13 +129,9 @@ public virtual void addBinaryPath(string binaryAbsolute, string accessKey, bool
this.binaryAbsolute = binaryAbsolute;
}

public virtual void addBinaryArguments(string binaryArguments)
public virtual void addBinaryArguments(List<string> binaryArguments)
{
if (binaryArguments == null)
{
binaryArguments = "";
}
this.binaryArguments = binaryArguments;
this.binaryArguments = binaryArguments ?? new List<string>();
}

public BrowserStackTunnel(string userAgentParam)
Expand Down Expand Up @@ -165,7 +161,17 @@ public void modifyBinaryPermission()
{
try
{
using (Process proc = Process.Start("/bin/bash", $"-c \"chmod 0755 {this.binaryAbsolute}\""))
// Invoke chmod directly rather than through "bash -c": binaryAbsolute is
// caller-controlled (the "binarypath" option), so interpolating it into a
// shell command line let it break out into arbitrary shell syntax.
ProcessStartInfo chmodStartInfo = new ProcessStartInfo("/bin/chmod")
{
UseShellExecute = false,
CreateNoWindow = true
};
chmodStartInfo.ArgumentList.Add("0755");
chmodStartInfo.ArgumentList.Add(this.binaryAbsolute);
using (Process proc = Process.Start(chmodStartInfo))
{
proc.WaitForExit();
}
Expand Down Expand Up @@ -260,15 +266,18 @@ public void downloadBinary()

public virtual void Run(string accessKey, string folder, string logFilePath, string processType)
{
string arguments = "-d " + processType + " ";
List<string> arguments = new List<string> { "-d", processType };
if (folder != null && folder.Trim().Length != 0)
{
arguments += "-f " + accessKey + " " + folder + " " + binaryArguments;
arguments.Add("-f");
arguments.Add(accessKey);
arguments.Add(folder);
}
else
{
arguments += accessKey + " " + binaryArguments;
arguments.Add(accessKey);
}
arguments.AddRange(binaryArguments);
if (!File.Exists(binaryAbsolute))
{
downloadBinary();
Expand All @@ -286,19 +295,24 @@ public virtual void Run(string accessKey, string folder, string logFilePath, str
RunProcess(arguments, processType);
}

private void RunProcess(string arguments, string processType)
private void RunProcess(List<string> arguments, string processType)
{
// ArgumentList passes each element to the child process as its own argv entry, so a
// value containing whitespace can never shift argument boundaries into extra flags.
ProcessStartInfo processStartInfo = new ProcessStartInfo()
{
FileName = binaryAbsolute,
Arguments = arguments,
CreateNoWindow = true,
WindowStyle = ProcessWindowStyle.Hidden,
RedirectStandardOutput = true,
RedirectStandardError = true,
RedirectStandardInput = true,
UseShellExecute = false
};
foreach (string argument in arguments)
{
processStartInfo.ArgumentList.Add(argument);
}

process = new Process();
process.StartInfo = processStartInfo;
Expand Down
Loading
Loading