Bug #17866: Incompatible changes with Psych 4.0.0

• Status: Closed
• Assignee: tenderlovemaking (Aaron Patterson)
• Target version: -
• Backport: 2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN

[hsbt (Hiroshi SHIBATA)]

Psych-4.0.0 changes Psych.safe_load by the default.

https://github.com/ruby/psych/pull/487

It breaks the several code like:

• https://github.com/ruby/ruby/commit/da5b28396397ace84d914cb188055cbeb46b8725
• https://github.com/ruby/ruby/commit/8e91b969df08b7a2eb27a5d6d38733eea42dc7ad
• https://github.com/ruby/ruby/commit/d8fd92f62024d85271a3f1125bc6928409f912e1
• https://github.com/ruby/ruby/commit/dfecc650c3f9bbd8b4fb0eefc1e3da65f151d3a8
• etc...

I and @mame (Yusuke Endoh) investigate them. We found 2 issues.

1. Symbol is still ignored Pysch.load. It break many of code like configuration store. https://github.com/ruby/psych/blob/master/lib/psych.rb#L368 passes Symbol used by permitted_classes. But It's not working now. see https://github.com/ruby/psych/issues/490
2. Pysch.load restrict Gem::Specification or RDoc::Options by the default. Should we add them withpermitted_classes to Psych.load or Psych.load_file? I'm not sure the right way about them.

@tenderlovemaking (Aaron Patterson) Do you have any ideas about the above concerns?

[hsbt (Hiroshi SHIBATA)]

• Description updated (diff)

[k0kubun (Takashi Kokubun)]

I noticed Psych.unsafe_load uses aliases: false by default. This means that default config/database.yml of Rails will stop working if you use Psych 4.0.0.

https://github.com/rails/rails/blob/v6.1.3.2/railties/lib/rails/generators/rails/app/templates/config/databases/sqlite3.yml.tt#L7
https://github.com/rails/rails/blob/v6.1.3.2/activesupport/lib/active_support/configuration_file.rb#L22
https://github.com/rails/rails/blob/v6.1.3.2/railties/lib/rails/application/configuration.rb#L275

How about using aliases: true by default in Psych.load? I guess the main purpose of the breaking change was to disallow deserialization of arbitrary objects, and disabling aliases wasn't really important.

[byroot (Jean Boussier)]

and disabling aliases wasn't really important.

It kinda is, as aliases allow for circular references which can cause some programs to end up in infinite loop.

How about using aliases: true by default in Psych.load?

Maybe we should have YAML.load_file use a more relaxed default, as we can assume that most of the time this is for loading configs etc rather than user submitted data.

[Eregon (Benoit Daloze)]

byroot (Jean Boussier) wrote in #note-3:

It kinda is, as aliases allow for circular references which can cause some programs to end up in infinite loop.

Doesn't the YAML implementation handle/avoid such recursive/circular references?

At least:

default: &default
  adapter: sqlite3
  foo: bar
  nested:
    <<: *default

gives:

$ ruby -ryaml -e 'pp YAML.load_file "circular.yml"'
{"default"=>
  {"adapter"=>"sqlite3",
   "foo"=>"bar",
   "nested"=>{"adapter"=>"sqlite3", "foo"=>"bar"}}}

so it doesn't loop infinitely.

[k0kubun (Takashi Kokubun)]

It kinda is, as aliases allow for circular references which can cause some programs to end up in infinite loop.

I thought of the same thing and I tested what Eregon wrote before writing the comment. So, unless you know of a YAML that crashes Psych.safe_load(..., aliases: true), I assume it's safe.

Maybe we should have YAML.load_file use a more relaxed default, as we can assume that most of the time this is for loading configs etc rather than user submitted data.

Just a reminder from my previous comment, ActiveSupport::ConfigurationFile.parse uses not YAML.load_file but YAML.load. Therefore you have to fix YAML.load as well if you want to keep existing Rails apps working.

