
SQL Injection Vulnerability in Ruby on Rails; affects all versions - danso
https://groups.google.com/forum/?fromgroups=#!topic/rubyonrails-security/DCNTNp_qjFM
======
venus
The overreaction on this page is ridiculous. Has anyone actually read the
steps required to exploit this vulnerability?

You do know that to be able to exploit it you have to know the application's
secret key, so you can create your own malicious encrypted session cookie that
includes hashes instead of strings for the auth token lookup?

You do know that if someone has your app's secret key they can just write
whatever they want into the session cookie, instantly compromising a large
number of apps anyway? That's the whole point of the secret key!

This is an obscure issue which can be used to get around one layer of defense
in rails security. It could never get around all of them. It requires intimate
in-depth knowledge of the app to even attempt the exploit. Sure, it's a bug,
and it's not to be taken lightly, but the howls of derision here are totally
out of proportion.

~~~
jtchang
It is not an overreaction.

This is a much more subtle SQL injection.

I believe the takeaway is that too much magic is a bad thing when it obscures
the underlying behavior.

Post.find_by_id( ) accepts an argument. Here are some normal assumptions:

1\. It might only take a number

2\. The method might coerce it to a string or integer for you

3\. The method might not coerce it.

4\. The method might throw an error if it isn't an int or the object isn't
found.

5\. The parameter is treated like a hash and used for lookups.

This last one seems a bit too much magic to me. I wouldn't even guess that
last one as normal, expected behavior.

~~~
gbelote
But that is an overreaction. The find_by_* dynamic handlers have always
accepted strings and integers, by design. What's insecure about
User.find_by_id("27")?

What people seem to be reacting to is the idea that
User.find_by_id(params[:id]) is exploitable because you can coerce params[:id]
into a hash instead of a string (by using a query string like
?id[select]=some_thing_here instead of ?id=27). True, but what most people
here are overlooking is that User.find_by_id actually rejects these hashes
because their keys are strings and not symbols. Try it out.

