Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Whitelist YAML classes demarshalled from metadata Edit #516

Merged
merged 7 commits into from

8 participants

@sj26

This is a temporary fix which should prevent the same injection vector being used again.

I'm going to gauntlet all gems through the upload process to see if there are any uncaught classes.

@qrush qrush commented on the diff
config/initializers/forbidden_yaml.rb
((34 lines not shown))
+module Gem
+ class Specification
+ def self.from_yaml input
+ input = normalize_yaml_input input
+ nodes = Psych.parse input
+ spec = Psych::Visitors::WhitelistedToRuby.new.accept nodes
+
+ if spec && spec.class == FalseClass then
+ raise Gem::EndOfYAMLException
+ end
+
+ unless Gem::Specification === spec then
+ raise Gem::Exception, "YAML data doesn't evaluate to gem specification"
+ end
+
+ unless (spec.instance_variables.include? '@specification_version' or
@qrush Owner
qrush added a note

maybe i'm really tired but this entire conditional hurts my brain. let's keep it positive!

@qrush Owner
qrush added a note

also use &&/|| instead of and/or :tired_face:

@sj26
sj26 added a note

This whole method was copied from rubygems verbatim, except for the YAML changes at the top. It's awful code, but I don't want to mess with the rubygems gods.

@jhulten
jhulten added a note

If it was copied from rubygems would it be better to alias and wrap the RG call in your conditions? Been a while since I paid attention to these practices...

This would allow the RG call to change without you missing out.

@sj26
sj26 added a note

No, we need to modify some of the internal method, hence it needs to be copied wholesale.

@sj26
sj26 added a note

Again, note this is a temporary patch and should be modified and pushed upstream into rubygems long term.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@qrush qrush commented on the diff
app/models/pusher.rb
@@ -38,6 +38,9 @@ def pull_spec
end
false
+ rescue Psych::ForbiddenClassException => e
+ Rails.logger.info "Attempted YAML metadata exploit: #{e}"
@qrush Owner
qrush added a note

Do we need to notify? I'm not sure if we care to log this.

@sj26
sj26 added a note

My reasoning was for diagnosing legitimate gem upload problems in case any classes have been missed.

This should be logged. It either points to someone attempting an attack, or that RubyGems has introduced a new class that is not yet whitelisted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
config/initializers/forbidden_yaml.rb
((12 lines not shown))
+
+ module Visitors
+ class WhitelistedToRuby < ToRuby
+ WHITELIST = %w(
+ Gem::Dependency
+ Gem::Platform
+ Gem::Requirement
+ Gem::Specification
+ Gem::Version
+ Gem::Version::Requirement
+ )
+
+ private
+
+ def resolve_class klassname
+ raise ForbiddenClassException, "Forbidden class in YAML: #{klassname}" unless WHITELIST.include? klassname

(whoops, my bad - misunderstood what was going on here)

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

A whitelist of trusted hydratable classes is the right way to do it.

It's also a good idea to blacklist symbols and aliases too.

@sj26

Rubygems metadata does legitimately use symbols. It's definitely a potential DoS vector which I was considering, but I'm not sure we can just whitelist some symbols. Let me look into it.

Aliases I'm not so worried about tbh. They can only alias other parts of the YAML which have all the same restrictions.

@jhulten jhulten commented on the diff
app/models/pusher.rb
@@ -38,6 +38,9 @@ def pull_spec
end
false
+ rescue Psych::ForbiddenClassException => e
+ Rails.logger.info "Attempted YAML metadata exploit: #{e}"
+ notify("RubyGems.org cannot process this gem.\nThe metadata is invalid.\n#{e}", 422)
@sj26
sj26 added a note

We understood the request, but it is invalid, hence 422.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@evanphx evanphx merged commit fdf2195 into from
@postmodern

There are other instances of Psych.load_tags that you want to hook. See postmodern/psych@64c6e80

Nevermind, YAML tags cannot map to arbitrary classes.

@rkh

instance_variable_defined? maybe?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
7 app/controllers/api/v1/rubygems_controller.rb
@@ -23,10 +23,9 @@ def show
end
def create
- render :text => "Pushing is temporarily disabled. Please check http://status.rubygems.org for more info.", :status => 503
- #gemcutter = Pusher.new(current_user, request.body, request.host_with_port)
- #gemcutter.process
- #render :text => gemcutter.message, :status => gemcutter.code
+ gemcutter = Pusher.new(current_user, request.body, request.host_with_port)
+ gemcutter.process
+ render :text => gemcutter.message, :status => gemcutter.code
end
def yank
View
5 app/models/pusher.rb
@@ -38,6 +38,9 @@ def pull_spec
end
false
+ rescue Psych::ForbiddenClassException => e
+ Rails.logger.info "Attempted YAML metadata exploit: #{e}"
@qrush Owner
qrush added a note

Do we need to notify? I'm not sure if we care to log this.

@sj26
sj26 added a note

My reasoning was for diagnosing legitimate gem upload problems in case any classes have been missed.

This should be logged. It either points to someone attempting an attack, or that RubyGems has introduced a new class that is not yet whitelisted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ notify("RubyGems.org cannot process this gem.\nThe metadata is invalid.\n#{e}", 422)
@sj26
sj26 added a note

We understood the request, but it is invalid, hence 422.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
rescue Gem::Package::FormatError
notify("RubyGems.org cannot process this gem.\nPlease try rebuilding it" +
" and installing it locally to make sure it's valid.", 422)
@@ -48,7 +51,7 @@ def pull_spec
end
def find
- @rubygem = Rubygem.find_or_initialize_by_name(spec.name)
+ @rubygem = Rubygem.find_or_initialize_by_name(spec.name.to_s)
@version = @rubygem.find_or_initialize_version_from_spec(spec)
if @version.new_record?
View
67 config/initializers/forbidden_yaml.rb
@@ -0,0 +1,67 @@
+# XXX: This is purely a monkey patch to close the exploit vector for now, a more
+# permanent solution should be pushed upstream into rubygems.
+
+require "rubygems"
+
+# Assert we're using Psych
+abort "Use Psych for YAML, install libyaml and reinstall ruby" unless YAML == Psych
+
+module Psych
+ class ForbiddenClassException < Exception
+ end
+
+ module Visitors
+ class WhitelistedToRuby < ToRuby
+ WHITELISTED_CLASSES = %w(
+ Gem::Dependency
+ Gem::Platform
+ Gem::Requirement
+ Gem::Specification
+ Gem::Version
+ Gem::Version::Requirement
+ )
+
+ def deserialize o
+ if klass = Psych.load_tags[o.tag]
+ raise ForbiddenClassException, "Forbidden class in YAML: #{klassname}" unless WHITELISTED_CLASSES.include? klass.name
+ end
+
+ super
+ end
+
+ private
+
+ def resolve_class klassname
+ raise ForbiddenClassException, "Forbidden class in YAML: #{klassname}" unless WHITELISTED_CLASSES.include? klassname
+ super klassname
+ end
+ end
+ end
+end
+
+module Gem
+ class Specification
+ def self.from_yaml input
+ input = normalize_yaml_input input
+ nodes = Psych.parse input
+ spec = Psych::Visitors::WhitelistedToRuby.new.accept nodes
+
+ if spec && spec.class == FalseClass then
+ raise Gem::EndOfYAMLException
+ end
+
+ unless Gem::Specification === spec then
+ raise Gem::Exception, "YAML data doesn't evaluate to gem specification"
+ end
+
+ unless (spec.instance_variables.include? '@specification_version' or
@qrush Owner
qrush added a note

maybe i'm really tired but this entire conditional hurts my brain. let's keep it positive!

@qrush Owner
qrush added a note

also use &&/|| instead of and/or :tired_face:

@sj26
sj26 added a note

This whole method was copied from rubygems verbatim, except for the YAML changes at the top. It's awful code, but I don't want to mess with the rubygems gods.

@jhulten
jhulten added a note

If it was copied from rubygems would it be better to alias and wrap the RG call in your conditions? Been a while since I paid attention to these practices...

This would allow the RG call to change without you missing out.

@sj26
sj26 added a note

No, we need to modify some of the internal method, hence it needs to be copied wholesale.

@sj26
sj26 added a note

Again, note this is a temporary patch and should be modified and pushed upstream into rubygems long term.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ spec.instance_variables.include? :@specification_version) and
+ spec.instance_variable_get :@specification_version
+ spec.instance_variable_set :@specification_version,
+ NONEXISTENT_SPECIFICATION_VERSION
+ end
+
+ spec
+ end
+ end
+end
View
BIN  test/gems/exploit.gem
Binary file not shown
View
11 test/unit/pusher_test.rb
@@ -80,6 +80,17 @@ class PusherTest < ActiveSupport::TestCase
assert_equal @cutter.code, 422
end
+ should "not be able to pull spec with metadata containing bad ruby objects" do
+ @gem = gem_file("exploit.gem")
+ @cutter = Pusher.new(@user, @gem)
+ @cutter.pull_spec
+ assert_nil @cutter.spec
+ assert_match %r{RubyGems\.org cannot process this gem}, @cutter.message
+ assert_match %r{The metadata is invalid.}, @cutter.message
+ assert_match %r{ActionController::Routing::RouteSet::NamedRouteCollection}, @cutter.message
+ assert_equal @cutter.code, 422
+ end
+
should "post info to the remote bundler API" do
@cutter.pull_spec
Something went wrong with that request. Please try again.