Merge pull request #27947 from mastahyeti/unsafe_raw_sql

Disallow raw SQL in dangerous AR methods

6 files changed, 98 insertions, 2 deletions

diff --git a/activerecord/lib/active_record/attribute_methods.rb b/activerecord/lib/active_record/attribute_methods.rb
index 23d2aef214..64f81ca582 100644
--- a/activerecord/lib/active_record/attribute_methods.rb
+++ b/activerecord/lib/active_record/attribute_methods.rb
@@ -167,6 +167,46 @@ module ActiveRecord
         end
       end
 
+      # Regexp whitelist. Matches the following:
+      #   "#{table_name}.#{column_name}"
+      #   "#{column_name}"
+      COLUMN_NAME_WHITELIST = /\A(?:\w+\.)?\w+\z/i
+
+      # Regexp whitelist. Matches the following:
+      #   "#{table_name}.#{column_name}"
+      #   "#{table_name}.#{column_name} #{direction}"
+      #   "#{column_name}"
+      #   "#{column_name} #{direction}"
+      COLUMN_NAME_ORDER_WHITELIST = /\A(?:\w+\.)?\w+(?:\s+asc|\s+desc)?\z/i
+
+      def enforce_raw_sql_whitelist(args, whitelist: COLUMN_NAME_WHITELIST) # :nodoc:
+        unexpected = args.reject do |arg|
+          arg.kind_of?(Arel::Node) ||
+            arg.is_a?(Arel::Nodes::SqlLiteral) ||
+            arg.is_a?(Arel::Attributes::Attribute) ||
+            arg.to_s.split(/\s*,\s*/).all? { |part| whitelist.match?(part) }
+        end
+
+        return if unexpected.none?
+
+        if allow_unsafe_raw_sql == :deprecated
+          ActiveSupport::Deprecation.warn(
+            "Dangerous query method (method whose arguments are used as raw " \
+            "SQL) called with non-attribute argument(s): " \
+            "#{unexpected.map(&:inspect).join(", ")}. Non-attribute " \
+            "arguments will be disallowed in Rails 6.0. This method should " \
+            "not be called with user-provided values, such as request " \
+            "parameters or model attributes. Known-safe values can be passed " \
+            "by wrapping them in Arel.sql()."
+          )
+        else
+          raise(ActiveRecord::UnknownAttributeReference,
+            "Query method called with non-attribute argument(s): " +
+            unexpected.map(&:inspect).join(", ")
+          )
+        end
+      end
+
       # Returns true if the given attribute exists, otherwise false.
       #
       #   class Person < ActiveRecord::Base
diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb
index 0f7a503c90..b97b14644e 100644
--- a/activerecord/lib/active_record/core.rb
+++ b/activerecord/lib/active_record/core.rb
@@ -76,6 +76,14 @@ module ActiveRecord
       # scope being ignored is error-worthy, rather than a warning.
       mattr_accessor :error_on_ignored_order, instance_writer: false, default: false
 
+      # :singleton-method:
+      # Specify the behavior for unsafe raw query methods. Values are as follows
+      #   deprecated - Warnings are logged when unsafe raw SQL is passed to
+      #                query methods.
+      #   disabled   - Unsafe raw SQL passed to query methods results in
+      #                UnknownAttributeReference exception.
+      mattr_accessor :allow_unsafe_raw_sql, instance_writer: false, default: :deprecated
+
       ##
       # :singleton-method:
       # Specify whether or not to use timestamps for migration versions
diff --git a/activerecord/lib/active_record/errors.rb b/activerecord/lib/active_record/errors.rb
index f77cd23e22..7382879fce 100644
--- a/activerecord/lib/active_record/errors.rb
+++ b/activerecord/lib/active_record/errors.rb
@@ -342,4 +342,29 @@ module ActiveRecord
   # StatementTimeout will be raised when statement timeout exceeded.
   class StatementTimeout < StatementInvalid
   end
