Disallow raw SQL in dangerous AR methods #27947
Conversation
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @eileencodes (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review. Please see the contribution instructions for more information. |
cc/ @rails/security |
I am Thanks for working on this! |
# rename methods like #order to #unsafe_raw_order and will validate | ||
# arguments to #order. A value of false will do nothing. | ||
mattr_accessor :guard_unsafe_raw_sql, instance_writer: false | ||
self.guard_unsafe_raw_sql = false |
eileencodes
Feb 8, 2017
Member
I think this would be better as allow_unsafe_raw_sql
so that it's more likely to motivate someone to use it.
I think this would be better as allow_unsafe_raw_sql
so that it's more likely to motivate someone to use it.
jeremy
Feb 9, 2017
Member
Agree it should be opt-out by using the unsafe_*
API.
However, for compatibility with existing code, would expect to be able to turn off checks on a model superclass: SomeEngine::Model.guard_unsafe_raw_sql = false
without affecting app models or other engines.
Agree it should be opt-out by using the unsafe_*
API.
However, for compatibility with existing code, would expect to be able to turn off checks on a model superclass: SomeEngine::Model.guard_unsafe_raw_sql = false
without affecting app models or other engines.
# | ||
# Person.effective_column_names | ||
# # => ["id", "created_at", "updated_at", "name", "age"] | ||
def effective_column_names |
eileencodes
Feb 8, 2017
Member
I think this method name is doesn't really explain what it's doing, but I haven't been able to come up with a better name yet. safe_column_names
? attribute_column_names
?
I think this method name is doesn't really explain what it's doing, but I haven't been able to come up with a better name yet. safe_column_names
? attribute_column_names
?
jeremy
Feb 9, 2017
Member
attribute_names_and_aliases
?
attribute_names_and_aliases
?
if has_include?(column_names.first) | ||
construct_relation_for_association_calculations.pluck(*column_names) | ||
else | ||
if ActiveRecord::Base.guard_unsafe_raw_sql && !@klass.effective_column_name?(column_names) |
eileencodes
Feb 8, 2017
Member
I'd consider moving this into a method of it's own column_valid?
or something like that.
I'd consider moving this into a method of it's own column_valid?
or something like that.
end | ||
end | ||
|
||
def unsafe_raw_pluck(*column_names) |
eileencodes
Feb 8, 2017
Member
Should this have documentation?
Should this have documentation?
Cool! |
# | ||
# Person.effective_column_names | ||
# # => ["id", "created_at", "updated_at", "name", "age"] | ||
def effective_column_names |
jeremy
Feb 9, 2017
Member
attribute_names_and_aliases
?
attribute_names_and_aliases
?
def effective_column_names | ||
@effective_column_names ||= begin | ||
(attribute_names + attribute_aliases.keys).uniq | ||
end |
jeremy
Feb 9, 2017
Member
Can drop the begin/end
Can drop the begin/end
end | ||
end | ||
|
||
def effective_column_name?(*names) |
jeremy
Feb 9, 2017
Member
"Can I ask this model for attributes with these names?"
Person.respond_to_attributes? :foo, :bar
"Can I ask this model for attributes with these names?"
Person.respond_to_attributes? :foo, :bar
end | ||
|
||
def effective_column_name?(*names) | ||
(names.flatten.map(&:to_s) - effective_column_names).empty? |
jeremy
Feb 9, 2017
Member
When names
is typically small, would expect that iterating through those and checking whether they're attributes would be quicker than subtracting each attr: names.all? { |name| has_attribute?(name) || has_attribute_alias?(name) }
When names
is typically small, would expect that iterating through those and checking whether they're attributes would be quicker than subtracting each attr: names.all? { |name| has_attribute?(name) || has_attribute_alias?(name) }
jeremy
Feb 9, 2017
Member
(That'd obviate the effective_column_names
method, too)
(That'd obviate the effective_column_names
method, too)
return records.pluck(*column_names) | ||
end | ||
|
||
if has_include?(column_names.first) |
jeremy
Feb 9, 2017
Member
Can drop the early return
above and make this an elsif
Can drop the early return
above and make this an elsif
hash = args.last.is_a?(Hash) ? args.pop : {} | ||
directions.concat(hash.values) | ||
columns.concat(hash.keys) | ||
columns.concat(args) |
jeremy
Feb 9, 2017
Member
Can do the assignments above here:
orderings = args.extract_options!
attribute_names = args | orderings.keys
Can do the assignments above here:
orderings = args.extract_options!
attribute_names = args | orderings.keys
raise ArgumentError, "Invalid order column: #{bad_columns}" | ||
end | ||
|
||
bad_directions = directions.map(&:to_s) - %w(asc desc ASC DESC) |
jeremy
Feb 9, 2017
Member
Rather than assign directions
above, it's clear to ask for orderings.values
here.
Rather than assign directions
above, it's clear to ask for orderings.values
here.
@@ -98,6 +98,13 @@ def self.error_on_ignored_order_or_limit=(value) | |||
self.error_on_ignored_order = value | |||
end | |||
|
|||
# :singleton-method: | |||
# Specify the behavior for unsafe raw query methods. A value of true will |
jeremy
Feb 9, 2017
Member
I get "unsafe," but how are these "raw?"
I get "unsafe," but how are these "raw?"
btoews
Feb 9, 2017
Author
Contributor
I went with unsafe_raw
because 1) they are unsafe and 2) they allow raw SQL. I'd be happy to drop the raw
bit if you'd like.
I went with unsafe_raw
because 1) they are unsafe and 2) they allow raw SQL. I'd be happy to drop the raw
bit if you'd like.
ensure | ||
ActiveRecord::Base.guard_unsafe_raw_sql = old_value | ||
end | ||
end |
jeremy
Feb 9, 2017
Member
Nice.
Coverage for the new unsafe_*
API, too?
Nice.
Coverage for the new unsafe_*
API, too?
# rename methods like #order to #unsafe_raw_order and will validate | ||
# arguments to #order. A value of false will do nothing. | ||
mattr_accessor :guard_unsafe_raw_sql, instance_writer: false | ||
self.guard_unsafe_raw_sql = false |
jeremy
Feb 9, 2017
Member
Agree it should be opt-out by using the unsafe_*
API.
However, for compatibility with existing code, would expect to be able to turn off checks on a model superclass: SomeEngine::Model.guard_unsafe_raw_sql = false
without affecting app models or other engines.
Agree it should be opt-out by using the unsafe_*
API.
However, for compatibility with existing code, would expect to be able to turn off checks on a model superclass: SomeEngine::Model.guard_unsafe_raw_sql = false
without affecting app models or other engines.
rename config option. fix tests. s/effective_column_names/attribute_names_and_aliases/ s/effective_column_name?/attribute_name_or_alias?/ move pluck column checking logic into its own method document unsafe_raw methods💄 s/attribute_name_or_alias?/respond_to_attributes?/ optimize respond_to_attributes? remove attribute_names_and_aliases💄 typo dedup pluck/unsafe_raw_pluck logic with private method only call restrict_order_args unless unsafe-raw enabled💄 refactor a bit typo test unsafe_raw methods too (and fix them)
Good call. I should have known there would be other crazy syntaxes |
I couldn't find any existing list of order modifiers that included |
Paraphrasing via @matthewd: We could use Then we could do a few modes for unsafe query checking: strict (new apps, raise on violations), deprecated (default, warn on violations that didn't use SQL literals), false (off, shush). |
records.pluck(*column_names) | ||
elsif has_include?(column_names.first) | ||
construct_relation_for_association_calculations.pluck(*column_names) | ||
elsif unsafe_raw || unrecognized.none? |
jeremy
Feb 9, 2017
Member
Feels like this is a conditional checking the guard condition, but it's actually checking whether not to guard, leaving the raise in the else
disconnected from the decisionmaking that leads to it. Compare with:
elsif !allow_unsafe_raw && unrecognized.any?
raise …
else
# actual pluck impl
end
Feels like this is a conditional checking the guard condition, but it's actually checking whether not to guard, leaving the raise in the else
disconnected from the decisionmaking that leads to it. Compare with:
elsif !allow_unsafe_raw && unrecognized.any?
raise …
else
# actual pluck impl
end
raise ArgumentError, "Invalid order direction: #{unrecognized}" | ||
end | ||
end | ||
|
jeremy
Feb 9, 2017
Member
👍
# TODO: find a better list of modifiers. | ||
unrecognized = orderings.values.reject { |d| VALID_DIRECTIONS.include?(d.to_s) } | ||
if unrecognized.any? | ||
raise ArgumentError, "Invalid order direction: #{unrecognized}" |
jeremy
Feb 9, 2017
Member
Consider unrecognized.inspect
so the message reflects the string/symbol syntax from the caller.
Consider unrecognized.inspect
so the message reflects the string/symbol syntax from the caller.
Think rolling with |
My inexperience working with AR is showing: I don't follow that statement at all |
My bad Boils down to this: >> Arel.sql('foo')
=> "foo"
>> Arel.sql('foo').class
=> Arel::Nodes::SqlLiteral If we get an ordering that's an Example unrecognized = column_names.reject do |cn|
cn.is_a?(Arel::Nodes::SqlLiteral) || @klass.respond_to_attribute?(cn)
end and unrecognized = orderings.values.reject do |d|
d.is_a?(Arel::Nodes::SqlLiteral) || VALID_DIRECTIONS.include?(d)
end To use unsafe raw SQL, you'd do Finally, to do a deprecation to encourage moves to Arel.sql style without breaking existing code, we could insert warnings like unrecognized = orderings.values.reject do |d|
d.is_a?(Arel::Nodes::SqlLiteral) || VALID_DIRECTIONS.include?(d) ||
(@klass.allow_unsafe_raw_sql == :deprecated).tap do |warn|
ActiveSupport::Deprecation.warn "Ordering in the #{d.inspect} direction is unrecognized and considered unsafe, vulnerable to SQL injection. Switch to `Arel.sql(#{d.to_s.inspect})` if this is intentional custom SQL ordering." if warn
end
end |
Tiny logic tweak: we should just let any Arel node through |
Sorry to interrupt, but I feel like this is putting the cart before the horse. It's relative common to pass raw SQL snippets into Example use case: class Game < ActiveRecord::Base
# id - integer
# platform_id - integer
# title - string
# ...
belongs_to :platform
end
class Platform < ActiveRecord::Base
# id - integer
# name - string
# ...
has_many :games
end If we want to display a table of games sorted by their platform, we have to do this in our code: @games = Game.joins(:platform).order("#{Platform.quoted_table_name}.name ASC") While I'm pretty sure most developers would love to instead be able to do this (similar to how @games = Game.joins(:platform).order(platforms: {name: :asc}) Even if the change is to allow all |
@t27duck I'm not sure what you're suggesting. Surely any change that addresses the core problem (that the method accepts raw SQL, and that callers might reasonably not account for that) is going to require some change to existing callers? |
Point taken about any changes to the public API would require developers to adjust. Perhaps I'm not 100% sold on blocking out raw strings to the interface. What if, instead, To your suggestion on allowing @posts = Post.order(Arel::Notes::SqlLiteral.new(params[:order_by])) All it would take is one person asking on stackoverflow "Why I can't pass a string into |
We need to protect against model attributes in addition to parameters.
This is why I'd rather prefix dangerous method names with |
Same for bypassing strong params, HTML safety, and any other security measure. "Why can't I pass a string to #order?" → "Oh just change that to #unsafe_raw_order." Of course there is a spectrum from "cuts hand on contact" to "can shoot self in foot" to "impossible to use in error," so we need to keep our heads on straight about where we land on that spectrum and why. Foremost, provide a fluent, sensible, obvious interface that Rails developers are happy to use and is secure in default, typical usage. Secondarily, make it awkward for users to inadvertently construct a foot-gun from otherwise-safe tools. People copy/pasting senseless API usage that bypasses the secure default? These are not the developers we want to hedge against when crafting our API. In this case, exposing a single, safe set of relation builder methods is a great way to tackle both. Arguments are allowlisted by default, which catches inadvertent injection vulns, but that can be explicitly bypassed with raw SQL when needed. That's right in the sweet spot of the Active Record way: safe abstraction plus a moderately-vinegared bypass. |
It's pretty obvious that Also I'd like to know if |
Ref: rails/rails#27947 - Use Arel objects instead of Strings whenever we can - Allow Arel objects with custom_order option - Add tests for Arel and Proc custom_orders - Update readme and changelog
rails#27947 was intended to address `Post.order(params[:order])` which leading to SQL injection. But the protection caused very annoying warnings, especially for calling by a string literal which invoke a function like `Post.order("length(title)")`. Calling by a string literal should be safe since it is not an user input. So I'd like to allow string literals as a safe raw SQL string. There is no reliable way to distinguish between`params[:order]` and `"length(title)"`, but at least `params[:order]` as an user input is always a mutable string. So I think that we can add frozen strings/objects to the whitelist to mitigate the annoying warnings without losing the original intention of rails#27947. Resolves rails#32995.
- Followup of rails/rails#27947 - Silences bunch of deprecation warnings.
- Followup of rails/rails#27947 - Silences bunch of deprecation warnings.
@harshal0608 this change adds an extra confirmation of intent for |
rails#27947 was intended to address `Post.order(params[:order])` which leading to SQL injection. But the protection caused very annoying warnings, especially for calling by a string literal which invoke a function like `Post.order("length(title)")`. Calling by a string literal should be safe since it is not an user input. So I'd like to allow string literals as a safe raw SQL string. There is no reliable way to distinguish between`params[:order]` and `"length(title)"`, but at least `params[:order]` as an user input is always a mutable string. So I think that we can add frozen strings/objects to the whitelist to mitigate the annoying warnings without losing the original intention of rails#27947. Resolves rails#32995.
rails#27947 was intended to address `Post.order(params[:order])` which leading to SQL injection. But the protection caused very annoying warnings, especially for calling by a string literal which invoke a function like `Post.order("length(title)")`. Calling by a string literal should be safe since it is not an user input. So I'd like to allow string literals as a safe raw SQL string. There is no reliable way to distinguish between`params[:order]` and `"length(title)"`, but at least `params[:order]` as an user input is always a mutable string. So I think that we can add frozen strings/objects to the whitelist to mitigate the annoying warnings without losing the original intention of rails#27947. Resolves rails#32995.
rails#27947 was intended to address `Post.order(params[:order])` which leading to SQL injection. But the protection caused very annoying warnings, especially for calling by a string literal which invoke a function like `Post.order("length(title)")`. Calling by a string literal should be safe since it is not an user input. So I'd like to allow string literals as a safe raw SQL string. There is no reliable way to distinguish between`params[:order]` and `"length(title)"`, but at least `params[:order]` as an user input is always a mutable string. So I think that we can add frozen strings/objects to the whitelist to mitigate the annoying warnings without losing the original intention of rails#27947. Resolves rails#32995.
rails#27947 was intended to address `Post.order(params[:order])` which leading to SQL injection. But the protection caused very annoying warnings, especially for calling by a string literal which invoke a function like `Post.order("length(title)")`. Calling by a string literal should be safe since it is not an user input. So I'd like to allow string literals as a safe raw SQL string. There is no reliable way to distinguish between `params[:order]` and `"length(title)"`, but at least `params[:order]` as an user input is always a mutable string, and `"length(title)"` will be a fstring if `# frozen_string_literal: true`. So I think that we can add fstring to the allowlist to mitigate the annoying warnings without losing the original intention of rails#27947. Resolves rails#32995.
rails#27947 was intended to address `Post.order(params[:order])` which leading to SQL injection. But the protection caused very annoying warnings, especially for calling by a string literal which invoke a function like `Post.order("length(title)")`. Calling by a string literal should be safe since it is not an user input. So I'd like to allow string literals as a safe raw SQL string. There is no reliable way to distinguish between `params[:order]` and `"length(title)"`, but at least `params[:order]` as an user input is always a mutable string, and `"length(title)"` will be a fstring if `# frozen_string_literal: true`. So I think that we can add fstring to the allowlist to mitigate the annoying warnings without losing the original intention of rails#27947. Resolves rails#32995.
rails#27947 was intended to address `Post.order(params[:order])` which leading to SQL injection. But the protection caused very annoying warnings, especially for calling by a string literal which invoke a function like `Post.order("length(title)")`. Calling by a string literal should be safe since it is not an user input. So I'd like to allow string literals as a safe raw SQL string. There is no reliable way to distinguish between `params[:order]` and `"length(title)"`, but at least `params[:order]` as an user input is always a mutable string, and `"length(title)"` will be a fstring if `# frozen_string_literal: true`. So I think that we can add fstring to the allowlist to mitigate the annoying warnings without losing the original intention of rails#27947. Resolves rails#32995.
rails#27947 was intended to address `Post.order(params[:order])` which leading to SQL injection. But the protection caused very annoying warnings, especially for calling by a string literal which invoke a function like `Post.order("length(title)")`. Calling by a string literal should be safe since it is not an user input. So I'd like to allow string literals as a safe raw SQL string. There is no reliable way to distinguish between `params[:order]` and `"length(title)"`, but at least `params[:order]` as an user input is always a mutable string, and `"length(title)"` will be a fstring if `# frozen_string_literal: true`. So I think that we can add fstring to the allowlist to mitigate the annoying warnings without losing the original intention of rails#27947. Resolves rails#32995.
Rails 5.2 adds deprecation warnings for several forms of SQL in ActiveRecord queries, which Rails 6 will no longer allow. As part of the ongoing work to upgrade to Rails 6, we want to address these warnings. Mostly, this involves wrapping SQL calls which we know are safe (ie, calls that don't reference any user-editable model fields) in `Arel.sql`; in some cases, we're also able to replace the SQL with built-in ActiveRecord methods. - [Rails 5.2 stops some raw SQL, prevents SQL injections](https://blog.bigbinary.com/2018/10/16/rails-5-2-disallows-raw-sql-in-active-record.html) - [Disallow raw SQL in dangerous AR methods](rails/rails#27947)
Rails 5.2 adds deprecation warnings for several forms of SQL in ActiveRecord queries, which Rails 6 will no longer allow. As part of the ongoing work to upgrade to Rails 6, we want to address these warnings. Mostly, this involves wrapping SQL calls which we know are safe (ie, calls that don't reference any user-editable model fields) in `Arel.sql`; in some cases, we're also able to replace the SQL with built-in ActiveRecord methods. - [Rails 5.2 stops some raw SQL, prevents SQL injections](https://blog.bigbinary.com/2018/10/16/rails-5-2-disallows-raw-sql-in-active-record.html) - [Disallow raw SQL in dangerous AR methods](rails/rails#27947)
rails#27947 was intended to address `Post.order(params[:order])` which leading to SQL injection. But the protection caused very annoying warnings, especially for calling by a string literal which invoke a function like `Post.order("length(title)")`. Calling by a string literal should be safe since it is not an user input. So I'd like to allow string literals as a safe raw SQL string. There is no reliable way to distinguish between `params[:order]` and `"length(title)"`, but at least `params[:order]` as an user input is always a mutable string, and `"length(title)"` will be a fstring if `# frozen_string_literal: true`. So I think that we can add fstring to the allowlist to mitigate the annoying warnings without losing the original intention of rails#27947. Resolves rails#32995.
rails#27947 was intended to address `Post.order(params[:order])` which leading to SQL injection. But the protection caused very annoying warnings, especially for calling by a string literal which invoke a function like `Post.order("length(title)")`. Calling by a string literal should be safe since it is not an user input. So I'd like to allow string literals as a safe raw SQL string. There is no reliable way to distinguish between `params[:order]` and `"length(title)"`, but at least `params[:order]` as an user input is always a mutable string, and `"length(title)"` will be a fstring if `# frozen_string_literal: true`. So I think that we can add fstring to the allowlist to mitigate the annoying warnings without losing the original intention of rails#27947. Resolves rails#32995.
Some AR methods allow raw SQL where raw SQL is rarely necessary. This opens up applications to SQL injection vulnerabilities when a developer unwittingly passes params or a model attribute to the AR method.
Code like this looks innocuous, though it actually is an exploitable SQL injection vulnerability:
A probably-not-comprehensive list of such dangerous APIs can be found at http://rails-sqli.org/.
These APIs should be changed to not allow raw SQL. In the rare case where raw SQL is needed, the developer should be forced to acknowledge the risk by using an API whose name indicates the danger.
This would be a breaking change though, which I'm assuming would not be acceptable. This PR adds an AR config flag that, when enabled, restricts the arguments that can be passed to previously dangerous AR APIs. The previous functionality can be accessed using
unsafe_raw_
prefixed APIs.These changes take a bit of work, so I'm opening this PR early for discussion. So far, I've made changes to the
#pluck
,#order
and#reorder
APIs.I'll be curious to hear what folks think about the idea. I'm happy to try a different approach if anyone has suggestions, but it would be really good to remove some sharp edges from AR.