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

[07souravkunda]

Problem

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 — so anything able to set one option value, or the BROWSERSTACK_ACCESS_KEY environment variable, could choose flags the documented API never exposes (CWE-88, argument injection).

Two things made it reachable:

• Local.start() called Regex.Replace(this.accessKey, @"\s+", "") but discarded the result (strings are immutable), so the strip was a no-op. It also only ran on the environment-variable path — a caller-supplied options["key"] was never normalised at all.
• Every option value, including unrecognised keys forwarded as -<key> <value>, was appended to one flat string that the OS then split.

Fix

• Build the child process command line with ProcessStartInfo.ArgumentList, so each value is its own argv entry and embedded whitespace can never shift argument boundaries. This is the same shape the Node.js binding already uses.
• Assign the access-key whitespace strip back to the field, and apply it on both the caller-supplied and environment-variable paths.
• Require an option key to look like a flag (^-{0,2}[A-Za-z0-9][A-Za-z0-9._-]*$) before forwarding it.
• Invoke chmod directly instead of through bash -c when marking the downloaded binary executable — that path is caller-controlled via binarypath, and interpolating it into a shell command line allowed breakout into arbitrary shell syntax.

Compatibility

Unknown option keys are still forwarded, so the documented pass-through modifiers (localProxyHost, localProxyPort, pac-file, …) keep working — a strict key allowlist would have broken them and the "full list of modifiers" contract in the README.

Values that legitimately contain spaces — folder paths, PAC file paths — are now passed through intact instead of being silently split, fixing a pre-existing bug.

BrowserStackTunnel.addBinaryArguments now takes List<string> instead of string. This is the one API-surface change; it is the internal seam between Local and BrowserStackTunnel.

Verification

• 8 new regression tests, all passing. Each was mutation-verified: reverting the corresponding behaviour makes them fail.
• Unit suite: 20 passed, with the same 11 pre-existing failures that master already has (verified by running the suite on an untouched checkout — no new failures).
• Integration suite: identical to master (1 pass / 1 fail; the failure is a macOS-only process-name mismatch — CI runs on windows-latest).
• Real tunnel session driven through the binding: local.start() → BrowserStack Automate session loading a local-only site over the tunnel → local.stop(), clean shutdown. Session 184ff50f0876cdc79c09194dd4ffe606216a3573.
• An argv-level harness (a stub binary that dumps its real argv) confirmed 7/7 injection payloads succeed before this change and none after, while legitimate options still produce the same argv.

[07souravkunda]

Independent security review of this CWE-88 fix. No blocking findings — 2 items for a human decision, 2 nits. Not approving: a human owns approval.

What I verified against origin (fresh clone of the branch, not a working tree):

• The fix actually closes the injection. .Arguments is gone from RunProcess; ProcessStartInfo.ArgumentList is the only path now, and the project targets net6.0, so ArgumentList is available (it would not have been on netstandard2.0/.NET Framework). The access-key Regex.Replace result is assigned back, Trim()ed, and moved outside the empty-key branch so it covers the caller-supplied options["key"] path — which it previously never touched. modifyBinaryPermission no longer interpolates a caller-controlled path into a shell command line.
• Scope is clean. 4 files, all in scope. The chmod change is adjacent rather than unrelated — same caller-controlled input class (binarypath), reaching a shell instead of argv — and it is disclosed in the PR body. Util.RunShellCommand still uses .Arguments, correctly left alone: it is only ever called with fixed literals (uname, grep -w 'NAME' /etc/os-release).
• Consumer sweep. addBinaryArguments is referenced nowhere outside the two test projects, both updated. Example.cs and IntegrationTests.cs are unaffected by the string → List<string> signature change.
• Every factual claim in the PR body and the tracker checks out. Cited line numbers on master are exact; the README pass-through options are genuinely absent from valueCommands/booleanCommands; the Node binding does forward unknown args the same way; CI is windows-latest; the package version is 3.1.0. The CVSS arithmetic is right too — the quoted vector recomputes to 9.1 under CVSS v3.1, and 7.7 with AV:L; this is a genuine internal inconsistency in the report, not a v3.1-vs-v4.0 artifact.
• Testing is real, not claimed. Session 184ff50f0876cdc79c09194dd4ffe606216a3573 verified independently: done, CLIENT_STOPPED_SESSION, local capability active, build locsec-csharp-argv, started ~5 min before this PR was opened. 8 new regression tests, mutation-verified, correct for a code-level fix. The 11 unit failures are named and baseline-matched against untouched master. Semgrep, semgrep/ci and both CodeQL analyses are green on this head. The limitation the author records honestly — TestFolderPathWithSpacesIsPreserved survives every mutation because BrowserStackTunnel.Run is mocked at that layer — is accurate, and the argv harness is the stronger evidence for that guarantee.

