diff options
author | Aaron Patterson <aaron.patterson@gmail.com> | 2010-12-07 09:49:37 -0800 |
---|---|---|
committer | Aaron Patterson <aaron.patterson@gmail.com> | 2011-02-08 14:21:12 -0800 |
commit | 0b58a7ff420d7ef4b643c521a62be7259dd2f5cb (patch) | |
tree | d5314aa04b853619912bec01f47526cddb1ef2f8 | |
parent | 6b1018526fb304727ee4191afc2d8a5e29e49eea (diff) | |
download | rails-0b58a7ff420d7ef4b643c521a62be7259dd2f5cb.tar.gz rails-0b58a7ff420d7ef4b643c521a62be7259dd2f5cb.tar.bz2 rails-0b58a7ff420d7ef4b643c521a62be7259dd2f5cb.zip |
limit() should sanitize limit values
This fixes CVE-2011-0448
3 files changed, 50 insertions, 16 deletions
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb index 01e53b46c8..5f30b972f5 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb @@ -276,6 +276,21 @@ module ActiveRecord "WHERE #{quoted_primary_key} IN (SELECT #{quoted_primary_key} FROM #{quoted_table_name} #{where_sql})" end + # Sanitizes the given LIMIT parameter in order to prevent SQL injection. + # + # +limit+ may be anything that can evaluate to a string via #to_s. It + # should look like an integer, or a comma-delimited list of integers. + # + # Returns the sanitized limit parameter, either as an integer, or as a + # string which contains a comma-delimited list of integers. + def sanitize_limit(limit) + if limit.to_s =~ /,/ + Arel.sql limit.to_s.split(',').map{ |i| Integer(i) }.join(',') + else + Integer(limit) + end + end + protected # Returns an array of record hashes with the column names as keys and # column values as values. @@ -299,21 +314,6 @@ module ActiveRecord update_sql(sql, name) end - # Sanitizes the given LIMIT parameter in order to prevent SQL injection. - # - # +limit+ may be anything that can evaluate to a string via #to_s. It - # should look like an integer, or a comma-delimited list of integers. - # - # Returns the sanitized limit parameter, either as an integer, or as a - # string which contains a comma-delimited list of integers. - def sanitize_limit(limit) - if limit.to_s =~ /,/ - limit.to_s.split(',').map{ |i| i.to_i }.join(',') - else - limit.to_i - end - end - # Send a rollback message to all records after they have been rolled back. If rollback # is false, only rollback records since the last save point. def rollback_transaction_records(rollback) #:nodoc diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index 2cbb103eb9..6a905b8588 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -171,7 +171,7 @@ module ActiveRecord arel.having(*@having_values.uniq.reject{|h| h.blank?}) unless @having_values.empty? - arel.take(@limit_value) if @limit_value + arel.take(connection.sanitize_limit(@limit_value)) if @limit_value arel.skip(@offset_value) if @offset_value arel.group(*@group_values.uniq.reject{|g| g.blank?}) unless @group_values.empty? diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index 1fa5d2ac5f..1730d9fb56 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -54,6 +54,40 @@ class BasicsTest < ActiveRecord::TestCase assert_nil Edge.primary_key end + def test_limit_with_comma + assert_nothing_raised do + Topic.limit("1,2").all + end + end + + def test_limit_without_comma + assert_nothing_raised do + assert_equal 1, Topic.limit("1").all.length + end + + assert_nothing_raised do + assert_equal 1, Topic.limit(1).all.length + end + end + + def test_invalid_limit + assert_raises(ArgumentError) do + Topic.limit("asdfadf").all + end + end + + def test_limit_should_sanitize_sql_injection_for_limit_without_comas + assert_raises(ArgumentError) do + Topic.limit("1 select * from schema").all + end + end + + def test_limit_should_sanitize_sql_injection_for_limit_with_comas + assert_raises(ArgumentError) do + Topic.limit("1, 7 procedure help()").all + end + end + def test_select_symbol topic_ids = Topic.select(:id).map(&:id).sort assert_equal Topic.find(:all).map(&:id).sort, topic_ids |