ActiveRecord Vulnerability - Circumvention of attr_protected | Hacker News Reader

ActiveRecord Vulnerability - Circumvention of attr_protected

groups.google.com

175 pointscraigkerstiens93 comments

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

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

I think Phenoelit is doing more than anything else to motivate us right now.

someone should just grep /^ $/

Attribute 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

Not to belittle Ryan's contribution, but the vulnerability he found was used in the Stripe CTF, so I have to assume he was already familiar with it. I don't believe you could randomly pick up a piece of code, not knowing the language it's written in and find this sort of thing otherwise.

At the same time it's slightly shocking that no-one has greped Rails for this kind of well-known vulnerability before, never mind auditing for less obvious ones.

Vulnerabilities that don't trace back to common and well-known implementation mistakes are pretty rare. You're almost always familiar with the root cause of an exploitable vulnerability; the trick is finding it.

Shortly followed by two others:

https://groups.google.com/forum/?fromgroups=#!topic/rubyonra... (Rails 2.3 and 3.0)

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

Also a good time to remind people to subscribe to the security mailing list:

https://groups.google.com/forum/#!forum/rubyonrails-security

We have new emails to rubyonrails-security triggering PagerDuty alarms as well.

Ouch. Sorry about that. :-(

Heh, it's okay. We just want to make sure we can patch right away. Your 5 emails at lunchtime today were fun. :)

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

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

More 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

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

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

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

I blogged in http://advogato.org/person/fxn/diary/498.html some key differences between Perl and Ruby regexp flags (which includes this gotcha).

http://homakov.blogspot.com/2012/05/saferweb-injects-in-vari... it is usual mistake in url validators

This is what everybody says when studying mathematical proofs of theorems, especially the good ones. Obvious, isn't it?

I feel a bit dumb, but I never would have thought about this one. So, is this best practice for any kind of validation regex?

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

So 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?

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

Is 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?

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

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

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

Python's YAML library is also capable of serializing python objects[1]. My understanding is the YAML spec allows these extensions but does not require them.

If you are using PyYaml.Loader instead of PyYaml.SafeLoader for anything coming from a user, you are at risk of this problem.

http://pyyaml.org/wiki/PyYAMLDocumentation#YAMLtagsandPython...

I'd be surprised if there wasn't at least one reasonably major PHP framework with similar sorts of vulnerabilities: while PHP's JSON decoder should be safe (it can't create any objects other than stdClass objects, which are simple property buckets), the use of unserialize() in older frameworks was rife (mostly due to a lack of alternatives), and that's definitely not safe with arbitrary user data -- as we've seen with the RoR issues, it only takes one code path where user data unexpectedly gets in somewhere it shouldn't.

AFAIK, the only way this could be an issue with the language is if you use serialize/unserialize, which can be used on classes. It goes without saying that this can be useful in certain contexts - such as file-based caching - but should not be relied upon too heavily.

I cannot remember the last time I came across a php project that did something similar.

CakePHP had an issue with unserialize() at one point. Since then we've wised up about using putting user data into unserialize()

Some popular Django third-party plugins had similar issues to the YAML problems back in 2011 (i.e. using YAML.load instead of safe_load). As noted in the sibling comment, the pickle module is well-known in the Python community for being susceptible to this as well.

Beyond that, your guess is as good as mine. I'm sure that /someone/ has been looking at Django at least to see if there are similar issues.

For all the complaints in the Python community that Django contains too much magic, Rails is far more magical; and that magic add complexity that allows these kinds of problems to show up.

That is not to say Django doesn't have issues; it undoubtably does. I just think the hidden surface area is smaller.

Are there complaints in the Python community that Django contains too much magic? Serious question: I don't follow many Python/etc mailing lists so I'm pretty out of the loop on the general impressions people have of Django. I was kind of hoping that Django's magic-removal branch (merged all the way back in 2006!) would have helped here.

As others have said, Django may or may not have security issues. I wouldn't bet against it.

With regard to this vulnerability, however, the '^' and '$' regex pattern characters in python match the beginning and end (or end + '\n') of the string by default. Multiline mode has to be enabled explicitly:

import re

re.match(r'^test$', 'test\n multiline') == None

re.match(r'^test$', 'test\n multiline', re.MULTILINE) != None

So, I think it's a little less likely that this particular vulnerability would be an issue. It's still possible for someone to leave off the '$', but at least that case is a little more obvious.

Also, the Django codebase doesn't have any param processing code that uses whitelisting/blacklisting like this; you have to explicitly lookup values in request.GET and request.POST or use specific field names in a Form. It's a little less convenient compared to mass assignment, but more secure by default.

