SQL Injection Vulnerability in Ruby on Rails; affects all versions

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.

The howls of derision from people who flat out say they don't even use the framework and clearly don't understand the vulnerability are particularly ridiculous.

From looking at it, it seems that ANY HTTP parameter (e.g. POST or GET - which are completely user-controlled) can be manipulated if you know how it's going to be used in the code, e.g. an obvious object ID.

EDIT: tenderlove sets it straight below

  params[:id] = {:select => "select * from users where admin = 1 limit 1; --"}
  User.find_by_id(params[:id]) # => finds the first admin
  # generated SQL:
  # SELECT * from users where admin = 1 limit 1; -- select * FROM `users` WHERE (`users`.`id` IS NULL) LIMIT 1

-- edit: maybe nvm on that, it requires an actual symbol, not a string key with a hash with indifferent access, like you'd get from JSON. Unless someone knows a way to get a symbol into a JSON or form-encoded field? Though you're vulnerable to this specifically if you use '.symbolize_keys' anywhere before it's passed in.

-- edit2: and I see I'm late to the game anyway, thanks tenderlove :)

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.

So when someone offers to do something inherently dangerous on your behalf, you should be incredibly deliberate.

There's no shortage of frameworks, platforms and libraries that have been bit (repeatedly) by SQL injection.

    Post.find(params[:id])

The methods that are affected by this are the dynamic finder methods `find_by_*` such as:

    Post.find_by_id(params[:id])

    User.find_by_token(params[:token])

What the hell is going on with ActiveRecord?

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.

The original post of the problem goes like this:

1. Gain an application's secret key, used to sign session cookies. 2. Inject a marshalled hash with _symbol_ keys into the session cookie, sign it with the secret key. 3. Now you can exploit the SQL vulnerability in the dynamic finders, assuming the session value is used directly as input.

    +  def test_find_by_id_with_hash
    +    assert_raises(ActiveRecord::StatementInvalid) do
    +      Post.find_by_id(:limit => 1)
    +    end
    +  end
    +
    +  def test_find_by_title_and_id_with_hash
    +    assert_raises(ActiveRecord::StatementInvalid) do
    +      Post.find_by_title_and_id('foo', :limit => 1)
    +    end
    +  end
    +

Example (real rails app):

    1.9.3p327 :017 > Forum::Thread.find_by_id_and_forum_id(1, {:limit => 10}.with_indifferent_access)
    ArgumentError: Unknown key: limit
        [backtrace skipped]

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

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

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

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

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.

If you're just learning and creating an app for your own edification, this is not really an issue that will affect you. That is, it doesn't affect how you construct the app, so if for some reason the gem update process doesn't work, you won't be hindered from using RoR.

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...

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).

    id=1
    User.where(:id => params[:id]).first
    User.where(:id => 1).first

    id[$gt]=0

    User.where(:id => params[:id]).first
    User.where(:id => {"$gt" => 0}).first

You should be performing casts (usually to strings) before you pass your data to your ODM.

    id[$gt]=0
    User.where(:id => params[:id].to_s).first
    User.where(:id => "{:$gt=>0}").first