ActiveRecord Vulnerability - Circumvention of attr_protected

https://groups.google.com/forum/?fromgroups=#!topic/rubyonrails-security/AFBKNY7VSH8

by craigkerstiens • 13 years ago

175 93 13 years ago

groups.google.com

sync• 13 years ago

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.

getsat• 13 years ago

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

heironimus• 13 years 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.

tptacek6• 13 years ago

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.

pjungwir• 13 years ago

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.

pfortuny• 13 years ago

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

jscheel• 13 years ago

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?

Groxx• 13 years ago

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.

tomjen3• 13 years ago

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?

mpk• 13 years ago

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.

calinet6• 13 years ago

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

chaz• 13 years ago

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

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

zwily3• 13 years ago

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

kclay• 13 years ago

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

zwily3• 13 years ago

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

bvdbijl• 13 years ago

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

jerhinesmith• 13 years ago

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.

eksith• 13 years ago

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.

papsosouid• 13 years ago

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.

igravious2• 13 years ago

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.

Tomdarkness• 13 years ago

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.

aaronbrethorst• 13 years ago

Legacy codebases, etc etc

tptacek6• 13 years ago

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

bdcravens• 13 years ago

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.

liquidise• 13 years ago

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?

jey2• 13 years ago

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.

yxhuvud• 13 years ago

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.

tptacek6• 13 years ago

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

TallboyOne• 13 years ago

Yikes, looks like my morning is shot now

petenixey• 13 years ago

Patio11 is indeed the security soothsayer

davidw2• 13 years ago

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

dasil0032• 13 years ago

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

homakov8• 13 years ago

what is bypass?

tinco• 13 years ago

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

homakov8• 13 years ago

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.

daviding2• 13 years ago

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

steveklabnik2• 13 years ago

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

daviding2• 13 years ago

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

sikachu_• 13 years ago

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'

steveklabnik2• 13 years ago

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

mpchlets• 13 years ago

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

phillmv4• 13 years ago

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

colinhowe• 13 years ago

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

phillmv4• 13 years ago

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.

wishbear• 13 years ago

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

bradleyland2• 13 years ago

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

phillmv4• 13 years ago

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

homakov8• 13 years ago

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

tptacek6• 13 years ago

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.

bradleyland2• 13 years ago

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.

tptacek6• 13 years ago

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

homakov8• 13 years ago

someone should just grep /^ $/

homakov8• 13 years ago

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

sc00ter• 13 years ago

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.

tptacek6• 13 years ago

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.

hayksaakian• 13 years ago

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

djbender• 13 years ago

It looks like this is specific to active_model.

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

IgorPartola• 13 years ago

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?

phillmv4• 13 years ago

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.

jerf• 13 years ago

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

jey2• 13 years ago

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.

LawnGnome• 13 years ago

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.

josegonzalez• 13 years ago

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.

mpyne• 13 years ago

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.

SoftwareMaven2• 13 years ago

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.

simonw• 13 years ago

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.

dnerdy• 13 years ago

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.

shabble• 13 years ago

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.

ubernostrum• 13 years ago

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)

dcu• 13 years ago

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.

saurabhnanda• 13 years ago

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.

static_typed2• 13 years ago

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.

donw• 13 years ago

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.

ActiveRecord Vulnerability - Circumvention of attr_protected

Related Stories