Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Whitelist YAML classes demarshalled from metadata Edit #516

Merged
merged 7 commits into from Feb 1, 2013
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 3 additions & 4 deletions app/controllers/api/v1/rubygems_controller.rb
Expand Up @@ -23,10 +23,9 @@ def show
end end


def create 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 = Pusher.new(current_user, request.body, request.host_with_port) gemcutter.process
#gemcutter.process render :text => gemcutter.message, :status => gemcutter.code
#render :text => gemcutter.message, :status => gemcutter.code
end end


def yank def yank
Expand Down
5 changes: 4 additions & 1 deletion app/models/pusher.rb
Expand Up @@ -38,6 +38,9 @@ def pull_spec
end end


false false
rescue Psych::ForbiddenClassException => e
Rails.logger.info "Attempted YAML metadata exploit: #{e}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

notify("RubyGems.org cannot process this gem.\nThe metadata is invalid.\n#{e}", 422)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

rescue Gem::Package::FormatError rescue Gem::Package::FormatError
notify("RubyGems.org cannot process this gem.\nPlease try rebuilding it" + notify("RubyGems.org cannot process this gem.\nPlease try rebuilding it" +
" and installing it locally to make sure it's valid.", 422) " and installing it locally to make sure it's valid.", 422)
Expand All @@ -48,7 +51,7 @@ def pull_spec
end end


def find 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) @version = @rubygem.find_or_initialize_version_from_spec(spec)


if @version.new_record? if @version.new_record?
Expand Down
67 changes: 67 additions & 0 deletions 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also use &&/|| instead of and/or 😫

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Binary file added test/gems/exploit.gem
Binary file not shown.
11 changes: 11 additions & 0 deletions test/unit/pusher_test.rb
Expand Up @@ -80,6 +80,17 @@ class PusherTest < ActiveSupport::TestCase
assert_equal @cutter.code, 422 assert_equal @cutter.code, 422
end 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 should "post info to the remote bundler API" do
@cutter.pull_spec @cutter.pull_spec


Expand Down