From db623c310a26a4da1537b70079acac8fa9220c59 Mon Sep 17 00:00:00 2001 From: 07souravkunda Date: Wed, 12 Aug 2026 15:02:06 +0530 Subject: [PATCH 1/3] fix: run the binary directly in verify_binary instead of through a shell (CWE-78) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- lib/browserstack/localbinary.rb | 4 +++- test/browserstack-local-test.rb | 31 +++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/lib/browserstack/localbinary.rb b/lib/browserstack/localbinary.rb index 737c590..df44438 100644 --- a/lib/browserstack/localbinary.rb +++ b/lib/browserstack/localbinary.rb @@ -133,7 +133,9 @@ def download_to(url, bin_path) end def verify_binary(bin_path) - 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 !!(binary_response =~ /BrowserStack Local version \d+\.\d+/) rescue StandardError false diff --git a/test/browserstack-local-test.rb b/test/browserstack-local-test.rb index 2c6218b..d11c175 100644 --- a/test/browserstack-local-test.rb +++ b/test/browserstack-local-test.rb @@ -159,6 +159,37 @@ def test_local_binary_accepts_proxy_conf assert_equal 8080, bin.instance_variable_get(:@proxy_port) end + # Regression: verify_binary must exec the binary directly, never via a shell, + # so shell metacharacters in the cached-binary path cannot run commands (CWE-78). + 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;#" + + assert_equal false, BrowserStack::LocalBinary.new(auth_token: 'fake').send(:verify_binary, injected) + refute File.exist?(marker), 'shell metacharacters in the binary path were executed' + ensure + File.delete(marker) if marker && File.exist?(marker) + end + + # Same fix, benign side: a legitimate path containing spaces must still verify + # (the shell used to split it and the check failed for every such user). + def test_verify_binary_accepts_a_path_containing_spaces + skip 'needs a POSIX shell to stand in for the binary' if Gem.win_platform? + + base = Dir.mktmpdir('bs_local') + dir = File.join(base, 'my binary dir') + 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_includes bin, ' ' + assert_equal true, BrowserStack::LocalBinary.new(auth_token: 'fake').send(:verify_binary, bin) + ensure + FileUtils.remove_entry(base) if base && File.directory?(base) + end + private def with_host_config(host_os, host_cpu) From ab487043276cf4d417e45d6ccf14bc22b80271a2 Mon Sep 17 00:00:00 2001 From: 07souravkunda Date: Wed, 12 Aug 2026 15:55:30 +0530 Subject: [PATCH 2/3] chore: scope-limited scanner suppression on the fixed verify_binary line 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. --- lib/browserstack/localbinary.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/browserstack/localbinary.rb b/lib/browserstack/localbinary.rb index df44438..11c6470 100644 --- a/lib/browserstack/localbinary.rb +++ b/lib/browserstack/localbinary.rb @@ -135,6 +135,11 @@ def download_to(url, bin_path) 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). + # + # The scanner rule below fires on any non-static first argument to IO.popen + # and does not model the array form -- which is exactly the fix here, since + # no shell is spawned at all. Suppressed for this rule only. + # nosemgrep: ruby.lang.security.dangerous-exec.dangerous-exec binary_response = IO.popen([bin_path, '--version']).readline !!(binary_response =~ /BrowserStack Local version \d+\.\d+/) rescue StandardError From e766c21cc59a46dde601bc04f32d606d73d2901f Mon Sep 17 00:00:00 2001 From: 07souravkunda Date: Wed, 12 Aug 2026 15:59:00 +0530 Subject: [PATCH 3/3] test: pin the array-form behaviour with a real binary at a hostile path 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. --- test/browserstack-local-test.rb | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/browserstack-local-test.rb b/test/browserstack-local-test.rb index d11c175..68fe9fa 100644 --- a/test/browserstack-local-test.rb +++ b/test/browserstack-local-test.rb @@ -172,6 +172,31 @@ def test_verify_binary_does_not_interpret_shell_metacharacters_in_path File.delete(marker) if marker && File.exist?(marker) end + # Stronger form of the above: a REAL binary living under a hostile-looking + # directory name. Pins the array-form behaviour itself rather than just an + # ENOENT, so a future "fix" that swapped the array form for a character + # allowlist would fail here — the injected command must not run AND the + # legitimate binary at that path must still verify. + def test_verify_binary_runs_a_real_binary_at_a_path_containing_shell_metacharacters + skip 'needs a POSIX shell to stand in for the binary' if Gem.win_platform? + + marker = File.join(Dir.tmpdir, "bs_local_verify_dir_injection_#{Process.pid}") + File.delete(marker) if File.exist?(marker) + + 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' + ensure + File.delete(marker) if marker && File.exist?(marker) + FileUtils.remove_entry(base) if base && File.directory?(base) + end + # Same fix, benign side: a legitimate path containing spaces must still verify # (the shell used to split it and the check failed for every such user). def test_verify_binary_accepts_a_path_containing_spaces