Skip to content

Commit

Permalink
Merge pull request #27947 from mastahyeti/unsafe_raw_sql
Browse files Browse the repository at this point in the history
Disallow raw SQL in dangerous AR methods
  • Loading branch information
matthewd committed Nov 14, 2017
2 parents ed10016 + 4a5b3ca commit a1ee43d
Show file tree
Hide file tree
Showing 13 changed files with 462 additions and 33 deletions.
30 changes: 30 additions & 0 deletions activerecord/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,33 @@
* Require raw SQL fragments to be explicitly marked when used in
relation query methods.

Before:
```
Article.order("LENGTH(title)")
```

After:
```
Article.order(Arel.sql("LENGTH(title)"))
```

This prevents SQL injection if applications use the [strongly
discouraged] form `Article.order(params[:my_order])`, under the
mistaken belief that only column names will be accepted.

Raw SQL strings will now cause a deprecation warning, which will
become an UnknownAttributeReference error in Rails 6.0. Applications
can opt in to the future behavior by setting `allow_unsafe_raw_sql`
to `:disabled`.

Common and judged-safe string values (such as simple column
references) are unaffected:
```
Article.order("title DESC")
```

*Ben Toews*

* `update_all` will now pass its values to `Type#cast` before passing them to
`Type#serialize`. This means that `update_all(foo: 'true')` will properly
persist a boolean.
Expand Down
40 changes: 40 additions & 0 deletions activerecord/lib/active_record/attribute_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,46 @@ def attribute_names
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:

This comment has been minimized.

Copy link
@bf4

bf4 Jan 20, 2020

Contributor

would be nice to be able to safe-list expected jsonb attributes so I don't get

called with non-attribute argument(s): "details->>'invoice_version' AS invoice_version, created_at"

looking at #33330 would address this without anything fancy

seems 7696f44#diff-0689d847b102af6667885c014182e48f moves in this direction a bit by moving some of the config out of a constant and into connection.column_name_with_order_matcher, (even though that is itself a reference to a constant, at least it's adapter specific)

I can make a formal issue. I don't think there is one right now

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
Expand Down
8 changes: 8 additions & 0 deletions activerecord/lib/active_record/core.rb
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,14 @@ def self.configurations
# 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
Expand Down
25 changes: 25 additions & 0 deletions activerecord/lib/active_record/errors.rb
Original file line number Diff line number Diff line change
Expand Up @@ -342,4 +342,29 @@ class TransactionTimeout < StatementInvalid
# 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
1 change: 1 addition & 0 deletions activerecord/lib/active_record/relation/calculations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ def pluck(*column_names)
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
Expand Down
14 changes: 13 additions & 1 deletion activerecord/lib/active_record/relation/query_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,7 @@ def order(*args)
spawn.order!(*args)
end

# Same as #order but operates on relation in-place instead of copying.
def order!(*args) # :nodoc:
preprocess_order_args(args)

Expand All @@ -316,6 +317,7 @@ def reorder(*args)
spawn.reorder!(*args)
end

# Same as #reorder but operates on relation in-place instead of copying.
def reorder!(*args) # :nodoc:
preprocess_order_args(args)

Expand Down Expand Up @@ -1076,7 +1078,7 @@ def reverse_sql_order(order_query)
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
Expand All @@ -1085,6 +1087,10 @@ def reverse_sql_order(order_query)
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.
Expand Down Expand Up @@ -1118,6 +1124,12 @@ def preprocess_order_args(order_args)
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)
Expand Down
12 changes: 11 additions & 1 deletion activerecord/lib/active_record/sanitization.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,17 @@ def sanitize_sql_for_assignment(assignments, default_table_name = table_name) #
# # => "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
Expand Down
10 changes: 5 additions & 5 deletions activerecord/test/cases/associations/eager_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,7 @@ def test_eager_association_loading_with_belongs_to_and_order_string_with_unquote
def test_eager_association_loading_with_belongs_to_and_order_string_with_quoted_table_name
quoted_posts_id = Comment.connection.quote_table_name("posts") + "." + Comment.connection.quote_column_name("id")
assert_nothing_raised do
Comment.includes(:post).references(:posts).order(quoted_posts_id)
Comment.includes(:post).references(:posts).order(Arel.sql(quoted_posts_id))
end
end

