Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Refactor usage of Dir[] to eliminate unsafe glob construction #331

Closed
wants to merge 3 commits into from

5 participants

@pixeltrix

Contructing globs by concatenation through File.join or string interpolation is
susceptible to errors caused by paths inadvertently including glob characters.
This commit fixes this by using Dir.chdir to change the current directory to a
safe point from which to execute the Dir.glob and then reconstructs the path.

There are no specific tests that have been added but the temporary test
directory name has been altered to include the {} characters. This causes a
significant number of the existing tests to fail which should prevent any
regressions.

@pixeltrix pixeltrix Refactor usage of Dir[] to eliminate unsafe glob construction
Contructing globs by concatenation through File.join or string interpolation is
susceptible to errors caused by paths inadvertently including glob characters.
This commit fixes this by using Dir.chdir to change the current directory to a
safe point from which to execute the Dir.glob and then reconstructs the path.

There are no specific tests that have been added but the temporary test
directory name has been altered to include the {} characters. This causes a
significant number of the existing tests to fail which should prevent any
regressions.
32326ea
@travisbot

This pull request fails (merged 32326ea into 8f39d56).

@travisbot

This pull request fails (merged ce34bd0 into 8f39d56).

@drbrain drbrain commented on the diff
lib/rubygems.rb
@@ -525,6 +525,21 @@ def self.find_home
private_class_method :find_home
##
+ # Dir.glob wrapper that takes a base directory
+
+ def self.glob(dir, pattern)
+ # TODO: move to utils
@drbrain Owner
drbrain added a note

o_O

I'm assuming at some point a whole bunch of these methods would be moved to Gem::Utils - that doesn't exist at the moment does it? I didn't want to pollute the pull request with unrelated changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@evanphx evanphx commented on the diff
lib/rubygems/test_case.rb
@@ -122,9 +122,9 @@ def setup
tmpdir = File.expand_path("tmp/test")
if ENV['KEEP_FILES'] then
- @tempdir = File.join(tmpdir, "test_rubygems_#{$$}.#{Time.now.to_i}")
+ @tempdir = File.join(tmpdir, "test_rubygems_{#{$$}}.#{Time.now.to_i}")
@evanphx Owner
evanphx added a note

I don't understand why you've wrapped the value with {}. Could you explain?

To test that glob characters don't cause a problem - writing a whole bunch of extra tests seemed excessive

@raggi Collaborator
raggi added a note

It's not excessive when someone comes along and removes that because they don't know why it's there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@evanphx evanphx commented on the diff
test/rubygems/test_gem_remote_fetcher.rb
@@ -292,8 +292,11 @@ def test_download_local_read_only
inst = Gem::RemoteFetcher.fetcher
end
- assert_equal(File.join(@tempdir, @a1.file_name),
- inst.download(@a1, local_path))
+ uri = URI.const_defined?(:DEFAULT_PARSER) ?
@evanphx Owner
evanphx added a note

Why is this here? It seems unrelated to the rest of this change.

The url returned by download is escaped, so the expected value needs to be escaped as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/rubygems.rb
@@ -525,6 +525,21 @@ def self.find_home
private_class_method :find_home
##
+ # Dir.glob wrapper that takes a base directory
+
+ def self.glob(dir, pattern)
+ # TODO: move to utils
+ dir = File.expand_path(dir)
+ return [] unless File.directory?(dir)
+
+ Dir.chdir dir do
+ Dir.glob(pattern).map do |filename|
+ File.join(dir, filename).untaint
@drbrain Owner
drbrain added a note

This untaint seems too aggressive, it's untainting paths that were previously tainted, why?

There were a few places that needed them untainted, it seemed easier to untaint them here - could move it back if that's what you want.

@drbrain Owner
drbrain added a note

Untainting is performed for security reasons. Blindly untainting removes these security protections.

I've moved the untaints back to there original locations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/rubygems.rb
@@ -525,6 +525,21 @@ def self.find_home
private_class_method :find_home
##
+ # Dir.glob wrapper that takes a base directory
+
+ def self.glob(dir, pattern)
+ # TODO: move to utils
+ dir = File.expand_path(dir)
+ return [] unless File.directory?(dir)
+
+ Dir.chdir dir do
+ Dir.glob(pattern).map do |filename|
+ File.join(dir, filename).untaint
+ end
+ end
@drbrain Owner
drbrain added a note

