Hacker News Reader

Hacker News

Comment by jtchang | Hacker News Reader

Parent Full thread

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.

View on HN

The standard way to find a post in Rails would be:

    Post.find(params[:id])

That method is unaffected.

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

    Post.find_by_id(params[:id])

This would most commonly occur when looking up users by a token or some other piece of data other than the id.

    User.find_by_token(params[:token])

I'm not sure why they chose to use find_by_id in the example. This is a serious bug, but it's not as serious as one might be lead to believe if one thought it was the standard way to find objects in Rails.

I believe if you do Post.find on a nonexistent id, a ActiveRecord::RecordNotFound exception is thrown, but this doesn't happen with the dynamically generated finders like find_by_id. Given this difference, it's possible you want to handle a missing record in a more graceful, non exception catching matter.

Personally I would do Post.where(id: params[:id]).first since I tend not to like dynamic finders anyway.

True, but :find_by_id is closer to :where in that it doesn't raise exceptions upon not finding a record. I've seen it being used in a few places, especially in front another scope, like `current_user.posts.find_by_id`.

If you have an invalid id coming in, wouldn't you generally want an Exception? What is the use case where the app would send an id that does not exist, but you would not want to fail?

nonexistent does not necessarily imply invalid. I don't use rails, so take my ignorance into account, but I'd hate to catch exceptions all the time instead of just checking for falsey values.

Ex: "Enter your student ID"

    s = Student.find_by_id(params[:id])
    if s
        # do stuff
    else
        # do other stuff
    end

    begin
        s = Student.find_by_id(params[:id])
        # do stuff
    rescue
        # do other stuff
    end

This is a good example, however I would argue that it is bad design to have a record's ID used for user facing lookups. For other parameters, sure (find_by_name, etc) a false would be okay. Just a matter of taste really.

No argument here, good point.

If you want an exception, there is a banged version:

    # note the exclamation mark
    User.find_by_id!(id) # => raises exception when nothing is found

While the example given here is :find_by_id, the reality of the isse is that it affects the dynamic finder methods. This means that it affects the :find_by_any_column_in_the_table methods. The only reasonable situation in which to throw an error in those methods is when you don't hit a column, not when you don't find a record, IMO.

I agree on this point

Rails does escape inputs with its finder and scope methods. I think the problem is that the "magic" in these methods allow for some edge cases to be parsed in unexpected ways...for example, when params[:id] contains a nested hash instead of a string or integer.

This pull request is probably what caused the alarm: https://github.com/binarylogic/authlogic/pull/341

With 3395 stars it seems to be a quite popular.

Deleted comment

I'm sorry but this is braindead. I'm sure there are valid use cases but optimize for the most common one: find_by_id accepts a single argument, the ID of the object you want to find.

That is why Python has kwargs. Those two stars stand out like a sore thumb and when you are passing positional arguments in the form of a hash it is pretty apparent.

Ruby does not yet have kwargs, but the upcoming Ruby 2.0 has a form of them.

that would be called .find(id) - accepts a single argument of an id. This is the canonical way to retrieve by id.

find_by_id, find_by_name, etc. aren't really methods, they trigger calls to method_missing which interprets the code to generate SQL. It's a "neat" feature, but one which I've only used once or twice in 6 years of Rails development.

Btw, this applies for my (Python) Flask apps using MongoDB ORMs. They escape the inputs.

What the hell is going on with ActiveRecord?

The main issue stems from Ruby using the last positional parameter to pass a hash representing keyword arguments.

This means if there's only one parameter, and someone can sneak a Hash in there where you weren't expecting it (params parsing, request body parsing, etc) then they can end up passing dodgy 'keyword arguments' into your method call.

This is not a rubyism, it is a Railsism, they wrote a method called extract_options! and use it everywhere to get this kind of behavior. This is not how vanilla ruby works.

If it wasn't a rubyism, why is there syntax sugar for passing a hash as the last positional parameter?

the sugar exists for passing it, but not for receiving it, and to get around this, rails uses splat (varargs) and extract_options! to get the last positional parameter behavior for vararg methods.

vanilla ruby:

  def find_by_name(name, options = {})
  ...
  end

rails:

  def find_by_name(*args)
    opts = args.extract_options!
    name = args.shift
    ...
  end

(note, this is not how the dynamic finders actually work, i just wanted to illustrate the difference in the calling and definition.)

So, ruby doesn't include support for varargs with last positional parameter for options, rails builds that in. The fact that it is variable arity is very important -- in fact, that is the root of the present issue. The patches now check the number of arguments.

In Python, the parameter escaping is done at the level of the database driver not the ORM. Isn't this the case with Ruby?

Of course, you could use the driver incorrectly to risk SQL injection, but that is a very obvious mistake that no experienced developer would make.

the ORM escapes the parameter but it lets you specify bits of SQL by hand (think "select foo, myfunc(bar) as BAZ, joineds.quux as quux").

In theory when you do that you have already given up on letting the framework handle it for you, and you must take care of not feeding raw user input as the select code, for example.

The issue here is that the option to do this is exposed in a functionality where people do not expect it (dynamic finders) and thus people may be passing risky input there.

Well, the Django ORM also allows you to write SQL by hand and if you make a mistake you can fall pray to SQL injection, so I'm assuming that there's something different about this exploit. From what I understand the current issue appears because the person who implemented the faulty method uses SQL directly and doesn't pass the parameters separately.

In Python, you would do something like this:

    execute('select name, age from employees where id=?', (params['id'],))

This passes the id as the second argument to the execute function. If you do this, on the other hand, you open yourself to SQL injection, because %s is replaced with params[id] and no escaping is done:

    execute('select name, age from employees where id=%s' % params['id'])

the thing that is different is that the raw sql facilities are made explicitly available not through the common methods that expect them, but through something else.

The orm supports building the sql piecemeal, e.g

     find(select: "name, foo(bar) as baz",    
          conditions: 'x=y',
          limit: 3)

this is a small step above a raw execute, and obviously ugly and low level. Also a somewhat obsolete practice, since for a few years you could write it as a composition of calls

        select("name, foo(bar) as baz").
          where('x=y').
          limit(3)

Anyway the functionality is there to compose SQL via bits using an hash of parameters, moving on.

Now remember that ruby <2.0 does not support keyword arguments, so the common practice is to use one normal argument with an hash value wich contains the keyword args.

Rails has this (antipattern imo) of accepting arguments in a dozen way for some methods e.g.

    find(1)
    find(:first)
    find([1])
    find(1,limit: 1)

let us not argue whether this is good, it's there.

And AR has dynamically generated finders (which Django does not have AFAIR).

One would expect the dynamically generated finder to be doing

     def find_by_foo  arg
        where(foo, arg).limit(1)
     end

but in reality it does

     def find_by_foo *args
       many_options = args.extract_options!
       opts = combine_with_foo_handling(many_options)
       find(opts)
     end

and here you get the problem that you may be unknowingly passing an hash object wich builds sql piecemeal.

Notice that, as others already pointed out, usually as a user you shouldn't be able to create custom objects of the kind that exploits this issue (an hash with symbols as keys) unless your have other vulnerabilities already.

Just so you know, Django has a sort of dynamic finder implemented with kwargs in the lookup

    Post.objects.filter(some_field_name=some_value)

I know that, but the equivalent AR is `Post.where(somefield: somevalue)`, dynamic finders are another thing.

With no obvious value over the former that I may think of anyway, I think they are mostly there for historical reasons.

Can't see how it's different from:

    (in a manager)

    def find_by_foo(self, arg):
      return self.filter(foo=arg)[:1]

please reread what I or others wrote, cause I don't understand why you can't see the difference.

What you wrote is exactly what I wrote that a finder method _could be but it's not and that is the issue_

You can pass a hash (dictionary) to represent kwargs in Python.

This seems like bad engineering fundamentals in the design of ActiveRecord for it to be perpetually subject to this sort of thing.

> You can pass a hash (dictionary) to represent kwargs in Python.

Eh, sure but you need to explicitly pass the dict as a kwarg with a double-asterisk, or else it's just a normal positional parameter.

In Ruby prior to 2.0, there is no formal concept of kwargs, so there is no distinction between passing a Hash as the last positional parameter and passing kwargs. This is the root problem, and I look forward to it going away when everyone moves to 2.0.

i don't know ruby, so forgive the dumb question, but doesn't that mean that a single syntax has two different semantics? what if you want to pass a hash to a method as the last parameter? what decides whether it is treated as a hash or keywords?

There are no 'keyword arguments', however Ruby does provide syntax sugar for options hashes.

For example, this:

    my_method(1, 2, three: 3, four: 4)

Is the same as this:

    my_method(1, 2, { three: 3, four: 4 })

Which can be picked up by the method like this:

    def my_method(one, two, opts)
      three = opts[:three]
      four = opts[:four]
      puts one, two, three, four
    end

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.

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.

Yes, it's true. You'd have to pass symbol keys. It's not easy to reproduce.

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.

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

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

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.

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.

This misses the point. The problem is not that unsafe SQL is produced it is that the method signature is not always what is expected by the user.

This isn't actually an SQL injection flaw.

Um... have you read the linked posting?

>Carefully crafted requests can use the scope to inject >arbitrary SQL.

It's also titled "SQL Injection Vulnerability". Are we all missing something?

um um um you sound like an idiot. shut up already you've failed to redeem and save face.