+
+  # UnknownAttributeReference is raised when an unknown and potentially unsafe
+  # value is passed to a query method when allow_unsafe_raw_sql is set to
+  # :disabled. For example, passing a non column name value to a relation's
+  # #order method might cause this exception.
+  #
+  # When working around this exception, caution should be taken to avoid SQL
+  # injection vulnerabilities when passing user-provided values to query
+  # methods. Known-safe values can be passed to query methods by wrapping them
+  # in Arel.sql.
+  #
+  # For example, with allow_unsafe_raw_sql set to :disabled, the following
+  # code would raise this exception:
+  #
+  #   Post.order("length(title)").first
+  #
+  # The desired result can be accomplished by wrapping the known-safe string
+  # in Arel.sql:
+  #
+  #   Post.order(Arel.sql("length(title)")).first
+  #
+  # Again, such a workaround should *not* be used when passing user-provided
+  # values, such as request parameters or model attributes to query methods.
+  class UnknownAttributeReference < ActiveRecordError
+  end
 end
diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb
index 11256ab3d9..d49472fc70 100644
--- a/activerecord/lib/active_record/relation/calculations.rb
+++ b/activerecord/lib/active_record/relation/calculations.rb
@@ -183,6 +183,7 @@ module ActiveRecord
         relation = apply_join_dependency
         relation.pluck(*column_names)
       else
+        enforce_raw_sql_whitelist(column_names)
         relation = spawn
         relation.select_values = column_names.map { |cn|
           @klass.has_attribute?(cn) || @klass.attribute_alias?(cn) ? arel_attribute(cn) : cn
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 1219737e78..749223422f 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -295,6 +295,7 @@ module ActiveRecord
       spawn.order!(*args)
     end
 
+    # Same as #order but operates on relation in-place instead of copying.
     def order!(*args) # :nodoc:
       preprocess_order_args(args)
 
@@ -316,6 +317,7 @@ module ActiveRecord
       spawn.reorder!(*args)
     end
 
+    # Same as #reorder but operates on relation in-place instead of copying.
     def reorder!(*args) # :nodoc:
       preprocess_order_args(args)
 
@@ -1076,7 +1078,7 @@ module ActiveRecord
             end
             o.split(",").map! do |s|
               s.strip!
-              s.gsub!(/\sasc\Z/i, " DESC") || s.gsub!(/\sdesc\Z/i, " ASC") || s.concat(" DESC")
+              s.gsub!(/\sasc\Z/i, " DESC") || s.gsub!(/\sdesc\Z/i, " ASC") || (s << " DESC")
             end
           else
             o
@@ -1085,6 +1087,10 @@ module ActiveRecord
       end
 
       def does_not_support_reverse?(order)
+        # Account for String subclasses like Arel::Nodes::SqlLiteral that
+        # override methods like #count.
+        order = String.new(order) unless order.instance_of?(String)
+
         # Uses SQL function with multiple arguments.
         (order.include?(",") && order.split(",").find { |section| section.count("(") != section.count(")") }) ||
           # Uses "nulls first" like construction.
@@ -1118,6 +1124,12 @@ module ActiveRecord
           klass.send(:sanitize_sql_for_order, arg)
         end
         order_args.flatten!
+
+        @klass.enforce_raw_sql_whitelist(
+          order_args.flat_map { |a| a.is_a?(Hash) ? a.keys : a },
+          whitelist: AttributeMethods::ClassMethods::COLUMN_NAME_ORDER_WHITELIST
+        )
+
         validate_order_args(order_args)
 
         references = order_args.grep(String)
diff --git a/activerecord/lib/active_record/sanitization.rb b/activerecord/lib/active_record/sanitization.rb
index 90cc3373fb..21f8bc7cb2 100644
--- a/activerecord/lib/active_record/sanitization.rb
+++ b/activerecord/lib/active_record/sanitization.rb
@@ -63,7 +63,17 @@ module ActiveRecord
         #   # => "id ASC"
         def sanitize_sql_for_order(condition) # :doc:
           if condition.is_a?(Array) && condition.first.to_s.include?("?")
-            sanitize_sql_array(condition)
+            enforce_raw_sql_whitelist([condition.first],
+              whitelist: AttributeMethods::ClassMethods::COLUMN_NAME_ORDER_WHITELIST
+            )
+
+            # Ensure we aren't dealing with a subclass of String that might
+            # override methods we use (eg. Arel::Nodes::SqlLiteral).
+            if condition.first.kind_of?(String) && !condition.first.instance_of?(String)
+              condition = [String.new(condition.first), *condition[1..-1]]
+            end
+
+            Arel.sql(sanitize_sql_array(condition))
           else
             condition
           end