Mysql adapters: improve security of untyped bound values

activerecord/CHANGELOG.md

@@ -1,3 +1,30 @@
+*   The MySQL adapter now cast numbers and booleans bind parameters to to string for safety reasons.
+
+    When comparing a string and a number in a query, MySQL convert the string to a number. So for
+    instance `"foo" = 0`, will implicitly cast `"foo"` to `0` and will evaluate to `TRUE` which can
+    lead to security vulnerabilities.
+
+    Active Record already protect against that vulnerability when it knows the type of the column
+    being compared, however until now it was still vulnerable when using bind parameters:
+    
+    ```ruby
+    User.where("login_token = ?", 0).first
+    ```
+
+    Would perform:
+
+    ```sql
+    SELECT * FROM `users` WHERE `login_token` = 0 LIMIT 1;
+    ```
+
+    Now it will perform:
+
+    ```sql
+    SELECT * FROM `users` WHERE `login_token` = '0' LIMIT 1;
+    ```
+
+    *Jean Boussier*
+
 *   Fixture configurations (`_fixture`) are now strictly validated.
 
     If an error will be raised if that entry contains unknown keys while previously it

activerecord/lib/active_record/connection_adapters/abstract/quoting.rb

@@ -43,6 +43,13 @@ module ActiveRecord
         _type_cast(value)
       end
 
+      # Quote a value to be used as a bound parameter of unknown type. For example,
+      # MySQL might perform dangerous castings when comparing a string to a number,
+      # so this method will cast numbers to string.
+      def quote_bound_value(value)
+        _quote(value)
+      end
+
       # If you are having to call this function, you are likely doing something
       # wrong. The column does not have sufficient type information if the user
       # provided a custom type on the class level either explicitly (via

activerecord/lib/active_record/connection_adapters/mysql/quoting.rb

@@ -6,6 +6,21 @@ module ActiveRecord
   module ConnectionAdapters
     module MySQL
       module Quoting # :nodoc:
+        def quote_bound_value(value)
+          case value
+          when Numeric
+            _quote(value.to_s)
+          when BigDecimal
+            _quote(value.to_s("F"))
+          when true
+            "'1'"
+          when false
+            "'0'"
+          else
+            _quote(value)
+          end
+        end
+
         def quote_column_name(name)
           self.class.quoted_column_names[name] ||= "`#{super.gsub('`', '``')}`"
         end

activerecord/lib/active_record/sanitization.rb

@@ -183,13 +183,13 @@ module ActiveRecord
           if value.respond_to?(:map) && !value.acts_like?(:string)
             values = value.map { |v| v.respond_to?(:id_for_database) ? v.id_for_database : v }
             if values.empty?
-              c.quote(nil)
+              c.quote_bound_value(nil)
             else
-              values.map! { |v| c.quote(v) }.join(",")
+              values.map! { |v| c.quote_bound_value(v) }.join(",")
             end
           else
             value = value.id_for_database if value.respond_to?(:id_for_database)
-            c.quote(value)
+            c.quote_bound_value(value)
           end
         end
 

activerecord/test/cases/adapters/mysql2/quoting_test.rb

@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+require "cases/helper"
+
+class Mysql2QuotingTest < ActiveRecord::Mysql2TestCase
+  def setup
+    super
+    @conn = ActiveRecord::Base.connection
+  end
+
+  def test_quote_bound_integer
+    assert_equal "'42'", @conn.quote_bound_value(42)
+  end
+
+  def test_quote_bound_big_decimal
+    assert_equal "'4.2'", @conn.quote_bound_value(BigDecimal("4.2"))
+  end
+
+  def test_quote_bound_true
+    assert_equal "'1'", @conn.quote_bound_value(true)
+  end
+
+  def test_quote_bound_false
+    assert_equal "'0'", @conn.quote_bound_value(false)
+  end
+end

activerecord/test/cases/insert_all_test.rb

