Protect against symbol injection DoS attacks #521

Merged
merged 1 commit into from Feb 5, 2013

Conversation

Projects
None yet
5 participants
Contributor

sj26 commented Feb 2, 2013

Expands the YAML whitelisting patch to include whitelisting symbols.

The five symbols whitelisted are surveyed from all metadata of all existing gems using this gist:

  • :development and :runtime, used legitimately by Gem::Dependency
  • :rdoc which is incorrectly serialized in rdoc_options in "fisheye-crucible-0.0.1"
  • :json and :mocha, which are incorrectly serialized gem names from Gem::Dependencys in some versions of "ezri" (0.03 - 0.07)

There are four different ways to serialize symbols in YAML:

  • :symbol parsed as a scalar
  • :"symbol" parsed as a scalar
  • !ruby/sym symbol parsed as a tagged value (also subclasses like !ruby/sym:MySymbolSubclass symbol)
  • !ruby/symbol symbol parsed as a tagged value (also subclasses like !ruby/symbol:MySymbolSubclass symbol)

@tenderlove tenderlove commented on the diff Feb 2, 2013

config/initializers/forbidden_yaml.rb
+ WHITELISTED_CLASSES = %w(
+ Gem::Dependency
+ Gem::Platform
+ Gem::Requirement
+ Gem::Specification
+ Gem::Version
+ Gem::Version::Requirement
+ )
+
+ # These are all unique symbols used across all currently published gems' metadata
+ WHITELISTED_SYMBOLS = %w(
+ development
+ json
+ mocha
+ rdoc
+ runtime
@tenderlove

tenderlove Feb 2, 2013

Contributor

How many gems have these in the gemspecs? Whitelisting particular symbols seems like a PITA.

@sj26

sj26 Feb 2, 2013

Contributor

Agreed, but these are already in a couple of gems mentioned in the pull request description. I wouldn't add any more.

Then again, there are already a bunch of gems that just have completely broken metadata, so maybe we don't care about it? Perhaps only development and runtime should be allowed?

@tenderlove

tenderlove Feb 2, 2013

Contributor

I've pushed a branch to psych that contains a whitelisting safe loader. I've introduced a class loader object that can be used for implementing the whitelist. We could run all the symbols through there as well.

@sj26

sj26 Feb 2, 2013

Contributor

Great! My next step was going to be sending you a pull with a more comprehensive whitelisting solution. :-)

Could you perhaps push the actual instantiation into the class loader as well? That was the symbol values can be vetted by the loader too.

@postmodern

postmodern Feb 5, 2013

Contributor

@tenderlove should I bother submitting a pull request for https://github.com/postmodern/psych/tree/safe_load or are you going to use your safe loader branch?

@tenderlove tenderlove commented on the diff Feb 2, 2013

config/initializers/forbidden_yaml.rb
@@ -7,32 +7,69 @@
abort "Use Psych for YAML, install libyaml and reinstall ruby" unless YAML == Psych
module Psych
- class ForbiddenClassException < Exception
+ class WhitelistException < Exception; end
+ class ForbiddenClassException < WhitelistException; end
+ class ForbiddenSymbolException < WhitelistException; end
@tenderlove

tenderlove Feb 2, 2013

Contributor

It it necessary to distinguish between these?

@sj26

sj26 Feb 2, 2013

Contributor

Probably not necessary, no, but it seemed like a nice idea. Do you think it should be reduced to a single exception class?

@tenderlove

tenderlove Feb 2, 2013

Contributor

shrug it's up to you. I've only put one exception class in the whitelist stuff in Psych. If you need two, I will provide it. :)

@sj26

sj26 Feb 2, 2013

Contributor

A single exception class is fine.

Contributor

tenderlove commented Feb 5, 2013

Any news on this PR? Is there more that needs to be done?

Contributor

sj26 commented Feb 5, 2013

This is ready to pull afaict, up to @evanphx

Member

sferik commented Feb 5, 2013

Looks good to me. Would you like me to deploy this as well? Seems good to prevent more downtime.

sferik merged commit 639bf6b into rubygems:master Feb 5, 2013

1 check was pending

default The Travis build is in progress
Details
Owner

evanphx commented Feb 5, 2013

This patch is fine. There are really 2 legit symbols, :development and :runtime, that are used in a Specification, everything else is Strings. The 3 others are weird Symbols so that we don't break gems, but really we should probably only allow :development and :runtime since those others don't need to be in future gems anyway, those authors can just fix their gem.

sj26 deleted the sj26:whitelist-yaml-symbols branch Feb 5, 2013

Member

sferik commented Feb 5, 2013

@evanphx Do you want me to remove json, mocha, and rdoc from WHITELISTED_SYMBOLS before I deploy? Also, there appears to be one failing cucumber scenario: "User pushes gem with bad name". Is that something we need to fix before we deploy? I believe it's failing in the current production code and deploying this patch may be a higher priority but it seems like something we should fix relatively quickly, if not immediately. I can look into. Hopping into #rubygems on IRC now.

Contributor

tenderlove commented Feb 5, 2013

@sj26 would you mind reviewing the changes I made to Psych for safe loading? If those changes are good enough for rubygems.org, then I'll merge it in and we can eventually rm this code from rubygems.org.

Contributor

sj26 commented Feb 6, 2013

@tenderlove definitely, I'll take a look.

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