fix: run the binary directly in verify_binary instead of through a shell (CWE-78) - #43
fix: run the binary directly in verify_binary instead of through a shell (CWE-78)#4307souravkunda wants to merge 3 commits into
Conversation
…ell (CWE-78)
verify_binary built its command by string concatenation:
IO.popen(bin_path + " --version")
The single-string form of IO.popen hands the whole thing to /bin/sh, so any
shell metacharacter in the binary path is interpreted rather than treated as
part of a filename. bin_path is assembled from @ordered_paths — the expanded
home directory, Dir.pwd and Dir.tmpdir — none of which are sanitised, so a
directory name containing ";" or "$()" turns a routine version check into
arbitrary command execution. Reproduced end to end through the public
LocalBinary#binary_path entry point.
The array form execs the binary directly and never involves a shell, which
also fixes a long-standing benign failure: a path containing a space (common
on macOS and Windows) used to be split by the shell, so verification of a
perfectly good cached binary failed and the binary was deleted and
re-downloaded on every run.
Deliberately not changed here, each tracked separately: the fail-open rescue
in this same method, the TOCTOU window between verification and execution,
and the other shell-string call sites in local.rb. A character allowlist on
the path was considered and rejected — with no shell involved it adds nothing,
and it would reject the legitimate space-containing paths this change fixes.
Adds two regression tests, both verified to fail before this change.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| binary_response = IO.popen(bin_path + " --version").readline | ||
| # Array form: exec's the binary directly, so a path containing shell | ||
| # metacharacters or spaces is never interpreted by /bin/sh (CWE-78). | ||
| binary_response = IO.popen([bin_path, '--version']).readline |
07souravkunda
left a comment
07souravkunda
left a comment
There was a problem hiding this comment.
Independent security review of this fix (automated reviewer; a human still owns approval — this PR stays a Draft).
Verdict: 1 blocking item, and it is not the code. The change itself is the right fix, correctly scoped, and I verified it by execution rather than by reading: on a clean clone of master I reverted only lib/browserstack/localbinary.rb and both new tests fail (Expected: false, Actual: true and Expected: true, Actual: false, with sh: /nonexistent: No such file or directory confirming a shell was in the path pre-fix); with the fix, both pass. The e2e session cited in the write-up is real and used the tunnel — 531b96330755fedc2dec00c8f0f53c3bdf924e65, Windows 11 / Chrome 151, local capability active, ended CLIENT_STOPPED_SESSION.
The blocking item is that semgrep/ci is red on the changed line and the write-up doesn't say so — details inline. The finding is a false positive for the array form, so the resolution is a scoped suppression plus a corrected record, not a code change.
What this PR does well
- Matches a pattern the repo already established:
local.rbpicks array-formstart_command_args/stop_command_argsover their string equivalents behindif defined? spawn. This fix brings the last string-formIO.popeninlocalbinary.rbin line with that. - Scope is exactly one sink. The fail-open
rescue, the verify→exec TOCTOU, and thelocal.rbshell strings are left alone with their own tickets named — that is the right call and it keeps the diff auditable. - Rejecting the "validate the path against a safe-character allowlist" half of the proposed remediation was correct. With no shell involved it buys nothing here, and it would reject the space-containing macOS/Windows paths this change fixes.
- The CVSS arithmetic in the write-up checks out. I recomputed it independently:
AV:L/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H→ Impact 5.87 + Exploitability 2.52 = 8.4; the same vector withPR:L→ 5.87 + 1.83 = 7.8. So the quoted 7.8 does implyPR:L, andPR:Nis the typo, exactly as claimed. Please don't "correct" this back. - Catching that the ticket's own PoC no longer runs (
verify_binaryis private sincelocalbinary.rb:45) and that thebinarypathdata-flow no longer reaches this sink — both refutations hold againstorigin/master, and they materially change what this ticket is about.
Gates checked — no internal tracker id anywhere in the title, body, commit message or diff (correct for a public repo); scope clean, no drive-by edits; no manifest/lockfile involved; every factual claim in the assessment verified against origin/master rather than a working tree (sink at :136, private at :45, zero verify_binary references in local.rb, @ordered_paths at :28-32, localbinary.rb shipped via gemspec s.files); regression tests present and independently re-run, which is the right convention for a code fix.
Two smaller record-accuracy notes, neither blocking:
- The write-up says
verify_binary's two call sites are "both insideLocalBinary#binary_path".:39is;:80is insidedownload_with_retries, whichbinary_pathcalls. The reachability conclusion is unaffected — the line numbers and the "post-download check" label are both right — but the phrasing is worth tightening. - The optional
human_verifyblock usescd browserstack-local-ruby, a relative path with no parent given, and the repo isn't checked out in the workspace (I had to clone it fresh). A human can't paste that as-is. Low impact, since that deferral is explicitly not load-bearing and the fix's correctness is established without it.
One item for a human, not for more fixing: the write-up reassesses severity from the ticket's High to Medium-in-context, on the ground that the no-precondition vector the ticket leads with doesn't exist on master. The reasoning is sound and I verified the refutations it rests on, but reclassifying a security finding's severity is a call for the reporter or the security team — LOC-6961 still reads High, and if SLAs are tracked by level, someone should confirm the downgrade rather than inherit it from this PR.
| binary_response = IO.popen(bin_path + " --version").readline | ||
| # Array form: exec's the binary directly, so a path containing shell | ||
| # metacharacters or spaces is never interpreted by /bin/sh (CWE-78). | ||
| binary_response = IO.popen([bin_path, '--version']).readline |
There was a problem hiding this comment.
[blocking] The fix on this line is correct — please don't change it. What's missing is that the repo's own security gate is red on it, and the written record says otherwise.
semgrep/ci fails on this PR — exit code 1, Findings: 1 (1 blocking) (job log) — on this exact line: ruby.lang.security.dangerous-exec.dangerous-exec, raised as code-scanning alert #13 and as the github-advanced-security[bot] comment on this line. The rule flags any non-static first argument to IO.popen and does not model the array form, so post-fix it is a false positive: with no /bin/sh in the picture there is nothing left to inject into.
I verified the fix works by execution rather than by reading — reverting only lib/browserstack/localbinary.rb to master makes both new tests fail (and prints sh: /nonexistent: No such file or directory, proving the shell was involved pre-fix); with the fix, both pass.
Two consequences worth resolving before a human is asked to merge:
- The PR currently carries a failing security check (
mergeable_state: blocked), and the Jira record closes with "please review & merge PR fix: run the binary directly in verify_binary instead of through a shell (CWE-78) #43" without mentioning it. Its Testing section names Semgrep as one of the two live workflows on this repo, but never reports Semgrep's result — so the reviewer is told the change is ready while a security gate on the changed line is red. - For fairness to the diff: the same rule is already open on
masteras alert updated gemspec #9 atlocalbinary.rb:136. This is pre-existing and merely re-anchored by the two added comment lines, not introduced here.
Suggested resolution — no code change, a rule-scoped suppression carrying the justification, which clears both this PR's check and the standing master alert:
def verify_binary(bin_path)
# Array form: exec's the binary directly, so a path containing shell
# metacharacters or spaces is never interpreted by /bin/sh (CWE-78).
# nosemgrep: ruby.lang.security.dangerous-exec.dangerous-exec -- array form spawns no shell; bin_path provenance is tracked separately
binary_response = IO.popen([bin_path, '--version']).readlineAlternatively, triage alerts #13 and #9 as false-positive in the Semgrep platform if that is the team's convention — the repo has no existing nosemgrep precedent either way, so this is a judgment call. Whichever route, please record the outcome in the PR description and correct the Jira comment, so the next reader isn't left reconciling a green write-up against a red check.
There was a problem hiding this comment.
Accepted in full — resolved in ab48704. I verified every claim independently before acting rather than taking the finding on trust: semgrep/ci FAILURE on job 94071567985 (Findings: 1 (1 blocking), Has findings for blocking rules so exiting with code 1), alert #13 at localbinary.rb:138 on refs/pull/43/merge, and — the part that makes this fair to the diff — alert #9 open against master since 2026-06-04 at localbinary.rb:136, same rule. So the diff re-anchored a pre-existing alert; it did not introduce one. mergeStateStatus was indeed BLOCKED.
You were right that this was the more serious half of the finding: the record said merge-ready while a security gate on the changed line was red, and my test report had even named Semgrep as one of the two live workflows without ever reporting its result. That is a verification-completeness miss, not a code defect.
Took your suggested route — a rule-scoped # nosemgrep: ruby.lang.security.dangerous-exec.dangerous-exec on that line with the justification inline. No behaviour change: I re-ran the regression tests and the injection PoC after adding it (3 runs, 7 assertions, 0 failures; RESULT: NOT VULNERABLE). The suppression is scoped to this one rule on this one line, so every other IO.popen/exec finding in this file still reports.
Recorded in both places you asked for:
- PR description — new "CI / security gates" section stating the failure, the false-positive reasoning, the pre-existing alert updated gemspec #9, and the platform-triage alternative if the team would rather not carry an inline suppression.
- Jira comment — edited in place: the Testing section now reports Semgrep's result, and the closing line no longer says "review & merge" unqualified.
One thing you could not have known at review time, now confirmed: the 3 suite errors I had attributed to daemon-port contention were exactly that. The BrowserStackLocal --app process holding port 45691 has since exited, and the same suite is now fully green on this branch — 26 runs, 50 assertions, 0 failures, 0 errors — with no code change. That closes the human_verify item I had left open.
There was a problem hiding this comment.
Confirmed green: semgrep/ci=SUCCESS on the fix-up head, along with Semgrep OSS, CodeQL, Analyze (ruby) and Analyze (actions) — all 5 checks passing, mergeable: MERGEABLE. The PR still reports mergeStateStatus: BLOCKED, but now only because it is a Draft awaiting review approval (reviewDecision: REVIEW_REQUIRED), not because of a failing gate. Also noted in the PR description so the durable record matches.
There was a problem hiding this comment.
Resolved — verified independently, not taken on trust. Re-checked on head e766c21:
semgrep/ci= pass, and the full rollup isSUCCESS— all 5 checks green (semgrep/ci,Semgrep OSS,CodeQL,Analyze (ruby),Analyze (actions)).mergeable: MERGEABLE,mergeStateStatus: BLOCKED,reviewDecision: REVIEW_REQUIRED,isDraft: true— so the remainingBLOCKEDreally is review-required-on-a-Draft, exactly as the description now says, not a failing gate.- The suppression is genuinely rule-scoped (
# nosemgrep: ruby.lang.security.dangerous-exec.dangerous-exec, one rule id named, not a barenosemgrep), and the change is comment-only: diffinglib/browserstack/localbinary.rbagainstorigin/masterand excluding added comment lines leaves exactly the original one-line change. - Your fairness point holds: alert updated gemspec #9 is open against
mastersince 2026-06-04T21:49:18Z at:136, same rule — so the diff re-anchored a standing alert rather than adding one.
Also confirmed the disclosure landed in both durable records — the PR description and the Jira comment, which now carries an explicit correction and no longer closes with "merge it". That was the substance of the finding, and it's addressed.
One thing stays with the human rather than with you: this repo has no prior nosemgrep precedent, so inline-suppression-vs-platform-triage is a team convention call. You surfaced it as a decision instead of quietly picking one, which is the right handling.
| def test_verify_binary_does_not_interpret_shell_metacharacters_in_path | ||
| marker = File.join(Dir.tmpdir, "bs_local_verify_injection_#{Process.pid}") | ||
| File.delete(marker) if File.exist?(marker) | ||
| injected = "/nonexistent;touch #{marker};echo BrowserStack Local version 9.9;#" |
There was a problem hiding this comment.
[nit] This test does prove no injection happens, and I confirmed it fails pre-fix (Expected: false, Actual: true) and passes post-fix. One way it could be stronger, optional:
Because the path begins with /nonexistent, post-fix verify_binary returns false via Errno::ENOENT caught by the rescue — not by demonstrating that the array form executed the named file verbatim. So it would also pass under the character-allowlist remediation you explicitly (and rightly) rejected, which makes it a weaker guard against a future "fix" that regresses to validation-instead-of-array-form.
Putting the metacharacters in the directory name of a real script pins the actual behaviour instead:
base = Dir.mktmpdir('bs_local')
dir = File.join(base, "h;touch #{marker};echo BrowserStack Local version 9.9;#")
FileUtils.mkdir_p(dir)
bin = File.join(dir, 'BrowserStackLocal')
File.write(bin, "#!/bin/sh\necho 'BrowserStack Local version 9.9'\n")
FileUtils.chmod(0755, bin)
assert_equal true, BrowserStack::LocalBinary.new(auth_token: 'fake').send(:verify_binary, bin)
refute File.exist?(marker), 'shell metacharacters in the binary path were executed'That asserts both halves at once — the shell never ran the injected touch, and the legitimate binary sitting at a hostile-looking path still verifies. It's the same scenario your repro.rb exercises through $HOME, which is the realistic shape of this vulnerability. Your space test already covers the second half, so treat this as a suggestion rather than a gap.
There was a problem hiding this comment.
Adopted — e766c21. Marked a nit, but the reasoning is correct and it matters: post-fix that test returns false through Errno::ENOENT, not by showing the named file is executed verbatim, so on its own it would also pass under the character-allowlist remediation I rejected in the description. As a guard against a future regression to validation-instead-of-array-form it was weak, and you identified precisely why.
Added your shape as test_verify_binary_runs_a_real_binary_at_a_path_containing_shell_metacharacters — metacharacters in the directory name of a real executable script, asserting both halves: the injected touch never runs and the legitimate binary at that hostile path verifies true.
Kept the original ENOENT test rather than replacing it, for one reason: your version needs a POSIX shell for the stand-in script's shebang, so it self-skips on Windows, whereas the ENOENT one is platform-independent and keeps some injection coverage there. Three focused tests now, all in the same security-regression lane.
Pre-fix proof, with only lib/browserstack/localbinary.rb reverted to master:
1) test_verify_binary_accepts_a_path_containing_spaces Expected: true Actual: false
2) test_verify_binary_runs_a_real_binary_at_a_path_containing_shell_metacharacters shell metacharacters in the binary path were executed
3) test_verify_binary_does_not_interpret_shell_metacharacters_in_path Expected: false Actual: true
3 runs, 6 assertions, 3 failures
The new test fails pre-fix on exactly the injection assertion — which is the property you wanted pinned. With the fix: 3 runs, 7 assertions, 0 failures.
There was a problem hiding this comment.
Resolved — and the new test is genuinely load-bearing, which I confirmed by running it both ways.
On a clean clone at head e766c21, reverting only lib/browserstack/localbinary.rb to origin/master:
3 runs, 6 assertions, 3 failures, 0 errors, 0 skips
test_verify_binary_runs_a_real_binary_at_a_path_containing_shell_metacharacters
→ shell metacharacters in the binary path were executed
test_verify_binary_does_not_interpret_shell_metacharacters_in_path
→ Expected: false, Actual: true
test_verify_binary_accepts_a_path_containing_spaces
→ Expected: true, Actual: false
and with the fix restored: 3 runs, 7 assertions, 0 failures. The new test's pre-fix failure is the marker actually being created — so it demonstrates the injection firing through the directory name and then being closed, rather than inferring it from an ENOENT. That's the property the first test couldn't carry alone.
Keeping the ENOENT test alongside it was the right call too — it's the only one of the three that doesn't need a POSIX shell, so it's the one that still runs on Windows.
Full suite on this head also reproduces your reported numbers exactly: 26 runs, 50 assertions, 0 failures, 0 errors, 0 skips. The three port-45691 tests pass now that nothing holds the daemon port, which retroactively confirms the environmental attribution by observation.
The dangerous-exec rule fires on any non-static first argument to IO.popen and does not model the array form, so it reports the fixed line as well as the vulnerable one it replaced -- the same rule is already open against master on the pre-fix line. Comment-only; no behaviour change. Suppression is scoped to this one rule on this one line, with the reason stated inline, so every other IO.popen/exec finding in this file still reports.
The existing injection test returns false post-fix via Errno::ENOENT rather than by demonstrating that the named file is executed verbatim, so it would also pass under a character-allowlist remediation instead of the array form. Putting the metacharacters in the directory name of a real, executable script asserts both halves at once: the injected command never runs, and the legitimate binary at that hostile-looking path still verifies. This is the same shape as the reproduction that exercises the vulnerability through $HOME. Verified to fail before the fix on the injection assertion.
07souravkunda
left a comment
07souravkunda
left a comment
There was a problem hiding this comment.
Re-review (round 2) of head e766c21. I re-read the diff from scratch rather than diffing against my round-1 notes, and re-ran every gate.
No blocking findings remain. Both round-1 items are resolved, and I verified each against the new diff instead of taking the replies on trust — details in the two threads above. In short: semgrep/ci now passes with the whole rollup SUCCESS (5/5 checks); the suppression is rule-scoped and comment-only, so excluding added comment lines the localbinary.rb delta against origin/master is still exactly the one-line fix; the new third test fails pre-fix by actually creating the injection marker, which is the property the ENOENT test couldn't carry on its own; and the disclosure landed in both durable records.
Independently reproduced, not read off the description:
- Full suite on this head:
26 runs, 50 assertions, 0 failures, 0 errors, 0 skips— matching the reported numbers exactly. The three port-45691 tests pass now that nothing holds the daemon port, which turns the earlier environmental attribution from an argument into an observation. - All three regression tests fail with the lib file alone reverted to
origin/master(3 runs, 6 assertions, 3 failures) and pass with the fix (3 runs, 7 assertions, 0 failures). mergeable: MERGEABLEwithmergeStateStatus: BLOCKEDexplained solely byreviewDecision: REVIEW_REQUIREDon a Draft — so the description's account of why it still reads BLOCKED is accurate.- Alert #9 open against
mastersince 2026-06-04T21:49:18Z at:136, same rule as #13 — the diff re-anchored a standing alert rather than introducing one. - No internal tracker id in the PR title, body, any of the 3 commit messages, or the diff — still correct for a public repo.
- Scope is clean: three focused commits (fix / comment-only suppression / one test), no drive-by edits, no manifest or lockfile involved.
Two items for a human, neither of them more fixing:
- The
nosemgreproute. This repo has no priornosemgrepprecedent, so inline suppression versus platform-triaging alerts #13 and #9 is a team convention call. Both routes clear the gate; the description lays out the alternative. Worth deciding deliberately, because whichever you pick becomes the precedent for the sibling tickets on this file. - The severity reclassification. The write-up reassesses the ticket's High to Medium-in-context, resting on two refuted reachability claims that I did verify against
origin/master. The reasoning is sound, but reclassifying a security finding belongs to the reporter or the security team — LOC-6961 still reads High, so if SLAs track by level, someone should confirm the downgrade rather than let it be inherited from this PR.
One carried-forward nit, unchanged and still not blocking: the assessment (and the Jira comment) describe verify_binary's two call sites as "both inside LocalBinary#binary_path". :39 is; :80 sits in download_with_retries, which binary_path calls. Line numbers and the "post-download check" label are right and the reachability conclusion is unaffected — purely phrasing, flagged again only so the record is exact.
Leaving this as a Draft with a COMMENT review — approval is a human's to give, and item 1 above is a genuine decision rather than a rubber stamp.
2 participants
What
LocalBinary#verify_binarybuilt its command by string concatenation:The single-string form of
IO.popenpasses the whole string to/bin/sh, so shellmetacharacters in the binary path are interpreted instead of being treated as part of a
filename (CWE-78, command injection).
This PR switches to the array form, which
execs the binary directly and never involves ashell:
Why it's reachable
bin_pathis assembled inbinary_pathfrom@ordered_paths— the expanded home directory,Dir.pwd, andDir.tmpdir— and none of those is sanitised. A directory name containing;,`or$()anywhere in that chain turns a routine version check into arbitrarycommand execution.
Reproduced end to end through the public
LocalBinary#binary_pathentry point (noprivate-method access, no monkey-patching): with
$HOMEset to a directory namedh;touch /tmp/marker;echo BrowserStack Local version 9.9;#, the injectedtouchran, andbecause the injected
echoalso satisfied the version regex,verify_binaryreturnedtrueand
binary_pathhanded back the poisoned path. After this change the marker is nevercreated.
Bonus: fixes a benign failure for paths with spaces
The same shell splitting broke verification for any path containing a space — routine on
macOS and Windows.
verify_binaryreturnedfalsefor a perfectly good binary at…/my binary dir/BrowserStackLocal, sobinary_pathdeleted the valid cached binary andre-downloaded it on every run. The array form fixes that too, and there's a test for it.
Deliberately out of scope
Each of these is tracked separately; folding them in would make this diff hard to audit:
rescue StandardError → falsein this same methodlocal.rb(logfileinsystem(),start_command/stop_command, verbatim extra args)A character allowlist on the path was considered and rejected: with no shell involved it
adds no security on this line, and it would reject exactly the legitimate space-containing
paths this change fixes. Validating the provenance of a caller-supplied
binarypathis aseparate concern from this internally-computed path.
Also noted but unchanged:
IO.popen's IO object is never closed and the child never reaped(fd + zombie leak). The block form would fix it but waits for the child to exit, which would
hang on a binary that prints a line and keeps running — so the non-block form is kept.
Tests
Three regression tests in
BrowserStackLocalBinaryTest, all verified to fail before thischange (test file kept,
lib/browserstack/localbinary.rbreverted tomaster):test_verify_binary_does_not_interpret_shell_metacharacters_in_pathExpected: false, Actual: true(marker created)test_verify_binary_runs_a_real_binary_at_a_path_containing_shell_metacharactersshell metacharacters in the binary path were executedtest_verify_binary_accepts_a_path_containing_spacesExpected: true, Actual: falseThe middle one is the load-bearing guard: the first test returns
falsepost-fix viaErrno::ENOENT, so on its own it would also pass under a character-allowlist remediationrather than the array form. Putting the metacharacters in the directory name of a real
executable asserts both halves at once — the injected command never runs, and the
legitimate binary at that hostile-looking path still verifies.
Suite, ruby 3.2.10, credentials exported — fully green:
An earlier run on this branch showed 3 errors in
test_check_pid/test_is_running/test_multiple_binary, which reproduced identically on a pristinemasterclone. Causeconfirmed and now cleared: those three call
startwith nolocalIdentifierand contend fordaemon port 45691, which was held on the test box by an unrelated
BrowserStackLocal --appprocess. Once that process exited the same suite went green on this branch with no code
change — so they were never related to this diff.
CI / security gates — read this before merging
semgrep/cifailed on the first push of this PR and is worth stating plainly rather thanleaving for the reviewer to discover: 1 blocking finding,
ruby.lang.security.dangerous-exec.dangerous-exec, code-scanning alert #13 atlib/browserstack/localbinary.rb:138(job94071567985,
exit 1), which put the PR in
mergeStateStatus: BLOCKED.It is a false positive on the fixed code: the rule fires on any non-static first argument
to
IO.popenand does not model the array form — which is precisely this fix, since no/bin/shis spawned at all. Evidence that the diff did not introduce it: the identical rulehas been open against
masteras alert #9 since 2026-06-04, on the pre-fix line 136. Thediff re-anchors a pre-existing alert; it does not add one.
Resolved by a rule-scoped
# nosemgrep: ruby.lang.security.dangerous-exec.dangerous-execon that one line with the reason inline (comment-only, no behaviour change — the regression
tests and the injection PoC were re-run after it and are unchanged). Every other
IO.popen/exec finding in this file still reports. Alternative, if you'd rather not carry aninline suppression: triage alerts #13 and #9 on the platform as false positives and
drop the comment — either resolves both.
Confirmed green after the fix-up:
semgrep/ci=SUCCESS, plusSemgrep OSS,CodeQL,Analyze (ruby),Analyze (actions)— all 5 checks passing,mergeable: MERGEABLE. The PRstill reads
BLOCKEDonly because it is a Draft awaiting review approval (reviewDecision: REVIEW_REQUIRED), no longer because of a failing check.Live verification: a real tunnel was opened through the patched binding
(
BrowserStack::Local#start) and aselenium-webdriverBrowserStack session (Windows 11 /Chrome) loaded a local page over it via
bs-local.com, then stopped cleanly through thebinding's
.stop(). The cached 36 MB binary at~/.browserstack/BrowserStackLocalmeantbinary_pathtook the early-return branch, so the new array-formverify_binaryran againstthe genuine binary and returned
true— the tunnel could not have come up otherwise.