Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow frozen string literal as a safe raw SQL string #33330

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@kamipo
Copy link
Member

kamipo commented Jul 10, 2018

#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 betweenparams[: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
#27947.

Resolves #32995.

@@ -1134,7 +1134,7 @@ def preprocess_order_args(order_args)
order_args.flatten!

@klass.enforce_raw_sql_whitelist(
order_args.flat_map { |a| a.is_a?(Hash) ? a.keys : a },
order_args.reject { |a| a.is_a?(Hash) },

This comment has been minimized.

@kamipo

kamipo Jul 10, 2018

Author Member

Hash keys should be safe since always quoted with table name qualified by arel_attribute.

order_args.map! do |arg|
case arg
when Symbol
arel_attribute(arg).asc
when Hash
arg.map { |field, dir|
case field
when Arel::Nodes::SqlLiteral
field.send(dir.downcase)
else
arel_attribute(field).send(dir.downcase)
end
}
else
arg
end
end.flatten!

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Jul 10, 2018

If "#{user_input}" wasn't frozen, I think this would be reasonably safe.. but as it is, I think it goes too far in the other direction -- it allows too much for a comparatively small ergonomic gain.

@kamipo

This comment has been minimized.

Copy link
Member Author

kamipo commented Jul 10, 2018

IMO if an user is using Post.order("#{user_input}"), it is a problem on the user side not the Rails side, since Post.where("id = #{user_input}") is also a problem on the user side.

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Jul 10, 2018

It's the same problem as in #27947, though: with where, the caller is definitely passing a string-like value that they intend as an SQL fragment. With order, we can't tell whether they meant SQL or a column name.

@kamipo kamipo added the activerecord label Jul 13, 2018

@eugeneius

This comment has been minimized.

Copy link
Member

eugeneius commented Aug 19, 2018

Here's one way to distinguish "#{user_input}" from "length(title)" - intern a copy of the string, and check whether it returns the original:

# frozen_string_literal: true

dynamic = DATA.read
constant = "bar"
interpolated = "#{dynamic} DESC"

puts "[dynamic] frozen: #{dynamic.frozen?}, interned: #{dynamic.equal?(-dynamic.dup)}"
puts "[constant] frozen: #{constant.frozen?}, interned: #{constant.equal?(-constant.dup)}"
puts "[interpolated] frozen: #{interpolated.frozen?}, interned: #{interpolated.equal?(-interpolated.dup)}"

__END__
foo
[dynamic] frozen: false, interned: false
[constant] frozen: true, interned: true
[interpolated] frozen: true, interned: false

This works because if the string is a constant literal, it will have already been interned at compile time.

@simi

This comment has been minimized.

Copy link
Contributor

simi commented Sep 25, 2018

@eugeneius this works only for Ruby 2.5 (at least at my side).

@kamipo

This comment has been minimized.

Copy link
Member Author

kamipo commented Sep 25, 2018

Yes, this is a way to distinguish fstring or not. fstring is introduced at Ruby 2.5.

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Sep 25, 2018

Are there any consequences to polluting the fstring set? I think they still get GCed, so maybe it's okay?

It'd certainly be nice for upstream to directly expose what we need to know.

@eugeneius

This comment has been minimized.

Copy link
Member

eugeneius commented Sep 25, 2018

From the commit that introduced the behaviour (ruby/ruby@4e90dcc):

There is no security concern in this being a DoS vector by causing immortal strings. The fstring table is not a GC-root and not walked during the mark phase. GC-able dynamic symbols since Ruby 2.2 are handled in the same manner, and that implementation also relies on the non-immortality of fstrings.

@kamipo kamipo force-pushed the kamipo:allow_string_literal branch from f46b3f9 to 0ac7988 Sep 28, 2018

@aeroastro

This comment has been minimized.

Copy link
Contributor

aeroastro commented Oct 16, 2018

I'm not so familiar with this problem, but how about using Object#tainted? to distinguish params[:order] from length(title)?
This method seems more straightforward with support for interpolation.
https://ruby-doc.org/core-2.5.1/Object.html#method-i-tainted-3F

@kamipo

This comment has been minimized.

Copy link
Member Author

kamipo commented Oct 16, 2018

For now, all parsed strings in params are not tainted. So we need to mark all user inputs as tainted manually. We (I and my colleague @nurse) considered about tainted, but finally we concluded that denylist like way is hard and risky than allowlist like way.

@aeroastro

This comment has been minimized.

Copy link
Contributor

aeroastro commented Oct 16, 2018

Thank you for explaining the reason behind this.
Then, what about tainting all the parsed params at Rails framework layer instead of letting all the developers manually mark all of them. I mean tainting user input just as Ruby does for some user input like ARGV.
IMHO, this kind of issue indicates that it is about time to reconsider the definition of what is safe and what is unsafe.

@kamipo

This comment has been minimized.

Copy link
Member Author

kamipo commented Oct 16, 2018

As I said, marking all user inputs as tainted by us as Rails is hard and risky.

As you can see now that params is not tainted, even if HTTP payload is marked as tainted, the marks are easily lose before reaching people.

Since the marks are easily lose, the way that marking as denylist is risky.

@nurse

This comment has been minimized.

Copy link

nurse commented Oct 16, 2018

In Rails ecosystem, user input come through just query string, JSON, or extended by plugin.
At this time those plugins can ignore tainted state.
But if Rails handle that state, plugins must keep tainted state.
If they accidentally failed to keep it, now it considered as a security issue.
I couldn't believe Rails ecosystem can handle such sensitive treatment.

@kamipo kamipo force-pushed the kamipo:allow_string_literal branch from 0ac7988 to fe120e2 Feb 5, 2019

@rails-bot rails-bot bot added the activesupport label Feb 5, 2019

Allow frozen string literal as a safe raw SQL string
#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
#27947.

Resolves #32995.

@kamipo kamipo force-pushed the kamipo:allow_string_literal branch from fe120e2 to 8188298 Feb 5, 2019

@kamipo kamipo removed the activesupport label Feb 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.