Merge pull request #548 from kyoshidajp/fix_command_injection

Prevent OS command injection

Author: flavorjones

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'