This vulnerability can be coupled with other vulnerabilities (like having
someone's session secret, which is a much worse vulnerability IMO), but people
are talking about it as if you can do something nasty with it by itself alone.
That's why it's an overreaction.

------
danso
FWIW, this is the third time in seven months that Rails had to issue a patch
related to how ActiveRecord handles method parameters.

3.2.6 (June 2012)
[https://groups.google.com/forum/?fromgroups=#!topic/rubyonra...](https://groups.google.com/forum/?fromgroups=#!topic/rubyonrails-
security/l4L0TEVAz1k)

3.2.4 (May 2012)
[https://groups.google.com/forum/?fromgroups=#!topic/rubyonra...](https://groups.google.com/forum/?fromgroups=#!topic/rubyonrails-
security/dUaiOOGWL1k)

~~~
dmix
I've always found Rails to be easy to secure compared to most other
frameworks.

I think the defaults are all pretty secure.

I'd prefer having constant security patches. It shows people are still
constantly testing it for vulnerabilities.

~~~
revelation
SQL injection is the web-equivalent of having a stack allocated buffer
overflow. We have built and use (expensive) abstractions that supposedly
_eliminate_ these whole class of issues.

With Rails, you get the expensive abstraction and apparently none of the
security.

~~~
phillmv
Yes, clearly, because Rails has introduced _absolutely no other kind of
default protection_.

Is this an argument for hand rolling your own code, or for using some other
framework that is apparently immune (or, to be charitable, has a stronger
security track record)?

~~~
srdev
Likely its an argument to use parameterized queries which fix the SQL
injection problem altogether. I'm not familiar with Ruby, but surely they
support it in 2013?

~~~
qxcv
They do, but what we're talking about here is an ORM, so there will always be
machine generated SQL somewhere. Or do you believe that GP is suggesting that
developers use parameterised SQL queries _instead of_ an ORM?

~~~
untog
Er, why can't the ORM use paramaterised queries?

~~~
jeltz
The problem is that ORMs like ActiveRecord really are just domain specific
languages for building queries. If these DSLs use inband are carelessly
constructed (e.g. they use some form of inband signaling) you can do the
injection attack against the actual ORM code and make it build queries the
author of the code did not intend.

~~~
sontek
<http://sqlalchemy.org/> is an ORM and does not have these security issues. So
it can be done.

~~~
qxcv
Searching for "sqlalchemy sql injection" brings up this:
<https://bugzilla.redhat.com/show_bug.cgi?id=783305>

------
benlower
So does the fact that "Rails is Omakase"
([http://david.heinemeierhansson.com/2012/rails-is-
omakase.htm...](http://david.heinemeierhansson.com/2012/rails-is-
omakase.html)) mean that the chef tried to serve Fugu
(<http://en.wikipedia.org/wiki/Fugu>) but cut it wrong?

~~~
xentronium
I think this is a correct analogy, but you have to take something else into
consideration: there are no chefs that don't cut fugu wrong from time to time.
We should cut fellow programmers some slack.

~~~
threedaymonk
"there are no chefs that don't cut fugu wrong from time to time"

This doesn't seem to be the case: fugu preparation is licensed and regulated,
and deaths are very rare indeed. I can't find exact numbers offhand, but
according to a paper cited on Japanese Wikipedia, there were 315 fugu
poisoning incidents in Japan in the 10 years from 1996 to 2005. Of those, 31
were fatal, and of those, the majority were due to preparation by unqualified
individuals (presumably those who had caught fugu themselves).

Given the quantity of fugu consumed nationwide, it seems reasonable to say
that, in fact, most chefs never cut fugu wrong - or at least that most chefs
never serve wrongly cut fugu.

[https://ja.wikipedia.org/wiki/%E3%83%95%E3%82%B0#.E3.83.95.E...](https://ja.wikipedia.org/wiki/%E3%83%95%E3%82%B0#.E3.83.95.E3.82.B0.E3.81.AB.E3.82.88.E3.82.8B.E9.A3.9F.E4.B8.AD.E6.AF.92)

~~~
xentronium
That sentence referred to original post, read it as "there are no programmers
that never make bugs". I didn't mean the real chefs!

------
jtchang
I am mostly a Django programmer so excuse my ignorance of rails. How does this
keep happening?

In Django you would do:

Post.objects.get(pk=request.GET['id'])

There really is no way to do SQL injection this way.

This line in rails looks almost exactly like how you would do it in Django:

Post.find_by_id(params[:id])

Also this seems really serious. It's not like a edge case where you need to
grab a post by id. This is probably a very common use case of that method
find_by_id.

~~~
numbsafari
You are going to have problems with this whenever you are composing SQL
statement with any type of user-provided data as part of the raw SQL string
passed to the server.

This generally happens in one of two says:

1) (most common) You have a SQL statement that takes a user-provided parameter
and you compose your SQL statement as a string, including that parameter (eg.,
sql = "SELECT * FROM person where id = " + form.id, or similar). This is
typically solved by using parametric, prepared statements. Basically, you
prepare a SQL statement that contains "?" for the parameter values and then
bind values to the statement.

2) (Common in ORM frameworks) A user provided string is used to compose some
other (non-parameter) piece of the SQL statement, such as a column or table
name. This is usually caused by laziness. Rather than combining the string
provided by the user (form values, URL components, etc.) you should instead
look up the string to use from some internal data source, such as a list of
domain classes, etc., and use that instead. In that way, the data that is user
provided is kept entirely separate from data that will be executed.

You'll hear a lot of people talk about "why isn't this being escaped". And,
frankly, it's a good question. But the real question is "why are you trusting
data that could come from anywhere on earth?". Don't take it for granted that
only your users will be sending queries to your application.

The code you write for Django (Post.objects.get(pk=request.GET['id']) is only
secure from 1 and 2 if the framework is written in an appropriate manner to
avoid trusting user provided data.

The fact that this keeps happening on Rails is the #1 reason I haven't
bothered to take the time to do anything real with it. I don't have the time
to read the code for the framework and I don't trust that it's written with
security in mind.

ps. This type of problem applies to any kind of "data that is executable", be
it strings passed to an "eval" function or strings passed to a web browser.
SQL is just a giant eval() function.

~~~
charliesome
This is not the problem. Please stop spreading misinformation.

ActiveRecord _does_ escape user input.

The exploit here is that under certain obscure circumstances it is possible
trick ActiveRecord into thinking the user input is an options hash passed by
the caller.

From my understanding this is non-trivial to exploit on most applications, and
requires passing in a Hash with symbol keys.

This is still a vulnerability that needs to be (and has been) fixed, but it is
nowhere near as stupidly obvious as you are claiming.

~~~
numbsafari
Umm.. I didn't say that ActiveRecord doesn't escape user input. So, stop
spreading misinformation about my post ;-)

The fact of the matter is, whether its in some dark edge-case or not, user-
provided data is being used to compose a SQL statement that is being passed to
the server. Escaped or otherwise, that's a recipe for an injection attack.

~~~
venus
> user-provided data is being used to compose a SQL statement that is being
> passed to the server. Escaped or otherwise, that's a recipe for an injection
> attack

How do you implement authentication if you can't check an email (user provided
data) matches a password (user provided data, probably hashed but still)? How
do you look up blog posts by a user-provided tag, without using that tag in
query composition? How do you save any user provided information at all
without somehow including that information in an SQL query?

