5 ms·
ActiveRecord Vulnerability - Circumvention of attr_protected
- sync 14y agoThis one seems pretty amateur and should have been caught as part of a pull request/code review process. - @regex = /^(#{Regexp.escape(@prefix)})(.+?)(#{Regexp.escape(@suffix)})$/ + @regex = /\A(#{Regexp.escape(@prefix)})(.+?)(#{Regexp.escape(@suffix)})\z/ ^ and $ only match the first line in ruby, whereas \A and \z match across all lines.
- getsat 14y agoMore specifically, \A and \z are "start of string" and "end of string" respectively. Why not \Z, you ask? \Z will match to the end of the string but ignore a trailing newline. irb(main):001:0> !!("hello\n" =~ /\Ahello\z/) => false irb(main):002:0> !!("hello\n" =~ /\Ahello\Z/) => true
- heironimus 14y ago"This one seems pretty amateur and should have been caught as part of a pull request/code review process" I say that about 95% of the bugs I ever see...especially ones written by me.
- tptacek 14y agoThis is an extremely common bug that is not specific to Rails. It would be worth reviewing your code to look at every regex to see if you have similar flaws.
- pjungwir 14y agoI seem to remember a blog post about this regex issue here on HN a few months ago. It definitely surprised me to learn that Ruby doesn't treat $ as end-of-string by default.
- homakov 14y agohttp://homakov.blogspot.com/2012/05/saferweb-injects-in-various-ruby.html http://homakov.blogspot.com/2012/05/saferweb-injects-in-vari... it is usual mistake in url validators
- fxn 14y agoI blogged in http://advogato.org/person/fxn/diary/498.html http://advogato.org/person/fxn/diary/498.html some key differences between Perl and Ruby regexp flags (which includes this gotcha).
- pfortuny 14y agoThis is what everybody says when studying mathematical proofs of theorems, especially the good ones. Obvious, isn't it?
- jscheel 14y agoI feel a bit dumb, but I never would have thought about this one. So, is this best practice for any kind of validation regex?
- Groxx 14y agoYes. Otherwise, say your username validation regex looks like /^[a-z0-9]+$/ (one which I see all the time). It's pretty simple for me to send this: "a\n☃" ("a\n<snowman>" if you can't see it) and it'll validate. I say "pretty simple" because you can do it in many browsers just by pasting text with a newline in it into a form field - it can even be done by accident, no malicious intent necessary. In general, you may be better off avoiding regexes when you can, especially if it's security-sensitive. They're very useful, but they're very easy to get wrong, especially when they get complex. This case, for instance, looks like it would have been impossible if they checked if the attribute were in a list, instead of building a regex. It might be faster with a regex in this case, but for most people that's a (massively) premature optimization for (imperceptibly) small gain.
- tomjen3 14y agoSo basically this attack relies on putting a not-blacklisted identifier on the first line? How do you then get rails to assign it to the correct (blacklisted) identifier?
- mpk 14y agoThat depends on how long it's been in the codebase. As this case is covered in the Rails security guide _and_ the Ruby security reviewer's guide I'd expect it to be quite old and now being found because it hasn't been properly audited before or indicate that the review process needs some work.
- calinet6 14y agoShortly followed by two others: https://groups.google.com/forum/?fromgroups=#!topic/rubyonrails-security/KtmwSbEpzrU https://groups.google.com/forum/?fromgroups=#!topic/rubyonra... (Rails 2.3 and 3.0) https://groups.google.com/forum/?fromgroups=#!topic/rubyonrails-security/4_YvCpLzL58 https://groups.google.com/forum/?fromgroups=#!topic/rubyonra... (JSON library) And in a short followup: "To be clear, updating Rails doesn't necessarily mean the JSON gem will be updated. Please ensure that you are running JSON version 1.7.7, 1.6.8, or 1.5.5. You can do this by adding the dependency to your Gemfile." -- Aaron Patterson
- chaz 14y agoAlso a good time to remind people to subscribe to the security mailing list: https://groups.google.com/forum/#!forum/rubyonrails-security https://groups.google.com/forum/#!forum/rubyonrails-security
- zwily 14y agoWe have new emails to rubyonrails-security triggering PagerDuty alarms as well.
- tenderlove 14y agoOuch. Sorry about that. :-(
- zwily 14y agoHeh, it's okay. We just want to make sure we can patch right away. Your 5 emails at lunchtime today were fun. :)
- patio11 14y agoI was considering putting up a one page app to SMS people when new patches dropped but this is not a good month for me to be adding more Rails apps to my stable...
- kclay 14y agoI don't use rails but it seems that the quality of releases have been going downhill over the pass months.
- zwily 14y agoThese vulnerabilities have been present for many years. They're just being found now. If anything, the quality is now going up. :)
- bvdbijl 14y agoIt's just that there has been a large amount of vulnerabilities found in the last month
- jerhinesmith 14y agoMost of the vulnerabilities you (likely) have been seeing are from code that's been a part of rails for a lot longer than the past few months. The spate of recent vulnerabilities has more to do with YAML in particular than with rails itself.
- eksith 14y agoI'm not a Rails dev so this doesn't affect me, but from a software standpoint, discovering vulnerabilities is a good thing in the longer run. These are not new vulnerabilities, but are being discovered now, which means more spotlights are shining on the project. That's a sign of maturity too. From a maintenance standpoint, it will be a pain to apply patches to older software and those seeing heavy use, but it will cause some reexamination of existing code and practices. There will be a domino effect of more eyes focusing on the code now, which in turn will lead to more discoveries of course, but hopefully, new fixes too.
- papsosouid 14y agoThe quality was always terrible, these are ancient bugs that are just being noticed now. Rails is designed with a 'convenience first, then use a couple regexes to "secure" it' mentality. Any software designed like that will be full of these sorts of holes.
- Tomdarkness 14y agoSurely you should be using attr_accessible with config.active_record.whitelist_attributes = true anyway? I can't imagine a situation where you'd want to have to manually blacklist attributes over whitelisting them.
- aaronbrethorst 14y agoLegacy codebases, etc etc
- tptacek 14y agoWe definitely think you should be using attr_accessible and not attr_protected.
- bdcravens 14y agoThat's made learning Rails a bit interesting. Seems that a number of these vulnerabilities are practically moot if you're following best practices brought about within the last year or so (for example, attr_accessible on all models as a result of the Github snafu). Of course, all vulnerabilities are worth paying attention to, but it'd be nice to know which ones are relevant to greenfield development.
- liquidise 14y agoHow about when you want the majority of your attributes whitelisted? I understand the urge to whitelist, but lets be reasonable here. If i have a table with 20 columns, 19 of which i want accessible (lets exclude a private UK). I also expect the schema for the table to be volatile. Why should i even consider while listing 19+ over blacklisting 1?
- jey 14y agoThe idea is to fail in the direction of being safer than unsafe, if for example someone adds a database column and forgets to write "attr_protected" in the Rails code.
- yxhuvud 14y agoI have 170 tables in an app and a similar amount of controllers. 3 of those refer to the logged in user and in no place except the admin controller itself can you modify an object, or what an object refer to, in such a way where an administrator can access anything he or she shouldn't. Admins have different accounts not to restrict what they can do but to (automatically outside what can be modified by posting params to models) keep track of who has made what changes and to keep them for making incompatible changes at the same time. So no, I'd rather not start whitelisting my models.
- TallboyOne 14y agoYikes, looks like my morning is shot now
- petenixey 14y agoPatio11 is indeed the security soothsayer
- davidw 14y agoIt didn't take much "ear to the ground" to tell that a lot of people had started very carefully sifting through Rails code.
- homakov 14y agowhat is bypass?
- tinco 14y agoit means it was circumvented, i.e. you evaded the piece of code that was supposed to lock you out. In this case, apparently it was possible to 'hide' your attribute behind a newline, making it invisible to the attr_protected code, but somehow the attribute could still be valid (for no reason rails calls #strip on it or something?).
- homakov 14y agothat's why I asked. I know attribute_assignment.rb code pretty well - no strip is called. So conclusion: this doesn't lead to mass assignment. only DoS.
- benmmurphy 14y agoi can't actually explain why it works but it does work. I think it is result of both of the buggy regular expressions.
- homakov 14y agoi am checking agains rails 4. [29] pry(main)> x.update_attributes("client_\nsecret"=>1) (0.1ms) begin transaction (0.1ms) rollback transaction ActiveRecord::UnknownAttributeError: unknown attribute: client_ secret But DEPRECATION WARNING: The method `sdf client_secret=', matching the attribute `client_secret' has dispatched through method_missing. This shouldn't happen, because `client_secret' is a column of the table. If this error has happened through normal usage of Active Record (rather than through your own code or external libraries), please report it as a bug. (called from block in assign_attributes at /Users/homakov/.rvm/gems/ruby-1.9.3-p194/bundler/gems/protected_attributes-369818eedeaa/lib/active_record/mass_assignment_security/attribute_assignment.rb:67) So it's hidden in method_missing!
- homakov 14y ago
- daviding 14y agoGiven that there is no 3.0.X update this time, where's a good resource to learn about applying the manual patch?
- steveklabnik 14y agoI have a blog post here: http://blog.steveklabnik.com/posts/2012-10-04-run-rails-with-custom-patches http://blog.steveklabnik.com/posts/2012-10-04-run-rails-with...
- daviding 14y agoThank you. Also, info about git patch rather than a git cherrypick, in-case it's useful to anyone: https://ariejan.net/2009/10/26/how-to-create-and-apply-a-patch-with-git https://ariejan.net/2009/10/26/how-to-create-and-apply-a-pat...
- sikachu_ 14y agoIf you're using Bundler (which I assume you do, for Rails 3.0) tl;dr is changing from: gem 'rails', '3.0.20' to gem 'rails', github: 'rails/rails', branch: '3-0-stable'
- steveklabnik 14y agoThis is slightly different: this will mean you're bundling the stable branch, which may or may not be the same as 'the last release plus a single security patch.'
- deleted 14y ago[deleted]
- mpchlets 14y agoWe setup a repo for you to use with 3.0.20+patch = 3.0.20.1 Maybe it will save you some time: https://www.assembla.com/code/assembla-rails/git/nodes https://www.assembla.com/code/assembla-rails/git/nodes
- phillmv 14y agoShameless self promotion: we're working on something to make dealing with these patches less painful. It's not out yet, but soon: http://gemcanary.com http://gemcanary.com
- colinhowe 14y agoxcanary would be better... e.g. alerts on mysql, django, ubuntu... anything :)
- phillmv 14y agoOne step at a time! We're going to start with bundler-enabled applications, but I see no reason why it couldn't be expanded to other communities.
- wishbear 14y agocould you please post link to xcanary? Google doesn't helps to find it :(
- voltagex_ 14y agoIt doesn't exist yet. Gemcanary is the MVP.
- bradleyland 14y agoDo you plan to open source Gemcanary. Like Rubygems and Rubygems.org?
- phillmv 14y agoYup. The most value we could provide, though, comes through the vulnerability feed which we'll compile and always provide for free.
- homakov 14y agovery interesting! had same idea (parse gemfile.lock and follow changelogs). Waiting for proto
- tptacek 14y agoI want to shamelessly give a shout-out to Ryan from our MTV team on this one; Rails ActiveModel was I think? the first real piece of Ruby code he ever looked at, and he found the permset Blacklist regex bypass (joernchen found the other one) inside of an hour. Everyone here will testify that I was no help to him at all; my contributions mostly consisted of throwing a large rubber balancing ball at him from the other side of the office. Finding people like Ryan to work with--- "sleepers" who don't have long track records of filing public vulnerabilities, but who are secretly terrifying killing machines --- is the single best thing about my job. Ryan found us on HN after beating the Stripe CTF. If you're like Ryan, ping me; I will go out of my way to make sure we tell you everything we can about why you should work with us. Back on topic: if I was going to place a bet on something about Rails security, it'd be that there are more regex vulnerabilities in the tree. I am uncomfortable with how much Rails leans on regex for policy decisions. Finally, said this before, saying it again: if you have a Rails app with customers, you need to be following @joernchen on Twitter full stop.
- homakov 14y agosomeone should just grep /^ $/
- bradleyland 14y agoI want to send a big thank you to everyone looking closely at Rails right now. I really appreciate all the work that's being done. I can't tell you how happy it makes me to know that Rails becomes more secure every day because of efforts by teams like yours.
- tptacek 14y agoI think Phenoelit is doing more than anything else to motivate us right now.
- homakov 14y agoAttribute is multiline anyway. As far as i know attribute_assignment.rb there is no .strip. So: what is profit of the "Circumvention"? Would love to know
- hayksaakian 14y agoIf I'm not using AR (mongoid instead) and I don't have any explicit JSON.parse in my code, am I OK?
- djbender 14y agoIt looks like this is specific to active_model. https://rubyonrails-security.googlegroups.com/attach/bb44b98a73ef1a06/3-2-attr_protected.patch?view=1&part=6 https://rubyonrails-security.googlegroups.com/attach/bb44b98...
- IgorPartola 14y agoIs stuff like this vulnerability present in Django as well, and just not being discovered as quickly, or is there something in the water (or was there years ago) in the Rails camp that caused all these bugs?
- phillmv 14y agoWe don't know. It's certainly possible. Maybe, maybe not. The recent spate of Rails vulnerabilities - the really scary ones at least - all stemmed from the same root cause: folks were a little too lenient with how they handled YAML parsing. Once that was discovered, a lot more attention has been directed to how Rails handles different kinds of parsing. It's possible! that other frameworks have had similar cascading mistakes, but we won't know until more code reviews occur. Maybe in this particular case Rails-core was especially lenient, but (as far as I remember) dedicated security people have only taken a keener interest in the past year or so.
- deleted 14y ago[deleted]
- jerf 14y agoOne of the core issues here was a serialization library that was too convenient, with the ability to deserialize into arbitrary classes and end up executing code from them. In the Python world, this vulnerability has been a bit better known for a while (which is not to say that they are immune, but it's at least been common community knowledge for a very long time now, at least a decade). Pickle is well-known to have it, but it's frequently pointed out in the docs that it is possible and that you shouldn't unpickle things from users. But other libraries may have this too, and if Django uses them, have a poke around. JSON libraries would be one obvious point. There's probably some extant Perl vulnerabilities in this area too. PHP wouldn't surprise me (though it would be a particular framework and probably not the language as a whole).
- jey 14y agoI doubt it's an issue of Python community vs Ruby community. Instantiating arbitrary objects is the sort of feature I'd expect an object serialization library to have, but not a document serialization library. Pickle is clearly an object serialization library, and I would have naively assume that YAML is a document serialization format, but apparently either the authors of the Ruby YAML parser or the creators of YAML don't see it that way, and see YAML as an object serialization format that is by default permitted to specify arbitrary objects to instantiate.
- dcu 14y agoLooks like this is actually an ActiveModel vulnerability and maybe mongoid is affected too. Just like when rails people said they had a vulnerability in action dispatch but it was actually a YAML vulnerability. (both used in a lot of non-rails projects) So please don't overlook this issue even if you are not using rails.
- saurabhnanda 14y agoI'm building a fairly large web-product for my startup in Rails, and I'm really worried now. I'd paint the recent security vulnerabilities with one broad stroke -- someone trusted the client too much. Isn't that something really, really basic when dealing with data received from a remote client? Never trust anything? Right from the Github mass-assignment [1] vulnerability to the recent YAML & JSON parsing vulnerabilities, it's the same core concept being violated. This gets me thinking -- is Rails the right choice for a large project with JSON, XML, & regular HTML endpoints? PS: I'm not sure what the code-review policy for Rails is, but now would be the time to call-out people who wrote this bad code and NOT auto-merge their future commits without at least two peer reviews.
- static_typed 14y agoNow is a good time to review alternative frameworks. A lot of them are simpler to understand, rely on less magic, and have communities around them that are interested in security as well as functionality.
- donw 14y agoNo, before starting to build a product is a good time to do that. Now is a good time to patch your code and keep building your company. Every framework has security bugs. Jumping ship to a framework you don't understand, possibly one that is harder to update, is a knee-jerk reactionary response to the problem. If all these compromises worry you, invest some time in setting a HIDS (Host Intrusion Detection System), subscribing to the relevant security mailing lists, and ensuring that your deployment workflow allows you to patch production code within a few minutes.
- static_typed 14y agoAt the end of the day, it is a trade-off: do I stick with a current framework full of security holes, indicative of poor design and keep the daily patch cycle fingers-crossed, or do I draw a line, migrate to a less magic less shiny but more secure better engineered framework and focus my time on building my apps instead of spending it all on patching. Tough call.