Changing directories feels wrong, for multithreaded programs it randomly disrupts Dir.pwd for other threads.

Checking if a directory exists and rebuilding paths feels like unnecessary work that glob can do for us.

Why not the simpler:

def self.glob dir, pattern
  dir = dir.gsub(/[*?{}\[\]]/, '\\\\\&')

  pattern = File.join dir, pattern

  Dir.glob pattern
end

That's what's I was originally going to do - but I discovered jruby/jruby#172 and @tenderlove suggested I use Dir.chdir in rack/rack#389

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@drbrain
Owner

Do you have a real-world problem this solves?

I've never heard of this happening with RubyGems and I've never seen a bug report due to users having glob specials in their files or paths when building gems, installing gems or using installed gems.

I think "fixing" this in RubyGems is the wrong place. I think you should open a feature request against ruby to add a method that will escape glob special characters so they may be used in Dir.glob. Once the feature is added RubyGems can then use it.

@pixeltrix

Do you have a real-world problem this solves?

rails/rails#6010

I think "fixing" this in RubyGems is the wrong place. I think you should open a feature request against
ruby to add a method that will escape glob special characters so they may be used in Dir.glob.
Once the feature is added RubyGems can then use it.

Or I just close rails/rails#6010 and tell them not to use * ? { } [ ] in their paths because that's the effect of what you're suggesting. That's not a jibe - I was all for printing a warning message and terminating because I knew that trying to solve this would be awkward.

@pixeltrix

One thing to note is that RubyGems uses rm_rf in quite a few places - if someone was crazy enough to start using * in their paths then it's possible that RubyGems could end up deleting extra files. In light of this I think waiting for Ruby to add a method is wrong - if you want to fix it by escaping glob characters within RubyGems and tell JRuby to fix their bug then I'm fine with that.

@drbrain
Owner

That rails has a problem isn't a demonstration that RubyGems also has a similar problem in the wild. While it's easy to quickly make one up, nobody has done it by accident in eight years. There are many other ways you could cause RubyGems to break due to odd input that haven't been fixed either. Can you provide a justification for this beyond the hypothetical?

I'd rather maintain code to fix bugs in RubyGems that people experience infrequently over maintaining code for hypothetical issues. There's only so many ways in which you can prevent a user from shooting themselves in the foot before you collapse under the weight of preventative measures.

I don't see what the decisions the rails committers make on their issue has to do with the decisions we make for RubyGems. I don't see why the rejection of one issue should cause the rejection of the other.

Bugs in ruby implementations besides CRuby generally don't affect decisions made for RubyGems, especially of that bug is on behavior from the 1.8 series. JRuby bundles a known good copy of RubyGems which protects them from their bugs. In this case, it seems the JRuby bug is only triggered in the hypothetical case of a glob character in a file path.

Since this is a potential problem for many gems, fixing it in ruby seems like the best place. Reimplementing the same fix across hundreds or thousands of gems seems like a waste of developer effort if they can all use a ruby-provided solution to the problem. I strongly suggest you file a feature request on bugs.ruby-lang.org for this.

While I am dubious, I will continue to discuss this issue with the other committers.

@travisbot

This pull request fails (merged a1d8313 into 8f39d56).

@pixeltrix

That rails has a problem isn't a demonstration that RubyGems also has a similar problem in the wild. While it's easy to quickly make one up, nobody has done it by accident in eight years. There are many other ways you could cause RubyGems to break due to odd input that haven't been fixed either. Can you provide a justification for this beyond the hypothetical?

Standard deployment configuration for a Rails app using Bundler is to install gems into a shared 'bundle' folder within the application root. If the path to that application root has any glob characters then installed_specs will return an empty array. Whilst the reporter in the Rails ticket appears to be using his folder structure within development mode later comments seem to suggest a wish to use some of those characters in a deployment configuration.

I'd rather maintain code to fix bugs in RubyGems that people experience infrequently over maintaining code for hypothetical issues. There's only so many ways in which you can prevent a user from shooting themselves in the foot before you collapse under the weight of preventative measures.