You have to use escaped user provided data _all the time_ in a real
application. Any actual web developer would know that.

~~~
numbsafari
Simple: use parametric prepared statements instead of composing a SQL string
with the user-provided data (escaped or otherwise).

This has been available in numerous database APIs for like, ever.

For example [1], [2], [3]. Any actual web developer will have read something
along the lines of [4].

A lot people seem confused by my original post, which was in response to a
Django user's question about how this sort of thing happens. I provided a
general response which seems to have offended some people.

Yes, Rails does appear to use parameterized statements. However, when it is
building those parameterized statements it's still using user provided data to
build the SQL. If that weren't the case, then this wouldn't be a bug at all,
would it? Of course not, so obviously it is using user-provided data in some
way, otherwise an HTTP cookie's value wouldn't be getting passed to the
database, would it? The prepared statement string shouldn't be composed with
anything user provided.

[1] <http://php.net/manual/en/mysqli-stmt.bind-param.php>

[2] <http://bit.ly/WlPEFR>

[3] <http://rubydoc.info/gems/mysql/2.9.0/Mysql/Stmt>

[4] <https://www.owasp.org/index.php/Top_10_2010-A1>

~~~
cdcarter
The attack is targeting a secondary method signature that #find_by_* can hold
with the express purpose of executing arbitrary SQL. That is, when #find_by_*
is invoked with a hash with a key such as :select or :conditions it expects a
SQL string, probably hard coded.

The bug however is that it's possible for user input, with a session
hijacking, to provide that hash with symbolic key. There is no SQL injection,
this is straight up arbitrary execution of SQL.

~~~
numbsafari
Out of curiosity, all other debates aside, would it not be helpful to have
(either built-in or as a separate plugin) a way for Rails to run a simple set
of sanity checks on the SQL it passes to the DB server? For example, checking
to make sure that the generated SQL doesn't contain "--" wouldn't fix the
underlying problem, but it could be used to prevent the exploit from
ultimately working (and, if someone tried this, would alert you to that fact,
assuming the error was logged).

I get that this would create some performance overhead, so it would ideally be
configurable.

------
thelarry
Im not a RoR developer, so forgive my ignorance. But how does this keep
happening? I thought ActiveRecord is a mature product by now. I hear about
these types of bugs happening with RoR pretty often, is the "magic" of
ActiveRecord and hiding SQL from the developer really worth it? I wonder how
many RoR developers even understand what SQL injection actually means.

