Skip to content

Commit 354da43

Browse files
tenderloveNZKoz
authored andcommitted
limit() should sanitize limit values
This fixes CVE-2011-0448
1 parent ad9d21d commit 354da43

File tree

4 files changed

+55
-28
lines changed

4 files changed

+55
-28
lines changed

Diff for: activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb

+15-15
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,21 @@ def limited_update_conditions(where_sql, quoted_table_name, quoted_primary_key)
251251
"WHERE #{quoted_primary_key} IN (SELECT #{quoted_primary_key} FROM #{quoted_table_name} #{where_sql})"
252252
end
253253

254+
# Sanitizes the given LIMIT parameter in order to prevent SQL injection.
255+
#
256+
# +limit+ may be anything that can evaluate to a string via #to_s. It
257+
# should look like an integer, or a comma-delimited list of integers.
258+
#
259+
# Returns the sanitized limit parameter, either as an integer, or as a
260+
# string which contains a comma-delimited list of integers.
261+
def sanitize_limit(limit)
262+
if limit.to_s =~ /,/
263+
Arel.sql limit.to_s.split(',').map{ |i| Integer(i) }.join(',')
264+
else
265+
Integer(limit)
266+
end
267+
end
268+
254269
protected
255270
# Returns an array of record hashes with the column names as keys and
256271
# column values as values.
@@ -274,21 +289,6 @@ def delete_sql(sql, name = nil)
274289
update_sql(sql, name)
275290
end
276291

277-
# Sanitizes the given LIMIT parameter in order to prevent SQL injection.
278-
#
279-
# +limit+ may be anything that can evaluate to a string via #to_s. It
280-
# should look like an integer, or a comma-delimited list of integers.
281-
#
282-
# Returns the sanitized limit parameter, either as an integer, or as a
283-
# string which contains a comma-delimited list of integers.
284-
def sanitize_limit(limit)
285-
if limit.to_s =~ /,/
286-
limit.to_s.split(',').map{ |i| i.to_i }.join(',')
287-
else
288-
limit.to_i
289-
end
290-
end
291-
292292
# Send a rollback message to all records after they have been rolled back. If rollback
293293
# is false, only rollback records since the last save point.
294294
def rollback_transaction_records(rollback) #:nodoc

Diff for: activerecord/lib/active_record/relation/query_methods.rb

+1-1
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,7 @@ def build_arel
180180

181181
arel = arel.having(*@having_values.uniq.reject{|h| h.blank?}) unless @having_values.empty?
182182

183-
arel = arel.take(@limit_value) if @limit_value
183+
arel = arel.take(connection.sanitize_limit(@limit_value)) if @limit_value
184184
arel = arel.skip(@offset_value) if @offset_value
185185

186186
arel = arel.group(*@group_values.uniq.reject{|g| g.blank?}) unless @group_values.empty?

Diff for: activerecord/test/cases/adapter_test.rb

-12
Original file line numberDiff line numberDiff line change
@@ -141,16 +141,4 @@ def test_foreign_key_violations_are_translated_to_specific_exception
141141
end
142142
end
143143
end
144-
145-
def test_add_limit_offset_should_sanitize_sql_injection_for_limit_without_comas
146-
sql_inject = "1 select * from schema"
147-
assert_no_match(/schema/, @connection.add_limit_offset!("", :limit=>sql_inject))
148-
assert_no_match(/schema/, @connection.add_limit_offset!("", :limit=>sql_inject, :offset=>7))
149-
end
150-
151-
def test_add_limit_offset_should_sanitize_sql_injection_for_limit_with_comas
152-
sql_inject = "1, 7 procedure help()"
153-
assert_no_match(/procedure/, @connection.add_limit_offset!("", :limit=>sql_inject))
154-
assert_no_match(/procedure/, @connection.add_limit_offset!("", :limit=>sql_inject, :offset=>7))
155-
end
156144
end

Diff for: activerecord/test/cases/base_test.rb

+39
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,45 @@ class Boolean < ActiveRecord::Base; end
4848
class BasicsTest < ActiveRecord::TestCase
4949
fixtures :topics, :companies, :developers, :projects, :computers, :accounts, :minimalistics, 'warehouse-things', :authors, :categorizations, :categories, :posts
5050

51+
def test_limit_with_comma
52+
assert_nothing_raised do
53+
Topic.limit("1,2").all
54+
end
55+
end
56+
57+
def test_limit_without_comma
58+
assert_nothing_raised do
59+
assert_equal 1, Topic.limit("1").all.length
60+
end
61+
62+
assert_nothing_raised do
63+
assert_equal 1, Topic.limit(1).all.length
64+
end
65+
end
66+
67+
def test_invalid_limit
68+
assert_raises(ArgumentError) do
69+
Topic.limit("asdfadf").all
70+
end
71+
end
72+
73+
def test_limit_should_sanitize_sql_injection_for_limit_without_comas
74+
assert_raises(ArgumentError) do
75+
Topic.limit("1 select * from schema").all
76+
end
77+
end
78+
79+
def test_limit_should_sanitize_sql_injection_for_limit_with_comas
80+
assert_raises(ArgumentError) do
81+
Topic.limit("1, 7 procedure help()").all
82+
end
83+
end
84+
85+
def test_select_symbol
86+
topic_ids = Topic.select(:id).map(&:id).sort
87+
assert_equal Topic.find(:all).map(&:id).sort, topic_ids
88+
end
89+
5190
def test_table_exists
5291
assert !NonExistentTable.table_exists?
5392
assert Topic.table_exists?

0 commit comments

Comments
 (0)