8 ms·

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'])

by jtchang 14y ago

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.

codewright 14y ago
Btw, this applies for my (Python) Flask apps using MongoDB ORMs. They escape the inputs. What the hell is going on with ActiveRecord?
- charliesome 14y ago
  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.
  - elbear 14y ago
    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.
    - riffraff 14y ago
      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.
      - elbear 14y ago
        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'])
        riffraff 14y ago
        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.
  - codewright 14y ago
    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.
    - charliesome 14y ago
      > 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.
  - andrewcooke 14y ago
    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?
    - charliesome 14y ago
      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
  - aaronblohowiak 14y ago
    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.
    - charliesome 14y ago
      If it wasn't a rubyism, why is there syntax sugar for passing a hash as the last positional parameter?
      - aaronblohowiak 14y ago
        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.
danso 14y ago
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.
- sandstrom 14y ago
  This pull request is probably what caused the alarm: https://github.com/binarylogic/authlogic/pull/341 https://github.com/binarylogic/authlogic/pull/341 With 3395 stars it seems to be a quite popular.
  - deleted 14y ago
    [deleted]
deleted 14y ago
[deleted]
- jtchang 14y ago
  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.
  - steveklabnik 14y ago
    Ruby does not yet have kwargs, but the upcoming Ruby 2.0 has a form of them.
  - deleted 14y ago
    [deleted]
  - mattmcknight 14y ago
    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.
numbsafari 14y ago
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.
- plaguuuuuu 14y ago
  >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?
Yahivin 14y ago
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.
- ultimoo 14y ago
  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`.
  - eclipxe 14y ago
    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?
    - RegEx 14y ago
      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 vs begin s = Student.find_by_id(params[:id]) # do stuff rescue # do other stuff end
      - eclipxe 14y ago
        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.

by jtchang 14y ago

codewright 14y ago

Btw, this applies for my (Python) Flask apps using MongoDB ORMs. They escape the inputs. What the hell is going on with ActiveRecord?

charliesome 14y ago
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.
- elbear 14y ago
  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.
  - riffraff 14y ago
    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.
    - elbear 14y ago
      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'])
      - riffraff 14y ago
        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.
- codewright 14y ago
  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.
  - charliesome 14y ago
    > 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.
- andrewcooke 14y ago
  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?
  - charliesome 14y ago
    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
- aaronblohowiak 14y ago
  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.
  - charliesome 14y ago
    If it wasn't a rubyism, why is there syntax sugar for passing a hash as the last positional parameter?
    - aaronblohowiak 14y ago
      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.

danso 14y ago

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.

sandstrom 14y ago
This pull request is probably what caused the alarm: https://github.com/binarylogic/authlogic/pull/341 https://github.com/binarylogic/authlogic/pull/341 With 3395 stars it seems to be a quite popular.
- deleted 14y ago
  [deleted]

deleted 14y ago

[deleted]

jtchang 14y ago
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.
- steveklabnik 14y ago
  Ruby does not yet have kwargs, but the upcoming Ruby 2.0 has a form of them.
- deleted 14y ago
  [deleted]
- mattmcknight 14y ago
  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.

numbsafari 14y ago

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.

plaguuuuuu 14y ago
>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?

Yahivin 14y ago

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.

ultimoo 14y ago
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`.
- eclipxe 14y ago
  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?
  - RegEx 14y ago
    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 vs begin s = Student.find_by_id(params[:id]) # do stuff rescue # do other stuff end
    - eclipxe 14y ago
      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.