Expand Down Expand Up @@ -874,14 +874,14 @@ def test_limited_eager_with_order
posts(:thinking, :sti_comments),
Post.all.merge!(
includes: [:author, :comments], where: { "authors.name" => "David" },
order: "UPPER(posts.title)", limit: 2, offset: 1
order: Arel.sql("UPPER(posts.title)"), limit: 2, offset: 1
).to_a
)
assert_equal(
posts(:sti_post_and_comments, :sti_comments),
Post.all.merge!(
includes: [:author, :comments], where: { "authors.name" => "David" },
order: "UPPER(posts.title) DESC", limit: 2, offset: 1
order: Arel.sql("UPPER(posts.title) DESC"), limit: 2, offset: 1
).to_a
)
end
Expand All @@ -891,14 +891,14 @@ def test_limited_eager_with_multiple_order_columns
posts(:thinking, :sti_comments),
Post.all.merge!(
includes: [:author, :comments], where: { "authors.name" => "David" },
order: ["UPPER(posts.title)", "posts.id"], limit: 2, offset: 1
order: [Arel.sql("UPPER(posts.title)"), "posts.id"], limit: 2, offset: 1
).to_a
)
assert_equal(
posts(:sti_post_and_comments, :sti_comments),
Post.all.merge!(
includes: [:author, :comments], where: { "authors.name" => "David" },
order: ["UPPER(posts.title) DESC", "posts.id"], limit: 2, offset: 1
order: [Arel.sql("UPPER(posts.title) DESC"), "posts.id"], limit: 2, offset: 1
).to_a
)
end
Expand Down
10 changes: 5 additions & 5 deletions activerecord/test/cases/calculations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -663,14 +663,14 @@ def test_pluck_not_auto_table_name_prefix_if_column_joined
end

def test_pluck_with_selection_clause
assert_equal [50, 53, 55, 60], Account.pluck("DISTINCT credit_limit").sort
assert_equal [50, 53, 55, 60], Account.pluck("DISTINCT accounts.credit_limit").sort
assert_equal [50, 53, 55, 60], Account.pluck("DISTINCT(credit_limit)").sort
assert_equal [50, 53, 55, 60], Account.pluck(Arel.sql("DISTINCT credit_limit")).sort
assert_equal [50, 53, 55, 60], Account.pluck(Arel.sql("DISTINCT accounts.credit_limit")).sort
assert_equal [50, 53, 55, 60], Account.pluck(Arel.sql("DISTINCT(credit_limit)")).sort

# MySQL returns "SUM(DISTINCT(credit_limit))" as the column name unless
# an alias is provided. Without the alias, the column cannot be found
# and properly typecast.
assert_equal [50 + 53 + 55 + 60], Account.pluck("SUM(DISTINCT(credit_limit)) as credit_limit")
assert_equal [50 + 53 + 55 + 60], Account.pluck(Arel.sql("SUM(DISTINCT(credit_limit)) as credit_limit"))
end

def test_plucks_with_ids
Expand Down Expand Up @@ -772,7 +772,7 @@ def test_pluck_loaded_relation_sql_fragment
companies = Company.order(:name).limit(3).load

assert_queries 1 do
assert_equal ["37signals", "Apex", "Ex Nihilo"], companies.pluck("DISTINCT name")
assert_equal ["37signals", "Apex", "Ex Nihilo"], companies.pluck(Arel.sql("DISTINCT name"))
end
end

Expand Down
4 changes: 2 additions & 2 deletions activerecord/test/cases/finder_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ def test_exists_with_order_and_distinct

# Ensure +exists?+ runs without an error by excluding order value.
def test_exists_with_order
assert_equal true, Topic.order("invalid sql here").exists?
assert_equal true, Topic.order(Arel.sql("invalid sql here")).exists?
end

def test_exists_with_joins
Expand Down Expand Up @@ -652,7 +652,7 @@ def test_last_on_loaded_relation_should_not_use_sql

def test_last_with_irreversible_order
assert_raises(ActiveRecord::IrreversibleOrderError) do
Topic.order("coalesce(author_name, title)").last
Topic.order(Arel.sql("coalesce(author_name, title)")).last
end
end

Expand Down
Loading

0 comments on commit a1ee43d

Please sign in to comment.