4 ms·

Denial of Service and Unsafe Object Creation Vulnerability in JSON Gem

72 points by ontoillogical 14y ago

Tho85 14y ago
Some details on how this can be exploited: http://www.zweitag.de/en/blog/ruby-on-rails-vulnerable-to-mass-assignment-and-sql-injection http://www.zweitag.de/en/blog/ruby-on-rails-vulnerable-to-ma...
- tenderlove 14y ago
  Thanks for reporting this issue to us! :-D <3<3<3<3
  - Tho85 14y ago
    Was a pleasure! With love :-) Thomas
- deleted 14y ago
  [deleted]
- benmmurphy 14y ago
  nice find. there is sql injection in 2.3.x and it effects all adapters and not just postgresql. i just tried quoted_id and it works against mysql on 3.2.x as well. quoted_id is defined in abstract/quoting.rb and any adapter that forwards quotes to the superclass will use it.
  - Tho85 14y ago
    You're right! Thanks for the hint, I updated the blog post accordingly.
zyang 14y ago
Is it Monday again?
mapgrep 14y ago
>`JSON.load` should never be given input from unknown sources. If you are processing JSON from an unknown source, always use `JSON.parse`. This seems like poor method naming; I would not intuitively understand that "load" is far more dangerous than "parse." Why not deprecate these and do names like JSON.load_trusted JSON.load_untrusted
- jdleesmiller 14y ago
  A few further notes on this, following some reading... The docs aren't very clear about it, either (http://flori.github.com/json/doc/index.html http://flori.github.com/json/doc/index.html). There is a JSON.parse! method that they explicitly say is to be used only for trusted input, and it looks like JSON.load has some of the same default options. To further confuse things, JSON.load has the very useful property that you can pass it a String or an IO (e.g. from Rack::Request#body), whereas JSON.parse only accepts a String. edit: the docs will soon contain a warning about JSON.load; see https://github.com/flori/json/blob/master/lib/json/common.rb https://github.com/flori/json/blob/master/lib/json/common.rb
- dbaupp 14y ago
  Better would be load and load_trusted so that the safer function has the short (and expected) name.
  - MatthewPhillips 14y ago
    Why have the unsafe function at all?
- mikeash 14y ago
  Better yet would be to remove the trusted functionality entirely and give a good talking-to to whoever wrote it. A generic JSON parser shouldn't have an unsafe parsing method on it period. If you want to implement clever nonsense like this, do it at the next layer up and call it something else.
