From 7fb5cd50c51f7590b2396bac5fe723eef53b1bba Mon Sep 17 00:00:00 2001 From: Samuel Cochran Date: Thu, 31 Jan 2013 15:55:59 +1100 Subject: [PATCH 1/7] Patch YAML to allow only some Gem classes to be unmarshalled --- config/initializers/forbidden_yaml.rb | 59 +++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 config/initializers/forbidden_yaml.rb diff --git a/config/initializers/forbidden_yaml.rb b/config/initializers/forbidden_yaml.rb new file mode 100644 index 00000000000..7fa9c2bc188 --- /dev/null +++ b/config/initializers/forbidden_yaml.rb @@ -0,0 +1,59 @@ +# 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 + 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 + 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 + 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 From c3a9f1f6e8ffb767b5bcbbb35c075350b61b0ada Mon Sep 17 00:00:00 2001 From: Samuel Cochran Date: Thu, 31 Jan 2013 15:57:16 +1100 Subject: [PATCH 2/7] Catch and report YAML exploit errors separately --- app/models/pusher.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/models/pusher.rb b/app/models/pusher.rb index 15763020ecc..60318e4e0a0 100644 --- a/app/models/pusher.rb +++ b/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) 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) From 173fe338e7cdcd907ba9f6a2a83721659970bc54 Mon Sep 17 00:00:00 2001 From: Samuel Cochran Date: Thu, 31 Jan 2013 15:57:25 +1100 Subject: [PATCH 3/7] Revert "Temporarily disable pushing" This reverts commit 3fe95b2e24171f0325e023e08d5472c3977e7b2b. --- app/controllers/api/v1/rubygems_controller.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/app/controllers/api/v1/rubygems_controller.rb b/app/controllers/api/v1/rubygems_controller.rb index a21fd564c34..e0a665ccb1d 100644 --- a/app/controllers/api/v1/rubygems_controller.rb +++ b/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 From 04d3093ba8b762a3a2e1f9587801fa5a76169eb9 Mon Sep 17 00:00:00 2001 From: Samuel Cochran Date: Thu, 31 Jan 2013 16:14:29 +1100 Subject: [PATCH 4/7] Add a test for exploit preventative behaviour --- test/gems/exploit.gem | Bin 0 -> 3584 bytes test/unit/pusher_test.rb | 11 +++++++++++ 2 files changed, 11 insertions(+) create mode 100644 test/gems/exploit.gem diff --git a/test/gems/exploit.gem b/test/gems/exploit.gem new file mode 100644 index 0000000000000000000000000000000000000000..a047cdfdc4046b7800654b7e6952d5faf71a0dcf GIT binary patch literal 3584 zcmeHI-*4MC5Y99Iid*;K7N{IrO4ETrw*}d-4cOYC0SdHDA<)!GVk?m-HtuwmMtx-4xxUlEY zU=N{{VVk}O3D;w1(8;y#bl8A4q=0bYH>#y9-=6*RH;dUy=g~cdn^vn9WY^gnpA>(5 z|KSDnNcmO}EUXa}y~PTw0qNE{IO#UmXVUHCOth;Z;r>l!xc z%XK&7h_?YvB*X+OZJjJ#WmQ@0t;y0GiY9=%WJSaM`j6(|2QWy(Q@HqoimYc>n3GCC-C+`P~G*Aw9)ZUmCC zhGMH_=aF`^k%XzBeMUdx&Cf3uw29mM=Jx#%HFka%d~r9V2=4zXtSQOZT@f+jOoQd% zY#I=dWOqYeEq_|(C(P@{Xw#IRa6>)ozZ@d_^a|$3z9){0)L^}&I%_-n2U;!m(z3I% zicPlT4hu+J*t2UMOxuCfO5chz6)8=P9Gs=*azebe#B$S24|s_|h?g|iswl_vsFaRS zEq|f1KB}#~GLN0q9$_(iz-sa2%9Q2rFMlh`EAxnSw@&Bb`HF_jWpJa4^Z^L5O(Iv~ zB`U1t*ac$cBp{JYJ_RU;)X;mm+6N94iQk&o5t*h78Hxp(Y>EG}a~6NC1CLFteXQUv n&J0AH)N;zHw-tREQer@vBKuB0bcZkx2pkYNAaFq74g&uI`cEl- literal 0 HcmV?d00001 diff --git a/test/unit/pusher_test.rb b/test/unit/pusher_test.rb index 4fda5ab4ec9..52fccd205b1 100644 --- a/test/unit/pusher_test.rb +++ b/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 From 8edff951ee02e751b03ce5fac4d0a9e6f6141db5 Mon Sep 17 00:00:00 2001 From: Samuel Cochran Date: Thu, 31 Jan 2013 17:58:02 +1100 Subject: [PATCH 5/7] YAML's scalar parsing yields numbers sometimes, which are erroneous but should be caught cleanly --- app/models/pusher.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/pusher.rb b/app/models/pusher.rb index 60318e4e0a0..6187a02d8b9 100644 --- a/app/models/pusher.rb +++ b/app/models/pusher.rb @@ -51,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? From acd74efd7c099f98d0ecc3a887f972fd6effdb48 Mon Sep 17 00:00:00 2001 From: Samuel Cochran Date: Fri, 1 Feb 2013 14:54:49 +1100 Subject: [PATCH 6/7] Clarify whitelist of classes --- config/initializers/forbidden_yaml.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/initializers/forbidden_yaml.rb b/config/initializers/forbidden_yaml.rb index 7fa9c2bc188..d4448cb69e2 100644 --- a/config/initializers/forbidden_yaml.rb +++ b/config/initializers/forbidden_yaml.rb @@ -12,7 +12,7 @@ class ForbiddenClassException < Exception module Visitors class WhitelistedToRuby < ToRuby - WHITELIST = %w( + WHITELISTED_CLASSES = %w( Gem::Dependency Gem::Platform Gem::Requirement @@ -24,7 +24,7 @@ class WhitelistedToRuby < ToRuby private def resolve_class klassname - raise ForbiddenClassException, "Forbidden class in YAML: #{klassname}" unless WHITELIST.include? klassname + raise ForbiddenClassException, "Forbidden class in YAML: #{klassname}" unless WHITELISTED_CLASSES.include? klassname super klassname end end From 8f2906efcd66a8b713b83375bbac35dbdfd6188c Mon Sep 17 00:00:00 2001 From: Samuel Cochran Date: Fri, 1 Feb 2013 14:59:09 +1100 Subject: [PATCH 7/7] Make sure preloaded tag coders are whitelisted too --- config/initializers/forbidden_yaml.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/config/initializers/forbidden_yaml.rb b/config/initializers/forbidden_yaml.rb index d4448cb69e2..d623a59c45b 100644 --- a/config/initializers/forbidden_yaml.rb +++ b/config/initializers/forbidden_yaml.rb @@ -21,6 +21,14 @@ class WhitelistedToRuby < ToRuby 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