Feature #7839

Symbol.freeze_symbols

Added by Aaron Patterson about 1 year ago. Updated about 1 month ago.

[ruby-core:52165]
Status:Rejected
Priority:Normal
Assignee:Yukihiro Matsumoto
Category:core
Target version:next minor

Description

Hi,

On team Rails, we're having troubles with Symbol creation DoS attacks. From our perspective, there should be a point in the application where symbols should stabilize, meaning we don't expect the number of symbols to increase while the process is running.

I'd like to be able to call a method like Symbol.freeze_symbols which would essentially freeze the symbol hash, such that if any new symbols are created, an exception would be thrown.

I can work on a patch for this, but I wanted to throw the idea out there.


Related issues

Related to ruby-trunk - Feature #7854: New method Symbol[string] Closed 02/15/2013

History

#1 Updated by Rodrigo Rosenfeld Rosas about 1 year ago

I really believe that letting the symbols to be garbage collected (#7791) is the way to go here.

You have mentioned that changing YAML#load to be safe by default would break existing apps and now you suggest that it would be safe to freeze symbols at some point as if it wouldn't break existing apps relying on marshalling behavior of Ruby libraries. Could you please explain better why do you think it is ok to freeze symbols in Rails and break existing apps but it is not ok to make YAML#load safe (preferring a new safe_load method instead)?

#2 Updated by Matthew Kerwin about 1 year ago

rosenfeld (Rodrigo Rosenfeld Rosas) wrote:

Could you please explain better why do you think it is ok to freeze symbols in Rails and break existing apps but it is not ok to make YAML#load safe (preferring a new safe_load method instead)?

Users can choose whether or not to call Symbol.freezesymbols , and 100% of current apps do not call it, so those apps will continue to function exactly as they always have until they are updated. Similary, they can choose to update their apps to use YAML#safeload instead of #load.

Changing the behaviour of YAML#load would change the behaviour of existing apps without any opt-in from their maintainers.

#3 Updated by Rodrigo Rosenfeld Rosas about 1 year ago

yeah, but if Rails calls Symbol.freeze_symbols it could break all Rails applications relying on YAML#load as an unmarshall method. Are you suggesting that Rails shouldn't care about breaking existing Rails apps but that Ruby should care about breaking existing Ruby apps?

#4 Updated by Eric Wong about 1 year ago

"tenderlovemaking (Aaron Patterson)" aaron@tenderlovemaking.com wrote:

I'd like to be able to call a method like Symbol.freeze_symbols
which would essentially freeze the symbol hash, such that if any new
symbols are created, an exception would be thrown.

How about the option to do a soft freeze which issues a warning instead
of exception? (but support both). Start using soft freeze, and move to
a real freeze later when apps/gems are fixed.

#5 Updated by Matthew Kerwin about 1 year ago

rosenfeld (Rodrigo Rosenfeld Rosas) wrote:

yeah, but if Rails calls Symbol.freeze_symbols it could break all Rails applications relying on YAML#load as an unmarshall method. Are you suggesting that Rails shouldn't care about breaking existing Rails apps but that Ruby should care about breaking existing Ruby apps?

Nobody said rails would call it. How would rails know when your particular app's symbols have stabilised? The biggest automation I'd have expected would be that they provide a trigger hook, so you can instruct rails/ruby to freeze the table when some condition is met. If they were going to add an automagic trigger at, for example, "all classes loaded" (whatever that means), surely it would be an opt-in configuration-driven behaviour.

#6 Updated by Matthew Kerwin about 1 year ago

normalperson (Eric Wong) wrote:

"tenderlovemaking (Aaron Patterson)" aaron@tenderlovemaking.com wrote:

I'd like to be able to call a method like Symbol.freeze_symbols
which would essentially freeze the symbol hash, such that if any new
symbols are created, an exception would be thrown.

How about the option to do a soft freeze which issues a warning instead
of exception? (but support both). Start using soft freeze, and move to
a real freeze later when apps/gems are fixed.

Also, would you expect to be able to thaw it out again? It might be enough in the short term to, e.g.
begin
Symbol.freezesymbols
YAML.load(...)
ensure
Symbol.thaw
symbols
end

#7 Updated by Shugo Maeda about 1 year ago

phluid61 (Matthew Kerwin) wrote:

Also, would you expect to be able to thaw it out again? It might be enough in the short term to, e.g.
begin
Symbol.freezesymbols
YAML.load(...)
ensure
Symbol.thaw
symbols
end

If this is a main use case of Symbol.freezesymbols, it might be better to have String#intern's option to control whether a symbol creation is allowed and to make YAML.safeload to use it.

:foo
"foo".intern #=> :foo
"bar".intern #=> :bar
"foo".intern(allownew: false) #=> :foo
"bar".intern(allow
new: false) #=> error

I guess it can be implemented easily compared to Symbol GC.

#8 Updated by Anonymous about 1 year ago

On Wed, Feb 13, 2013 at 11:16:31AM +0900, shugo (Shugo Maeda) wrote:

Issue #7839 has been updated by shugo (Shugo Maeda).

phluid61 (Matthew Kerwin) wrote:

Also, would you expect to be able to thaw it out again? It might be enough in the short term to, e.g.
begin
Symbol.freezesymbols
YAML.load(...)
ensure
Symbol.thaw
symbols
end

I think having a freeze and thaw would be fine.

If this is a main use case of Symbol.freezesymbols, it might be better to have String#intern's option to control whether a symbol creation is allowed and to make YAML.safeload to use it.

:foo
"foo".intern #=> :foo
"bar".intern #=> :bar
"foo".intern(allownew: false) #=> :foo
"bar".intern(allow
new: false) #=> error

The problem with this is we can be calling foreign code. We have to
force all library authors to use it. Library authors may not expect
that calls to ".intern" will be fed user input.

Most security issues we have to deal with (even the YAML example) are
cases where we do not expect to process foreign input.

I guess it can be implemented easily compared to Symbol GC.

Even freezing and thawing would definitely be easier than Symbol GC.

--
Aaron Patterson
http://tenderlovemaking.com/

#9 Updated by Shugo Maeda about 1 year ago

If this is a main use case of Symbol.freezesymbols, it might be better to have String#intern's option to control whether a symbol creation is allowed and to make YAML.safeload to use it.
(snip)
The problem with this is we can be calling foreign code. We have to
force all library authors to use it. Library authors may not expect
that calls to ".intern" will be fed user input.

Most security issues we have to deal with (even the YAML example) are
cases where we do not expect to process foreign input.

Hmm.... I'm worried that the following code is not thread safe.

begin
  Symbol.freeze_symbols
  YAML.load(...)
ensure
  Symbol.thaw_symbols
end

I guess it can be implemented easily compared to Symbol GC.

Even freezing and thawing would definitely be easier than Symbol GC.

Sasada-san might implement Symbol GC for MRI.

So I'm worried that he might get less popular with women.

If it can be implemented in other implementations, it would be the best solution.

#10 Updated by Nathan Zook about 1 year ago

Creating global state has global implications. Yes, there are threading implications. Yes, there are issues with called libraries. But what really annoys me is yet again some software providers (again from the Rails team) that thinks he understands everything that users of his software might do.

I'm NOT just talking about poorly designed code. Honestly, I think that normal Rails behaviour is not expected to match this. I know that dynamic finders are supposed to be passe', but if Rails continues to implement ANY dynamic method generation that happens at runtime, then there is no way to know when the particular piece of code that does the generation gets called. In fact, this entire conversation got started because the Rails framework intentionally generated symbols during runtime. That's not to mention (Rails) application code doing so for perfectly reasonable conditions.

I very much favour making the symbol table queriable. (#7795) Let YAML.safeload check it. Let tojson(:safe => true) check it. But don't break existing code simply because you think your global solution is going to make life good for everyone.

#11 Updated by Alexey Muranov about 1 year ago

Sorry about a naïve idea, but what would you say about prohibiting interning tainted strings instead?

#12 Updated by Yutaka HARA about 1 year ago

  • Category set to core
  • Target version set to next minor

#13 Updated by Nathan Zook about 1 year ago

+1 to prohibiting interning of tainted strings. Probably $SAFE >= 1, though. Currently, this is a problem for $SAFE <= 2.

#14 Updated by Anonymous about 1 year ago

On Wed, Feb 13, 2013 at 05:15:25PM +0900, alexeymuranov (Alexey Muranov) wrote:

Issue #7839 has been updated by alexeymuranov (Alexey Muranov).

Sorry about a naïve idea, but what would you say about prohibiting interning tainted strings instead?

It seems like a good idea, except that tainting is not reliable. For
example:

irb(main):001:0> require 'json'
=> true
irb(main):002:0> userinput = "{\"foo\":\"bar\"}".taint
=> "{\"foo\":\"bar\"}"
irb(main):003:0> user
input.tainted?
=> true
irb(main):004:0> params = JSON.parse user_input
=> {"foo"=>"bar"}
irb(main):005:0> params['foo'].tainted?
=> false

In this case, a tainted string becomes untainted. I guess it's OK for
tainted strings to become untainted, but how is someone supposed to
verify if the JSON is safe without first parsing the JSON before sending
the JSON to the JSON parser? My head is spinning.

C extension authors need to use rbtaintedstr*, but most of them just
use rbstrnew. You can increase $SAFE high enough that all objects are
tainted on creation, but nobody uses $SAFE (I think MRI is the only
implementation of Ruby that has it).

In short: :-(

--
Aaron Patterson
http://tenderlovemaking.com/

#15 Updated by Anonymous about 1 year ago

On Wed, Feb 13, 2013 at 01:10:24PM +0900, shugo (Shugo Maeda) wrote:

Issue #7839 has been updated by shugo (Shugo Maeda).

If this is a main use case of Symbol.freezesymbols, it might be better to have String#intern's option to control whether a symbol creation is allowed and to make YAML.safeload to use it.
(snip)
The problem with this is we can be calling foreign code. We have to
force all library authors to use it. Library authors may not expect
that calls to ".intern" will be fed user input.

Most security issues we have to deal with (even the YAML example) are
cases where we do not expect to process foreign input.

Hmm.... I'm worried that the following code is not thread safe.

begin
  Symbol.freeze_symbols
  YAML.load(...)
ensure
  Symbol.thaw_symbols
end

Yes, this is probably not thread safe, but if people know it's not
thread safe, then they can lock:

 begin
   Symbol.lock
   Symbol.freeze_symbols
   YAML.load(...)
 ensure
   Symbol.thaw_symbols
   Symbol.unlock
 end

I guess it can be implemented easily compared to Symbol GC.

Even freezing and thawing would definitely be easier than Symbol GC.

Sasada-san might implement Symbol GC for MRI.

So I'm worried that he might get less popular with women.

If it can be implemented in other implementations, it would be the best solution.

I think so too. If Symbol GC turns out to be too hard though, I'd like
to explore this option! :-)

--
Aaron Patterson
http://tenderlovemaking.com/

#16 Updated by Anonymous about 1 year ago

On Wed, Feb 13, 2013 at 10:08:28AM +0900, Eric Wong wrote:

"tenderlovemaking (Aaron Patterson)" aaron@tenderlovemaking.com wrote:

I'd like to be able to call a method like Symbol.freeze_symbols
which would essentially freeze the symbol hash, such that if any new
symbols are created, an exception would be thrown.

How about the option to do a soft freeze which issues a warning instead
of exception? (but support both). Start using soft freeze, and move to
a real freeze later when apps/gems are fixed.

Even having a warning would be nice!

--
Aaron Patterson
http://tenderlovemaking.com/

#17 Updated by Charlie Somerville about 1 year ago

On Thursday, 14 February 2013 at 11:30 AM, Aaron Patterson wrote:

Yes, this is probably not thread safe, but if people know it's not
thread safe, then they can lock:

begin
Symbol.lock
Symbol.freezesymbols
YAML.load(...)
ensure
Symbol.thaw
symbols
Symbol.unlock
end

I think this is a perfect contender for a block. There can be a global 'symbolfreezecount' that is incremented when the block enters, and decremented on the way out.

For example:

Symbol.frozen do
:foo # ok because this is symbol-ified by the parser beforehand
eval ":foo" # exception
"foo".to_sym # exception
end

This is better because it is re-entrant, unlike a pair of 'freeze' and 'thaw' methods.

#18 Updated by Nathan Zook about 1 year ago

Would my proposal #7854 not allow an easy and clean solution to this problem?

#19 Updated by Shugo Maeda about 1 year ago

Student (Nathan Zook) wrote:

+1 to prohibiting interning of tainted strings. Probably $SAFE >= 1, though. Currently, this is a problem for $SAFE <= 2.

$SAFE is not implemented in other implementations, so this issue should be addressed without $SAFE.

#20 Updated by Koichi Sasada about 1 year ago

  • Assignee set to Yukihiro Matsumoto

#21 Updated by Yui NARUSE about 1 month ago

#22 Updated by Yukihiro Matsumoto about 1 month ago

  • Status changed from Open to Rejected

Although I agree that we need something to prevent Symbol DoS attack, #freeze_symbols has too destructive I think.
I think that something should be a variation of #intern that refuses addition to the symbol table.

Matz.

Also available in: Atom PDF