rails / rails fork watch download tarball
public
Forks668Right
Watchers3817Right
Description: Ruby on Rails
Homepage: http://rubyonrails.org
Clone URL: git://github.com/rails/rails.git 
Backport offset/limit SQL injection fix to 2-0-stable

Signed-off-by: Michael Koziarski <michael@koziarski.com>
jonleighton (author)
Thu Sep 04 09:41:49 -0700 2008
NZKoz (committer)
Tue Sep 23 08:07:28 -0700 2008
commit  213f31513e4cb640fa3ed45f387f221401023646
tree    8dc0e69f85087d82f702120747040dad64b5365a
parent  760d525a3afc3b03d6246120c0db47d4a98b784d
0
activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb 
...
106
107
108
109
 
110
111
 
112
113
 
 
 
 
 
114
115
116
 
...
106
107
108
 
109
110
 
111
112
113
114
115
116
117
118
119
120
121
 
0
@@ -106,11 +106,16 @@ module ActiveRecord
0
       #  SELECT * FROM suppliers LIMIT 10 OFFSET 50
0
       def add_limit_offset!(sql, options)
0
         if limit = options[:limit]
0
-          sql << " LIMIT #{limit}"
0
+          sql << " LIMIT #{sanitize_limit(limit)}"
0
           if offset = options[:offset]
0
-            sql << " OFFSET #{offset}"
0
+            sql << " OFFSET #{offset.to_i}"
0
           end
0
         end
0
+        sql
0
+      end
0
+
0
+      def sanitize_limit(limit)
0
+        limit.to_s[/,/] ? limit.split(',').map{ |i| i.to_i }.join(',') : limit.to_i
0
       end
0
 
0
       # Appends a locking clause to an SQL statement.
0
activerecord/lib/active_record/connection_adapters/mysql_adapter.rb 
...
318
319
320
 
321
322
323
324
 
325
326
327
 
...
318
319
320
321
322
323
324
 
325
326
327
328
 
0
@@ -318,10 +318,11 @@ module ActiveRecord
0
 
0
       def add_limit_offset!(sql, options) #:nodoc:
0
         if limit = options[:limit]
0
+          limit = sanitize_limit(limit)
0
           unless offset = options[:offset]
0
             sql << " LIMIT #{limit}"
0
           else
0
-            sql << " LIMIT #{offset}, #{limit}"
0
+            sql << " LIMIT #{offset.to_i}, #{limit}"
0
           end
0
         end
0
       end
0
activerecord/test/adapter_test.rb 
...
103
104
105
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
106
 
...
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
 
0
@@ -103,4 +103,24 @@ class AdapterTest < Test::Unit::TestCase
0
     end
0
   end
0
 
0
+  def test_add_limit_offset_should_sanitize_sql_injection_for_limit_without_comas
0
+    sql_inject = "1 select * from schema"
0
+      assert_equal " LIMIT 1", @connection.add_limit_offset!("", :limit=>sql_inject)
0
+    if current_adapter?(:MysqlAdapter)
0
+      assert_equal " LIMIT 7, 1", @connection.add_limit_offset!("", :limit=>sql_inject, :offset=>7)
0
+    else
0
+      assert_equal " LIMIT 1 OFFSET 7", @connection.add_limit_offset!("", :limit=>sql_inject, :offset=>7)
0
+    end
0
+  end
0
+
0
+  def test_add_limit_offset_should_sanitize_sql_injection_for_limit_with_comas
0
+    sql_inject = "1, 7 procedure help()"
0
+    if current_adapter?(:MysqlAdapter)
0
+      assert_equal " LIMIT 1,7", @connection.add_limit_offset!("", :limit=>sql_inject)
0
+      assert_equal " LIMIT 7, 1", @connection.add_limit_offset!("", :limit=> '1 ; DROP TABLE USERS', :offset=>7)
0
+    else
0
+      assert_equal " LIMIT 1,7", @connection.add_limit_offset!("", :limit=>sql_inject)
0
+      assert_equal " LIMIT 1,7 OFFSET 7", @connection.add_limit_offset!("", :limit=>sql_inject, :offset=>7)
0
+    end
0
+  end
0
 end

Comments