I agree, but the pattern I'm using is already implemented in a couple of place within the RubyGems codebase - see install_executables and install_lib.

I don't see what the decisions the rails committers make on their issue has to do with the decisions we make for RubyGems. I don't see why the rejection of one issue should cause the rejection of the other.

If you decide not to fix it then it will affect how I resolve the ticket - if it will never work because of issues within RubyGems then I'd prefer to exit the application with an error message informing the developer that they should correct their paths. It's completely your choice - in fact you not fixing it will mean less work for me.

Bugs in ruby implementations besides CRuby generally don't affect decisions made for RubyGems, especially of that bug is on behavior from the 1.8 series. JRuby bundles a known good copy of RubyGems which protects them from their bugs. In this case, it seems the JRuby bug is only triggered in the hypothetical case of a glob character in a file path.

As I'm not currently using JRuby, or likely to to be in the future I'm not particularly fussed - just trying to be a good OSS citizen by not breaking things intentionally.

Since this is a potential problem for many gems, fixing it in ruby seems like the best place. Reimplementing the same fix across hundreds or thousands of gems seems like a waste of developer effort if they can all use a ruby-provided solution to the problem. I strongly suggest you file a feature request on bugs.ruby-lang.org for this.

Whilst adding a method to Ruby would be of benefit it would possibly only reduce the developer effort required by a fraction unless Dir[] could magically detect whether a glob should be interpreted literally or not. This may be possible - perhaps Ruby could see if a directory entry exists for the literal path segment and then only apply the glob pattern if it doesn't.

While I am dubious, I will continue to discuss this issue with the other committers.

Thank you.

@pixeltrix

Just to prove the bundler failure: https://gist.github.com/28c43d6ea2f3aca3f8ec

@pixeltrix pixeltrix closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 16, 2012
  1. @pixeltrix

    Refactor usage of Dir[] to eliminate unsafe glob construction

    pixeltrix authored
    Contructing globs by concatenation through File.join or string interpolation is
    susceptible to errors caused by paths inadvertently including glob characters.
    This commit fixes this by using Dir.chdir to change the current directory to a
    safe point from which to execute the Dir.glob and then reconstructs the path.
    
    There are no specific tests that have been added but the temporary test
    directory name has been altered to include the {} characters. This causes a
    significant number of the existing tests to fail which should prevent any
    regressions.
  2. @pixeltrix
Commits on May 17, 2012
  1. @pixeltrix

    Don't globally untaint globs

    pixeltrix authored
This page is out of date. Refresh to see the latest.
View
23 lib/rubygems.rb
@@ -261,7 +261,7 @@ def self.all_load_paths
# Return all the partial paths in +gemdir+.
def self.all_partials(gemdir)
- Dir[File.join(gemdir, "gems/*")]
+ Gem.glob(gemdir, "gems/*")
end
private_class_method :all_partials
@@ -469,7 +469,7 @@ def self.find_files(glob, check_load_path=true)
if check_load_path
files = $LOAD_PATH.map { |load_path|
- Dir["#{File.expand_path glob, load_path}#{Gem.suffix_pattern}"]
+ Gem.glob(load_path, "#{glob}#{Gem.suffix_pattern}")
}.flatten.select { |file| File.file? file.untaint }
end
@@ -525,6 +525,21 @@ def self.find_home
private_class_method :find_home
##
+ # Dir.glob wrapper that takes a base directory
+
+ def self.glob(dir, pattern)
+ # TODO: move to utils
@drbrain Owner
drbrain added a note

o_O

