From b7b0d1a39e0bf130a37044323cb945caffe6aafa Mon Sep 17 00:00:00 2001
From: 07souravkunda <souravkunda00@gmail.com>
Date: Mon, 21 Sep 2026 13:30:47 +0530
Subject: [PATCH] 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.
---
 lib/browserstack/local.rb       | 15 ++++++---
 test/browserstack-local-test.rb | 57 +++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+), 4 deletions(-)

diff --git a/lib/browserstack/local.rb b/lib/browserstack/local.rb
index 0a01b1c..93f55ed 100644
--- a/lib/browserstack/local.rb
+++ b/lib/browserstack/local.rb
@@ -1,6 +1,7 @@
 require 'browserstack/localbinary'
 require 'browserstack/localexception'
 require 'json'
+require 'fileutils'
 
 module BrowserStack
 
@@ -73,10 +74,16 @@ def start(options = {})
         @binary_path
       end
     
-    if @is_windows
-      system("echo > #{@logfile}")
-    else
-      system("echo '' > '#{@logfile}'")
+    # Create/truncate the logfile without a shell. The previous
+    # `system("echo ... > #{@logfile}")` passed @logfile to /bin/sh (or cmd.exe),
+    # so shell metacharacters in a caller-supplied logfile path executed as commands
+    # (CWE-78). File.write treats the path purely as a filename.
+    logfile_dir = File.dirname(@logfile)
+    FileUtils.mkdir_p(logfile_dir) unless File.directory?(logfile_dir)
+    begin
+      File.write(@logfile, "")
+    rescue SystemCallError => e
+      raise BrowserStack::LocalException.new("Unable to open logfile: #{e.message}")
     end
 
     if defined? spawn
diff --git a/test/browserstack-local-test.rb b/test/browserstack-local-test.rb
index 2c6218b..50ed3a9 100644
--- a/test/browserstack-local-test.rb
+++ b/test/browserstack-local-test.rb
@@ -1,6 +1,8 @@
 require 'rubygems'
 require 'minitest'
 require 'minitest/autorun'
+require 'minitest/mock'
+require 'tmpdir'
 require 'browserstack/local'
 
 class BrowserStackLocalTest < Minitest::Test
@@ -101,6 +103,61 @@ def teardown
   end
 end
 
+# Regression tests for the logfile-creation step in Local#start (CWE-78).
+# The logfile used to be created with `system("echo ... > #{@logfile}")`, which
+# passed the caller-supplied path through a shell. These tests drive the public
+# `start` entry point but abort just after the logfile step (a fake binarypath
+# skips the download; stubbing start_command_args prevents launching the binary),
+# so they need no credentials, network, or tunnel.
+class BrowserStackLocalLogfileTest < Minitest::Test
+  class AbortAfterLogfile < StandardError; end
+
+  # Runs `start` with the given logfile value, aborting right after the logfile
+  # is created (before the real binary is spawned).
+  def start_up_to_logfile(logfile_value)
+    bs = BrowserStack::Local.new('dummy_key')
+    bs.stub(:start_command_args, ->(*) { raise AbortAfterLogfile }) do
+      begin
+        # An existing, harmless executable as binarypath skips the binary download.
+        bs.start('binarypath' => existing_executable, 'logfile' => logfile_value)
+      rescue AbortAfterLogfile
+        # expected: we intentionally stop before launching the binary
+      end
+    end
+  end
+
+  def existing_executable
+    ['/bin/true', '/usr/bin/true'].find { |p| File.executable?(p) } || RbConfig.ruby
+  end
+
+  def test_shell_metacharacters_in_logfile_path_are_not_executed
+    Dir.mktmpdir do |dir|
+      Dir.chdir(dir) do
+        marker = File.join(dir, 'pwned')
+        # Unix payload: close the single quote around @logfile, run touch, reopen.
+        # Pre-fix this expands to: echo '' > 'log' ; touch <marker> ; echo 'x'
+        payload = "log' ; touch #{marker} ; echo 'x"
+
+        start_up_to_logfile(payload)
+
+        refute File.exist?(marker),
+               'shell metacharacters in the logfile path were executed (command injection)'
+      end
+    end
+  end
+
+  def test_logfile_path_is_treated_as_a_literal_filename
+    Dir.mktmpdir do |dir|
+      logfile = File.join(dir, 'sub', 'my log.txt') # spaces + missing subdir
+      start_up_to_logfile(logfile)
+
+      assert File.file?(logfile),
+             'the logfile should be created as a literal path, even with spaces / a missing dir'
+      assert_equal '', File.read(logfile), 'the logfile should be truncated to empty'
+    end
+  end
+end
+
 class BrowserStackLocalBinaryTest < Minitest::Test
   def test_default_user_agent_contains_gem_name_and_version
     ua = BrowserStack::LocalBinary.new(auth_token: 'fake').instance_variable_get(:@user_agent)