For a human to decide (neither is a code defect):

1. hosts is the one option whose value was legitimately whitespace-delimited — it is emitted as a bare positional and the binary's positional form takes a list. Multi-host previously worked via the concatenation and now arrives as one token. Not covered by the harness or the new tests.
2. Pass-through is retained by design, so a caller who controls the options list can still choose arbitrary binary flags — only the key's shape is constrained. Defensible (that caller already runs code in the process), but the shipped remediation is deliberately not the one the report asked for, and that should be on record before the findings are signed off.

Agreed with the author's ask: run CI via workflow_dispatch on windows-latest before merge — ArgumentList re-quoting is per-platform and this was only executed on macOS.

[07souravkunda]

[for-human] hosts is the one option whose value was legitimately whitespace-delimited, so it is the one place where "stop splitting on whitespace" is a behaviour change rather than a fix.

valueCommands maps hosts → "", i.e. its value is emitted as a bare positional argument, and the binary's positional form takes a list: BrowserStackLocal <key> host1,port1,ssl1 host2,port2,ssl2. Before this change, hosts: "a,80,0 b,443,1" was concatenated into the flat argument string and the OS tokenizer split it back into two positional argv elements — multi-host worked by accident of the concatenation. After this change it becomes a single argv element containing a space.

Every other value (proxyPass, logfile, f, localIdentifier, unknown keys) is a single scalar, so preserving embedded whitespace there is strictly correct. hosts is the exception.

Question for author: does the BrowserStackLocal binary accept a space-containing positional host spec as one token, and does anyone pass more than one host through this binding? hosts isn't documented in this repo's README, and the Node binding sends its equivalent (--only) as a single element — so this may well be a non-issue. But it isn't covered by the argv harness or the new regression tests, and it's the one place this change could plausibly regress an existing user.

[07souravkunda]

[for-human] Worth being explicit about what this check does and does not close, since the reported finding's first remediation bullet asked for a strict key allowlist and what shipped is a key format check.

Keeping pass-through is the right call and the rationale in the PR body holds up: localProxyHost / localProxyPort / localProxyUser / localProxyPass / -pac-file are all documented in the README and none of them appear in valueCommands or booleanCommands, so an allowlist would have broken documented options in a published package — and TestWorksWithCustomOptions encodes that pass-through contract.

The consequence is that the two halves of the reported issue are closed to different depths:

• Closed. An attacker who controls only a value (or BROWSERSTACK_ACCESS_KEY) can no longer shift argument boundaries. That was the actual injection primitive, and ArgumentList kills it for accessKey, f and logfile too — none of which pass through addArgs, so an allowlist would not have protected them at all.
• Retained by design. A caller who controls the options list itself can still choose an arbitrary binary flag (options["log-file"] = "/some/path" → -log-file /some/path); only the key's shape is constrained here.

The second one is defensible — anyone who can add entries to the caller's options list is already executing code in the test process, so argv control isn't an escalation. But that's a security-owner call rather than a code call, and it's worth having on record before the underlying findings are signed off, since their stated remediation is deliberately not what shipped.

[07souravkunda]

[nit] addBinaryArguments now stores a live reference to Local's own argumentList, where the old string was immutable and so gave the tunnel its own snapshot.

Nothing is broken today — BrowserStackTunnel only reads it (arguments.AddRange(binaryArguments)). But argumentList is instance state that is never reset, so a second start() on the same Local re-appends -logFile … --source … and now mutates the tunnel's list in place as a side effect. The double-append predates this PR; the aliasing is new.

Cheapest way to close it, if you want to:

tunnel.addBinaryArguments(new List<string>(argumentList));

[07souravkunda]

[nit] Minor asymmetry worth a conscious decision: an empty value now survives as a real argv element where it used to disappear.

("localIdentifier", "") previously produced -localIdentifier in the flat string and the tokenizer collapsed the empty value away, so the binary saw just -localIdentifier. Now it receives -localIdentifier followed by an empty argument (rendered "" on Windows). Same for ("hosts", ""), which becomes a stray empty positional.

Plausible in CI where an option is wired to an env var that happens to be unset. if (!string.IsNullOrEmpty(value)) in addArgument would restore the old shape — though passing the empty value through is arguably the more honest behaviour, so this is a judgement call, not a defect.