Skip to content

Commit db623c3

Browse files
committed
fix: run the binary directly in verify_binary instead of through a shell (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.
1 parent 6a67875 commit db623c3

2 files changed

Lines changed: 34 additions & 1 deletion

File tree

lib/browserstack/localbinary.rb

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,9 @@ def download_to(url, bin_path)
133133
end
134134

135135
def verify_binary(bin_path)
136-
binary_response = IO.popen(bin_path + " --version").readline
136+
# Array form: exec's the binary directly, so a path containing shell
137+
# metacharacters or spaces is never interpreted by /bin/sh (CWE-78).
138+
binary_response = IO.popen([bin_path, '--version']).readline
137139
!!(binary_response =~ /BrowserStack Local version \d+\.\d+/)
138140
rescue StandardError
139141
false

test/browserstack-local-test.rb

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,37 @@ def test_local_binary_accepts_proxy_conf
159159
assert_equal 8080, bin.instance_variable_get(:@proxy_port)
160160
end
161161

162+
# Regression: verify_binary must exec the binary directly, never via a shell,
163+
# so shell metacharacters in the cached-binary path cannot run commands (CWE-78).
164+
def test_verify_binary_does_not_interpret_shell_metacharacters_in_path
165+
marker = File.join(Dir.tmpdir, "bs_local_verify_injection_#{Process.pid}")
166+
File.delete(marker) if File.exist?(marker)
167+
injected = "/nonexistent;touch #{marker};echo BrowserStack Local version 9.9;#"
168+
169+
assert_equal false, BrowserStack::LocalBinary.new(auth_token: 'fake').send(:verify_binary, injected)
170+
refute File.exist?(marker), 'shell metacharacters in the binary path were executed'
171+
ensure
172+
File.delete(marker) if marker && File.exist?(marker)
173+
end
174+
175+
# Same fix, benign side: a legitimate path containing spaces must still verify
176+
# (the shell used to split it and the check failed for every such user).
177+
def test_verify_binary_accepts_a_path_containing_spaces
178+
skip 'needs a POSIX shell to stand in for the binary' if Gem.win_platform?
179+
180+
base = Dir.mktmpdir('bs_local')
181+
dir = File.join(base, 'my binary dir')
182+
FileUtils.mkdir_p(dir)
183+
bin = File.join(dir, 'BrowserStackLocal')
184+
File.write(bin, "#!/bin/sh\necho 'BrowserStack Local version 9.9'\n")
185+
FileUtils.chmod(0755, bin)
186+
187+
assert_includes bin, ' '
188+
assert_equal true, BrowserStack::LocalBinary.new(auth_token: 'fake').send(:verify_binary, bin)
189+
ensure
190+
FileUtils.remove_entry(base) if base && File.directory?(base)
191+
end
192+
162193
private
163194

164195
def with_host_config(host_os, host_cpu)

0 commit comments

Comments
 (0)