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.
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.
Yes, this page has a real torches-and-pitchforks feel to it. It's quite absurd.
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.
You need a way to inject symbols as hash keys. Normal parameter handling does not allow this, so you need a different way to exploit the bug. As venus says, session forging is one way. Regular parameter handling is not.
That's a bug/exploit in Authlogic, a third party library for rails, not rails.
However, that bug/exploit is based on the rails vuln and was patched in authlogic exactly how the Rails report instructed people to work around it (casting the parameter to a string).
Reading the actual report linked in the OP you'll see that generic and boilerplate code (e.g. the extremely common pattern: "Post.find_by_id(params[:id])") is going to be vulnerable to this bug.
"This leaves persist_by_session open to sql attacks (such as logging in as any user), if a malicious user can write their own rails session cookie (if they have the rails secret_token)."
The key is: "if they have the rails secret_token"
The secret token is autogenerated when the application is initially bootstrapped. Here is more information about it from any config/initializers/secret_token.rb file:
# Your secret key for verifying the integrity of signed cookies.
# If you change this key, all old signed cookies will become invalid!
# Make sure the secret is at least 30 characters and all random,
# no regular words or you'll be exposed to dictionary attacks.
Good catch, I'm glad that it's a best practice. I wasn't trying to shame those projects, I'm not a rubiest so I was just trying to figure out how someone might gain access to a secret token.
That's an interesting issue (Django has the same issue with its SECRET_KEY). If you have an open-source project that utilizes these kinds of technologies, you need to keep your secret key secret.
As it says in the Django settings:
"Make this unique, and don't share it with anybody."
Incidentally, this is one reason why the 12-factor app methodology stores configuration in environment variables, not source files. http://www.12factor.net/config
The linked pull request described a method for getting unescaped code into the application so the issue with find_by_id might be exploited.
Have you got another way of getting unescaped code into the application such that this issue might be exploited? If so, the core team will be very, very interested to hear from you.
I've been going through the code for ActiveRecord and now AuthLogic. I'm admittedly not well versed in Ruby.
However from the description of the vulnerability found at [1], it would appear as though the cookie value can be set to a Ruby string value that is then parsed by server to produce a Ruby Hash value (rather than the AuthLogic plugin's assumed string value).
Is the eval() of the cookie value done by Rails or is it done by AuthLogic? Is that a potential security vulnerability in itself?
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
Seems like a bigger bug than you seem to think it is. Merely accepting JSON and not calling .to_i is enough for me to select any user.
-- 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 :)
Let's see. Phrack Volume Three, Issue 25 says "In March of 1982, the 5ESS switch first went into operation."
Esquire published "Secrets of the Little Blue Box" in the October 1971 issue of Esquire, based on the phone system MF design from the 1950s/1960s. That means blue boxing started at least 15 years before 5ESS, as tptacek pointed out.
The 1987 Phreakers Manual (http://fringe.davesource.com/Fringe/Hacking/Phreaking/Phreak... ) says "Blue boxing becomes harder as all Bell switching and transmission facilities go under to CCIS. Then to further complicate things, digital microwave, fiber optic, and satellite transmission are all coming to be digital and do not recognize 2600hz for the hang up signal. I predict that around 1990, blue boxes will be obsolete from all major cities."
The 2600 FAQ, Section C-07, says (the earliest date I found for this was August 9, 1993): "Because of ESS Blue boxing is impossible". This is incorrect. ... While the advent of ESS (and other electronic switches) has made the blue boxers task a bit more difficult, ESS is not the reason most of you are unable to blue box. The main culprit is the "forward audio mute" feature of CCIS (out of band signalling). ... So for the clever amongst you, you must somehow get yourself to the 1000's of trunks out there that still utilize MF signalling but bypass/disable the CCIS audio mute problem.
I don't know the switch models enough to say if it was exactly 5ESS, but everything suggests that that is the case.
This isn't an in-band signaling error, per se. This is not an issue of escaping the quotes or something, rather the way this issue works is that User.find_by_username(params[:username]) is interpreted wildly differently if params[:username] is a string compared to if it is a different data type. If it is not a string, then the "options" for the dynamic finder are automatically extracted. It turns out that you probably don't want to leave it up to your user to decide if that param should be a string or a more complex data type...
for instance, before this patch you could do User.find_by_username('whocares', :conditions=>"ARBITRARY SQL")
While the method itself may not be doing in-band signaling, the vulnerability is certainly very related. What was supposed to be strictly data, params[:username], actually becomes an in-band signal -- and that's the exploit.
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.
This is silly. The stinger on this vulnerability is SQL injection, but the vulnerability itself is closer in spirit to code injection. The issue here isn't database hygiene. Parameterized queries would not have helped.
There's a real critique of Rails to be leveled here (there is some fucked up stuff going on with Rails request processing), but yours isn't it.
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)?
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?
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?
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.
NB: what I'm about to write isn't specific to ActiveRecord, it's about SQL injection in general. So please, don't read this thinking I'm making specific claims about AR. I'm just responding to one part of the above claim.
Anyhow...
Parameterized queries do not "fix the SQL injection problem altogether". They solve the most common issue where someone is simply building up a full SQL statement and passing user inputs as part of the string (not as parameters). I call that "Class 1" SQL injection problem. You find this a lot in hand-rolled web apps (especially PHP apps since the legacy MySQL library hasn't been snipped out yet and most tutorials explicitly tell you to do this, even though PHP has long supported parametric SQL).
However, many DB access libraries and ORMs offer facilities that generally revolve around the desire to allow the client application to customize or optimize the generated SQL created by the library (or bypass the SQL generation but leverage the library managed connection state). The API typically just trusts that you know what you are doing, blindly accepts the SQL you give it and injects it into (or replaces) whatever it generated on its own. These are the source of what I call those Class 2 injection vulnerabilities. That is, SQL gets injected in what cannot otherwise be parameterized. These can be mitigated by running a sanity check on the full SQL before it goes to the server (for example, searching it for comment strings and raising and exception or returning an error if they are detected). They can also be detected by scanning your query logs for the same things. Also, this is typically caused by a bug in your code, not the library, since it was trusting you to give it clean SQL.
I am not sure I think of abstractions as featuring improved security. I recently did an (internal to client) blog post about the job of a penetration tester as "puncturer of abstractions". I do have permission to publish it publicly, so I plan to do so.
But briefly, if you think of a grammar such as HTML in terms of an abstraction, the very fact that you need to encode output when creating HTML indicates that the abstraction can be "punctured".
And many abstractions, such as stored procedures, don't help you out at all. There is a oft-repeated untruth that somehow stored procedures magically prevent SQLi.
That's a false dichotomy. I'd prefer to have people constantly test something for security vulnerabilities and find nothing because the code was thoroughly audited and shown to be secure before it was released.
That's a leap. Yehuda and I knew about the mass-assignment vulnerability during DataMapper v0.3, way back in the Merb days, before Rails3.
Yet it didn't get addressed until a few months ago.
PS: It's still broken IMO. It needed a rethinking of strategy and purpose. Instead it got a quick hack. If you want to see mass-assignment done right, look to Play Framework's First Class Forms support.
Mass assignment as a feature is completely removed from Rails 4, and a gem with the replacement (strong_parameters) is available _today_ for you to use with any 3.x application.
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.
"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.
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.
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.
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
vs
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.
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.
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.
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.
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.
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.
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.
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
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.
> 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?
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.
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.
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.
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?
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.
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?
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.
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.
Yes, exactly. This is only directly exploitable if the user can submit a hash with symbol keys. Otherwise, it seems like it would take some unusual code path in the app to exploit the vulnerability.
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.
Not everything that gets passed to the find_by_ methods has to come from the params hash. The sessions hash is another source of data that gets fed to such methods. See this PR https://github.com/binarylogic/authlogic/pull/341
if you can control session hash it may also be possible to execute arbitrary ruby code. if you take an object from the session hash and call a method on it then depending on the method name it is possible to eval arbitrary ruby code.
No, it's a bit more elaborate than that. If what you said were strictly true you could DoS any Rails app as symbols are never garbage collected - you could just continue sending new param keys to the app until it runs out of memory.
If I submit a form where the param is "login[select]=* from users limit 1 --" when I inspect params[:login] I get {"select"=>"* from users limit 1 --"}. Is there a different way of submitting things that converts it to symbols? params[:login] works due to it being a HashWithIndifferentAccess
Yea I'm not seeing it either. Maybe if someone explicitly called `params.symbolize_keys!`? If that's the only time its vulnerable it seems like less of a big deal, though still obviously something that should be patched ASAP.
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.
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.
Yes, the article does mention session secrets. However, this exploit does not require session secrets. The person who wrote the blog post wrote about essentially two vulnerabilities: session forging and SQL injection.
No, the guy showed a way to to sql injections by using a forged session. The problem is that the sql injection requires a hash with symbols as key and params are stored in HashWithIndifferentAccess which should not symbolize the keys. So to exploit the SQL injection you need a vector that allows you to inject symbolized keys. It might be possible to corrupt the params hash, but I can't think of any at the moment. However, the session can contain any ruby object and thus is a possible vector.
I'm pretty sure that the injection only works when you can forge a session because sessions may contain marshalled symbols, and the dynamic finders only accepted symbol option keys as valid. You can't get Rails to construct symbols out of a params hash. Is this a separate vulnerability?
Seconding icambron - how? Because I've been up and down that code and can't see any way to do it. Frankly, I don't think it's possible, because otherwise you would have a trivial DOS vector into any Rails application.
No; the theory behind that attack is, Rails doesn't GC symbols, so you could just repeatedly stuff requests that created new symbols until memory was exhausted. I don't care about that attack (there are others like it), but it's viable.
Symbols are interned and never garbage collected, so if you can cause an app to create arbitrary symbols, you can cause it to use up all the RAM on the machine and throw it into swap, effectively killing its ability to respond to requests in any kind of timely fashion.
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.
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
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.
That method predates the "Forum.where(:url => ...)" syntax of xentronium's demo code. Previously it looked like this: "Forum.find(:first, :conditions => {:url => ...})". Dynamic finders were really sugar over this form, which was always kind of an eyesore.
The vulnerability doesn't get triggered normally. As the grandparent said, request parameters are stored in an object of class HashWithIndifferentAccess, which stores all keys as strings. For the vulnerability to be triggered, the keys must be symbols. You cannot trigger this vulnerability unless your have written code in your app which converts the HashWithIndifferentAccess to a normal hash.
This patch does not fix a wide vulnerability. It just fixes a corner case, a just-in-case-somebody-might-write-vulnerable-code fix.
It just so appears that Authlogic does this. They pass a cookie value into a dynamic finder, so you can tamper the cookie to inject SQL.
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.
It should be as easy as 'gem update rails' at your command line (Terminal window).
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.
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.
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.
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.
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).
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.
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.
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.
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.
The root cause of the bug here is not sql injection. Its that one of the query functions is overloaded to do different things depending on what datatype you pass to it and that the user can manipulate that datatype (for example, by passing an object in a part of a JSON message that would usully conatin a string). Of course, this is only really serious if one of the overloads gices too much power (in this vulnerability's case it would let you run an arbitrary query)
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.
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?
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.