
Rails SQL injection vulnerability: here are the facts - blacktulip
http://blog.phusion.nl/2013/01/03/rails-sql-injection-vulnerability-hold-your-horses-here-are-the-facts/#.UOV2h_j3Giw
======
tptacek
This post is written very confidently. I am less confident than the author of
this post that the likelihood of a generic exploit via application input (that
is, not using forged cookies) is remote.

Wish there was more I could say right now. I'm not saying I have a curl
command that exploits the vulnerability. I'd just be careful about making
assumptions about this bug.

~~~
PommeDeTerre
This smug attitude is something we've seen a lot from the Ruby, and especially
the Rails, community, and it always burns them.

They had this attitude when it came to the maintainability of Ruby apps.
They'd say that Ruby code was much more maintainable than Java code, for
instance. Now that we've got some Ruby apps that are several years old, and
that have been worked on by a number of different people, it has become quite
apparent that Ruby code is much less maintainable, over the long term, than
Java code is.

This same attitude was shown when it came to Ruby's performance. Many of us
had serious misgivings about just how poorly it performed, and the suitability
of Ruby on Rails for anything beyond simple sites. Then when some Ruby on
Rails sites started facing moderate traffic, they basically collapsed. The
answer from the Rails advocates was to "throw more hardware at it".

We even saw this same attitude when it came to this very issue. We'd heard
about how Ruby on Rails is far more "secure" than alternatives. Yet that's
proven not to be the case.

When Rubyists make claims, it's best to doubt what they're saying, and to
question every single detail about it. They have given themselves a bad
reputation for making incorrect statements.

~~~
petercooper
Maybe. If so, it's only slightly more bearable than the attitude of many
observers towards what or how they think "the Ruby community" collectively
believe, do, or act.

There's no singular Ruby community anymore than there's a C, PHP or "Linux
community" whose behavior can be collectively judged.

~~~
PommeDeTerre
No, I disagree. The Ruby community is, generally, far more cohesive and
homogeneous than the others you listed. This may partly be due to it being a
relatively young community, compared to some others.

Anyone who has been to a Ruby conference, especially while not being overly
involved with the community otherwise, would likely know what I'm talking
about, for example.

Almost the entire community is male. There are very, very, very few females
involved. While other communities have an imbalance, it is nowhere near as
lopsided as it is within the Ruby community.

Another common trait is the use of Apple hardware. It's rare to see anything
but Apple laptops or other devices being used by those within the Ruby
community. I've been at talks where there are rows of 20 people, and over 15
of them are using a MacBook of some sort.

There's very little true dissent within the community. The emphasis on
"convention over configuration" ends up chiseling those conventions into
stone, and nobody dares question them, even when they're obviously wrong.

While I'm not saying every single member of the Ruby community is exactly like
every other, there is a commonality that is not found in any other computing
community. It's undeniable.

~~~
bithive123
I think you're conflating "the Ruby community" with "the kinds of people you
see at RailsConf". Homogenous, maybe. Cohesive? Hardly.

I work with three other Ruby programmers, two of whom are female, and older
than me (I'm 30). Your assertion that there are "few females involved" might
as well be characterization of IT in general for all the evidence you present.
In my experience diversity is a reflection of institutional values, not the
culture of the programming languages used.

Apple hardware is tremendously popular with web developers in general, and
while I don't blame you for your impression (I've been to RailsConf), there
are plenty of Rubyists who prefer Linux on a Thinkpad. I bet you could find
rich veins of Apple hardware at almost any type of conference.

There is plenty of disagreement about the best way to do things; that's why we
have both Rails and Sinatra (both of which have been imitated in Python,
node.js, and more) several implementations, and lots of discussions about new
language features (like refinements).

I suspect your confirmation bias means you don't even notice Rubyists who
don't fit your preconceptions.

------
phillc
If you are scanning your code base for find_by_ _'s, make sure to look at your
find_all_by__ 's as well if you have them.

~~~
awj
...and while you're at it, rewrite those into some other form. Everything but
the find_by_* dynamic finders is going to be deprecated soon anyways.

[http://edgeguides.rubyonrails.org/4_0_release_notes.html#ext...](http://edgeguides.rubyonrails.org/4_0_release_notes.html#extraction-
of-features-to-gems)

------
jrochkind1
thanks for this, phusion.

I have no idea why THIS vulnerability is getting so much attention. There have
actually been OTHER Raisl vulnerabilities in the past 6-8 months which were
_more dangerous_, but did not really get attention.