------
oomkiller
It seems like this is being conflated with the session token issue? How do you
submit params with symbolized keys? Hashes are easy enough, but it doesn't
work with hashes that have strings as keys, only symbols.

EDIT: Just to be clear, tenderlove (Ruby/Rails committer) confirms that you do
not need to edit the session to exploit this
(<http://news.ycombinator.com/item?id=4999767>). It's still unclear how it is
possible otherwise though, I assume he's being purposefully vague.

~~~
latortuga
Rails param parsing automatically converts all param keys to symbols.

~~~
mnarayan01
Just to expand on what others have already said...Rails converts params to
[http://api.rubyonrails.org/classes/ActiveSupport/HashWithInd...](http://api.rubyonrails.org/classes/ActiveSupport/HashWithIndifferentAccess.html)
which means that `params[:foo]` and `params["foo"]` will both return the same
thing. The function
[`assert_valid_keys`]([http://api.rubyonrails.org/classes/Hash.html#method-i-
assert...](http://api.rubyonrails.org/classes/Hash.html#method-i-
assert_valid_keys)), which is called in
[`apply_finder_options`]([http://api.rubyonrails.org/classes/ActiveRecord/SpawnMethods...](http://api.rubyonrails.org/classes/ActiveRecord/SpawnMethods.html#method-
i-apply_finder_options)) does not, however, treat symbol and string keys as
the same, even if given a `HashWithIndifferentAccess`.

~~~
xentronium
To expand on your expansion: when a hash with indifferent access is asserted
over a list of symbols (as is the keys) it will _always fail_.

    
    
        1.9.3p327 :025 > {:a => "b"}.with_indifferent_access.assert_valid_keys([:a])
        ArgumentError: Unknown key: a

------
michaelfairley
This affects all versions of Rails, not just 3.x.

This is a really, really bad hole, and you should patch or upgrade ASAP.

~~~
danso
Thanks, fixed the headline. All versions are affected, only 3.1, 3.2 have
official support

------
clamstar
You need the contents of secret_token.rb to exploit this (via a forged
session). This makes it much more of a danger to OSS projects than to those in
the closed source space.

It's not _just_ a SQL Injection vulnerability. With that secret token, you can
set any session value you like.

~~~
jroes
This is how I understand the issue as well. Many people in this thread are
commenting about massive dangers, but I don't think anyone has bothered to
actually read the references in the CVE.

Also, even open source projects typically ensure or recommend that the secret
token be regenerated when using in production environments.

~~~
nbpoole
I think the CVE description is inaccurate in this case. Check out the Rails
security email list description, which never mentions sessions.

[https://groups.google.com/forum/?fromgroups=#!topic/rubyonra...](https://groups.google.com/forum/?fromgroups=#!topic/rubyonrails-
security/DCNTNp_qjFM)

------
alberth
The irony of this security vulnerability being announced by 37signals on
Google Groups on the same day they release a new product called Basecamp
Breeze [1] which is a direct competitor to Google Groups (and they compare it
to Google Groups) ... hasn't gone unnoticed.

[1] <http://basecamp.com/breeze/compare#google>

------
eranation
Would appreciate if someone could explain the issue in a little more detail
for non RoR developers

~~~
xentronium
Assuming you have no ruby knowledge:

ActiveRecord (rails' default ORM) has a feature called "dynamic finders". When
you call a method like `Forum.find_by_url('news.ycombinator.org')` it gets a
first forum with such url. This is a sugar over `Forum.where(:url =>
'news.ycombinator.org').first`.

Normally, you use it like that `Forum.find_by_url(params[:url])` where
`params` is a hash of parameters (that is auto-generated from http get/post
params). What happens if instead of "normal" value like "news.ycombinator.org"
you pass a hash?

    
    
        1.9.3p327 :018 > Forum.find_by_id({:select => 'id FROM forums --'})
        Forum Load (0.5ms)  SELECT id FROM forums -- FROM `forums` WHERE `forums`.`id` IS NULL LIMIT 1
    

Uh-oh.

However, not everything is lost. Rails params are converted to a special hash
class. It's called HashWithIndifferentAccess, because you can access its
values by string keys and symbol keys likewise. So that

    
    
        h = {:a => 1, "b" => 2}.with_indifferent_access
        h["a"] # => 1
        h[:a]  # => 1
        h["b"] # => 2
        h[:b]  # => 2
    

What happens if we pass user-generated params? It seems like not much:

    
    
        1.9.3p327 :024 > Forum.find_by_id({:select => 'id FROM forums --'}.with_indifferent_access)
        ArgumentError: Unknown key: select
    

So I guess they are erring on a safe side here.

~~~
MichaelGG
What's the great benefit of these methods? Why not just do find_by("url", x)?
I just don't understand the desire to move string parameters into member
names.

~~~
xentronium
There is slight benefit in readability for cases like
find_by_name_and_role("Jack", "admin"), but other than that, no benefit
whatsoever.

In Rails 4.0, most dynamic finders are deprecated and removed from the source
into a separate gem.

------
FooBarWidget
I've written a blog post which explains what this vulnerability is, how it
works, what the facts and non-facts are:
[http://blog.phusion.nl/2013/01/03/rails-sql-injection-
vulner...](http://blog.phusion.nl/2013/01/03/rails-sql-injection-
vulnerability-hold-your-horses-here-are-the-facts/)

~~~
timblair
Thanks for that: really clears things up.

It would have been really useful if the upgrade notification had included this
level of detail to start with so people could make a much more informed
decision about when/if to upgrade, rather than having to dive through bug
reports, commits etc just to work out if our apps are vulnerable.

------
zacksjoden
For someone just learning RoR and having installed it via
<http://railsinstaller.org/>, how should I upgrade?

~~~
cheald
Run "bundle update rails" in your project's root directory (where your Gemfile
is).

~~~
bonzoesc
This won't work if your Gemfile declares a version of Rails (Rails 3.2.9 did
this). You have to update the Gemfile and then bundle update.

------
FuzzyDunlop
It seems easy to blame the magic finders for this (because they seem magic),
or ActiveRecord alone, but really the problem is that 'secure by default' can
trick you into thinking the framework will do it all for you.

It probably will most, if not all, of the time for you. But the complexity of
such things understandably means there will be obscure vulnerabilities that
are hard to track down.

Sanitising -- and even validating -- your params at the controller level is a
nice way to stop some of these problems before they reach your models.

------
webjprgm
This isn't a problem if you check the type of data you get from a user before
you use it. You should be doing that anyway.

The article says a work-around is to use .to_s or .to_i on user input. My
standard practice is to cast to int for all IDs I get externally (in PHP too).
Some projects I work on go so far as to validate type and data ranges of every
argument passed in. Those applications would therefore not be vulnerable even
though Rails itself is.

~~~
colinsidoti
"You should be doing that anyway."

A huge selling point of Rails, other web frameworks, and ORMs in general, is
that you don't have to be doing these checks. You can push the query through
without concern of SQL injection, and handle the failure case there. This is
convenient because you often have to handle that failure case anyway.

Rails' own "Getting Started" guide uses this technique because it's assumed to
be secure:
[http://guides.rubyonrails.org/getting_started.html#showing-a...](http://guides.rubyonrails.org/getting_started.html#showing-
an-individual-post)

We know you can "break" the application by tampering with IDs, but you
shouldn't be able to pose a security threat.

Validating type and data ranges is common with things like dropdowns, where
you need to ensure an actual option was selected. This is a very common
security flaw, and it's definitely good practice to be checking (especially
when it leads to adding/updating database rows).