aaroncray 14y ago
HN =? Bugtraq
lkrubner 14y ago
I apologize for the ignorant question, but how does Ruby survive this in normal operation? "Since Ruby symbols are not garbage collected, this can result in a denial of service attack." If you have a long running Ruby app,and it does not garbage collect symbols, then those symbols are... constants I guess?That survive till the app stops operating? So I guess the assumption is that no app should use too many symbols (and they don't use much memory anyway?)
- Confusion 14y ago
  A long running app will normally reach a steady state in which no new symbols are created anymore. If you for any reason create new symbols (which are basically interned strings) every n seconds or requests, you will eventually run out of memory. However, that is almost never a concern in practice, because you rarely need to do that.
- oinksoft 14y ago
  This is not an ignorant question. Any half-experienced Erlang developer can tell you that you use `list_to_existing_atom' rather than `list_to_atom' if you are ever doing dynamic things with atoms. So, if you're trying to accomplish dynamic module lookup and foo_baz.beam is in your code path, default startup will create the `foo_baz' atom, and you know that this atom will exist at runtime. Symbols in Ruby are atoms (the term "atom" spans languages), and GC/space issues plague any persistent term like an atom, in any language. And thus I arrive at a key question: Does Ruby have something like `list_to_existing_atom', or some mechanism for telling if a symbol exists already? I see no analog to this, only the `ID2SYM' macro in the extensions API, and similar calls like String#to_sym. Perhaps there is some way to clean up symbols after they are created. This to me would seem like the ideal route. It's good they've got a stop-gap fix by changing defaults, but it feels to me like they're punting here. Perhaps users who do [ab]use this feature also would not like DOS attacks? I hope others who know more about Ruby extension development, and symbol management capabilities, can chime in on these questions.
  - benmmurphy 14y ago
    there are methods in the ruby reflection api that take strings and silently convert them to symbols so it is really easy to accidentally leak memory. however, on ruby trunk they added a method: rb_check_id which can be used to check if a string has been already symbolized (https://github.com/ruby/ruby/blob/trunk/parse.y#L10465 https://github.com/ruby/ruby/blob/trunk/parse.y#L10465). this means when these reflection methods get passed a string and it hasn't been symbolized they can bail out and not symbolize the string. (https://github.com/ruby/ruby/blob/trunk/object.c#L2073 https://github.com/ruby/ruby/blob/trunk/object.c#L2073)
benmmurphy 14y ago
also if you have done require 'json/add/rails' you are in for fun (https://github.com/ruby/ruby/blob/v1_9_2_381/ext/json/lib/json/add/rails.rb#L10 https://github.com/ruby/ruby/blob/v1_9_2_381/ext/json/lib/js...) irb(main):001:0> require 'json/add/rails' => true irb(main):002:0> class Foo irb(main):003:1> end => nil irb(main):004:0> Foo.json_create({"x" => "bar"}) => #<Foo:0x007fc5f3149540> https://github.com/search?q=require+%27json%2Fadd%2Frails%27&type=Code&ref=searchresults https://github.com/search?q=require+%27json%2Fadd%2Frails%27...

72 points by ontoillogical 14y ago

Tho85 14y ago

Some details on how this can be exploited: http://www.zweitag.de/en/blog/ruby-on-rails-vulnerable-to-mass-assignment-and-sql-injection http://www.zweitag.de/en/blog/ruby-on-rails-vulnerable-to-ma...

tenderlove 14y ago
Thanks for reporting this issue to us! :-D <3<3<3<3
- Tho85 14y ago
  Was a pleasure! With love :-) Thomas
deleted 14y ago
[deleted]
benmmurphy 14y ago
nice find. there is sql injection in 2.3.x and it effects all adapters and not just postgresql. i just tried quoted_id and it works against mysql on 3.2.x as well. quoted_id is defined in abstract/quoting.rb and any adapter that forwards quotes to the superclass will use it.
- Tho85 14y ago
  You're right! Thanks for the hint, I updated the blog post accordingly.

zyang 14y ago

Is it Monday again?

mapgrep 14y ago

>`JSON.load` should never be given input from unknown sources. If you are processing JSON from an unknown source, always use `JSON.parse`. This seems like poor method naming; I would not intuitively understand that "load" is far more dangerous than "parse." Why not deprecate these and do names like JSON.load_trusted JSON.load_untrusted

jdleesmiller 14y ago
A few further notes on this, following some reading... The docs aren't very clear about it, either (http://flori.github.com/json/doc/index.html http://flori.github.com/json/doc/index.html). There is a JSON.parse! method that they explicitly say is to be used only for trusted input, and it looks like JSON.load has some of the same default options. To further confuse things, JSON.load has the very useful property that you can pass it a String or an IO (e.g. from Rack::Request#body), whereas JSON.parse only accepts a String. edit: the docs will soon contain a warning about JSON.load; see https://github.com/flori/json/blob/master/lib/json/common.rb https://github.com/flori/json/blob/master/lib/json/common.rb
dbaupp 14y ago
Better would be load and load_trusted so that the safer function has the short (and expected) name.
- MatthewPhillips 14y ago
  Why have the unsafe function at all?
mikeash 14y ago
Better yet would be to remove the trusted functionality entirely and give a good talking-to to whoever wrote it. A generic JSON parser shouldn't have an unsafe parsing method on it period. If you want to implement clever nonsense like this, do it at the next layer up and call it something else.

aaroncray 14y ago

HN =? Bugtraq

lkrubner 14y ago

I apologize for the ignorant question, but how does Ruby survive this in normal operation? "Since Ruby symbols are not garbage collected, this can result in a denial of service attack." If you have a long running Ruby app,and it does not garbage collect symbols, then those symbols are... constants I guess?That survive till the app stops operating? So I guess the assumption is that no app should use too many symbols (and they don't use much memory anyway?)

Confusion 14y ago
A long running app will normally reach a steady state in which no new symbols are created anymore. If you for any reason create new symbols (which are basically interned strings) every n seconds or requests, you will eventually run out of memory. However, that is almost never a concern in practice, because you rarely need to do that.
oinksoft 14y ago
This is not an ignorant question. Any half-experienced Erlang developer can tell you that you use `list_to_existing_atom' rather than `list_to_atom' if you are ever doing dynamic things with atoms. So, if you're trying to accomplish dynamic module lookup and foo_baz.beam is in your code path, default startup will create the `foo_baz' atom, and you know that this atom will exist at runtime. Symbols in Ruby are atoms (the term "atom" spans languages), and GC/space issues plague any persistent term like an atom, in any language. And thus I arrive at a key question: Does Ruby have something like `list_to_existing_atom', or some mechanism for telling if a symbol exists already? I see no analog to this, only the `ID2SYM' macro in the extensions API, and similar calls like String#to_sym. Perhaps there is some way to clean up symbols after they are created. This to me would seem like the ideal route. It's good they've got a stop-gap fix by changing defaults, but it feels to me like they're punting here. Perhaps users who do [ab]use this feature also would not like DOS attacks? I hope others who know more about Ruby extension development, and symbol management capabilities, can chime in on these questions.
- benmmurphy 14y ago
  there are methods in the ruby reflection api that take strings and silently convert them to symbols so it is really easy to accidentally leak memory. however, on ruby trunk they added a method: rb_check_id which can be used to check if a string has been already symbolized (https://github.com/ruby/ruby/blob/trunk/parse.y#L10465 https://github.com/ruby/ruby/blob/trunk/parse.y#L10465). this means when these reflection methods get passed a string and it hasn't been symbolized they can bail out and not symbolize the string. (https://github.com/ruby/ruby/blob/trunk/object.c#L2073 https://github.com/ruby/ruby/blob/trunk/object.c#L2073)

benmmurphy 14y ago

also if you have done require 'json/add/rails' you are in for fun (https://github.com/ruby/ruby/blob/v1_9_2_381/ext/json/lib/json/add/rails.rb#L10 https://github.com/ruby/ruby/blob/v1_9_2_381/ext/json/lib/js...) irb(main):001:0> require 'json/add/rails' => true irb(main):002:0> class Foo irb(main):003:1> end => nil irb(main):004:0> Foo.json_create({"x" => "bar"}) => #<Foo:0x007fc5f3149540> https://github.com/search?q=require+%27json%2Fadd%2Frails%27&type=Code&ref=searchresults https://github.com/search?q=require+%27json%2Fadd%2Frails%27...