diff --git a/CHANGELOG.rdoc b/CHANGELOG.rdoc index 31f96028..26ebd49a 100644 --- a/CHANGELOG.rdoc +++ b/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 diff --git a/lib/mechanize.rb b/lib/mechanize.rb index b12befa1..3e8850c3 100644 --- a/lib/mechanize.rb +++ b/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 diff --git a/lib/mechanize/cookie_jar.rb b/lib/mechanize/cookie_jar.rb index c8d462d7..c60e279e 100644 --- a/lib/mechanize/cookie_jar.rb +++ b/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, diff --git a/lib/mechanize/download.rb b/lib/mechanize/download.rb index 5ae7c87f..64338cc8 100644 --- a/lib/mechanize/download.rb +++ b/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 diff --git a/lib/mechanize/file.rb b/lib/mechanize/file.rb index a7b67be7..cfe27b7c 100644 --- a/lib/mechanize/file.rb +++ b/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 diff --git a/lib/mechanize/file_response.rb b/lib/mechanize/file_response.rb index 195cf167..8b44883e 100644 --- a/lib/mechanize/file_response.rb +++ b/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 diff --git a/lib/mechanize/test_case.rb b/lib/mechanize/test_case.rb index 3f3f043d..c4b85128 100644 --- a/lib/mechanize/test_case.rb +++ b/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] diff --git a/lib/mechanize/test_case/gzip_servlet.rb b/lib/mechanize/test_case/gzip_servlet.rb index 58d4bfbb..1573a978 100644 --- a/lib/mechanize/test_case/gzip_servlet.rb +++ b/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| diff --git a/lib/mechanize/test_case/verb_servlet.rb b/lib/mechanize/test_case/verb_servlet.rb index b44edf8e..a3acf985 100644 --- a/lib/mechanize/test_case/verb_servlet.rb +++ b/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 diff --git a/test/test_mechanize.rb b/test/test_mechanize.rb index ae336061..b4c3b373 100644 --- a/test/test_mechanize.rb +++ b/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' diff --git a/test/test_mechanize_cookie_jar.rb b/test/test_mechanize_cookie_jar.rb index ce705c14..96f01aef 100644 --- a/test/test_mechanize_cookie_jar.rb +++ b/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/' diff --git a/test/test_mechanize_download.rb b/test/test_mechanize_download.rb index ef79e259..3215f40c 100644 --- a/test/test_mechanize_download.rb +++ b/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 diff --git a/test/test_mechanize_file.rb b/test/test_mechanize_file.rb index e4dff35b..cb66b135 100644 --- a/test/test_mechanize_file.rb +++ b/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 diff --git a/test/test_mechanize_file_response.rb b/test/test_mechanize_file_response.rb index 05e08ea8..1f77837f 100644 --- a/test/test_mechanize_file_response.rb +++ b/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