From: shyouhei@... Date: 2014-10-18T19:54:50+00:00 Subject: [ruby-core:65784] [ruby-trunk - Feature #10351] [PATCH] prevent CVE-2014-6277 Issue #10351 has been updated by Shyouhei Urabe. Akira Tanaka wrote: > Shyouhei Urabe wrote: > > Akira, isn't it obvious this patch introduce a backward incompatibility? > > > > I'd be surprised if it could passed the tests, because that means our tests are insufficient. Is it that important to pass make check at this point? > > > > I don't think it's an "at least" thing for this rather conceptual patch. Is it your only concern? > > > > Of course tests shall be modified to fit this when it is going to land. > > The problem is how easy/difficult to modify software to follow this change. > > Trying to modify test-all is a way to estimate the difficulty. > I recommend it because it should be done anyway if this issue is accepted. I don't understand why you think it is a problem. Difficult? is it? ~~~ --- lib/mkmf.rb | 7 +++++-- lib/rake/file_utils.rb | 5 +++-- lib/webrick/httpservlet/cgi_runner.rb | 10 ++++------ test/lib/test/unit.rb | 5 ++++- test/rake/support/ruby_runner.rb | 4 +++- test/rake/test_rake_file_utils.rb | 13 ++++--------- test/ruby/envutil.rb | 5 ++++- test/ruby/test_process.rb | 11 ++++++----- test/ruby/test_require.rb | 32 +++++++++++++++++++++----------- test/ruby/test_rubyoptions.rb | 28 ++++++++++++++-------------- test/rubygems/test_gem.rb | 10 ++-------- tool/runruby.rb | 5 ++--- 12 files changed, 72 insertions(+), 63 deletions(-) diff --git a/lib/mkmf.rb b/lib/mkmf.rb index cb66d88..d5b5fe7 100644 --- a/lib/mkmf.rb +++ b/lib/mkmf.rb @@ -384,15 +384,18 @@ module MakeMakefile end Logging::open do puts command.quote + envp = { + "PATH"=>ENV["PATH"], + } if opts and opts[:werror] result = nil Logging.postpone do |log| - result = (system(libpath_env, command) and File.zero?(log.path)) + result = (system(envp.merge(libpath_env), command) and File.zero?(log.path)) "" end result else - system(libpath_env, command) + system(envp.merge(libpath_env), command) end end end diff --git a/lib/rake/file_utils.rb b/lib/rake/file_utils.rb index 27f4e2e..14e8686 100644 --- a/lib/rake/file_utils.rb +++ b/lib/rake/file_utils.rb @@ -90,10 +90,11 @@ module FileUtils # def ruby(*args, &block) options = (Hash === args.last) ? args.pop : {} + envp = (Hash === args.first) ? args.shift : {} if args.length > 1 - sh(*([RUBY] + args + [options]), &block) + sh(envp, *([RUBY] + args + [options]), &block) else - sh("#{RUBY} #{args.first}", options, &block) + sh(envp, "#{RUBY} #{args.first}", options, &block) end end diff --git a/lib/webrick/httpservlet/cgi_runner.rb b/lib/webrick/httpservlet/cgi_runner.rb index 32ecb6f..85722d5 100644 --- a/lib/webrick/httpservlet/cgi_runner.rb +++ b/lib/webrick/httpservlet/cgi_runner.rb @@ -31,16 +31,14 @@ STDERR.reopen(open(err, "w")) len = sysread(STDIN, 8).to_i dump = sysread(STDIN, len) hash = Marshal.restore(dump) -ENV.keys.each{|name| ENV.delete(name) } -hash.each{|k, v| ENV[k] = v if v } -dir = File::dirname(ENV["SCRIPT_FILENAME"]) +dir = File::dirname(hash["SCRIPT_FILENAME"]) Dir::chdir dir if ARGV[0] argv = ARGV.dup - argv << ENV["SCRIPT_FILENAME"] - exec(*argv) + argv << hash["SCRIPT_FILENAME"] + exec(hash, *argv) # NOTREACHED end -exec ENV["SCRIPT_FILENAME"] +exec hash, ENV["SCRIPT_FILENAME"] diff --git a/test/lib/test/unit.rb b/test/lib/test/unit.rb index c4fdf67..e527a1b 100644 --- a/test/lib/test/unit.rb +++ b/test/lib/test/unit.rb @@ -275,7 +275,10 @@ module Test class Worker def self.launch(ruby,args=[]) - io = IO.popen([*ruby, + envp = { + "PATH" => ENV["PATH"], + } + io = IO.popen(envp, [*ruby, "#{File.dirname(__FILE__)}/unit/parallel.rb", *args], "rb+") new(io, io.pid, :waiting) diff --git a/test/rake/support/ruby_runner.rb b/test/rake/support/ruby_runner.rb index d51dd24..d35f9bd 100644 --- a/test/rake/support/ruby_runner.rb +++ b/test/rake/support/ruby_runner.rb @@ -18,7 +18,9 @@ module RubyRunner def run_ruby(option_list) puts "COMMAND: [#{RUBY} #{option_list.join ' '}]" if @verbose - Open3.popen3(RUBY, *option_list) {|inn, out, err, wait| + # It seems vital for some tests to pass RAKE_* envitonment variables + env = ENV.keys.grep(/^RAKE/).each_with_object({}){|k, r| r[k] = ENV[k] } + Open3.popen3(env, RUBY, *option_list) {|inn, out, err, wait| inn.close @exit = wait ? wait.value : $? diff --git a/test/rake/test_rake_file_utils.rb b/test/rake/test_rake_file_utils.rb index 37d33dc..80971d4 100644 --- a/test/rake/test_rake_file_utils.rb +++ b/test/rake/test_rake_file_utils.rb @@ -130,18 +130,16 @@ class TestRakeFileUtils < Rake::TestCase def test_sh_with_a_single_string_argument check_expansion - ENV['RAKE_TEST_SH'] = 'someval' verbose(false) { - sh %{#{RUBY} check_expansion.rb #{env_var} someval} + sh({'RAKE_TEST_SH'=>'someval'}, %{#{RUBY} check_expansion.rb #{env_var} someval}) } end def test_sh_with_multiple_arguments check_no_expansion - ENV['RAKE_TEST_SH'] = 'someval' verbose(false) { - sh RUBY, 'check_no_expansion.rb', env_var, 'someval' + sh({'RAKE_TEST_SH'=>'someval'}, RUBY, 'check_no_expansion.rb', env_var, 'someval') } end @@ -225,11 +223,9 @@ class TestRakeFileUtils < Rake::TestCase def test_ruby_with_a_single_string_argument check_expansion - ENV['RAKE_TEST_SH'] = 'someval' - verbose(false) { replace_ruby { - ruby %{check_expansion.rb #{env_var} someval} + ruby({'RAKE_TEST_SH'=>'someval'}, %{check_expansion.rb #{env_var} someval}) } } end @@ -237,10 +233,9 @@ class TestRakeFileUtils < Rake::TestCase def test_ruby_with_multiple_arguments check_no_expansion - ENV['RAKE_TEST_SH'] = 'someval' verbose(false) { replace_ruby { - ruby 'check_no_expansion.rb', env_var, 'someval' + ruby({'RAKE_TEST_SH'=>'someval'}, 'check_no_expansion.rb', env_var, 'someval') } } end diff --git a/test/ruby/envutil.rb b/test/ruby/envutil.rb index f5fbb7c..b02bed3 100644 --- a/test/ruby/envutil.rb +++ b/test/ruby/envutil.rb @@ -47,7 +47,10 @@ module EnvUtil err_p.set_encoding(encoding) if err_p end c = "C" - child_env = {} + # some defaults + child_env = { + 'RUBYLIB' => ENV['RUBYLIB'], + } LANG_ENVS.each {|lc| child_env[lc] = c} if Array === args and Hash === args.first child_env.update(args.shift) diff --git a/test/ruby/test_process.rb b/test/ruby/test_process.rb index b314b7f..56700d1 100644 --- a/test/ruby/test_process.rb +++ b/test/ruby/test_process.rb @@ -282,7 +282,8 @@ class TestProcess < Test::Unit::TestCase end end cmd << '-e' << 'puts ENV.keys.map{|e|e.upcase}' - IO.popen(cmd) {|io| + # @shyouhei's note: it seems this test intended to pass all envs. + IO.popen(cmd, unsetenv_others: false) {|io| assert_equal("PATH\n", io.read) } @@ -298,11 +299,11 @@ class TestProcess < Test::Unit::TestCase old = ENV["hmm"] begin ENV["hmm"] = "fufu" - IO.popen(ENVCOMMAND) {|io| assert_match(/^hmm=fufu$/, io.read) } + IO.popen(ENVCOMMAND) {|io| assert_match(/^$/, io.read) } IO.popen([{"hmm"=>""}, *ENVCOMMAND]) {|io| assert_match(/^hmm=$/, io.read) } IO.popen([{"hmm"=>nil}, *ENVCOMMAND]) {|io| assert_not_match(/^hmm=/, io.read) } ENV["hmm"] = "" - IO.popen(ENVCOMMAND) {|io| assert_match(/^hmm=$/, io.read) } + IO.popen(ENVCOMMAND) {|io| assert_match(/^$/, io.read) } IO.popen([{"hmm"=>""}, *ENVCOMMAND]) {|io| assert_match(/^hmm=$/, io.read) } IO.popen([{"hmm"=>nil}, *ENVCOMMAND]) {|io| assert_not_match(/^hmm=/, io.read) } ENV["hmm"] = nil @@ -323,11 +324,11 @@ class TestProcess < Test::Unit::TestCase old = ENV["hmm"] begin ENV["hmm"] = "fufu" - IO.popen(cmd) {|io| assert_match(/^hmm=fufu$/, io.read, message)} + IO.popen(cmd) {|io| assert_match(/^$/, io.read, message)} IO.popen({"hmm"=>""}, cmd) {|io| assert_match(/^hmm=$/, io.read, message)} IO.popen({"hmm"=>nil}, cmd) {|io| assert_not_match(/^hmm=/, io.read, message)} ENV["hmm"] = "" - IO.popen(cmd) {|io| assert_match(/^hmm=$/, io.read, message)} + IO.popen(cmd) {|io| assert_match(/^$/, io.read, message)} IO.popen({"hmm"=>""}, cmd) {|io| assert_match(/^hmm=$/, io.read, message)} IO.popen({"hmm"=>nil}, cmd) {|io| assert_not_match(/^hmm=/, io.read, message)} ENV["hmm"] = nil diff --git a/test/ruby/test_require.rb b/test/ruby/test_require.rb index 3bd327c..9e76bbf 100644 --- a/test/ruby/test_require.rb +++ b/test/ruby/test_require.rb @@ -118,9 +118,11 @@ class TestRequire < Test::Unit::TestCase env_rubypath, env_home = ENV["RUBYPATH"], ENV["HOME"] pathname_too_long = /pathname too long \(ignored\).*\(LoadError\)/m - ENV["RUBYPATH"] = "~" - ENV["HOME"] = "/foo" * 1024 - assert_in_out_err(%w(-S -w test_ruby_test_require), "", [], pathname_too_long) + env = { + "RUBYPATH" => "~", + "HOME" => "/foo" * 1024, + } + assert_in_out_err([env] + %w(-S -w test_ruby_test_require), "", [], pathname_too_long) ensure env_rubypath ? ENV["RUBYPATH"] = env_rubypath : ENV.delete("RUBYPATH") @@ -131,9 +133,11 @@ class TestRequire < Test::Unit::TestCase env_rubypath, env_home = ENV["RUBYPATH"], ENV["HOME"] pathname_too_long = /pathname too long \(ignored\).*\(LoadError\)/m - ENV["RUBYPATH"] = "~" + "/foo" * 1024 - ENV["HOME"] = "/foo" - assert_in_out_err(%w(-S -w test_ruby_test_require), "", [], pathname_too_long) + env = { + "RUBYPATH" => "~" + "/foo" * 1024, + "HOME" => "/foo", + } + assert_in_out_err([env] + %w(-S -w test_ruby_test_require), "", [], pathname_too_long) ensure env_rubypath ? ENV["RUBYPATH"] = env_rubypath : ENV.delete("RUBYPATH") @@ -147,12 +151,18 @@ class TestRequire < Test::Unit::TestCase t.puts "p :ok" t.close - ENV["RUBYPATH"] = "~" - ENV["HOME"] = t.path - assert_in_out_err(%w(-S test_ruby_test_require), "", [], /\(LoadError\)/) + env = { + "RUBYPATH" => "~", + "HOME" => t.path, + } + assert_in_out_err([env] + %w(-S test_ruby_test_require), "", [], /\(LoadError\)/) - ENV["HOME"], name = File.split(t.path) - assert_in_out_err(["-S", name], "", %w(:ok), []) + home, name = File.split(t.path) + env = { + "RUBYPATH" => "~", + "HOME" => home, + } + assert_in_out_err([env, "-S", name], "", %w(:ok), []) } ensure env_rubypath ? ENV["RUBYPATH"] = env_rubypath : ENV.delete("RUBYPATH") diff --git a/test/ruby/test_rubyoptions.rb b/test/ruby/test_rubyoptions.rb index d4e5b2a..ba62969 100644 --- a/test/ruby/test_rubyoptions.rb +++ b/test/ruby/test_rubyoptions.rb @@ -242,20 +242,20 @@ class TestRubyOptions < Test::Unit::TestCase def test_rubyopt rubyopt_orig = ENV['RUBYOPT'] - ENV['RUBYOPT'] = ' - -' - assert_in_out_err([], "", [], []) + env = {'RUBYOPT' => ' - -'} + assert_in_out_err([env], "", [], []) - ENV['RUBYOPT'] = '-e "p 1"' - assert_in_out_err([], "", [], /invalid switch in RUBYOPT: -e \(RuntimeError\)/) + env['RUBYOPT'] = '-e "p 1"' + assert_in_out_err([env], "", [], /invalid switch in RUBYOPT: -e \(RuntimeError\)/) - ENV['RUBYOPT'] = '-T1' - assert_in_out_err(["--disable-gems"], "", [], /no program input from stdin allowed in tainted mode \(SecurityError\)/) + env['RUBYOPT'] = '-T1' + assert_in_out_err([env, "--disable-gems"], "", [], /no program input from stdin allowed in tainted mode \(SecurityError\)/) - ENV['RUBYOPT'] = '-T4' - assert_in_out_err(["--disable-gems"], "", [], /no program input from stdin allowed in tainted mode \(SecurityError\)/) + env['RUBYOPT'] = '-T4' + assert_in_out_err([env, "--disable-gems"], "", [], /no program input from stdin allowed in tainted mode \(SecurityError\)/) - ENV['RUBYOPT'] = '-Eus-ascii -KN' - assert_in_out_err(%w(-Eutf-8 -KU), "p '\u3042'") do |r, e| + env['RUBYOPT'] = '-Eus-ascii -KN' + assert_in_out_err([env]+%w(-Eutf-8 -KU), "p '\u3042'") do |r, e| assert_equal("\"\u3042\"", r.join.force_encoding(Encoding::UTF_8)) assert_equal([], e) end @@ -279,13 +279,13 @@ class TestRubyOptions < Test::Unit::TestCase @verbose = $VERBOSE $VERBOSE = nil - ENV['PATH'] = File.dirname(t.path) + env={'PATH' => File.dirname(t.path)} - assert_in_out_err(%w(-S) + [File.basename(t.path)], "", %w(1), []) + assert_in_out_err([env] + %w(-S) + [File.basename(t.path)], "", %w(1), []) - ENV['RUBYPATH'] = File.dirname(t.path) + env['RUBYPATH'] = File.dirname(t.path) - assert_in_out_err(%w(-S) + [File.basename(t.path)], "", %w(1), []) + assert_in_out_err([env] + %w(-S) + [File.basename(t.path)], "", %w(1), []) } ensure diff --git a/test/rubygems/test_gem.rb b/test/rubygems/test_gem.rb index 0da28bb..85556ae 100644 --- a/test/rubygems/test_gem.rb +++ b/test/rubygems/test_gem.rb @@ -1260,10 +1260,7 @@ class TestGem < Gem::TestCase install_gem b, :install_dir => path install_gem c, :install_dir => path - ENV['GEM_PATH'] = path - ENV['RUBYGEMS_GEMDEPS'] = "-" - - out = `#{Gem.ruby.dup.untaint} -I #{LIB_PATH.untaint} -rubygems -e "p Gem.loaded_specs.values.map(&:full_name).sort"` + out = `GEM_PATH=#{path} RUBYGEMS_GEMDEPS=- #{Gem.ruby.dup.untaint} -I #{LIB_PATH.untaint} -rubygems -e "p Gem.loaded_specs.values.map(&:full_name).sort"` assert_equal '["a-1", "b-1", "c-1"]', out.strip end @@ -1290,12 +1287,9 @@ class TestGem < Gem::TestCase install_gem b, :install_dir => path install_gem c, :install_dir => path - ENV['GEM_PATH'] = path - ENV['RUBYGEMS_GEMDEPS'] = "-" - Dir.mkdir "sub1" out = Dir.chdir "sub1" do - `#{Gem.ruby.dup.untaint} -I #{LIB_PATH.untaint} -rubygems -e "p Gem.loaded_specs.values.map(&:full_name).sort"` + `GEM_PATH=#{path} RUBYGEMS_GEMDEPS=- #{Gem.ruby.dup.untaint} -I #{LIB_PATH.untaint} -rubygems -e "p Gem.loaded_specs.values.map(&:full_name).sort"` end Dir.rmdir "sub1" diff --git a/tool/runruby.rb b/tool/runruby.rb index 86b9327..cf63055 100755 --- a/tool/runruby.rb +++ b/tool/runruby.rb @@ -66,6 +66,7 @@ config["bindir"] = abs_archdir env = {} +env["HOME"] = ENV["HOME"] # RDoc relies on it env["RUBY"] = File.expand_path(ruby) env["PATH"] = [abs_archdir, ENV["PATH"]].compact.join(File::PATH_SEPARATOR) @@ -84,8 +85,6 @@ if File.file?(libruby_so) end end -ENV.update env - cmd = [ruby] cmd.concat(ARGV) cmd.unshift(*precommand) unless precommand.empty? @@ -96,4 +95,4 @@ if show puts Shellwords.join(cmd) end -exec(*cmd) +exec(env, *cmd) -- 1.9.1 ~~~ ---------------------------------------- Feature #10351: [PATCH] prevent CVE-2014-6277 https://bugs.ruby-lang.org/issues/10351#change-49522 * Author: Shyouhei Urabe * Status: Feedback * Priority: Normal * Assignee: * Category: core * Target version: current: 2.2.0 ---------------------------------------- ~~~ From 4636ca0308f1933c9b191f36e808a8d3bcf5e88e Mon Sep 17 00:00:00 2001 From: "Urabe, Shyouhei" Date: Wed, 8 Oct 2014 15:44:27 +0900 Subject: [PATCH] prevent CVE-2014-6277 ShellShock was about bash. I think ruby is torellant for that kind of attack. However the concept was that environment variables can potentially contain malicious data. To pass them to victim subprocecsses can sometimes cause catastrophic situation, like arbitrary code execution. Even though ruby itself is not affected, it can be an accomplice in attacks by blindly passing through any uncontrolled info through. Let's just change this, more secure by default. This patch does not add a new feature, nor delete anything. It just changes the default behaviour when ruby spawns subprocesses. Process.spawn('/usr/bin/printenv') # -> prints nothing You can explicitly pass what you need: Process.spawn({'FOO'=>'BAR'}, '/usr/bin/printenv') Or if you are 128% sure what you are doing, can pass everything. ENV['FOO'] = 'BAR' Process.spawn('/usr/bin/printenv', unsetenv_others: false) I don't intend to make things impossible; just give it a better default. It doesn't ultimately solve everything but it should prevent casual faults. --- process.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/process.c b/process.c index f9bc01c..395333d 100644 --- a/process.c +++ b/process.c @@ -2295,7 +2295,7 @@ rb_execarg_fixup(VALUE execarg_obj) eargp->dup2_tmpbuf = tmpbuf; } - unsetenv_others = eargp->unsetenv_others_given && eargp->unsetenv_others_do; + unsetenv_others = !eargp->unsetenv_others_given || eargp->unsetenv_others_do; envopts = eargp->env_modification; if (unsetenv_others || envopts != Qfalse) { VALUE envtbl, envp_str, envp_buf; @@ -2936,7 +2936,7 @@ rb_execarg_run_options(const struct rb_execarg *eargp, struct rb_execarg *sargp, #endif #if !defined(HAVE_WORKING_FORK) - if (eargp->unsetenv_others_given && eargp->unsetenv_others_do) { + if (!eargp->unsetenv_others_given || eargp->unsetenv_others_do) { save_env(sargp); rb_env_clear(); } @@ -3998,8 +3998,8 @@ rb_f_system(int argc, VALUE *argv) * [cmdname, argv0], arg1, ... : command name, argv[0] and zero or more arguments (no shell) * options: hash * clearing environment variables: - * :unsetenv_others => true : clear environment variables except specified by env - * :unsetenv_others => false : don't clear (default) + * :unsetenv_others => true : clear environment variables except specified by env (default) + * :unsetenv_others => false : don't clear * process group: * :pgroup => true or 0 : make a new process group * :pgroup => pgid : join to specified process group -- 1.9.1 ~~~ -- https://bugs.ruby-lang.org/