Merge pull request #548 from kyoshidajp/fix_command_injection

Prevent OS command injection

CHANGELOG.rdoc

@@ -2,6 +2,22 @@
 
 === Unreleased
 
+* Security
+
+  Mechanize `>= v2.0`, `< v2.7.7` allows for OS commands to be injected into several classes'
+  methods via implicit use of Ruby's `Kernel.open` method. Exploitation is possible only if
+  untrusted input is used as a local filename and passed to any of these calls:
+
+  - `Mechanize::CookieJar#load`: since v2.0 (see 208e3ed)
+  - `Mechanize::CookieJar#save_as`: since v2.0 (see 5b776a4)
+  - `Mechanize#download`: since v2.2 (see dc91667)
+  - `Mechanize::Download#save` and `#save!` since v2.1 (see 98b2f51, bd62ff0)
+  - `Mechanize::File#save` and `#save_as`: since v2.1 (see 2bf7519)
+  - `Mechanize::FileResponse#read_body`: since v2.0 (see 01039f5)
+
+  See https://github.com/sparklemotion/mechanize/security/advisories/GHSA-qrqm-fpv6-6r8g for more
+  information.
+
 * New Features
   * Support for Ruby 3.0 by adding `webrick` as a runtime dependency. (#557) @pvalena
 

lib/mechanize.rb

@@ -396,7 +396,7 @@ def download uri, io_or_filename, parameters = [], referer = nil, headers = {}
     io = if io_or_filename.respond_to? :write then
            io_or_filename
          else
-           open io_or_filename, 'wb'
+           ::File.open(io_or_filename, 'wb')
          end
 
     case page

lib/mechanize/cookie_jar.rb

@@ -65,7 +65,7 @@ def dump_cookiestxt(io)
   class CookieJar < ::HTTP::CookieJar
     def save(output, *options)
       output.respond_to?(:write) or
-        return open(output, 'w') { |io| save(io, *options) }
+        return ::File.open(output, 'w') { |io| save(io, *options) }
 
       opthash = {
         :format => :yaml,
@@ -119,7 +119,7 @@ def save(output, *options)
 
     def load(input, *options)
       input.respond_to?(:write) or
-        return open(input, 'r') { |io| load(io, *options) }
+        return ::File.open(input, 'r') { |io| load(io, *options) }
 
       opthash = {
         :format => :yaml,

lib/mechanize/download.rb

@@ -71,7 +71,7 @@ def save! filename = nil
     dirname = File.dirname filename
     FileUtils.mkdir_p dirname
 
-    open filename, 'wb' do |io|
+    ::File.open(filename, 'wb')do |io|
       until @body_io.eof? do
         io.write @body_io.read 16384
       end

lib/mechanize/file.rb

@@ -82,7 +82,7 @@ def save! filename = nil
     dirname = File.dirname filename
     FileUtils.mkdir_p dirname
 
-    open filename, 'wb' do |f|
+    ::File.open(filename, 'wb')do |f|
       f.write body
     end
 

lib/mechanize/file_response.rb

@@ -15,7 +15,7 @@ def read_body
     if directory?
       yield dir_body
     else
-      open @file_path, 'rb' do |io|
+      ::File.open(@file_path, 'rb') do |io|
         yield io.read
       end
     end

lib/mechanize/test_case.rb

@@ -230,9 +230,9 @@ def request(req, *data, &block)
     else
       filename = "htdocs#{path.gsub(/[^\/\\.\w\s]/, '_')}"
       unless PAGE_CACHE[filename]
-        open("#{Mechanize::TestCase::TEST_DIR}/#{filename}", 'rb') { |io|
+        ::File.open("#{Mechanize::TestCase::TEST_DIR}/#{filename}", 'rb') do |io|
           PAGE_CACHE[filename] = io.read
-        }
+        end
       end
 
       res.body = PAGE_CACHE[filename]

lib/mechanize/test_case/gzip_servlet.rb

@@ -13,7 +13,7 @@ def do_GET(req, res)
     end
 
     if name = req.query['file'] then
-      open "#{TEST_DIR}/htdocs/#{name}" do |io|
+      ::File.open("#{TEST_DIR}/htdocs/#{name}") do |io|
         string = String.new
         zipped = StringIO.new string, 'w'
         Zlib::GzipWriter.wrap zipped do |gz|

lib/mechanize/test_case/verb_servlet.rb

@@ -1,11 +1,9 @@
 class VerbServlet < WEBrick::HTTPServlet::AbstractServlet
   %w[HEAD GET POST PUT DELETE].each do |verb|
-    eval <<-METHOD
-      def do_#{verb}(req, res)
-        res.header['X-Request-Method'] = #{verb.dump}
-        res.body = #{verb.dump}
-      end
-    METHOD
+    define_method "do_#{verb}" do |req, res|
+      res.header['X-Request-Method'] = verb
+      res.body = verb
+    end
   end
 end
 

test/test_mechanize.rb

@@ -345,6 +345,14 @@ def test_download_filename_error
     end
   end
 
+  def test_download_does_not_allow_command_injection
+    in_tmpdir do
+      @mech.download('http://example', '| ruby -rfileutils -e \'FileUtils.touch("vul.txt")\'')
+
+      refute_operator(File, :exist?, "vul.txt")
+    end
+  end
+
   def test_get
     uri = URI 'http://localhost'
 

test/test_mechanize_cookie_jar.rb

@@ -1,4 +1,5 @@
 require 'mechanize/test_case'
+require 'fileutils'
 
 class TestMechanizeCookieJar < Mechanize::TestCase
 
@@ -500,6 +501,35 @@ def test_save_and_read_cookiestxt_with_session_cookies
     assert_equal(0, @jar.cookies(url).length)
   end
 
+  def test_prevent_command_injection_when_saving
+    url = URI 'http://rubygems.org/'
+    path = '| ruby -rfileutils -e \'FileUtils.touch("vul.txt")\''
+
+    @jar.add(url, Mechanize::Cookie.new(cookie_values))
+
+    in_tmpdir do
+      @jar.save_as(path, :cookiestxt)
+      assert_equal(false, File.exist?('vul.txt'))
+    end
+  end
+
+  def test_prevent_command_injection_when_loading
+    url = URI 'http://rubygems.org/'
+    path = '| ruby -rfileutils -e \'FileUtils.touch("vul.txt")\''
+
+    @jar.add(url, Mechanize::Cookie.new(cookie_values))
+
+    in_tmpdir do
+      @jar.save_as("cookies.txt", :cookiestxt)
+      @jar.clear!
+
+      assert_raises Errno::ENOENT do
+        @jar.load(path, :cookiestxt)
+      end
+      assert_equal(false, File.exist?('vul.txt'))
+    end
+  end
+
   def test_save_and_read_expired_cookies
     url = URI 'http://rubygems.org/'
 

test/test_mechanize_download.rb

@@ -46,6 +46,18 @@ def test_save_bang
     end
   end
 
+  def test_save_bang_does_not_allow_command_injection
+    uri = URI.parse 'http://example/foo.html'
+    body_io = StringIO.new '0123456789'
+
+    download = @parser.new uri, nil, body_io
+
+    in_tmpdir do
+      download.save!('| ruby -rfileutils -e \'FileUtils.touch("vul.txt")\'')
+      refute_operator(File, :exist?, "vul.txt")
+    end
+  end
+
   def test_save_tempfile
     uri = URI.parse 'http://example/foo.html'
     Tempfile.open @NAME do |body_io|
@@ -84,6 +96,5 @@ def test_filename
 
     assert_equal "foo.html", download.filename
   end
-
 end
 

test/test_mechanize_file.rb

@@ -103,5 +103,14 @@ def test_save_overwrite
     end
   end
 
+  def test_save_bang_does_not_allow_command_injection
+    uri = URI 'http://example/test.html'
+    page = Mechanize::File.new uri, nil, ''
+
+    in_tmpdir do
+      page.save!('| ruby -rfileutils -e \'FileUtils.touch("vul.txt")\'')
+      refute_operator(File, :exist?, "vul.txt")
+    end
+  end
 end
 

test/test_mechanize_file_response.rb

@@ -1,7 +1,6 @@
 require 'mechanize/test_case'
 
 class TestMechanizeFileResponse < Mechanize::TestCase
-
   def test_content_type
     Tempfile.open %w[pi .nothtml] do |tempfile|
       res = Mechanize::FileResponse.new tempfile.path
@@ -19,5 +18,24 @@ def test_content_type
     end
   end
 
-end
+  def test_read_body
+    Tempfile.open %w[pi .html] do |tempfile|
+      tempfile.write("asdfasdfasdf")
+      tempfile.close
 
+      res = Mechanize::FileResponse.new(tempfile.path)
+      res.read_body do |input|
+        assert_equal("asdfasdfasdf", input)
+      end
+    end
+  end
+
+  def test_read_body_does_not_allow_command_injection
+    in_tmpdir do
+      FileUtils.touch('| ruby -rfileutils -e \'FileUtils.touch("vul.txt")\'')
+      res = Mechanize::FileResponse.new('| ruby -rfileutils -e \'FileUtils.touch("vul.txt")\'')
+      res.read_body { |_| }
+      refute_operator(File, :exist?, "vul.txt")
+    end
+  end
+end