Skip to content

Commit b7b0d1a

Browse files
committed
fix: create the logfile without a shell (CWE-78 command injection)
Local#start created the logfile with system("echo > #{@logfile}") # Windows system("echo '' > '#{@logfile}'") # Unix Both pass the caller-supplied logfile path through /bin/sh (or cmd.exe), so shell metacharacters in the path are interpreted as commands. A logfile value like "log' ; touch /tmp/pwned ; echo 'x" (Unix) or "NUL & calc.exe" (Windows) runs arbitrary OS commands as the user running the gem (CWE-78). Replace the block with a shell-free create/truncate: mkdir_p the parent dir, then File.write(@logfile, ""), raising LocalException if the path is unwritable. File.write treats the path purely as a filename, so no shell is involved. This also fixes logfile paths containing spaces or a missing subdirectory, which the old shell form silently failed on. Mirrors the fix already shipped in the python binding. Adds BrowserStackLocalLogfileTest with two regression tests (no creds/network): a metacharacter payload no longer executes, and a space/missing-dir path is created literally. Both fail on the pre-fix code.
1 parent 6a67875 commit b7b0d1a

2 files changed

Lines changed: 68 additions & 4 deletions

File tree

‎lib/browserstack/local.rb‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
require 'browserstack/localbinary'
22
require 'browserstack/localexception'
33
require 'json'
4+
require 'fileutils'
45

56
module BrowserStack
67

@@ -73,10 +74,16 @@ def start(options = {})
7374
@binary_path
7475
end
7576

76-
if @is_windows
77-
system("echo > #{@logfile}")
78-
else
79-
system("echo '' > '#{@logfile}'")
77+
# Create/truncate the logfile without a shell. The previous
78+
# `system("echo ... > #{@logfile}")` passed @logfile to /bin/sh (or cmd.exe),
79+
# so shell metacharacters in a caller-supplied logfile path executed as commands
80+
# (CWE-78). File.write treats the path purely as a filename.
81+
logfile_dir = File.dirname(@logfile)
82+
FileUtils.mkdir_p(logfile_dir) unless File.directory?(logfile_dir)
83+
begin
84+
File.write(@logfile, "")
85+
rescue SystemCallError => e
86+
raise BrowserStack::LocalException.new("Unable to open logfile: #{e.message}")
8087
end
8188

8289
if defined? spawn

‎test/browserstack-local-test.rb‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
require 'rubygems'
22
require 'minitest'
33
require 'minitest/autorun'
4+
require 'minitest/mock'
5+
require 'tmpdir'
46
require 'browserstack/local'
57

68
class BrowserStackLocalTest < Minitest::Test
@@ -101,6 +103,61 @@ def teardown
101103
end
102104
end
103105

106+
# Regression tests for the logfile-creation step in Local#start (CWE-78).
107+
# The logfile used to be created with `system("echo ... > #{@logfile}")`, which
108+
# passed the caller-supplied path through a shell. These tests drive the public
109+
# `start` entry point but abort just after the logfile step (a fake binarypath
110+
# skips the download; stubbing start_command_args prevents launching the binary),
111+
# so they need no credentials, network, or tunnel.
112+
class BrowserStackLocalLogfileTest < Minitest::Test
113+
class AbortAfterLogfile < StandardError; end
114+
115+
# Runs `start` with the given logfile value, aborting right after the logfile
116+
# is created (before the real binary is spawned).
117+
def start_up_to_logfile(logfile_value)
118+
bs = BrowserStack::Local.new('dummy_key')
119+
bs.stub(:start_command_args, ->(*) { raise AbortAfterLogfile }) do
120+
begin
121+
# An existing, harmless executable as binarypath skips the binary download.
122+
bs.start('binarypath' => existing_executable, 'logfile' => logfile_value)
123+
rescue AbortAfterLogfile
124+
# expected: we intentionally stop before launching the binary
125+
end
126+
end
127+
end
128+
129+
def existing_executable
130+
['/bin/true', '/usr/bin/true'].find { |p| File.executable?(p) } || RbConfig.ruby
131+
end
132+
133+
def test_shell_metacharacters_in_logfile_path_are_not_executed
134+
Dir.mktmpdir do |dir|
135+
Dir.chdir(dir) do
136+
marker = File.join(dir, 'pwned')
137+
# Unix payload: close the single quote around @logfile, run touch, reopen.
138+
# Pre-fix this expands to: echo '' > 'log' ; touch <marker> ; echo 'x'
139+
payload = "log' ; touch #{marker} ; echo 'x"
140+
141+
start_up_to_logfile(payload)
142+
143+
refute File.exist?(marker),
144+
'shell metacharacters in the logfile path were executed (command injection)'
145+
end
146+
end
147+
end
148+
149+
def test_logfile_path_is_treated_as_a_literal_filename
150+
Dir.mktmpdir do |dir|
151+
logfile = File.join(dir, 'sub', 'my log.txt') # spaces + missing subdir
152+
start_up_to_logfile(logfile)
153+
154+
assert File.file?(logfile),
155+
'the logfile should be created as a literal path, even with spaces / a missing dir'
156+
assert_equal '', File.read(logfile), 'the logfile should be truncated to empty'
157+
end
158+
end
159+
end
160+
104161
class BrowserStackLocalBinaryTest < Minitest::Test
105162
def test_default_user_agent_contains_gem_name_and_version
106163
ua = BrowserStack::LocalBinary.new(auth_token: 'fake').instance_variable_get(:@user_agent)

0 commit comments

Comments
 (0)