fix: build BrowserStackLocal argv as discrete elements; repair no-op access-key strip - #61
fix: build BrowserStackLocal argv as discrete elements; repair no-op access-key strip#6107souravkunda wants to merge 2 commits into
Conversation
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.
07souravkunda
left a comment
07souravkunda
left a comment
There was a problem hiding this comment.
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.
.Argumentsis gone fromRunProcess;ProcessStartInfo.ArgumentListis the only path now, and the project targetsnet6.0, soArgumentListis available (it would not have been onnetstandard2.0/.NET Framework). The access-keyRegex.Replaceresult is assigned back,Trim()ed, and moved outside the empty-key branch so it covers the caller-suppliedoptions["key"]path — which it previously never touched.modifyBinaryPermissionno longer interpolates a caller-controlled path into a shell command line. - Scope is clean. 4 files, all in scope. The
chmodchange 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.RunShellCommandstill uses.Arguments, correctly left alone: it is only ever called with fixed literals (uname,grep -w 'NAME' /etc/os-release). - Consumer sweep.
addBinaryArgumentsis referenced nowhere outside the two test projects, both updated.Example.csandIntegrationTests.csare unaffected by thestring→List<string>signature change. - Every factual claim in the PR body and the tracker checks out. Cited line numbers on
masterare exact; the README pass-through options are genuinely absent fromvalueCommands/booleanCommands; the Node binding does forward unknown args the same way; CI iswindows-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 withAV:L; this is a genuine internal inconsistency in the report, not a v3.1-vs-v4.0 artifact. - Testing is real, not claimed. Session
184ff50f0876cdc79c09194dd4ffe606216a3573verified independently:done,CLIENT_STOPPED_SESSION,localcapability active, buildlocsec-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 untouchedmaster. Semgrep,semgrep/ciand both CodeQL analyses are green on this head. The limitation the author records honestly —TestFolderPathWithSpacesIsPreservedsurvives every mutation becauseBrowserStackTunnel.Runis 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):
hostsis 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.- Pass-through is retained by design, so a caller who controls the
optionslist 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.
| // 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. |
There was a problem hiding this comment.
[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.
| // 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)) |
There was a problem hiding this comment.
[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, andArgumentListkills it foraccessKey,fandlogfiletoo — none of which pass throughaddArgs, so an allowlist would not have protected them at all. - Retained by design. A caller who controls the
optionslist 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.
| argumentList.Add(customLogPath); | ||
| argumentList.Add("--source"); | ||
| argumentList.Add("c-sharp:" + bindingVersion); | ||
| tunnel.addBinaryArguments(argumentList); |
There was a problem hiding this comment.
[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));| if (!result.Equals(emptyStringPair)) | ||
| { | ||
| argumentString += result.Value + " " + value + " "; | ||
| addArgument(result.Value, value); |
There was a problem hiding this comment.
[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.
Reverts the only part of the previous commit that was justified purely by a threat model that does not hold for this library. browserstack-local-csharp is a client-side binding: whoever can set an option key or the access key is the same principal that runs the process and already has arbitrary code execution in it, so smuggling flags into a subprocess they launched themselves escalates nothing. Rejecting an oddly-shaped option key therefore buys no security, while introducing a new exception on input that previously worked. It was also redundant. With arguments built via ProcessStartInfo.ArgumentList, a key containing whitespace becomes a single argv element and cannot split into extra flags regardless - verified against a stub binary that dumps its real argv. What remains in this branch stands on correctness merit alone: - the discarded Regex.Replace result (a strip that normalised nothing) - argv built as discrete elements, which also stops folder and PAC paths containing spaces from being silently split - chmod invoked directly rather than through a shell command line built by string interpolation
07souravkunda
changed the title
1 participant
What this is
Three small correctness fixes in how the binding launches the
BrowserStackLocalsubprocess. This is hardening, not a high-severity security fix — see Threat model below.Changes
1. The access-key whitespace strip did nothing.
Local.start()calledRegex.Replace(this.accessKey, @"\s+", "")and discarded the result. Strings are immutable, so the call normalised nothing — a guard that looked intentional and did zero work. It also sat inside the empty-key branch, so it never ran at all when the key came fromoptions["key"]. Now the result is assigned back,Trim()ed, and applied on both paths.2. Arguments are built as discrete argv elements.
Every value was concatenated into one
ProcessStartInfo.Argumentsstring, which the runtime re-tokenises on whitespace. Besides being fragile, this silently corrupted legitimate input:f: "/my/awesome folder"arrived at the binary as two arguments, so folder testing with a spaced path was broken, and the same applied to PAC file paths. Now built withProcessStartInfo.ArgumentList, so each value is its own argv entry — matching what the Node.js binding already does. Argument boundaries can no longer shift.3.
chmodno longer goes through a shell.Marking the downloaded binary executable ran
bash -c "chmod 0755 {binaryAbsolute}"with the path interpolated unquoted, so any binary path containing a space (easy viaDirectory.GetCurrentDirectory()) broke with a spurious "Error in changing permission". Now invokes/bin/chmoddirectly with discrete arguments.Threat model — why this is hardening
An earlier revision of this branch also added an option-key format check. That has been reverted. This is a client-side library: whoever can set an option value or the access key is the same principal that runs the process and already has arbitrary code execution in it. Smuggling extra flags into a subprocess they launched themselves gains them nothing, so there is no privilege boundary to defend and no justification for rejecting input that previously worked. The change was also redundant — with
ArgumentList, a key containing whitespace becomes a single argv element and cannot split into flags anyway.A strict allowlist of option keys was likewise not implemented. Unknown keys are deliberately forwarded: the README documents
localProxyHost,localProxyPort,localProxyUser,localProxyPassandpac-file— none of which are in the known-key lists — and points users at the full list of BrowserStack Local modifiers. An allowlist would break that documented contract, break the existingTestWorksWithCustomOptionstest, and create an ongoing obligation to mirror the binary's flag list across all six language bindings.Compatibility
Unknown-key pass-through is unchanged. Values that legitimately contain spaces now survive intact instead of being split.
BrowserStackTunnel.addBinaryArgumentschanges fromstringtoList<string>— the internal seam betweenLocalandBrowserStackTunnel; the documentedLocalAPI is unchanged.Verification
master(verified against a clean checkout), so no new failures.master(1 pass / 1 fail; that failure is a macOS-only process-name mismatch — CI runs onwindows-latest).local.start()→ BrowserStack Automate session loading a local-only site over the tunnel →local.stop(), clean shutdown.Please run CI via
workflow_dispatchonwindows-latestbefore merge —ArgumentListquoting is implemented per-platform and this was only executed on macOS.