Django ModelForms[1] do seem (to my rails-ignorant self) to be quite similar to what's being described here.

    class SomeForm(ModelForm):
        class Meta:
            model = SomeModel
            fields = [ whitelist ]
            exclude = [ blacklist ]

Both fields and exclude are optional, if neither are specified 'all'[2] fields for the model will be included in the form.

[1] https://docs.djangoproject.com/en/1.4/topics/forms/modelform...

[2] The model can blacklist certain fields with editable=False in the field definition as well, which afaik trumps anything a ModelForm does.

Don't know, but I do know we'll be investigating it.

(we tend to keep an eye out for issues affecting other frameworks/libraries, both to coordinate and to check our own stuff -- security is really damned hard, and the thing to do is watch and learn rather than point and laugh)

Shameless 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

xcanary would be better... e.g. alerts on mysql, django, ubuntu... anything :)

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

could you please post link to xcanary? Google doesn't helps to find it :(

It doesn't exist yet. Gemcanary is the MVP.

Do you plan to open source Gemcanary. Like Rubygems and Rubygems.org?

Yup. The most value we could provide, though, comes through the vulnerability feed which we'll compile and always provide for free.

very interesting! had same idea (parse gemfile.lock and follow changelogs). Waiting for proto

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

Legacy codebases, etc etc

We definitely think you should be using attr_accessible and not attr_protected.

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

How 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?

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

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

This is ostensibly why attr_accessible has a "role" parameter.

Yes, however it's also worth noting that Rails core has acknowledged the awkwardness of relying solely on model-level protection for vulnerabilities that should be nipped in the bud at the controller level. Rails 4 will include DHH's new strong_parameters gem that allows params to be filtered proactively on every controller. This will of course help prevent a much broader class of vulnerabilities than ActiveRecord bugs.

http://rubysource.com/rails-4-quick-look-strong-parameters/

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

Patio11 is indeed the security soothsayer

It didn't take much "ear to the ground" to tell that a lot of people had started very carefully sifting through Rails code.

The idea for his post was nothing special, but the post itself was very well written and deserves a lot of credit.

Sure, fair enough, he's a good writer.

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

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

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

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

If I'm not using AR (mongoid instead) and I don't have any explicit JSON.parse in my code, am I OK?

It looks like this is specific to active_model.

https://rubyonrails-security.googlegroups.com/attach/bb44b98...

Yikes, looks like my morning is shot now

Given that there is no 3.0.X update this time, where's a good resource to learn about applying the manual patch?

I have a blog post here: http://blog.steveklabnik.com/posts/2012-10-04-run-rails-with...

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

If you're using Bundler (which I assume you do, for Rails 3.0) tl;dr is changing from:

    gem 'rails', '3.0.20'

    gem 'rails', github: 'rails/rails', branch: '3-0-stable'

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

We 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

what is bypass?

it 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?).

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

i can't actually explain why it works but it does work. I think it is result of both of the buggy regular expressions.

i 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!

so there is a chain: "notprotected\nprotected" it's not found in include? so tryes to assign, then method_missing parses it and founds another attribute just below the first one.

I don't use rails but it seems that the quality of releases have been going downhill over the pass months.

These vulnerabilities have been present for many years. They're just being found now. If anything, the quality is now going up. :)

It's just that there has been a large amount of vulnerabilities found in the last month

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

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

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

I've spent the last 10 minutes reading your comment history. You come across as an opinionated argumentative snide jerk. Note: before you take my comment apart and feed it back to me realize that I have no desire in getting into a verbal sparring match with you and won't reply to you.

I suggest that you follow the advice, "if you can't think of anything nice or constructive to say, bite your tongue". The irony is not lost on me that in taking you to task for the tone you couch your comments in I am failing to live up to my own advice, but I'll make an exception here.

http://en.wikipedia.org/wiki/Ad_hominem

Indeed, but not quite. Ask yourself if it was my intent to counter the person's claims by attacking the person. No, that was not my intent. I never intended to counter the person's claims. I was explicitly calling the person out based on tone and style of his/her comment and others in his/her comment history.

The thought even crossed my mind while carefully drafting the above comment that I ought to preempt the accusation that I was committing this logical fallacy but I decided not to and now I wish the opposite.

Anyway, I hope you see the difference?

Pro-tip: People are going to have strong opinions here. Rebutting their message rather than the delivery ends up rebutting both.

This is not an Ad Hominem at all..

The bummer is that 'extreme position' posts on hacker news attract lots of up votes; moderate ones not so much.

So it's basically a training system to encourage people to comment like this (remember; you need 500+ karma to down vote, so for many people, you can only ignore or up vote; therefore, all you see for a post is upvotes).

My jerk-ometer is pretty good, but I have to disagree with you here. I read his history too and I don't see jerk behaviour.

Reply on news.ycombinator.com