Bad Regexp exploits "fix" master branch #6671

Merged
merged 1 commit into from Jun 14, 2012

Projects

None yet

2 participants

@mrbrdo
Contributor
mrbrdo commented Jun 7, 2012

See #6569 for info.

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.

Discussion:
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!
So if the problem is present even in core Rails then I don't think there's anything to think about.

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

@josevalim josevalim commented on an outdated diff Jun 14, 2012
guides/source/security.textile
@@ -592,7 +592,7 @@ Ruby uses a slightly different approach than many other languages to match the e
<ruby>
class File < ActiveRecord::Base
- validates :name, :format => /^[\w\.\-\<plus>]<plus>$/
+ validates :name, :format => { :with => /^[\w\.\-\<plus>]<plus>$/, :multiline => true }
josevalim
josevalim Jun 14, 2012 Member

The whole paragraph here needs to be evaluated, you cannot simply change the example otherwise the explanation preceding it no longer makes sense.

Contributor
mrbrdo commented Jun 14, 2012

I added a note at the bottom but you're right, I will fix this. Should I just amend and push to overwrite this commit, is that ok?
PS: Should I note that this was changed in version XY or do I just simply state the fact?

Member

That's perfect. Also, please remove the 3.2.6 entry on the CHANGELOG.

Contributor
mrbrdo commented Jun 14, 2012

Where should I put the change description though?

Member

I am just saying to remove the 3.2.6 one, the master one should stay (this commit is for master).

Contributor
mrbrdo commented Jun 14, 2012

Okay. Since I am already changing the security guide I will take into account what Egor said when he described the problem:
"Example is awful - why to add <script> to file name? Rails will escape it in html_safe easily. Much better to demonstrate XSS in URL, Shell Inject or XML inject because they all are pretty dangerous."
I think he is right so I will try to come up with a better example. I think email is a good example.

Contributor
mrbrdo commented Jun 14, 2012

I updated the pull request with requested changes. It's rebased on current master.

@josevalim josevalim merged commit 68021d3 into rails:master Jun 14, 2012
Contributor
mrbrdo commented Jun 14, 2012

Thanks. Let me know if you need any help with backporting later.

@mrbrdo mrbrdo referenced this pull request Oct 18, 2012
Merged

Credit for previous commit #7990

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment