rails/rails — pull request #6569 by mrbrdo (2012-05-31 13:30:41+00:00)
Title: Bad Regexp exploits "fix" (with warning)

Explanation of the "issue": http://homakov.blogspot.co.uk/2012/05/saferweb-injects-in-various-ruby.html

While I do not agree with Egor that this is an actual vulnerability in Ruby or something that can be fixed, I do agree it is an issue that should be addressed (e.g. through an educated warning). I propose a "solution" by incorporating a warning into validates_format_of, which I believe is the source of most of related vulnerabilities (caused by programmer error).

The proposed change is to throw an exception from validates_format_of when a Regexp containing the ^ or $ anchor is used, unless the programmer explicitly acknowledges his awareness about the multiline behavior by passing a :multiline => true option to validates_format_of.

I believe this solution is good because of several reasons:
1. The vast majority of users that are using validates_format_of do not want multiline matching. Consequently, the vast majority of users that are using proper Regexp will not be affected by this change at all.
2. The vast majority of users who are using ^ or $ inside validates_format_of are not aware of their multiline behavior, or are copy-pasting Regexp, and will be affected by this change, consequently fixing a security vulnerability in their application.
3. The minority of users who do need multiline behavior in validates_format_of simply need to add the new :multiline => true option to their validators. So if this is mentioned in the changelog or the users are running any kind of tests that at least load the model using validates_format_of (exception is raised when validates_format_of is called on the class - so at model load), they will be aware of what needs to be done (trivial - add an option).
4. When people fix their validators they will most likely become familiar with the issue and will be able to fix potential vulnerabilities related to Regexp used outside of validates_format_of.

While this check does not cover all possible related vulnerabilities, I think it should cover most of them, and the cost of this is very low (not a lot of people need multiline behavior). Performance hit is also negligible as the check is only performed when the model is loaded.

Regarding tests in ActiveModel:
I added a test for this case. Due to the change, some existing test cases needed to have the :multiline => true option added. It may be possible that in these existing tests, use of \A and \Z would be more appropriate - I will let the core developers decide this. In any case I have made all the ActiveModel pass, but I did not run tests of other parts of Rails so the whole test suite should be ran before thinking about merging this commit. The exception message string might also need to be written differently.

I hope this work will be appreciated and considered for merging. I think the issue is important as Egon demonstrated in his blog post - this vulnerability was present in many big Rails projects such as Github, Soundcloud, Tumblr etc., so it should not be completely ignored.