~~~
Yahivin
The getting started guide does not use that technique. It uses
`Post.find(parms[:id])` The vulnerability is in `Post.find_by_id(params[:id])`
and any other `find_by_*` dynamic finders.

------
clickonchris
Having just finished upgrading my production rails because of today's news I
thought I would pass along a how-to for rails 3.x

update your Gemfile and set the version you want. In my case:

gem 'rails', '3.2.10'

locally, run

'bundle update rails' which will update your Gemfile.lock

check-in and deploy your code. If you are using capistranso, the default
'deploy' task should handle everything for you. Otherwise, run 'bundle update
rails' on your production server.

------
simon_kun
Does anyone know of a patched version of rails 2.3 series on rubygems?

~~~
rob-anderson
No but you could do what we did:
[http://robanderson123.wordpress.com/2013/01/05/applying-
back...](http://robanderson123.wordpress.com/2013/01/05/applying-backported-
security-patches-to-rails-2-3/)

------
mpchlets
I believe that its a test of how quickly you can patch your applications when
something like this does come out - We did it in 3 hours from time of bulletin
- how about you?
[http://blog.assembla.com/assemblablog/tabid/12618/bid/93731/...](http://blog.assembla.com/assemblablog/tabid/12618/bid/93731/Continuous-
Deployment-is-Secure-Howto-Patch-3rd-Party-Apps-Uber-Fast.aspx)

------
NoPiece
So it only the only the dynamic finders (find_by_) that have the
vulnerability, not Model.find(params[:id])?

~~~
colinyoung
Yes, it's the dynamic matchers only.

------
gnuvince
The more I read these stories (and not just in Rails, though they seem to be
the most talked about), the more I seriously consider taking the time to
evaluate frameworks such as Yesod where the type system makes these kinds of
security flaws impossible.

------
dholowiski
Oh. I always figured it was bad coding, but I normally use the workaround
anyway. Post.find_by_id(params[:id].to_s)

Regardless, I think I have some patching to do tonight.

------
hayksaakian
Does something similar affect mongodb or am I OK?

~~~
danso
This is an ActiveRecord issue...MongoDB uses its own ORM (Mongoid/Mongomapper)

~~~
hayksaakian
That's what I was referring to. Do mongomapper and mongoid have this problem?

~~~
amorphid
On the bright side you need not fear this bug for your apps using MongoDB.
Mongoid and MongoMapper do not use SQL, so there should be no SQL injection
problems using those two options.

~~~
hayksaakian
I'm aware they don't use SQL, but someone on HN once told me there was some
kind of JavaScript injection you could do in theory. Was wondering how mongoid
would handle this technique.

~~~
nbpoole
Example: [http://www.idontplaydarts.com/2010/07/mongodb-is-
vulnerable-...](http://www.idontplaydarts.com/2010/07/mongodb-is-vulnerable-
to-sql-injection-in-php-at-least/)

------
mkilling
Does this affect friendly_id?

edit: friendly_id does not use the dynamic find_by_x methods. I guess it
should be safe.

~~~
bonzoesc
This particular exploit doesn't either.

------
mikec3k
Just wait until you find someone named Robert'); DROP TABLE STUDENTS; --

------
martinced
One could lament that the real issue is the (maybe not so) accidental
complexity (and hidden deep down complexity lies insecurity) that results when
you're using utter non-sense like an ORM.

The very concept of an ORM is broken. I know most devs don't know SQL well
enough to do more code directly from SQL and I know most devs don't know
anything else than SQL... But it's a bit sad to see all these "frameworks"
tailored to the masses.

dev: "I want objects. I don't really understand set theory but I still want
SQL because it's all I know."

chef: OK, here's a framework doing nearly everything from you.

Seriously guys. Mixing orthogonal concepts and posting provocatively titled
blog entries like "OO and SQL aren't orthogonal concepts, there's no mismatch
impedance" ain't going to help.

The only thing this creates is a lot of needless complexity and, of course,
there shall always be major SNAFU like this one.

Maybe, just maybe, that we could realize that if the reader is set to "from
now on do not eval anymore" code shall not be eval'ed? And maybe, just maybe,
that we could realize that if you're not using SQL you're not subject to these
monthly SQL injections issues?

How far does the SNAFU need to go for you to consider that OO + SQL is
actually a very, very poor mix?