You're the one calling people names. The guy who wrote the fix that was actually accepted by the Rails core team called this a "SQL Injection" and it has been filed in that category by numerous independent bug trackers.

I don't quite understand the angst about this defect being called a SQL injection vulnerability. The vector for the attack doesn't change the end result.

The cause might be that the API was broken, but it doesn't change the fact that a guy wrote SQL code that was injected into the middle of the rest of the SQL generated by the ORM.

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

What frameworks do you use? Have you performed your own audit?

Totally fair question.

I personally prefer not to use ORMs for this specific reason: they are typically way too complicated to be able to plow through the code in any reasonable way. It's also generally not that hard to design your application in such a way that using a minimalist "ORM-ish" layer of your own making isn't exactly a waste of time. I've also found that they rarely follow these best practices (it's maddening).

I have, however, had to make use of Hibernate, SQL Alchemy and Django's ORM on projects where I didn't make the calls. I'm pretty sure Hibernate uses parametric, prepared statements. I believe SQLAlchemy and Django ORM do not, but use their own escaping mechanism internally. In addition, I don't know about Hibernate and SQLA, but I'm pretty sure that Django's ORM API does make it possible to cause the framework to generate SQL using user-provided data for column/table names in a manner similar to ActiveRecord.

By way of contradicting myself, I do believe that ORMs are great for writing internal use-utilities that are one-offs or quick-and-dirty tools. In general, those cases preclude the use of autonomously provided user data for query building. For world-facing code, ORMs are risky unless you've got someone on the team who knows it and has the ability to ensure it doesn't suffer from these types of design flaws.

I think this kind of view point is mostly influenced by having to deal with poorly designed ORMs and (tho occasionally very necessary) convoluted database designs.

> For world-facing code, ORMs are risky unless you've got someone on the team who knows it and has the ability to ensure it doesn't suffer from these types of design flaws.

This I think is wrong, for the same reason you don't want to be putting together your crypto package. If anything, these kind of security vulnerabilities demonstrate just how hard it is to get all of the subtleties pinned down. Rails is used widely and has been inspected by far more domain experts than you'd ever have on your team and yet.

Sometimes ActiveRecord gets a little in the way - especially back when has_and_belongs_to_many associations were considered best practice - but for the most part I haven't been able to empathize with these kinds of claims. AR is really flexible and gives you a lot of functionality for free.

I think you're making a valid criticism of my position. That said, I don't know that I would (personally) put SQL calls in the same boat as crypto. Crypto involves a lot of complex math and code that, if off by a single bit can wreck the whole house of cards.

In general, I think the problem that ORMs face is that they try and match every single problem thrown at them. People criticize your ORM saying "it doesn't handle egde case XYZ in my legacy data model" or "it suffers from this performance problem when somebody puts a tire boot on the server". Rather than saying "don't use an ORM to solve your unpaid parking ticket problem", the ORM team will devise a way of providing multiple method signatures in a language that loosely supports the feature so that unpaid parking tickets will always be paid prior to the server getting a boot.

Eventually the support for all these edge cases adds up to a very complex piece of software that, to your point, rivals the complexity and fragility of crypto code.

To me... it's more about saying "I have a limited set of use cases here, I don't need a leatherman to cut this noose around my neck I just need a steak knife". ActiveRecord is an impressive freaking tool and I don't begrudge anyone for using. If you ship working code using it then it did it's job.

My personal taste is to stick with simpler tools that don't have so many edge cases so I can sleep easier at night.

Suffice it to say, where you draw the line on "too complex for my taste" and where I would draw that line is probably different and the result of both our personal experiences as well as the problems we are trying to solve.

Totally reasonable post. Sad to see it greyed out.

After years of working in ORMs I've come around to your thinking for bigger projects. ORMs are great, but they are large, complex, and sometimes opaque project dependencies and therefor should be employed sparingly. Parameterized SQL isn't that tough to write (and Python makes it easy) and often faster. The biggest drawback: it requires a dev team comfortable with SQL or the NoSQL library bindings you're using.

I'm disturbed by the fact that you are getting downvoted (here and in another comment thread on this page), largely because I largely agree. I am not an expert by any means so I try to stay atop of "best practices," but what you are saying resonates with what I thought I understood. So if you are way wrong I'd love it if somebody would come out and definitively say so.

I am using my first framework (yiiframework) at the suggestion of another dev. I still do things like

  query(user_input){
     switch(user_input){
       case(x):
         do query_x;
         break;
       case(y):
         do query_y;
         break;
   ...
   }
   }

The other dev thinks I'm nuts, but I avoid a lot of worry with this. I may lose some performance I suppose...

> I'm pretty sure Hibernate uses parametric, prepared statements. I believe SQLAlchemy and Django ORM do not, but use their own escaping mechanism internally.