@@ -476,7 +476,7 @@ class InsertAllTest < ActiveRecord::TestCase
   def test_upsert_all_updates_using_provided_sql
     skip unless supports_insert_on_duplicate_update?
 
-    operator = sqlite? ? "MAX" : "GREATEST"
+    operator = current_adapter?(:SQLite3Adapter) ? "MAX" : "GREATEST"
 
     Book.upsert_all(
       [{ id: 1, status: 1 }, { id: 2, status: 1 }],
@@ -506,8 +506,4 @@ class InsertAllTest < ActiveRecord::TestCase
         ActiveRecord::Base.logger = old_logger
       end
     end
-
-    def sqlite?
-      ActiveRecord::Base.connection.adapter_name.match?(/sqlite/i)
-    end
 end

activerecord/test/cases/relation/merging_test.rb

@@ -205,12 +205,22 @@ class RelationMergingTest < ActiveRecord::TestCase
 
     only_david = Author.where("#{author_id} IN (?)", david)
 
-    assert_sql(/WHERE \(#{Regexp.escape(author_id)} IN \(1\)\)\z/) do
-      assert_equal [david], only_david.merge(only_david)
-    end
+    if current_adapter?(:Mysql2Adapter)
+      assert_sql(/WHERE \(#{Regexp.escape(author_id)} IN \('1'\)\)\z/) do
+        assert_equal [david], only_david.merge(only_david)
+      end
 
-    assert_sql(/WHERE \(#{Regexp.escape(author_id)} IN \(1\)\)\z/) do
-      assert_equal [david], only_david.merge(only_david, rewhere: true)
+      assert_sql(/WHERE \(#{Regexp.escape(author_id)} IN \('1'\)\)\z/) do
+        assert_equal [david], only_david.merge(only_david, rewhere: true)
+      end
+    else
+      assert_sql(/WHERE \(#{Regexp.escape(author_id)} IN \(1\)\)\z/) do
+        assert_equal [david], only_david.merge(only_david)
+      end
+
+      assert_sql(/WHERE \(#{Regexp.escape(author_id)} IN \(1\)\)\z/) do
+        assert_equal [david], only_david.merge(only_david, rewhere: true)
+      end
     end
   end
 

activerecord/test/cases/relations_test.rb

@@ -473,7 +473,11 @@ class RelationTest < ActiveRecord::TestCase
 
   def test_finding_with_sanitized_order
     query = Tag.order([Arel.sql("field(id, ?)"), [1, 3, 2]]).to_sql
-    assert_match(/field\(id, 1,3,2\)/, query)
+    if current_adapter?(:Mysql2Adapter)
+      assert_match(/field\(id, '1','3','2'\)/, query)
+    else
+      assert_match(/field\(id, 1,3,2\)/, query)
+    end
 
     query = Tag.order([Arel.sql("field(id, ?)"), []]).to_sql
     assert_match(/field\(id, NULL\)/, query)

activerecord/test/cases/sanitize_test.rb

@@ -31,7 +31,11 @@ class SanitizeTest < ActiveRecord::TestCase
   def test_sanitize_sql_array_handles_named_bind_variables
     quoted_bambi = ActiveRecord::Base.connection.quote("Bambi")
     assert_equal "name=#{quoted_bambi}", Binary.sanitize_sql_array(["name=:name", name: "Bambi"])
-    assert_equal "name=#{quoted_bambi} AND id=1", Binary.sanitize_sql_array(["name=:name AND id=:id", name: "Bambi", id: 1])
+    if current_adapter?(:Mysql2Adapter)
+      assert_equal "name=#{quoted_bambi} AND id='1'", Binary.sanitize_sql_array(["name=:name AND id=:id", name: "Bambi", id: 1])
+    else
+      assert_equal "name=#{quoted_bambi} AND id=1", Binary.sanitize_sql_array(["name=:name AND id=:id", name: "Bambi", id: 1])
+    end
 
     quoted_bambi_and_thumper = ActiveRecord::Base.connection.quote("Bambi\nand\nThumper")
     assert_equal "name=#{quoted_bambi_and_thumper}", Binary.sanitize_sql_array(["name=:name", name: "Bambi\nand\nThumper"])
@@ -101,8 +105,13 @@ class SanitizeTest < ActiveRecord::TestCase
   end
 
   def test_named_bind_variables
-    assert_equal "1", bind(":a", a: 1) # ' ruby-mode
-    assert_equal "1 1", bind(":a :a", a: 1)  # ' ruby-mode
+    if current_adapter?(:Mysql2Adapter)
+      assert_equal "'1'", bind(":a", a: 1) # ' ruby-mode
+      assert_equal "'1' '1'", bind(":a :a", a: 1)  # ' ruby-mode
+    else
+      assert_equal "1", bind(":a", a: 1) # ' ruby-mode
+      assert_equal "1 1", bind(":a :a", a: 1)  # ' ruby-mode
+    end
 
     assert_nothing_raised { bind("'+00:00'", foo: "bar") }
   end
@@ -128,16 +137,32 @@ class SanitizeTest < ActiveRecord::TestCase
   def test_bind_enumerable
     quoted_abc = %(#{ActiveRecord::Base.connection.quote('a')},#{ActiveRecord::Base.connection.quote('b')},#{ActiveRecord::Base.connection.quote('c')})
 
-    assert_equal "1,2,3", bind("?", [1, 2, 3])
+    if current_adapter?(:Mysql2Adapter)
+      assert_equal "'1','2','3'", bind("?", [1, 2, 3])
+    else
+      assert_equal "1,2,3", bind("?", [1, 2, 3])
+    end
     assert_equal quoted_abc, bind("?", %w(a b c))
 
-    assert_equal "1,2,3", bind(":a", a: [1, 2, 3])
+    if current_adapter?(:Mysql2Adapter)
+      assert_equal "'1','2','3'", bind(":a", a: [1, 2, 3])
+    else
+      assert_equal "1,2,3", bind(":a", a: [1, 2, 3])
+    end
     assert_equal quoted_abc, bind(":a", a: %w(a b c)) # '
 
-    assert_equal "1,2,3", bind("?", SimpleEnumerable.new([1, 2, 3]))
+    if current_adapter?(:Mysql2Adapter)
+      assert_equal "'1','2','3'", bind("?", SimpleEnumerable.new([1, 2, 3]))
+    else
+      assert_equal "1,2,3", bind("?", SimpleEnumerable.new([1, 2, 3]))
+    end
     assert_equal quoted_abc, bind("?", SimpleEnumerable.new(%w(a b c)))
 
-    assert_equal "1,2,3", bind(":a", a: SimpleEnumerable.new([1, 2, 3]))
+    if current_adapter?(:Mysql2Adapter)
+      assert_equal "'1','2','3'", bind(":a", a: SimpleEnumerable.new([1, 2, 3]))
+    else
+      assert_equal "1,2,3", bind(":a", a: SimpleEnumerable.new([1, 2, 3]))
+    end
     assert_equal quoted_abc, bind(":a", a: SimpleEnumerable.new(%w(a b c))) # '
   end
 
@@ -150,8 +175,13 @@ class SanitizeTest < ActiveRecord::TestCase
 
   def test_bind_range
     quoted_abc = %(#{ActiveRecord::Base.connection.quote('a')},#{ActiveRecord::Base.connection.quote('b')},#{ActiveRecord::Base.connection.quote('c')})
-    assert_equal "0", bind("?", 0..0)
-    assert_equal "1,2,3", bind("?", 1..3)
+    if current_adapter?(:Mysql2Adapter)
+      assert_equal "'0'", bind("?", 0..0)
+      assert_equal "'1','2','3'", bind("?", 1..3)
+    else
+      assert_equal "0", bind("?", 0..0)
+      assert_equal "1,2,3", bind("?", 1..3)
+    end
     assert_equal quoted_abc, bind("?", "a"..."d")
   end