Skip to content

Bad Regexp exploits "fix" backport into 3-2-stable #6670

Closed
wants to merge 1 commit into from

3 participants

@mrbrdo
mrbrdo commented Jun 7, 2012

See #6569
for master: #6671
This is a backport for 3-2-stable branch with exception changed to a deprecation warning.

I only ran ActiveModel tests, so if you have bad test isolation you might have to fix some tests outside ActiveModel, if they use validates_format_of :)

@mrbrdo
mrbrdo commented Jun 14, 2012

@josevalim guys, I was expecting this to get merged into 3.2.6, what happened?
IIRC this used to have 3.2.6 as the milestone but it seems it has been removed...

@rafaelfranca
Ruby on Rails member

We discussed this and we agreed that this would not enter in this release. We think that this problem is more educational, and right now we already explain it in the security guides.

We still need to discuss if this will be merged in the next releases, because this pull request only solves the problem partially, as you pointed here

@mrbrdo
mrbrdo commented Jun 14, 2012

Obviously the description in the security guides is not enough since ^$ are misused even in most of the ActiveModel tests. You even give an exploitable regexp as an example in http://edgeguides.rubyonrails.org/active_model_basics.html (which I fixed in my pull mind you)!
So if the problem is present even in core Rails then I don't think there's anything to think about.

I thought we all agreed that this problem is big enough to deserve a place in Rails. We have seen this exploited on Github, Soundcloud, Tumblr, your own docs, your own tests, even veteran Rails developers make this mistake. Yes I always said this only partially solves the problem BUT it is the best solution I've seen, because it does cover the vast majority of vulnerabilities and at the same time the effect on proper code is negligible, even more so because proper code usually does not need to use ^$ in format validation (it really is a corner case). And even if you have this corner case you just get a warning that goes away by adding one simple option to your validator.
There is no way of completely solving this without changing the Ruby language defaults and breaking loads of existing code, which wouldn't make sense because this is how behavior for RegExp is defined in Ruby and I doubt this will change, nor is there a real reason to change it.
And again I will reiterate, the vast majority of related vulnerabilities come when a bad regexp is used with validates_format_of, and in the vast majority of cases using ^$ in validates_format_of is wrong so this change affects almost no one who already has proper code. The ^$ are mostly useful with gsub and extracting subpatterns, not for validation.

I mean think about it, when did you need a validation that accepts multiline input and only needs 1 of the lines to match the RegExp to be valid? Never? With that in mind it would not be such a stretch anymore to say it's actually a vulnerability or at least poor behavior and not just an educational problem. Using ^$ in format validators is a corner case and should be treated as such. If you need validation like that you are doing something really weird and there's probably a better way to do it without requiring validation like that.

@rafaelfranca
Ruby on Rails member

@mrbrdo we can start changing the ActiveModel tests, guide and documentation. Can you provide a pull request?

I'm not saying that we will not accept this, we only needs to discuss better.

@mrbrdo
mrbrdo commented Jun 14, 2012

I have already fixed all of those that I could find in this pull request. Just discuss it, there is no reason not to merge this. If the chapter in security docs was present for a long time and this vulnerability was in Github, Soundcloud, Tumblr then we need this change, just changing docs will not help much.

@josevalim
Ruby on Rails member

Why are we even discuss a "backport" if the feature was not even merged to master yet?

@mrbrdo I have added a note to the master pull request, could you please address it? Let's iron things out on master and I will personally merge it. Then we will talk about backporting it.

@mrbrdo
mrbrdo commented Jun 14, 2012

Yes you are right. Since we discussed it in pull 6569 I thought everything is ready for merging that's why I made a version for 3-2.

@mrbrdo
mrbrdo commented Jun 14, 2012

I will close this and we can backport when we merge into master #6671

@mrbrdo mrbrdo closed this Jun 14, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.