Skip to content

Commit

Permalink
Use Psych.safe_load by default
Browse files Browse the repository at this point in the history
Psych.load is not safe for use with untrusted data.  Too many
applications make the mistake of using `Psych.load` with untrusted data
and that ends up with some kind of security vulnerability.

This commit changes the default `Psych.load` to use `safe_load`.  Users
that want to parse trusted data can use Psych.unsafe_load.
  • Loading branch information
tenderlove committed May 10, 2021
1 parent 8fcdc38 commit f3cc8b1
Show file tree
Hide file tree
Showing 26 changed files with 151 additions and 118 deletions.
8 changes: 5 additions & 3 deletions lib/psych.rb
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ module Psych
# YAML documents that are supplied via user input. Instead, please use the
# safe_load method.
#
def self.load yaml, legacy_filename = NOT_GIVEN, filename: nil, fallback: false, symbolize_names: false, freeze: false
def self.unsafe_load yaml, legacy_filename = NOT_GIVEN, filename: nil, fallback: false, symbolize_names: false, freeze: false
if legacy_filename != NOT_GIVEN
warn_with_uplevel 'Passing filename with the 2nd argument of Psych.load is deprecated. Use keyword argument like Psych.load(yaml, filename: ...) instead.', uplevel: 1 if $VERBOSE
filename = legacy_filename
Expand Down Expand Up @@ -361,6 +361,7 @@ def self.safe_load yaml, legacy_permitted_classes = NOT_GIVEN, legacy_permitted_
result = visitor.accept result
result
end
class << self; alias load safe_load end

###
# Parse a YAML string in +yaml+. Returns the Psych::Nodes::Document.
Expand Down Expand Up @@ -577,9 +578,9 @@ def self.load_stream yaml, legacy_filename = NOT_GIVEN, filename: nil, fallback:
# NOTE: This method *should not* be used to parse untrusted documents, such as
# YAML documents that are supplied via user input. Instead, please use the
# safe_load_file method.
def self.load_file filename, **kwargs
def self.unsafe_load_file filename, **kwargs
File.open(filename, 'r:bom|utf-8') { |f|
self.load f, filename: filename, **kwargs
self.unsafe_load f, filename: filename, **kwargs
}
end

Expand All @@ -593,6 +594,7 @@ def self.safe_load_file filename, **kwargs
self.safe_load f, filename: filename, **kwargs
}
end
class << self; alias load_file safe_load_file end

# :stopdoc:
def self.add_domain_type domain, type_tag, &block
Expand Down
30 changes: 21 additions & 9 deletions test/psych/helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,24 +41,30 @@ def with_default_internal(enc)
# Convert between Psych and the object to verify correct parsing and
# emitting
#
def assert_to_yaml( obj, yaml )
assert_equal( obj, Psych::load( yaml ) )
def assert_to_yaml( obj, yaml, loader = :load )
assert_equal( obj, Psych.send(loader, yaml) )
assert_equal( obj, Psych::parse( yaml ).transform )
assert_equal( obj, Psych::load( obj.to_yaml ) )
assert_equal( obj, Psych.send(loader, obj.to_yaml) )
assert_equal( obj, Psych::parse( obj.to_yaml ).transform )
assert_equal( obj, Psych::load(
assert_equal( obj, Psych.send(loader,
obj.to_yaml(
:UseVersion => true, :UseHeader => true, :SortKeys => true
)
))
rescue Psych::DisallowedClass, Psych::BadAlias
assert_to_yaml obj, yaml, :unsafe_load
end

#
# Test parser only
#
def assert_parse_only( obj, yaml )
assert_equal( obj, Psych::load( yaml ) )
assert_equal( obj, Psych::parse( yaml ).transform )
begin
assert_equal obj, Psych::load( yaml )
rescue Psych::DisallowedClass, Psych::BadAlias
assert_equal obj, Psych::unsafe_load( yaml )
end
assert_equal obj, Psych::parse( yaml ).transform
end

def assert_cycle( obj )
Expand All @@ -69,9 +75,15 @@ def assert_cycle( obj )
assert_nil Psych::load(Psych.dump(obj))
assert_nil Psych::load(obj.to_yaml)
else
assert_equal(obj, Psych.load(v.tree.yaml))
assert_equal(obj, Psych::load(Psych.dump(obj)))
assert_equal(obj, Psych::load(obj.to_yaml))
begin
assert_equal(obj, Psych.load(v.tree.yaml))
assert_equal(obj, Psych::load(Psych.dump(obj)))
assert_equal(obj, Psych::load(obj.to_yaml))
rescue Psych::DisallowedClass, Psych::BadAlias
assert_equal(obj, Psych.unsafe_load(v.tree.yaml))
assert_equal(obj, Psych::unsafe_load(Psych.dump(obj)))
assert_equal(obj, Psych::unsafe_load(obj.to_yaml))
end
end
end

Expand Down
12 changes: 6 additions & 6 deletions test/psych/test_alias_and_anchor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def test_mri_compatibility
- *id001
- *id001
EOYAML
result = Psych.load yaml
result = Psych.unsafe_load yaml
result.each {|el| assert_same(result[0], el) }
end

Expand All @@ -33,7 +33,7 @@ def test_mri_compatibility_object_with_ivars
- *id001
EOYAML

result = Psych.load yaml
result = Psych.unsafe_load yaml
result.each do |el|
assert_same(result[0], el)
assert_equal('test1', el.var1)
Expand All @@ -50,7 +50,7 @@ def test_mri_compatibility_substring_with_ivars
- *id001
- *id001
EOYAML
result = Psych.load yaml
result = Psych.unsafe_load yaml
result.each do |el|
assert_same(result[0], el)
assert_equal('test', el.var1)
Expand All @@ -62,7 +62,7 @@ def test_anchor_alias_round_trip
original = [o,o,o]

yaml = Psych.dump original
result = Psych.load yaml
result = Psych.unsafe_load yaml
result.each {|el| assert_same(result[0], el) }
end

Expand All @@ -73,7 +73,7 @@ def test_anchor_alias_round_trip_object_with_ivars
original = [o,o,o]

yaml = Psych.dump original
result = Psych.load yaml
result = Psych.unsafe_load yaml
result.each do |el|
assert_same(result[0], el)
assert_equal('test1', el.var1)
Expand All @@ -87,7 +87,7 @@ def test_anchor_alias_round_trip_substring_with_ivars
original = [o,o,o]

yaml = Psych.dump original
result = Psych.load yaml
result = Psych.unsafe_load yaml
result.each do |el|
assert_same(result[0], el)
assert_equal('test', el.var1)
Expand Down
6 changes: 3 additions & 3 deletions test/psych/test_array.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ def test_enumerator
def test_another_subclass_with_attributes
y = Y.new.tap {|o| o.val = 1}
y << "foo" << "bar"
y = Psych.load Psych.dump y
y = Psych.unsafe_load Psych.dump y

assert_equal %w{foo bar}, y
assert_equal Y, y.class
Expand All @@ -42,13 +42,13 @@ def test_subclass
end

def test_subclass_with_attributes
y = Psych.load Psych.dump Y.new.tap {|o| o.val = 1}
y = Psych.unsafe_load Psych.dump Y.new.tap {|o| o.val = 1}
assert_equal Y, y.class
assert_equal 1, y.val
end

def test_backwards_with_syck
x = Psych.load "--- !seq:#{X.name} []\n\n"
x = Psych.unsafe_load "--- !seq:#{X.name} []\n\n"
assert_equal X, x.class
end

Expand Down
14 changes: 7 additions & 7 deletions test/psych/test_coder.rb
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ def init_with(c)

def test_self_referential
x = Referential.new
copy = Psych.load Psych.dump x
copy = Psych.unsafe_load Psych.dump x
assert_equal copy, copy.a
end

Expand Down Expand Up @@ -153,23 +153,23 @@ def test_map_with_tag_and_style
end

def test_represent_map
thing = Psych.load(Psych.dump(RepresentWithMap.new))
thing = Psych.unsafe_load(Psych.dump(RepresentWithMap.new))
assert_equal({ "string" => 'a', :symbol => 'b' }, thing.map)
end

def test_represent_sequence
thing = Psych.load(Psych.dump(RepresentWithSeq.new))
thing = Psych.unsafe_load(Psych.dump(RepresentWithSeq.new))
assert_equal %w{ foo bar }, thing.seq
end

def test_represent_with_init
thing = Psych.load(Psych.dump(RepresentWithInit.new))
thing = Psych.unsafe_load(Psych.dump(RepresentWithInit.new))
assert_equal 'bar', thing.str
end

def test_represent!
assert_match(/foo/, Psych.dump(Represent.new))
assert_instance_of(Represent, Psych.load(Psych.dump(Represent.new)))
assert_instance_of(Represent, Psych.unsafe_load(Psych.dump(Represent.new)))
end

def test_scalar_coder
Expand All @@ -179,7 +179,7 @@ def test_scalar_coder

def test_load_dumped_tagging
foo = InitApi.new
bar = Psych.load(Psych.dump(foo))
bar = Psych.unsafe_load(Psych.dump(foo))
assert_equal false, bar.implicit
assert_equal "!ruby/object:Psych::TestCoder::InitApi", bar.tag
assert_equal Psych::Nodes::Mapping::BLOCK, bar.style
Expand All @@ -198,7 +198,7 @@ def test_dump_encode_with

def test_dump_init_with
foo = InitApi.new
bar = Psych.load(Psych.dump(foo))
bar = Psych.unsafe_load(Psych.dump(foo))
assert_equal foo.a, bar.a
assert_equal foo.b, bar.b
assert_nil bar.c
Expand Down
4 changes: 2 additions & 2 deletions test/psych/test_date_time.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def test_non_utc
def test_timezone_offset
times = [Time.new(2017, 4, 13, 12, 0, 0, "+09:00"),
Time.new(2017, 4, 13, 12, 0, 0, "-05:00")]
cycled = Psych::load(Psych.dump times)
cycled = Psych::unsafe_load(Psych.dump times)
assert_match(/12:00:00 \+0900/, cycled.first.to_s)
assert_match(/12:00:00 -0500/, cycled.last.to_s)
end
Expand All @@ -39,7 +39,7 @@ def test_datetime_non_utc
def test_datetime_timezone_offset
times = [DateTime.new(2017, 4, 13, 12, 0, 0, "+09:00"),
DateTime.new(2017, 4, 13, 12, 0, 0, "-05:00")]
cycled = Psych::load(Psych.dump times)
cycled = Psych::unsafe_load(Psych.dump times)
assert_match(/12:00:00\+09:00/, cycled.first.to_s)
assert_match(/12:00:00-05:00/, cycled.last.to_s)
end
Expand Down
4 changes: 2 additions & 2 deletions test/psych/test_deprecated.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ def to_yaml opts = {}
def test_recursive_quick_emit_encode_with
qeew = QuickEmitterEncodeWith.new
hash = { :qe => qeew }
hash2 = Psych.load Psych.dump hash
hash2 = Psych.unsafe_load Psych.dump hash
qe = hash2[:qe]

assert_equal qeew.name, qe.name
Expand Down Expand Up @@ -72,7 +72,7 @@ def yaml_initialize tag, vals
# receive the yaml_initialize call.
def test_yaml_initialize_and_init_with
hash = { :yi => YamlInitAndInitWith.new }
hash2 = Psych.load Psych.dump hash
hash2 = Psych.unsafe_load Psych.dump hash
yi = hash2[:yi]

assert_equal 'TGIF!', yi.name
Expand Down
8 changes: 4 additions & 4 deletions test/psych/test_exception.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,13 @@ def make_ex msg = 'oh no!'

def test_backtrace
err = make_ex
new_err = Psych.load(Psych.dump(err))
new_err = Psych.unsafe_load(Psych.dump(err))
assert_equal err.backtrace, new_err.backtrace
end

def test_naming_exception
err = String.xxx rescue $!
new_err = Psych.load(Psych.dump(err))
new_err = Psych.unsafe_load(Psych.dump(err))
assert_equal err.message, new_err.message
end

Expand All @@ -56,7 +56,7 @@ def test_load_takes_file

# deprecated interface
ex = assert_raise(Psych::SyntaxError) do
Psych.load '--- `', 'deprecated'
Psych.unsafe_load '--- `', 'deprecated'
end
assert_equal 'deprecated', ex.file
end
Expand Down Expand Up @@ -165,7 +165,7 @@ def test_attributes
end

def test_convert
w = Psych.load(Psych.dump(@wups))
w = Psych.unsafe_load(Psych.dump(@wups))
assert_equal @wups.message, w.message
assert_equal @wups.backtrace, w.backtrace
assert_equal 1, w.foo
Expand Down
16 changes: 8 additions & 8 deletions test/psych/test_hash.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def setup
def test_hash_with_ivar
t1 = HashWithIvar.new
t1[:foo] = :bar
t2 = Psych.load(Psych.dump(t1))
t2 = Psych.unsafe_load(Psych.dump(t1))
assert_equal t1, t2
assert_cycle t1
end
Expand All @@ -54,14 +54,14 @@ def test_referenced_hash_with_ivar
def test_custom_initialized
a = [1,2,3,4,5]
t1 = HashWithCustomInit.new(a)
t2 = Psych.load(Psych.dump(t1))
t2 = Psych.unsafe_load(Psych.dump(t1))
assert_equal t1, t2
assert_cycle t1
end

def test_custom_initialize_no_ivar
t1 = HashWithCustomInitNoIvar.new(nil)
t2 = Psych.load(Psych.dump(t1))
t2 = Psych.unsafe_load(Psych.dump(t1))
assert_equal t1, t2
assert_cycle t1
end
Expand All @@ -70,25 +70,25 @@ def test_hash_subclass_with_ivars
x = X.new
x[:a] = 'b'
x.instance_variable_set :@foo, 'bar'
dup = Psych.load Psych.dump x
dup = Psych.unsafe_load Psych.dump x
assert_cycle x
assert_equal 'bar', dup.instance_variable_get(:@foo)
assert_equal X, dup.class
end

def test_load_with_class_syck_compatibility
hash = Psych.load "--- !ruby/object:Hash\n:user_id: 7\n:username: Lucas\n"
hash = Psych.unsafe_load "--- !ruby/object:Hash\n:user_id: 7\n:username: Lucas\n"
assert_equal({ user_id: 7, username: 'Lucas'}, hash)
end

def test_empty_subclass
assert_match "!ruby/hash:#{X}", Psych.dump(X.new)
x = Psych.load Psych.dump X.new
x = Psych.unsafe_load Psych.dump X.new
assert_equal X, x.class
end

def test_map
x = Psych.load "--- !map:#{X} { }\n"
x = Psych.unsafe_load "--- !map:#{X} { }\n"
assert_equal X, x.class
end

Expand All @@ -102,7 +102,7 @@ def test_cycles
end

def test_ref_append
hash = Psych.load(<<-eoyml)
hash = Psych.unsafe_load(<<-eoyml)
---
foo: &foo
hello: world
Expand Down
6 changes: 3 additions & 3 deletions test/psych/test_marshalable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ module Psych
class TestMarshalable < TestCase
def test_objects_defining_marshal_dump_and_marshal_load_can_be_dumped
sd = SimpleDelegator.new(1)
loaded = Psych.load(Psych.dump(sd))
loaded = Psych.unsafe_load(Psych.dump(sd))

assert_instance_of(SimpleDelegator, loaded)
assert_equal(sd, loaded)
Expand Down Expand Up @@ -46,15 +46,15 @@ def class

def test_init_with_takes_priority_over_marshal_methods
obj = PsychCustomMarshalable.new(1)
loaded = Psych.load(Psych.dump(obj))
loaded = Psych.unsafe_load(Psych.dump(obj))

assert(PsychCustomMarshalable === loaded)
assert_equal(2, loaded.foo)
end

def test_init_symbolize_names
obj = PsychCustomMarshalable.new(1)
loaded = Psych.load(Psych.dump(obj), symbolize_names: true)
loaded = Psych.unsafe_load(Psych.dump(obj), symbolize_names: true)

assert(PsychCustomMarshalable === loaded)
assert_equal(2, loaded.foo)
Expand Down
Loading

0 comments on commit f3cc8b1

Please sign in to comment.