I'm assuming at some point a whole bunch of these methods would be moved to Gem::Utils - that doesn't exist at the moment does it? I didn't want to pollute the pull request with unrelated changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ dir = File.expand_path(dir)
+ return [] unless File.directory?(dir)
+
+ Dir.chdir dir do
+ Dir.glob(pattern).map do |filename|
+ File.join(dir, filename)
+ end
+ end
+ end
+
+ ##
# Zlib::GzipReader wrapper that unzips +data+.
def self.gunzip(data)
@@ -1099,11 +1114,9 @@ def self.load_plugins
# Find all 'rubygems_plugin' files in $LOAD_PATH and load them
def self.load_env_plugins
- path = "rubygems_plugin"
-
files = []
$LOAD_PATH.each do |load_path|
- globbed = Dir["#{File.expand_path path, load_path}#{Gem.suffix_pattern}"]
+ globbed = Gem.glob(load_path, "rubygems_plugin#{Gem.suffix_pattern}")
globbed.each do |load_path_file|
files << load_path_file if File.file?(load_path_file.untaint)
View
6 lib/rubygems/commands/contents_command.rb
@@ -81,9 +81,9 @@ def execute
end
gem_path = spec.full_gem_path
- extra = "/{#{spec.require_paths.join ','}}" if options[:lib_only]
- glob = "#{gem_path}#{extra}/**/*"
- files = Dir[glob]
+ extra = "{#{spec.require_paths.join ','}}/" if options[:lib_only]
+ glob = "#{extra}**/*"
+ files = Gem.glob(gem_path, glob)
gem_path = File.join gem_path, '' # add trailing / if missing
View
2  lib/rubygems/commands/setup_command.rb
@@ -254,7 +254,7 @@ def install_rdoc
(not File.exist? rubygems_doc_dir or
File.writable? rubygems_doc_dir) then
say "Removing old RubyGems RDoc and ri" if @verbose
- Dir[File.join(Gem.dir, 'doc', 'rubygems-[0-9]*')].each do |dir|
+ Gem.glob(File.join(Gem.dir, 'doc'), 'rubygems-[0-9]*').each do |dir|
rm_rf dir
end
View
2  lib/rubygems/commands/stale_command.rb
@@ -13,7 +13,7 @@ def execute
gem_to_atime = {}
Gem::Specification.each do |spec|
name = spec.full_name
- Dir["#{spec.full_gem_path}/**/*.*"].each do |file|
+ Gem.glob(spec.full_gem_path, "**/*.*").each do |file|
next if File.directory?(file)
stat = File.stat(file)
gem_to_atime[name] ||= stat.atime
View
2  lib/rubygems/indexer.rb
@@ -400,7 +400,7 @@ def compress(filename, extension)
# List of gem file names to index.
def gem_file_list
- Dir[File.join(@dest_directory, "gems", '*.gem')]
+ Gem.glob(File.join(@dest_directory, 'gems'), '*.gem')
end
##
View
2  lib/rubygems/installer.rb
@@ -299,7 +299,7 @@ def installed_specs
@specs ||= begin
specs = []
- Dir[File.join(gem_home, "specifications", "*.gemspec")].each do |path|
+ Gem.glob(File.join(gem_home, "specifications"), "*.gemspec").each do |path|
spec = Gem::Specification.load path.untaint
specs << spec if spec
end
View
2  lib/rubygems/request_set.rb
@@ -83,7 +83,7 @@ def sorted_requests
end
def specs_in(dir)
- Dir["#{dir}/specifications/*.gemspec"].map do |g|
+ Gem.glob(dir, "specifications/*.gemspec").map do |g|
Gem::Specification.load g
end
end
View
4 lib/rubygems/security/trust_dir.rb
@@ -27,9 +27,7 @@ def cert_path certificate
def each_certificate
return enum_for __method__ unless block_given?
- glob = File.join @dir, '*.pem'
-
- Dir[glob].each do |certificate_file|
+ Gem.glob(@dir, '*.pem').each do |certificate_file|
begin
certificate = load_certificate certificate_file
View
8 lib/rubygems/specification.rb
@@ -632,7 +632,7 @@ def self._all # :nodoc:
specs = {}
self.dirs.each { |dir|
- Dir[File.join(dir, "*.gemspec")].each { |path|
+ Gem.glob(dir, "*.gemspec").each { |path|
spec = Gem::Specification.load path.untaint
# #load returns nil if the spec is bad, so we just ignore
# it at this stage
@@ -1827,10 +1827,8 @@ def mark_version
# Return all files in this gem that match for +glob+.
def matches_for_glob glob # TODO: rename?
- # TODO: do we need these?? Kill it
- glob = File.join(self.lib_dirs_glob, glob)
-
- Dir[glob].map { |f| f.untaint } # FIX our tests are broken, run w/ SAFE=1
+ matches = Gem.glob(full_gem_path, "{#{require_paths.join ','}}/#{glob}")
+ matches.map{ |f| f.untaint } # FIX our tests are broken, run w/ SAFE=1
end
##
View
8 lib/rubygems/test_case.rb
@@ -107,7 +107,7 @@ def refute_path_exists path, msg = nil
# or <tt>i686-darwin8.10.1</tt> otherwise.
#
# If the +KEEP_FILES+ environment variable is set the files will not be
- # removed from <tt>/tmp/test_rubygems_#{$$}.#{Time.now.to_i}</tt>.
+ # removed from <tt>/tmp/test_rubygems_{#{$$}}.#{Time.now.to_i}</tt>.
def setup
super
@@ -122,9 +122,9 @@ def setup
tmpdir = File.expand_path("tmp/test")
if ENV['KEEP_FILES'] then
- @tempdir = File.join(tmpdir, "test_rubygems_#{$$}.#{Time.now.to_i}")
+ @tempdir = File.join(tmpdir, "test_rubygems_{#{$$}}.#{Time.now.to_i}")
@evanphx Owner
evanphx added a note

I don't understand why you've wrapped the value with {}. Could you explain?

To test that glob characters don't cause a problem - writing a whole bunch of extra tests seemed excessive

@raggi Collaborator
raggi added a note

It's not excessive when someone comes along and removes that because they don't know why it's there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
else
- @tempdir = File.join(tmpdir, "test_rubygems_#{$$}")
+ @tempdir = File.join(tmpdir, "test_rubygems_{#{$$}}")
end
@tempdir.untaint
@gemhome = File.join @tempdir, 'gemhome'
@@ -298,7 +298,7 @@ def uninstall_gem spec
def create_tmpdir
tmpdir = nil
Dir.chdir Dir.tmpdir do tmpdir = Dir.pwd end # HACK OSX /private/tmp
- tmpdir = File.join tmpdir, "test_rubygems_#{$$}"
+ tmpdir = File.join tmpdir, "test_rubygems_{#{$$}}"
FileUtils.mkdir_p tmpdir
return tmpdir
end
View
4 test/rubygems/test_gem_commands_install_command.rb
@@ -391,7 +391,7 @@ def test_execute_remote_ignores_files
FileUtils.rm_rf a1_gemspec
FileUtils.rm_rf a2_gemspec
- start = Dir["#{gemdir}/*"]
+ start = Gem.glob(gemdir, "*")
use_ui @ui do
Dir.chdir @tempdir do
@@ -408,7 +408,7 @@ def test_execute_remote_ignores_files
assert_equal "1 gem installed", out.shift
assert out.empty?, out.inspect
- fin = Dir["#{gemdir}/*"]
+ fin = Gem.glob(gemdir, "*")
assert_equal [a1_gemspec], fin - start
end
View
2  test/rubygems/test_gem_indexer.rb
@@ -28,7 +28,7 @@ def setup
gems = File.join(@tempdir, 'gems')
FileUtils.mkdir_p gems
- FileUtils.mv Dir[File.join(@gemhome, "cache", '*.gem')], gems
+ FileUtils.mv Gem.glob(File.join(@gemhome, "cache"), '*.gem'), gems
@indexer = Gem::Indexer.new(@tempdir,
:rss_title => 'ExampleForge gems',
View
7 test/rubygems/test_gem_remote_fetcher.rb
@@ -292,8 +292,11 @@ def test_download_local_read_only
inst = Gem::RemoteFetcher.fetcher
end
- assert_equal(File.join(@tempdir, @a1.file_name),
- inst.download(@a1, local_path))
+ uri = URI.const_defined?(:DEFAULT_PARSER) ?
@evanphx Owner
evanphx added a note

Why is this here? It seems unrelated to the rest of this change.

The url returned by download is escaped, so the expected value needs to be escaped as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ URI::DEFAULT_PARSER.escape(File.join(@tempdir, @a1.file_name)) :
+ URI.escape(File.join(@tempdir, @a1.file_name))
+
+ assert_equal(uri, inst.download(@a1, local_path))
ensure
FileUtils.chmod 0755, @a1.cache_dir
end
Something went wrong with that request. Please try again.