[byroot (Jean Boussier)]

So indeed I was wrong about the circular reference.

As for Rails I fixed it this morning: https://github.com/rails/rails/commit/179d0a1f474ada02e0030ac3bd062fc653765dbe, but yes, if load would allow aliases by default it would make things much easier for everyone.

[tenderlovemaking (Aaron Patterson)]

Eregon (Benoit Daloze) wrote in #note-4:

byroot (Jean Boussier) wrote in #note-3:

It kinda is, as aliases allow for circular references which can cause some programs to end up in infinite loop.

Doesn't the YAML implementation handle/avoid such recursive/circular references?

No, you can definitely make recursive data structures:

irb(main):006:0> x = []
=> []
irb(main):007:0> x << x
=> [[...]]
irb(main):008:0> puts Psych.dump x
--- &1
- *1
=> nil
irb(main):009:0> y = Psych.load Psych.dump x
=> [[...]] 

So, unless you know of a YAML that crashes Psych.safe_load(..., aliases: true), I assume it's safe.

No circular aliases will cause Psych.safe_load to crash. The problem is any code that processes the return value of Psych.load. Sometime like this:

require "psych"

def process thing
  thing.each do |item|
    case item
    when Array
      process item
    when Hash
      # ...
    when String
      # ...
    end
  end
end

user_input = DATA.read

process Psych.load(user_input)

__END__
--- &1
- *1

if load would allow aliases by default it would make things much easier for everyone.

Ya, I agree. If we can allow aliases but not recursive aliases, I think that would be great. I'm not 100% sure how to do it though.

I'm ok with enabling aliases by default. DoS attacks using recursive structures have only been theoretical. However, I'm very tired of dealing with YAML related security issues, so I'm not 100% sure. 😅

[byroot (Jean Boussier)]

I'm ok with enabling aliases by default.

Just to clarify: load would have them on by default, but safe_load wouldn't, right?

[tenderlovemaking (Aaron Patterson)]

byroot (Jean Boussier) wrote in #note-8:

I'm ok with enabling aliases by default.

Just to clarify: load would have them on by default, but safe_load wouldn't, right?

Ya, that's what I was considering. safe_load shouldn't change behavior whatsoever.

[naruse (Yui NARUSE)]

Since I want Ruby 3.1 to keep as much as compatibility for Ruby 3.0 to ensure application/library developer adopt Ruby 3, I’m negative to go Ruby 3.1 with Psych 4.0.0.

[cabo (Carsten Bormann)]

naruse (Yui NARUSE) wrote in #note-10:

Since I want Ruby 3.1 to keep as much as compatibility for Ruby 3.0 to ensure application/library developer adopt Ruby 3, I’m negative to go Ruby 3.1 with Psych 4.0.0.

Unfortunately, users will pick up this incompatibility with their next gem update.

So it won't make much of a difference whether you put it in Ruby 3.1 or not.

(The updated major gem version also does not protect against this update as psych historically has not been a gem, so few Gemfiles have a ~> 3.0 or some such.)

[yahonda (Yasuo Honda)]

Can I get a final decision if Psych 4 will be shipped with Ruby 3.1 as a standard library?

I think this is one of the biggest incompatibilities which will be introduced with Ruby 3.1.
As far as I understand, Psych is a standard library of Ruby, so it is not recommended to use Psych 3 with Ruby 3.1.

Now I'm not against releasing Ruby 3.1 with Psych 4 because it may be too late to revert changes to ship Ruby 3.1 with Psych 3. I wish Ruby 3.1 release note mention this incompatibility introduced in Psych 4.

[hsbt (Hiroshi SHIBATA)]

• Status changed from Assigned to Closed

I discussed this with @naruse (Yui NARUSE). We decided to bundle Psych 4 with Ruby 3.1. And we should note this incompatibility and migration way with Psych 3.3.2 to NEWS.