11 ms·
Rails SQL injection vulnerability: here are the facts
- Jgrubb 14y agoI swear I saw this same bug discussed on Reddit from some security mailing list last week, unless this is another SQL injection vulnerability with Authlogic.
- FooBarWidget 14y agoYou talking about this? https://github.com/binarylogic/authlogic/pull/341 https://github.com/binarylogic/authlogic/pull/341 It's the same thing.
- why-el 14y agoIt was also emailed to the Rails Core mailing list, thats where I read about it first.
- dsl 14y agoThe author wastes a lot of breath refuting claims that this is a huge Rails bug, then finally comes clean 3/4 of the way in: Other exploitable scenarios Your code is vulnerable if you call Foo.find_by_whatever(bar), where bar can be an arbitrary user-specified hash with symbol keys. This is a fundamental flaw that requires a very specific set of circumstances (which the Rails community is clinging to as a get out of jail free card), similar to the Python Pickle boondoggle [1], which will result in lots of application specific vulnerabilities down the road. 1. http://nadiana.com/python-pickle-insecure http://nadiana.com/python-pickle-insecure
- FooBarWidget 14y ago> The author wastes a lot of breath refuting claims that this is a huge Ruby bug Get your facts right first. It's not a Ruby bug. It's a Rails bug. Ruby != Rails. Many words are used to explain how it works. That is not a refute, nowhere did I claim the bug does not exist. But the requirement for specific circumstances is a fact. The Python pickle example is not only totally irrelevant, it is also not a security vulnerability. Pickle does exactly what it is supposed to do. You weren't supposed to unpickle arbitrary untrusted data in the first place. If you require a safer alternative, use JSON or something, but don't expect as many features as pickle provides. In the end, whether this is a "bug" or a "huge bug" is left as an opinion. The article provides hard facts, and a little bit of commentary. Fact is, we've written a ton of Rails apps that all use find_by_* quite extensively but none turned out to be vulnerable because the case where 'foo' is a symbol hash is rare. Whether you think this is an excuse or not, I'll let you decide.
- dsl 14y agoThanks. Sorry, that was a freudian slip from just reading a rage tweet about Ruby vs. Rails.
- mileszs 14y agoI don't see it as a "huge Ruby bug". Could you elaborate, please? I would understand "huge Ruby on Rails bug", though, by convention, it is still a fairly unlikely case. Edit: I think your edit answers my "Could you elaborate" question somewhat, as you relate a Python library issue to this issue.
- tptacek 14y agoPython's Pickle is insecure by design. It causes application-specific vulnerabilities the way bad crypto libraries do, by giving developers a tool to blow their feet off with. It's hard to blame Python too much for that; languages are going to provide unsafe libraries. This, on the other hand, is just a Rails bug. It has a simple fix. That fix is provided transparently by Rails. It isn't going to cause "lots of application specific vulnerabilities" because nobody is going to care about it 6 months from now, except as yet another reason to keep Rails at the most recent version. This is a problem no different from that faced by people on J2EE stacks. So I disagree with both of your points.
- dsl 14y agoNote that I didn't refer to the Pickle issue as a bug. I think its quite a useful feature, but it's still a pointy stick that many have fallen on.
- freshhawk 14y agoYou referred to it as a "boondoggle" which, if it wasn't even a bug, it also was not.
- jeswin 14y agoThere should be a higher bar for libraries shipping with languages, than those shipping with frameworks like Rails.
- dasil003 14y agoIt's not a matter of anyone egotistically "clinging" to something. The fact is that user params come in with string keys, so an app has to be doing something unusual to be vulnerable. This drastically cuts down the number of vulnerable apps. It is what it is.
- amalag 14y agoThe fact of the matter is people don't know enough about Rails to know what this means and like to go with the easier to spread story: "Rails vulnerability found in all versions related to user input" Sounds much more sexy and they can do more Rails bashing that way. The fact is true as you and the article says, it's pretty obscure. In addition to being obscure, you need the secret session key.
- dasil003 14y agoThere are also things a specific app could be doing that could unwittingly trigger the vulnerability as well. The core issue is worse than simply requiring the session key to be kept secret (which by itself would probably p0wn most apps much worse than the potential exploit of this vulnerability) because the dynamic finder option handling is unexpectedly a magical mine-field that no one would expect to behave that way. So I'd characterize it as a serious problem, but not widespread in the wild, and also with some unknown risk that another major gem like AuthLogic could be as-of-now unknowingly extending the footprint of the vulnerability.
- amalag 14y agoI would love to see an example in the wild of someone taking user input, running some logic on it to create a hash with symbols and then passing it to this finder. So far noone can give an example, but Rails developers have proactively found this not very exploitable bug and patched it. Yet the attitude seems to be backwards to me. The attitude should be that an edge cause was proactively fixed and there was no known exploit of this in the wild.
- 14y ago
- amalag 14y agoYes except that bar cannot be from params since params will never return symbols as keys. So it is pretty obscure unless you take user input and do something pretty special to make it return symbols and then run find_by_whatever on it.
- tptacek 14y agoThis post is written very confidently. I am less confident than the author of this post that the likelihood of a generic exploit via application input (that is, not using forged cookies) is remote. Wish there was more I could say right now. I'm not saying I have a curl command that exploits the vulnerability. I'd just be careful about making assumptions about this bug.
- FooBarWidget 14y agoNot disagreeing with you there. After all there's no way to prove that something doesn't exist. What's why I wrote that everyone should upgrade, just in case. :) The goal of the article is not to defend Rails. It is to inform about the nature of the vulnerability and to replace the feeling of panic with rational thoughts.
- judofyr 14y agoI'm not quite sure what you're hinting at. I know there's been found a DoS-exploit related to the parameter handling (expect a new security release soon), but at least it seems to be safe to say that this bug can't be triggered from `params`.
- mef 14y agoThis bug is pretty edge, but if there was an easy way for a user to put a symbol in the params hash the bug would be a pretty gnarly universal SQL injection vulnerability for all versions of Rails. Though if that was easy it would most likely have been caught much, much earlier as there are a multitude of find helpers that allow literal SQL to be injected.
- FooBarWidget 14y agoEarly versions of Rails explicitly made the decision to never symbolize user inputted hashes because symbols are never garbage collected. This would allow memory to grow unbounded. As a result Rails stores all user hash keys as strings.
- scanr 14y agoThere are suggestions that it is possible (potentially easy) for a user to get symbols into the params hash.
- phillc 14y agoIf you are scanning your code base for find_by_'s, make sure to look at your find_all_by_'s as well if you have them.
- awj 14y ago...and while you're at it, rewrite those into some other form. Everything but the find_by_* dynamic finders is going to be deprecated soon anyways. http://edgeguides.rubyonrails.org/4_0_release_notes.html#extraction-of-features-to-gems http://edgeguides.rubyonrails.org/4_0_release_notes.html#ext...
- bhauer 14y agoIf I understand this correctly, given that a private key is needed to successfully modify the signed HTTP cookie, for a closed-source project using Ruby, Rails, and Authlogic, this vulnerability might be better characterized as an "unintentional backdoor."
- numbsafari 14y agoYep, but even if you have a closed-source project you should avoid checking in any secret keys or other credentials to your source control system.
- macca321 14y agoI wish ruby had named the Hash class properly. It makes reading the article slightly confusing. Hash != Hashtable
- tabbyjabby 14y agoOf course the Hash class in Ruby implements a hash table. What makes you think it doesn't?
- mike 14y agoI believe that macca321 is wishing that, to avoid confusion, the term "hash" was reserved for referring to an individual hash code and that "hash table" was used for the data structure and the Ruby class that implements it.
- __david__ 14y agoYou can blame Perl for that. Ruby took "Hash" from Perl's name for its native hash tables.
- jrochkind1 14y agothanks for this, phusion. I have no idea why THIS vulnerability is getting so much attention. There have actually been OTHER Raisl vulnerabilities in the past 6-8 months which were _more dangerous_, but did not really get attention. The Rails team did NOT help by being very vague about the nature of the problem in their announcement. I imagine they were trying to not reveal the method of exploitation; but it has just led to the current hysteria instead. They would have been better off being taking the extra time to be fully transparent about the nature of the vulnerability -- developers need to know to assess their own risk, as well as to judge the quality of Rails (how stupid was the problem exactly, what does it say about Rails etc?), etc. By being vague about it, Rails core team has just led to everyone assuming the worst, and the current weird hysteria. Yes, it's a vulnerability which COULD be dangerous, and it's hard to tell FOR SURE if your app is vulnerable (perhaps due to code in gem dependencies), the only safe thing to do is update to a patched version. But the chances that your app is vulnerable are pretty small (if you aren't using AuthLogic; if you are, you can update auth_logic and fix it whether or not you update rails). And past recent vulnerabilities which were actually MORE dangerous have not received this level of attention.
- trustfundbaby 14y ago> I have no idea why THIS vulnerability is getting so much attention Its getting attention because its the third SQL vulnerability in 7 months. It just feels like Rails is mature enough at this point that it shouldn't have to be going through this now
- catch23 14y agoI don't see how after a project matures, there should be no exploitable bugs? Even Java still has serious bugs on a yearly basis... The only secure systems are the ones nobody uses.
- jrochkind1 14y agoThat's potentially legit (people could argue about whether these rails vulnerabilities represent something to worry about in Rails, or whether even mature robust projects will still have bugs, including security bugs). But the fact remains that THIS vulnerability isn't NEARLY as dangerous as some of those OTHER ones you mention in the last 7 months, but those other ones people mostly ignored, and THIS one they're going crazy thinking they need to fix right away. What I think about the general question? Most (all?) of those Rails SQL injection bugs actually are related to a similar underlying design: Attempt to create methods with 'variable signatures', where you can give it a string OR a hash, or a list of various strings and hashes, and all of those things mean different things. I think all of the Rails SQL injection bugs are actually related to variable arguments like that. When those variable argument methods were designed in Rails, it's probably safe to say nobody realized there were security implications, that it opens you up to a whole class of bugs where someone puts a hash where you expect a string and it change the semantics of the method call because your variable argument interpreting logic had some flaws. In retrospect, it's possibly not a great thing to do. But Rails is not the only offender here, it's a pretty common design pattern in lots of ruby -- I think it's probably a mistaken one, but it is one that developers tend to like the convenience of.
- ajross 14y agoI'm starting to wonder if the DRY-uber-alles notion popularized by Rails is turning into a security antipattern. It's just too easy for code like this (intended to "make things work cleanly") to forget all the needed checks and all the constraints on the design. The end user ends up repeating themselves less, but that means that the library code ends up getting used in lots of places and for lots of purposes that the author didn't necessarily think through...
- __david__ 14y agoIt also means that when a bug is found in that code that you only have to fix it in one place, not 75,000 random places where it's duplicated, and probably slightly differently each time, making it hard to search for.
- ajross 14y agoThat seems to be belied by this very case, no? Rails had two ways of doing something, one of which forgot where the input could come from...
- __david__ 14y agoHmm... How would copy-pasta code have prevented this?
- ajross 14y agoI fear from your language that you aren't really interested in reasonable argument, but I'll try anyway: ActiveRecord had two variant find() interfaces, one of which tried to make it "easy" on the programmer by avoiding the need to extract an ID to do the lookup. And it went further by adding an overloaded argument syntax that allowed you to elide the first argument entirely. And it forgot that that second argument simultaneously (1) was defined by a remote client and (2) could contain arbitrary SQL. So yes: "copy pasta" of the id boilerplate around an AR find() call would absolutely have prevented this. Rails got slick, and got burned. DRY helped reduce "copy pasta" (sigh) but hurt security.
- benmmurphy 14y agothe other thing to note is that if your application is deserializing untrusted input using Marshall.load and calling methods on the deserialized objects then there is a chance that you are vulnerable to arbitrary ruby code execution. in that case an attacker could execute whatever sql he wanted to anyway.
- digitalboss 14y agoExcellent post - clear and with example. But - I'm confused why one of the readers on the post (Jonas) is calling this a PR Stunt?
- FooBarWidget 14y agoAfter giving many of the comments here a thought, I've written a follow-up article "Securing the Rails session secret" in which different ways to secure the session secret are considered. Feedback is more than welcome. http://blog.phusion.nl/2013/01/04/securing-the-rails-session-secret/ http://blog.phusion.nl/2013/01/04/securing-the-rails-session...