[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" <[e-mail]>
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/