You have no credibility to talk about database if you can't tell what kind of statements are being executed.

Um... it's been about a year since I sat around looking at the output of any of those three libraries and it wasn't worth the effort to go back and fire something up to be "certain". So I used the appropriate words "I believe" and "I'm pretty sure" to indicate that I was make statements based upon the recollections of my frail human memory ... also, those libraries are actively developed (last I checked) and could have changed since I last used them.

You have no credibility to talk about my credibility if you read too much into every single sentence I write without context.

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

True, but Rails is not doing that, was never doing that, and the patch has nothing to do with this. So you're talking about something unrelated to this security flaw.

I'm confused. If it's not doing that, and was never doing that, then how does an HTTP cookie's value end up injected into a SQL statement generated by Rails?

>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 is still, mathematically-speaking, a bug. The function is supposed to find a post by ID. If its implementation causes side effects or returns unexpected results for a certain subset of possible input data, then it doesn't conform to spec.

This becomes a question of trust. Do you trust ActiveRecord/ORM of choice to be bug-free, or do you treat it as untrusted code and basically have to worry about the implementation of data persistence in your non-DB code even though that's what ORMs are supposed to abstract away? Why is that shit running around in your codebase anyway and not part of the ORM?

I didn't say it wasn't a bug. It's a bug that indicates that ActiveRecord was written/designed in such a way that it trusts user-provided data to be executed.

And, yes, I would fully expect to be able to trust my data-abstraction layer to be bug free. Since Rails seems to have this problem regularly, I can't trust it and therefore choose not to use it for those purposes.

So, I think we agree here.

--- Edit ---

To whit, if you look at the bug report it says that the problem is when an application is passing user-provided data into the framework. They say "don't do that" and then apparently provide a patch to somehow get around if you don't (I don't know enough about the internals of rails to understand the patch).

To my mind, the problem is that Rails should be treating any data passed to it as user-provided data, rather than trusting somebody who just took a 21-day "hacker college" class to do anything other than just pass along user-provided data. The framework should be implementing this kind of security in a consistent and reliable way, rather than trusting you. That's kinda what a framework is for.

Whoa whoa whoa, let's be fair. As a graduate of one of these "hacker colleges" that used RoR [1], and who was a hobbyist programmer long before it, these magically generated function names (and plurals, etc) scared the hell out of me.

In all my experience with programming, I had become accustomed to the mentality of "the compiler/interpreter expects identifiers to be exactly right; it doesn't figure out what you 'really' mean". So it was frustrating to see these auto-generated methods, as I couldn't see the rhyme or reason behind where these methods were coming from.

Fortunately, I found work at a Django shop, where the framework is so much easier to follow and more explicit about how it does things.

[1] devbootcamp.com, first cohort, Spring 2012, though it was actually more like 60 days with 40 days of instruction. I'm now employed as a developer and trusted with production code.

> To whit, if you look at the bug report it says that the problem is when an application is passing user-provided data into the framework. They say "don't do that" and then apparently provide a patch to somehow get around if you don't

They never tell you not to pass in user provided data. I have no idea where you got that conclusion, but you're obviously off and running with it. Quit spreading misinformation.

The post includes a simple workaround for people who are not able to upgrade to a version that includes this security release. It's not a "don't do that" statement.

> To my mind, the problem is that Rails should be treating any data passed to it as user-provided data

It does. This is a bug. Please quit spreading misinformation.

This is the exact quote from the posting linked for this article:

---- Impacted code passes user provided data to a dynamic finder like this:

Post.find_by_id(params[:id]) ----

It later tells you to apply the "to_s" function to the "user provided data" in order to avoid the problem.

I'm unclear on how that is "misinformation".

It tells you to do that in the "Workarounds" section when talking about how the vulnerability can be mitigated. At no point do they tell you not to pass user provided data to this method.

The problem is an argument parsing bug that leads to user provided data being used as programmer provided data. Rails does not force SQL sanity off on the developer.

While I sort of agree with your argument, I question whether or not it's our responsibility to protect the world from the 21-day "hacker college" crowd. I would welcome throwing an error instead however.

There needs to be some sort of expertise cutoff and I think it's reasonable to expect in a web framework that it's user's are informed enough to avoid these sort of mistakes.

Eventually the scissors become so safe that you can't cut anything with them.

I don't think we disagree. Developers should know about these issues and no framework will ever truly protect you from these kinds of mistakes. The question is, I think, is ActiveRecord an ORM that properly mitigates these issues.

Anecdotally, it seems to have recurring problems with SQL injection.

Not really a rubyist, but if you must do something as crazy as composing a query at runtime, isn't Object#tainted? the way to find out whether a string came from application code or from a user?

Reply on news.ycombinator.com