From 40f10114002d0270bf218ab0167c558b7b7ac46d Mon Sep 17 00:00:00 2001 From: 07souravkunda Date: Fri, 7 Aug 2026 21:32:21 +0530 Subject: [PATCH] fix: pass BrowserStackLocal arguments as discrete argv elements (CWE-88) Option values and the access key were concatenated into a single ProcessStartInfo.Arguments string, which the runtime re-tokenises on whitespace. A value containing a space followed by a "-" token therefore reached the BrowserStackLocal binary as additional command-line flags, letting anything that controls one option value (or the BROWSERSTACK_ACCESS_KEY environment variable) choose flags the documented API never exposes - argument injection, CWE-88. - build the child process command line with ProcessStartInfo.ArgumentList so each option value is its own argv entry and embedded whitespace can never shift argument boundaries - assign the result of the access-key whitespace strip back to the field (strings are immutable, so the previous call discarded its own output) and apply it on both the caller-supplied and environment-variable paths - require an option key to look like a flag before forwarding it - invoke chmod directly instead of through "bash -c" when marking the downloaded binary executable, since the path is caller-controlled Unknown option keys are still forwarded, so the documented pass-through modifiers (localProxyHost, pac-file, ...) keep working. Values that legally contain spaces - folder paths, PAC file paths - are now passed through intact instead of being silently split. --- .../BrowserStackTunnelTests.cs | 9 +- .../LocalTests.cs | 192 +++++++++++++++++- .../BrowserStackLocal/BrowserStackTunnel.cs | 40 ++-- BrowserStackLocal/BrowserStackLocal/Local.cs | 50 ++++- 4 files changed, 254 insertions(+), 37 deletions(-) diff --git a/BrowserStackLocal/BrowserStackLocal Unit Tests/BrowserStackTunnelTests.cs b/BrowserStackLocal/BrowserStackLocal Unit Tests/BrowserStackTunnelTests.cs index 8c83b0a..a8ea58b 100644 --- a/BrowserStackLocal/BrowserStackLocal Unit Tests/BrowserStackTunnelTests.cs +++ b/BrowserStackLocal/BrowserStackLocal Unit Tests/BrowserStackTunnelTests.cs @@ -5,6 +5,7 @@ using NUnit.Framework; using BrowserStack; +using System.Collections.Generic; using System.Text; using System.IO; @@ -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 { "-dummyFlag", "dummyValue" }); + CollectionAssert.AreEqual(new List { "-dummyFlag", "dummyValue" }, tunnel.getBinaryArguments()); } [TestMethod] public void TestBinaryArgumentsAreEmptyOnNull() { tunnel = new TunnelClass(); tunnel.addBinaryArguments(null); - Assert.AreEqual(tunnel.getBinaryArguments(), ""); + Assert.IsEmpty(tunnel.getBinaryArguments()); } @@ -141,7 +142,7 @@ public string getBinaryAbsolute() { return binaryAbsolute; } - public string getBinaryArguments() + public List getBinaryArguments() { return binaryArguments; } diff --git a/BrowserStackLocal/BrowserStackLocal Unit Tests/LocalTests.cs b/BrowserStackLocal/BrowserStackLocal Unit Tests/LocalTests.cs index 918afa9..1346015 100644 --- a/BrowserStackLocal/BrowserStackLocal Unit Tests/LocalTests.cs +++ b/BrowserStackLocal/BrowserStackLocal Unit Tests/LocalTests.cs @@ -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>(a => + InOrder(a, "-logFile", logAbsolute, "--source") && StartsWithAny(a, "c-sharp:"))), Times.Once()); tunnelMock.Verify(mock => mock.Run("dummyKey", "", logAbsolute, "start"), Times.Once()); local.stop(); } @@ -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>(a => + InOrder(a, "-logFile", logAbsolute))), Times.Once()); tunnelMock.Verify(mock => mock.Run("envDummyKey", "", logAbsolute, "start"), Times.Once()); local.stop(); } @@ -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>(a => + InOrder(a, "-logFile", logAbsolute))), Times.Once()); tunnelMock.Verify(mock => mock.Run("dummyKey", "dummyFolderPath", logAbsolute, "start"), Times.Once()); local.stop(); } @@ -108,7 +111,8 @@ public void TestWorksForBinaryPath() local.setTunnel(tunnelMock.Object); local.start(options); tunnelMock.Verify(mock => mock.addBinaryPath("dummyPath", "", It.IsAny(), It.IsAny()), Times.Once); - tunnelMock.Verify(mock => mock.addBinaryArguments(It.IsRegex("-logFile \"" + logAbsolute + "\" .*")), Times.Once()); + tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is>(a => + InOrder(a, "-logFile", logAbsolute))), Times.Once()); tunnelMock.Verify(mock => mock.Run("dummyKey", "", logAbsolute, "start"), Times.Once()); local.stop(); } @@ -130,7 +134,8 @@ public void TestWorksWithBooleanOptions() local.setTunnel(tunnelMock.Object); local.start(options); tunnelMock.Verify(mock => mock.addBinaryPath("", "", It.IsAny(), It.IsAny()), Times.Once); - tunnelMock.Verify(mock => mock.addBinaryArguments(It.IsRegex("-vvv.*-force.*-forcelocal.*-forceproxy.*-onlyAutomate.*")), Times.Once()); + tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is>(a => + InOrder(a, "-vvv", "-force", "-forcelocal", "-forceproxy", "-onlyAutomate"))), Times.Once()); tunnelMock.Verify(mock => mock.Run("dummyKey", "", logAbsolute, "start"), Times.Once()); local.stop(); } @@ -153,8 +158,9 @@ public void TestWorksWithValueOptions() local.setTunnel(tunnelMock.Object); local.start(options); tunnelMock.Verify(mock => mock.addBinaryPath("", "", It.IsAny(), It.IsAny()), 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>(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(); @@ -176,8 +182,9 @@ public void TestWorksWithCustomOptions() local.setTunnel(tunnelMock.Object); local.start(options); tunnelMock.Verify(mock => mock.addBinaryPath("", "", It.IsAny(), It.IsAny()), Times.Once); - tunnelMock.Verify(mock => mock.addBinaryArguments( - It.IsRegex("-customBoolKey1.*-customBoolKey2.*-customKey1.*customValue1.*-customKey2.*customValue2.*") + tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is>(a => + InOrder(a, "-customBoolKey1", "-customBoolKey2", "-customKey1", "customValue1", + "-customKey2", "customValue2")) ), Times.Once()); tunnelMock.Verify(mock => mock.Run("dummyKey", "", logAbsolute, "start"), Times.Once()); local.stop(); @@ -201,7 +208,8 @@ public void TestCallsFallbackOnFailure() local.setTunnel(tunnelMock.Object); local.start(options); tunnelMock.Verify(mock => mock.addBinaryPath("", "", It.IsAny(), It.IsAny()), Times.Once); - tunnelMock.Verify(mock => mock.addBinaryArguments(It.IsRegex("-logFile \"" + logAbsolute + "\" .*")), Times.Once()); + tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is>(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(); @@ -220,7 +228,8 @@ public void TestKillsTunnel() local.start(options); local.stop(); tunnelMock.Verify(mock => mock.addBinaryPath("", "", It.IsAny(), It.IsAny()), Times.Once); - tunnelMock.Verify(mock => mock.addBinaryArguments(It.IsRegex("-logFile \"" + logAbsolute + "\" .*")), Times.Once()); + tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is>(a => + InOrder(a, "-logFile", logAbsolute))), Times.Once()); tunnelMock.Verify(mock => mock.Run("dummyKey", "", logAbsolute, "start"), Times.Once()); } @@ -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 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 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>(); + options.Add(new KeyValuePair("key", "dummyKey")); + options.Add(new KeyValuePair("proxyPass", "p@ss --proxy evil.example.com")); + + local = new LocalClass(); + Mock tunnelMock = new Mock("test-user-agent"); + local.setTunnel(tunnelMock.Object); + local.start(options); + + tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is>(a => + InOrder(a, "-proxyPass", "p@ss --proxy evil.example.com") + && !a.Contains("--proxy"))), Times.Once()); + local.stop(); + } + + [TestMethod] + public void TestUnknownOptionValueWithSpacesStaysOneArgument() + { + options = new List>(); + options.Add(new KeyValuePair("key", "dummyKey")); + options.Add(new KeyValuePair("customKey", "legit --config /tmp/attacker.cfg")); + + local = new LocalClass(); + Mock tunnelMock = new Mock("test-user-agent"); + local.setTunnel(tunnelMock.Object); + local.start(options); + + tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is>(a => + InOrder(a, "-customKey", "legit --config /tmp/attacker.cfg") + && !a.Contains("--config"))), Times.Once()); + local.stop(); + } + + [TestMethod] + public void TestLogFilePathWithQuoteStaysOneArgument() + { + options = new List>(); + options.Add(new KeyValuePair("key", "dummyKey")); + options.Add(new KeyValuePair("logfile", "/tmp/x\" --proxy evil.example.com \"")); + + local = new LocalClass(); + Mock tunnelMock = new Mock("test-user-agent"); + local.setTunnel(tunnelMock.Object); + local.start(options); + + tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is>(a => + InOrder(a, "-logFile", "/tmp/x\" --proxy evil.example.com \"") + && !a.Contains("--proxy"))), Times.Once()); + local.stop(); + } + + [TestMethod] + public void TestOptionKeyWithWhitespaceIsRejected() + { + options = new List>(); + options.Add(new KeyValuePair("key", "dummyKey")); + options.Add(new KeyValuePair("foo --proxy evil.example.com", "bar")); + + local = new LocalClass(); + Mock tunnelMock = new Mock("test-user-agent"); + local.setTunnel(tunnelMock.Object); + + Assert.Throws(typeof(ArgumentException), new TestDelegate(startWithOptions)); + } + + [TestMethod] + public void TestAccessKeyWhitespaceIsStrippedFromOptions() + { + options = new List>(); + options.Add(new KeyValuePair("key", " dummy Key --proxy evil.example.com ")); + + local = new LocalClass(); + Mock tunnelMock = new Mock("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>(); + + local = new LocalClass(); + Mock tunnelMock = new Mock("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>(); + options.Add(new KeyValuePair("key", "dummyKey")); + options.Add(new KeyValuePair("f", "/my/awesome folder")); + + local = new LocalClass(); + Mock tunnelMock = new Mock("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>(); + options.Add(new KeyValuePair("key", "dummyKey")); + options.Add(new KeyValuePair("localProxyHost", "127.0.0.1")); + options.Add(new KeyValuePair("localProxyPort", "8000")); + options.Add(new KeyValuePair("-pac-file", "/tmp/my proxy.pac")); + + local = new LocalClass(); + Mock tunnelMock = new Mock("test-user-agent"); + local.setTunnel(tunnelMock.Object); + local.start(options); + + tunnelMock.Verify(mock => mock.addBinaryArguments(It.Is>(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); diff --git a/BrowserStackLocal/BrowserStackLocal/BrowserStackTunnel.cs b/BrowserStackLocal/BrowserStackLocal/BrowserStackTunnel.cs index f825c29..862c8b2 100644 --- a/BrowserStackLocal/BrowserStackLocal/BrowserStackTunnel.cs +++ b/BrowserStackLocal/BrowserStackLocal/BrowserStackTunnel.cs @@ -45,7 +45,7 @@ public class BrowserStackTunnel : IDisposable public int basePathsIndex = -1; protected string binaryAbsolute = ""; - protected string binaryArguments = ""; + protected List binaryArguments = new List(); protected StringBuilder output; public LocalState localState; @@ -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 binaryArguments) { - if (binaryArguments == null) - { - binaryArguments = ""; - } - this.binaryArguments = binaryArguments; + this.binaryArguments = binaryArguments ?? new List(); } public BrowserStackTunnel(string userAgentParam) @@ -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(); } @@ -260,15 +266,18 @@ public void downloadBinary() public virtual void Run(string accessKey, string folder, string logFilePath, string processType) { - string arguments = "-d " + processType + " "; + List arguments = new List { "-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(); @@ -286,12 +295,13 @@ 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 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, @@ -299,6 +309,10 @@ private void RunProcess(string arguments, string processType) RedirectStandardInput = true, UseShellExecute = false }; + foreach (string argument in arguments) + { + processStartInfo.ArgumentList.Add(argument); + } process = new Process(); process.StartInfo = processStartInfo; diff --git a/BrowserStackLocal/BrowserStackLocal/Local.cs b/BrowserStackLocal/BrowserStackLocal/Local.cs index 7d27d5e..02b2730 100644 --- a/BrowserStackLocal/BrowserStackLocal/Local.cs +++ b/BrowserStackLocal/BrowserStackLocal/Local.cs @@ -11,7 +11,7 @@ public class Local private string folder = ""; private string accessKey = ""; private string customLogPath = ""; - private string argumentString = ""; + private List argumentList = new List(); private string customBinaryPath = ""; private string bindingVersion = ""; private string userAgent = "browserstack-local-csharp"; @@ -40,12 +40,27 @@ public class Local new KeyValuePair("onlyAutomate", "-onlyAutomate"), }; + // An option key is forwarded to the binary as a flag, so it must look like one. + // Anything carrying whitespace (or other argument-delimiter characters) is rejected + // rather than being smuggled into the child process argv. A leading "-"/"--" is + // allowed because the README documents keys in that form (e.g. "-pac-file"). + private static readonly Regex optionKeyPattern = new Regex(@"^-{0,2}[A-Za-z0-9][A-Za-z0-9._-]*$"); + public bool isRunning() { if (tunnel == null) return false; return tunnel.IsConnected(); } + // Appends one flag/value pair as DISCRETE argv elements. Keeping them separate is + // what stops an embedded space in a value from being re-tokenised into extra flags. + private void addArgument(string flag, string value) + { + // "hosts" maps to an empty flag name: its value is positional, so emit no flag. + if (!string.IsNullOrEmpty(flag)) argumentList.Add(flag); + if (value != null) argumentList.Add(value); + } + private void addArgs(string key, string value) { KeyValuePair result; @@ -92,7 +107,7 @@ private void addArgs(string key, string value) result = valueCommands.Find(pair => pair.Key == key); if (!result.Equals(emptyStringPair)) { - argumentString += result.Value + " " + value + " "; + addArgument(result.Value, value); return; } @@ -101,18 +116,29 @@ private void addArgs(string key, string value) { if (value.Trim().ToLower() == "true") { - argumentString += result.Value + " "; + addArgument(result.Value, null); return; } } + // Unrecognised keys are still forwarded: the binding deliberately passes through + // BrowserStackLocal modifiers it does not know about (see README, "for the full + // list of modifiers"), and documented options such as localProxyHost and pac-file + // arrive here. Validate the key's shape instead of rejecting it outright. + if (!optionKeyPattern.IsMatch(key)) + { + throw new ArgumentException( + "Invalid BrowserStackLocal option key: \"" + key + "\". Option keys may contain " + + "only letters, digits, '.', '_' and '-'."); + } + if (value.Trim().ToLower() == "true") { - argumentString += "-" + key + " "; + addArgument("-" + key, null); } else { - argumentString += "-" + key + " " + value + " "; + addArgument("-" + key, value); } } } @@ -229,17 +255,23 @@ public void start(List> options) throw new Exception("BROWSERSTACK_ACCESS_KEY cannot be empty. " + "Specify one by adding key to options or adding to the environment variable BROWSERSTACK_ACCESS_KEY."); } - Regex.Replace(this.accessKey, @"\s+", ""); } + // Strip whitespace from the access key on BOTH paths (caller-supplied and env var). + // The result must be assigned back - strings are immutable, so the previous call + // discarded its own output and normalised nothing. + accessKey = Regex.Replace(accessKey.Trim(), @"\s+", ""); + if (customLogPath == null || customLogPath.Trim().Length == 0) { customLogPath = Path.Combine(BrowserStackTunnel.basePaths[1], "local.log"); } - argumentString += "-logFile \"" + customLogPath + "\" "; - argumentString += "--source \"c-sharp:" + bindingVersion + "\" "; - tunnel.addBinaryArguments(argumentString); + argumentList.Add("-logFile"); + argumentList.Add(customLogPath); + argumentList.Add("--source"); + argumentList.Add("c-sharp:" + bindingVersion); + tunnel.addBinaryArguments(argumentList); tunnel.SetProxy(proxyHost, proxyPort); DownloadVerifyAndRunBinary();