diff --git a/activemodel/CHANGELOG.md b/activemodel/CHANGELOG.md index 5ee439fa3f864..847ae7f237089 100644 --- a/activemodel/CHANGELOG.md +++ b/activemodel/CHANGELOG.md @@ -37,6 +37,11 @@ * Trim down Active Model API by removing `valid?` and `errors.full_messages` *José Valim* +* When `^` or `$` are used in the regular expression provided to `validates_format_of` and the :multiline option is not set to true, an exception will be raised. This is to prevent security vulnerabilities when using `validates_format_of`. The problem is described in detail in the Rails security guide. + +## Rails 3.2.6 (Jun 12, 2012) ## + +* No changes. ## Rails 3.2.5 (Jun 1, 2012) ## diff --git a/activemodel/lib/active_model/validations/format.rb b/activemodel/lib/active_model/validations/format.rb index dd87e312f9326..ffdf842d94b57 100644 --- a/activemodel/lib/active_model/validations/format.rb +++ b/activemodel/lib/active_model/validations/format.rb @@ -32,11 +32,21 @@ def option_call(record, name) def record_error(record, attribute, name, value) record.errors.add(attribute, :invalid, options.except(name).merge!(:value => value)) end - + + def regexp_using_multiline_anchors?(regexp) + regexp.source.start_with?("^") || + (regexp.source.end_with?("$") && !regexp.source.end_with?("\\$")) + end + def check_options_validity(options, name) option = options[name] if option && !option.is_a?(Regexp) && !option.respond_to?(:call) raise ArgumentError, "A regular expression or a proc or lambda must be supplied as :#{name}" + elsif option && option.is_a?(Regexp) && + regexp_using_multiline_anchors?(option) && options[:multiline] != true + raise ArgumentError, "The provided regular expression is using multiline anchors (^ or $), " \ + "which may present a security risk. Did you mean to use \\A and \\z, or forgot to add the " \ + ":multiline => true option?" end end end @@ -47,7 +57,7 @@ module HelperMethods # attribute matches the regular expression: # # class Person < ActiveRecord::Base - # validates_format_of :email, :with => /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\Z/i, :on => :create + # validates_format_of :email, :with => /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\z/i, :on => :create # end # # Alternatively, you can require that the specified attribute does _not_ @@ -63,12 +73,16 @@ module HelperMethods # class Person < ActiveRecord::Base # # Admin can have number as a first letter in their screen name # validates_format_of :screen_name, - # :with => lambda{ |person| person.admin? ? /\A[a-z0-9][a-z0-9_\-]*\Z/i : /\A[a-z][a-z0-9_\-]*\Z/i } + # :with => lambda{ |person| person.admin? ? /\A[a-z0-9][a-z0-9_\-]*\z/i : /\A[a-z][a-z0-9_\-]*\z/i } # end # # Note: use \A and \Z to match the start and end of the # string, ^ and $ match the start/end of a line. # + # Due to frequent misuse of ^ and $, you need to pass the + # :multiline => true option in case you use any of these two anchors in the provided + # regular expression. In most cases, you should be using \A and \z. + # # You must pass either :with or :without as an option. # In addition, both must be a regular expression or a proc or lambda, or # else an exception will be raised. @@ -98,6 +112,9 @@ module HelperMethods # method, proc or string should return or evaluate to a true or false value. # * :strict - Specifies whether validation should be strict. # See ActiveModel::Validation#validates! for more information. + # * :multiline - Set to true if your regular expression contains + # anchors that match the beginning or end of lines as opposed to the + # beginning or end of the string. These anchors are ^ and $. def validates_format_of(*attr_names) validates_with FormatValidator, _merge_attributes(attr_names) end diff --git a/activemodel/test/cases/validations/format_validation_test.rb b/activemodel/test/cases/validations/format_validation_test.rb index 41a1131bcbf67..308a3c6cefa1c 100644 --- a/activemodel/test/cases/validations/format_validation_test.rb +++ b/activemodel/test/cases/validations/format_validation_test.rb @@ -11,7 +11,7 @@ def teardown end def test_validate_format - Topic.validates_format_of(:title, :content, :with => /^Validation\smacros \w+!$/, :message => "is bad data") + Topic.validates_format_of(:title, :content, :with => /\AValidation\smacros \w+!\z/, :message => "is bad data") t = Topic.new("title" => "i'm incorrect", "content" => "Validation macros rule!") assert t.invalid?, "Shouldn't be valid" @@ -27,7 +27,7 @@ def test_validate_format end def test_validate_format_with_allow_blank - Topic.validates_format_of(:title, :with => /^Validation\smacros \w+!$/, :allow_blank => true) + Topic.validates_format_of(:title, :with => /\AValidation\smacros \w+!\z/, :allow_blank => true) assert Topic.new("title" => "Shouldn't be valid").invalid? assert Topic.new("title" => "").valid? assert Topic.new("title" => nil).valid? @@ -36,7 +36,7 @@ def test_validate_format_with_allow_blank # testing ticket #3142 def test_validate_format_numeric - Topic.validates_format_of(:title, :content, :with => /^[1-9][0-9]*$/, :message => "is bad data") + Topic.validates_format_of(:title, :content, :with => /\A[1-9][0-9]*\z/, :message => "is bad data") t = Topic.new("title" => "72x", "content" => "6789") assert t.invalid?, "Shouldn't be valid" @@ -63,11 +63,21 @@ def test_validate_format_numeric end def test_validate_format_with_formatted_message - Topic.validates_format_of(:title, :with => /^Valid Title$/, :message => "can't be %{value}") + Topic.validates_format_of(:title, :with => /\AValid Title\z/, :message => "can't be %{value}") t = Topic.new(:title => 'Invalid title') assert t.invalid? assert_equal ["can't be Invalid title"], t.errors[:title] end + + def test_validate_format_of_with_multiline_regexp_should_raise_error + assert_raise(ArgumentError) { Topic.validates_format_of(:title, :with => /^Valid Title$/) } + end + + def test_validate_format_of_with_multiline_regexp_and_option + assert_nothing_raised(ArgumentError) do + Topic.validates_format_of(:title, :with => /^Valid Title$/, :multiline => true) + end + end def test_validate_format_with_not_option Topic.validates_format_of(:title, :without => /foo/, :message => "should not contain foo") diff --git a/activemodel/test/cases/validations/i18n_validation_test.rb b/activemodel/test/cases/validations/i18n_validation_test.rb index 6b6aad3bd1d68..bb751cf9c5fb5 100644 --- a/activemodel/test/cases/validations/i18n_validation_test.rb +++ b/activemodel/test/cases/validations/i18n_validation_test.rb @@ -141,7 +141,7 @@ def test_errors_full_messages_uses_format COMMON_CASES.each do |name, validation_options, generate_message_options| test "validates_format_of on generated message #{name}" do - Person.validates_format_of :title, validation_options.merge(:with => /^[1-9][0-9]*$/) + Person.validates_format_of :title, validation_options.merge(:with => /\A[1-9][0-9]*\z/) @person.title = '72x' @person.errors.expects(:generate_message).with(:title, :invalid, generate_message_options.merge(:value => '72x')) @person.valid? @@ -291,7 +291,7 @@ def self.set_expectations_for_validation(validation, error_type, &block_that_set # validates_format_of w/o mocha set_expectations_for_validation "validates_format_of", :invalid do |person, options_to_merge| - Person.validates_format_of :title, options_to_merge.merge(:with => /^[1-9][0-9]*$/) + Person.validates_format_of :title, options_to_merge.merge(:with => /\A[1-9][0-9]*\z/) end # validates_inclusion_of w/o mocha diff --git a/guides/source/active_model_basics.textile b/guides/source/active_model_basics.textile index d373f4ac85df8..7cafff2ad8133 100644 --- a/guides/source/active_model_basics.textile +++ b/guides/source/active_model_basics.textile @@ -187,7 +187,7 @@ class Person attr_accessor :name, :email, :token validates :name, :presence => true - validates_format_of :email, :with => /^([^\s]+)((?:[-a-z0-9]\.)[a-z]{2,})$/i + validates_format_of :email, :with => /\A([^\s]+)((?:[-a-z0-9]\.)[a-z]{2,})\z/i validates! :token, :presence => true end diff --git a/guides/source/security.textile b/guides/source/security.textile index 0931dd639376f..626d6fa5087d6 100644 --- a/guides/source/security.textile +++ b/guides/source/security.textile @@ -588,26 +588,43 @@ h4. Regular Expressions INFO: _A common pitfall in Ruby's regular expressions is to match the string's beginning and end by ^ and $, instead of \A and \z._ -Ruby uses a slightly different approach than many other languages to match the end and the beginning of a string. That is why even many Ruby and Rails books make this wrong. So how is this a security threat? Imagine you have a File model and you validate the file name by a regular expression like this: +Ruby uses a slightly different approach than many other languages to match the end and the beginning of a string. That is why even many Ruby and Rails books make this wrong. So how is this a security threat? Say you wanted to loosely validate a URL field and you used a simple regular expression like this: -class File < ActiveRecord::Base - validates :name, :format => /^[\w\.\-\]$/ -end + /^https?:\/\/[^\n]+$/i -This means, upon saving, the model will validate the file name to consist only of alphanumeric characters, dots, + and -. And the programmer added ^ and $ so that file name will contain these characters from the beginning to the end of the string. However, _(highlight)in Ruby ^ and $ matches the *line* beginning and line end_. And thus a file name like this passes the filter without problems: +This may work fine in some languages. However, _(highlight)in Ruby ^ and $ match the *line* beginning and line end_. And thus a URL like this passes the filter without problems: -file.txt%0A +javascript:exploit_code();/* +http://hi.com +*/ -Whereas %0A is a line feed in URL encoding, so Rails automatically converts it to "file.txt\n<script>alert('hello')</script>". This file name passes the filter because the regular expression matches – up to the line end, the rest does not matter. The correct expression should read: +This URL passes the filter because the regular expression matches – the second line, the rest does not matter. Now imagine we had a view that showed the URL like this: + + + link_to "Homepage", @user.homepage + + +The link looks innocent to visitors, but when it's clicked, it will execute the javascript function "exploit_code" or any other javascript the attacker provides. + +To fix the regular expression, \A and \z should be used instead of ^ and $, like so: -/\A[\w\.\-\]\z/ + /\Ahttps?:\/\/[^\n]+\z/i +Since this is a frequent mistake, the format validator (validates_format_of) now raises an exception if the provided regular expression starts with ^ or ends with $. If you do need to use ^ and $ instead of \A and \z (which is rare), you can set the :multiline option to true, like so: + + + # content should include a line "Meanwhile" anywhere in the string + validates :content, :format => { :with => /^Meanwhile$/, :multiline => true } + + +Note that this only protects you against the most common mistake when using the format validator - you always need to keep in mind that ^ and $ match the *line* beginning and line end in Ruby, and not the beginning and end of a string. + h4. Privilege Escalation WARNING: _Changing a single parameter may give the user unauthorized access. Remember that every parameter may be changed, no matter how much you hide or obfuscate it._