The Rails team did NOT help by being very vague about the nature of the
problem in their announcement. I imagine they were trying to not reveal the
method of exploitation; but it has just led to the current hysteria instead.
They would have been better off being taking the extra time to be fully
transparent about the nature of the vulnerability -- developers need to know
to assess their own risk, as well as to judge the quality of Rails (how stupid
was the problem exactly, what does it say about Rails etc?), etc. By being
vague about it, Rails core team has just led to everyone assuming the worst,
and the current weird hysteria.

Yes, it's a vulnerability which COULD be dangerous, and it's hard to tell FOR
SURE if your app is vulnerable (perhaps due to code in gem dependencies), the
only safe thing to do is update to a patched version. But the chances that
your app is vulnerable are pretty small (if you aren't using AuthLogic; if you
are, you can update auth_logic and fix it whether or not you update rails).
And past recent vulnerabilities which were actually MORE dangerous have not
received this level of attention.

~~~
trustfundbaby
> I have no idea why THIS vulnerability is getting so much attention

Its getting attention because its the third SQL vulnerability in 7 months. It
just feels like Rails is mature enough at this point that it shouldn't have to
be going through this now

~~~
jrochkind1
That's potentially legit (people could argue about whether these rails
vulnerabilities represent something to worry about in Rails, or whether even
mature robust projects will still have bugs, including security bugs).

But the fact remains that THIS vulnerability isn't NEARLY as dangerous as some
of those OTHER ones you mention in the last 7 months, but those other ones
people mostly ignored, and THIS one they're going crazy thinking they need to
fix right away.

What I think about the general question? Most (all?) of those Rails SQL
injection bugs actually are related to a similar underlying design: Attempt to
create methods with 'variable signatures', where you can give it a string OR a
hash, or a list of various strings and hashes, and all of those things mean
different things.

I think all of the Rails SQL injection bugs are actually related to variable
arguments like that. When those variable argument methods were designed in
Rails, it's probably safe to say nobody realized there were security
implications, that it opens you up to a whole class of bugs where someone puts
a hash where you expect a string and it change the semantics of the method
call because your variable argument interpreting logic had some flaws. In
retrospect, it's possibly not a great thing to do.

But Rails is not the only offender here, it's a pretty common design pattern
in lots of ruby -- I think it's probably a mistaken one, but it is one that
developers tend to like the convenience of.

~~~
tptacek
I wish you'd stop telling people how dangerous you're sure this vulnerability
is. I think you're probably wrong, but, more importantly, I think the only
reason you're saying it is that you're echoing a meme started in an article by
someone who isn't really involved in the research behind the vulnerability.

~~~
jrochkind1
And of course, the truth of what you are saying is now evident with the
vulnerabilty that 3.2.11 fixes.

What I still think this shows is that trying to keep information on the nature
of the vulnerability to yourself is not a great idea. People who figured
"Well, there's no way for params[:id] to be a hash with a symbol key" (such as
myself) were wrong about the severity of the vulnerability.

It would have increased everyone's security to admit "Oh yeah, there might be
a way to make params[:id] be a hash with symbol key even though you don't
think so, yes you should worry about it."

Why not do that? Becuase you're worried you're giving someone exploitation
hints, probably. But anyone that wanted to exploit had all the hints they need
anyway -- as evidenced by the half dozen people credited with reporting the
new vulnerability, all of whom got the hint from the 3.2.10 announcement
anyway.

------
dsl
The author wastes a lot of breath refuting claims that this is a huge Rails
bug, then finally comes clean 3/4 of the way in:

    
    
       Other exploitable scenarios
    
       Your code is vulnerable if you call Foo.find_by_whatever(bar), where bar can be
       an arbitrary user-specified hash with symbol keys.
    
    

This is a fundamental flaw that requires a very specific set of circumstances
(which the Rails community is clinging to as a get out of jail free card),
similar to the Python Pickle boondoggle [1], which will result in lots of
application specific vulnerabilities down the road.

1\. <http://nadiana.com/python-pickle-insecure>

~~~
tptacek
Python's Pickle is insecure by design. It causes application-specific
vulnerabilities the way bad crypto libraries do, by giving developers a tool
to blow their feet off with. It's hard to blame Python too much for that;
languages are going to provide unsafe libraries.

This, on the other hand, is just a Rails bug. It has a simple fix. That fix is
provided transparently by Rails. It isn't going to cause "lots of application
specific vulnerabilities" because nobody is going to care about it 6 months
from now, except as yet another reason to keep Rails at the most recent
version. This is a problem no different from that faced by people on J2EE
stacks.

So I disagree with both of your points.

~~~
dsl
Note that I didn't refer to the Pickle issue as a bug. I think its quite a
useful feature, but it's still a pointy stick that many have fallen on.

~~~
freshhawk
You referred to it as a "boondoggle" which, if it wasn't even a bug, it also
was not.

------
mef
This bug is pretty edge, but if there was an easy way for a user to put a
symbol in the params hash the bug would be a pretty gnarly universal SQL
injection vulnerability for all versions of Rails.

Though if that was easy it would most likely have been caught much, much
earlier as there are a multitude of find helpers that allow literal SQL to be
injected.

~~~
FooBarWidget
Early versions of Rails explicitly made the decision to never symbolize user
inputted hashes because symbols are never garbage collected. This would allow
memory to grow unbounded. As a result Rails stores all user hash keys as
strings.

~~~
phillc
Thank you... I always wondered why that was true.

------
Jgrubb
I swear I saw this same bug discussed on Reddit from some security mailing
list last week, unless this is another SQL injection vulnerability with
Authlogic.

~~~
FooBarWidget
You talking about this? <https://github.com/binarylogic/authlogic/pull/341>
It's the same thing.

------
bhauer
If I understand this correctly, given that a private key is needed to
successfully modify the signed HTTP cookie, for a closed-source project using
Ruby, Rails, and Authlogic, this vulnerability might be better characterized
as an "unintentional backdoor."

~~~
numbsafari
Yep, but even if you have a closed-source project you should avoid checking in
any secret keys or other credentials to your source control system.

------
ajross
I'm starting to wonder if the DRY-uber-alles notion popularized by Rails is
turning into a security antipattern. It's just too easy for code like this
(intended to "make things work cleanly") to forget all the needed checks and
all the constraints on the design.

The end user ends up repeating themselves less, but that means that the
library code ends up getting used in lots of places and for lots of purposes
that the author didn't necessarily think through...

~~~
__david__
It also means that when a bug is found in that code that you only have to fix
it in one place, not 75,000 random places where it's duplicated, and probably
slightly differently each time, making it hard to search for.

~~~
ajross
That seems to be belied by this very case, no? Rails had two ways of doing
something, one of which forgot where the input could come from...

~~~
__david__
Hmm... How would copy-pasta code have prevented this?

~~~
ajross
I fear from your language that you aren't really interested in reasonable
argument, but I'll try anyway: ActiveRecord had two variant find() interfaces,
one of which tried to make it "easy" on the programmer by avoiding the need to
extract an ID to do the lookup. And it went further by adding an overloaded
argument syntax that allowed you to elide the first argument entirely. _And it
forgot that that second argument simultaneously (1) was defined by a remote
client and (2) could contain arbitrary SQL_.

So yes: "copy pasta" of the id boilerplate around an AR find() call would
absolutely have prevented this. Rails got slick, and got burned. DRY helped
reduce "copy pasta" (sigh) but hurt security.

~~~
__david__
Isn't that really the fault of an overloaded (as in doing too much) function
API rather than a DRY related problem?

~~~
ajross
Well, yes, but my point was that I see them as part of the same thing. A
function (or whatever) that does lots of related things can be "DRY" if it
avoids writing out all the related things longhand, or it can be "too much" if
it introduces bugs.

DRY as a general philosophy to avoid cut-and-paste code is fine. But it's also
an ethic that in my experience prioritizes concision at the expense of
clarity. Loss of clarity is a factor in half the security bugs on the
internet.

------
FooBarWidget
After giving many of the comments here a thought, I've written a follow-up
article "Securing the Rails session secret" in which different ways to secure
the session secret are considered. Feedback is more than welcome.
[http://blog.phusion.nl/2013/01/04/securing-the-rails-
session...](http://blog.phusion.nl/2013/01/04/securing-the-rails-session-
secret/)

------
benmmurphy
the other thing to note is that if your application is deserializing untrusted
input using Marshall.load and calling methods on the deserialized objects then
there is a chance that you are vulnerable to arbitrary ruby code execution. in
that case an attacker could execute whatever sql he wanted to anyway.

------
macca321
I wish ruby had named the Hash class properly. It makes reading the article
slightly confusing. Hash != Hashtable

~~~
tabbyjabby
Of course the Hash class in Ruby implements a hash table. What makes you think
it doesn't?

~~~
mike
I believe that macca321 is wishing that, to avoid confusion, the term "hash"
was reserved for referring to an individual hash code and that "hash table"
was used for the data structure and the Ruby class that implements it.

------
digitalboss
Excellent post - clear and with example.

But - I'm confused why one of the readers on the post (Jonas) is calling this
a PR Stunt?

