Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

moved all system commands outside of fork so security enhancements do…

…nt interfere

refactored code and tests
  • Loading branch information...
commit b7e3f636febf7a0cd3ab473b6d30081786d2c5b6 1 parent fe3c867
@coderrr coderrr authored
View
47 gem_eval.rb
@@ -11,24 +11,20 @@
pid = nil
begin
- Timeout::timeout(3) do
- pid = fork do
- r.close
+ repo = params[:repo]
+ data = params[:data]
+ tmpdir = "tmp/#{repo}"
+ spec = nil
- require File.dirname(__FILE__)+'/security'
- require File.dirname(__FILE__)+'/lazy_dir'
+ Timeout::timeout(3) do
+ `git clone --depth 1 git://github.com/#{repo} #{tmpdir}`
+ pid = fork do
begin
- repo = params[:repo]
- data = params[:data]
-
- tmpdir = "tmp/#{repo}"
- # DEBUG REMOVE
- #tmpdir = "."
- spec = nil
+ r.close
- # DEBUG
- `git clone --depth 1 git://github.com/#{repo} #{tmpdir}`
+ require File.dirname(__FILE__)+'/security'
+ require File.dirname(__FILE__)+'/lazy_dir'
Dir.chdir(tmpdir) do
Thread.new do
spec = eval <<-EOE
@@ -42,6 +38,7 @@
params = data = spec = repo = nil
$SAFE = 3
OrigDir.set_safe_level
+
#{data}
ensure
Object.class_eval do
@@ -52,18 +49,16 @@
EOE
end.join
Dir.set_safe_level
+
spec.rubygems_version = Gem::RubyGemsVersion # make sure validation passes
spec.validate
end
- # DEBUG
- `rm -rf #{tmpdir}`
+
w.write YAML.dump(spec)
- rescue Object => e
- # DEBUG
- `rm -rf #{tmpdir}`
- puts e
- puts e.backtrace
- w.write "ERROR: #{e}"
+ rescue Object
+ puts $!,$@
+
+ w.write "ERROR: #$!"
end
end
w.close
@@ -71,10 +66,12 @@
Process.wait pid
r.read
end
- rescue Timeout::Error
+ rescue Exception
Process.kill 9, pid
puts $!,$@
- "ERROR: #{$!}"
- end
+ "ERROR: #$!"
+ ensure
+ `rm -rf #{tmpdir}` if tmpdir
+ end
end
View
27 gem_eval_test.rb
@@ -9,12 +9,14 @@
class GemEvalTest < Test::Unit::TestCase
def setup
- @pid = fork { exec("ruby gem_eval.rb #{' > /dev/null 2>&1' unless OUTPUT}") }
+ system("mv git_mock git")
+ @pid = fork { exec("PATH=.:$PATH ruby gem_eval.rb #{' > /dev/null 2>&1' unless OUTPUT}") }
sleep 0.5
end
- def teardown
- Process.kill 9, @pid
+ def teardown
+ system("pkill -f 'ruby gem_eval.rb'")
+ system("mv git git_mock")
end
def test_access_to_untainted_locals
@@ -50,7 +52,7 @@ def test_legit_gemspec_works
s.version = "0.0.9"
s.summary = ""
s.authors = ["coderrr"]
- s.files = [ "x" ]
+ s.files = ['x']
end
EOS
expected_response = <<-EOS
@@ -107,7 +109,7 @@ def test_legit_gemspec_works
summary: ""
test_files: []
EOS
- assert_equal expected_response.strip, req(gemspec).strip
+ assert_equal clean_yaml(expected_response), clean_yaml(req(gemspec))
end
def test_gemspec_with_glob_works
@@ -187,20 +189,31 @@ def test_gemspec_with_glob_works
- globdir/a.rb
- globdir/c.txt
EOS
- assert_equal expected_response.strip, req(gemspec).strip
+ assert_equal clean_yaml(expected_response), clean_yaml(req(gemspec))
ensure
system("rm -rf globdir")
end
+ def test_tmpdir_is_destroyed
+ Dir.mkdir('tmp/gem_eval_test')
+ assert File.exist?('tmp/gem_eval_test')
+ req('')
+ assert ! File.exist?('tmp/gem_eval_test')
+ end
+
private
+ def clean_yaml(y)
+ y.strip.sub(/^date:.+$/,'').sub(/^rubygems_version:.+$/,'')
+ end
+
def assert_nil_error(v)
assert req("#{v}.abc").include?("undefined method `abc' for nil"), "#{v} was not nil"
end
def req(data)
Net::HTTP.start 'localhost', 4567 do |h|
- h.post('/', "data=#{CGI.escape data}").body
+ h.post('/', "data=#{CGI.escape data}&repo=gem_eval_test").body
end
end
end
View
3  git_mock
@@ -0,0 +1,3 @@
+#!/usr/bin/env ruby
+
+`mkdir -p #{ARGV.last}`
View
14 lazy_dir.rb
@@ -5,8 +5,9 @@ def initialize(method, args, block = nil)
@method, @args, @block = method, args, block
end
+ # this method is meant to be called lazily after the $SAFE has reverted to 0
def to_a
- raise SecurityError unless [:[], :glob].include? @method
+ raise SecurityError unless %w([] glob).include? @method
files = OrigDir.send(@method, *@args, &@block)
# only return files within the current directory
@@ -22,12 +23,11 @@ def to_yaml(opts = {})
end
class << self
- define_method :glob do |*a|
- LazyDir.new :glob, a
- end
-
- define_method :[] do |*a|
- LazyDir.new :[], a
+ # these methods are meant to be called with tainted data in a $SAFE >= 3
+ %w(glob []).each do |method_name|
+ define_method method_name do |*a|
+ LazyDir.new method_name, a
+ end
end
def method_missing m, *a, &b
View
21 lazy_dir_test.rb
@@ -2,7 +2,6 @@
require 'fileutils'
require File.dirname(__FILE__) + '/lazy_dir'
-#Dir
class LazyDirTest < Test::Unit::TestCase
def setup
FileUtils.mkdir('test_glob_dir')
@@ -26,7 +25,18 @@ def teardown
end
def test_lazy_glob
- lazy = Thread.new { $SAFE=4; Dir['test_glob_dir/*'] }.value
+ assert_raises(SecurityError) do
+ Thread.new do
+ $SAFE=4
+ OrigDir['test_glob_dir/*']
+ end.join
+ end
+
+ lazy = Thread.new do
+ $SAFE=4
+ Dir['test_glob_dir/*']
+ end.value
+
assert_equal OrigDir['test_glob_dir/*'], lazy.to_a
assert_equal OrigDir['test_glob_dir/*'], lazy.to_ary
end
@@ -48,7 +58,10 @@ def test_lazy_glob_secure
def test_call_original_dir_methods
assert Dir.pwd
- assert Dir.mkdir('asfasdfsaf')
- assert Dir.rmdir('asfasdfsaf')
+ dir = 'asfasdfsaf'
+ assert Dir.mkdir(dir)
+ assert File.exist?(dir)
+ assert Dir.rmdir(dir)
+ assert ! File.exist?(dir)
end
end
View
30 security.rb
@@ -32,7 +32,7 @@ class String
%w(sub! gsub!).each do |method_name|
m = instance_method(method_name)
- define_method "__call__#{method_name}" do |b, *a|
+ define_method "__real__#{method_name}" do |b, *a|
begin
m.bind(self).call(*a, &b)
ensure
@@ -42,7 +42,7 @@ class String
eval <<-EOF
def #{method_name} *a, &b
- __call__#{method_name}(b, *a)
+ __real__#{method_name}(b, *a)
end
EOF
end
@@ -51,28 +51,24 @@ def #{method_name} *a, &b
# Bug in ruby doesn't check taint when an array of globs is passed
class << Dir
- # we need to track $SAFE level manually because define_method captures the $SAFE level
- # of the current scope
+ # we need to track $SAFE level manually because define_method captures the $SAFE level
+ # of the current scope, as it would a local varaible, and of course the current scope has a $SAFE of 0
@@safe_level = 0
+ # since this method is defined with def instead of define_method, $SAFE will be taken from
+ # the calling scope which is what we want
def set_safe_level
@@safe_level = $SAFE
end
- m1 = instance_method :[]
- define_method :[] do |*args|
- $SAFE = @@safe_level
- raise SecurityError if $SAFE >= 3 and args.any? {|a| a.tainted? }
-
- m1.bind(self).call(*args)
- end
-
- m2 = instance_method :glob
- define_method :glob do |*args|
- $SAFE = @@safe_level
- raise SecurityError if $SAFE >= 3 and [*args.first].any? {|a| a.tainted? }
+ %w([] glob).each do |method_name|
+ m = instance_method method_name
+ define_method method_name do |*args|
+ $SAFE = @@safe_level
+ raise SecurityError if $SAFE >= 3 and args.flatten.any? {|a| a.tainted? }
- m2.bind(self).call(*args)
+ m.bind(self).call(*args)
+ end
end
end
Please sign in to comment.
Something went wrong with that request. Please try again.