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 11, 2021
1 parent 8fcdc38 commit f351e51
Show file tree
Hide file tree
Showing 25 changed files with 199 additions and 127 deletions.
59 changes: 50 additions & 9 deletions lib/psych.rb
Original file line number Diff line number Diff line change
Expand Up @@ -249,11 +249,11 @@ module Psych
#
# Example:
#
# Psych.load("--- a") # => 'a'
# Psych.load("---\n - a\n - b") # => ['a', 'b']
# Psych.unsafe_load("--- a") # => 'a'
# Psych.unsafe_load("---\n - a\n - b") # => ['a', 'b']
#
# begin
# Psych.load("--- `", filename: "file.txt")
# Psych.unsafe_load("--- `", filename: "file.txt")
# rescue Psych::SyntaxError => ex
# ex.file # => 'file.txt'
# ex.message # => "(file.txt): found character that cannot start any token"
Expand All @@ -262,16 +262,16 @@ module Psych
# When the optional +symbolize_names+ keyword argument is set to a
# true value, returns symbols for keys in Hash objects (default: strings).
#
# Psych.load("---\n foo: bar") # => {"foo"=>"bar"}
# Psych.load("---\n foo: bar", symbolize_names: true) # => {:foo=>"bar"}
# Psych.unsafe_load("---\n foo: bar") # => {"foo"=>"bar"}
# Psych.unsafe_load("---\n foo: bar", symbolize_names: true) # => {:foo=>"bar"}
#
# Raises a TypeError when `yaml` parameter is NilClass
#
# 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 method.
# load method or 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 @@ -362,6 +362,46 @@ def self.safe_load yaml, legacy_permitted_classes = NOT_GIVEN, legacy_permitted_
result
end

###
# Load +yaml+ in to a Ruby data structure. If multiple documents are
# provided, the object contained in the first document will be returned.
# +filename+ will be used in the exception message if any exception
# is raised while parsing. If +yaml+ is empty, it returns
# the specified +fallback+ return value, which defaults to +false+.
#
# Raises a Psych::SyntaxError when a YAML syntax error is detected.
#
# Example:
#
# Psych.load("--- a") # => 'a'
# Psych.load("---\n - a\n - b") # => ['a', 'b']
#
# begin
# Psych.load("--- `", filename: "file.txt")
# rescue Psych::SyntaxError => ex
# ex.file # => 'file.txt'
# ex.message # => "(file.txt): found character that cannot start any token"
# end
#
# When the optional +symbolize_names+ keyword argument is set to a
# true value, returns symbols for keys in Hash objects (default: strings).
#
# Psych.load("---\n foo: bar") # => {"foo"=>"bar"}
# Psych.load("---\n foo: bar", symbolize_names: true) # => {:foo=>"bar"}
#
# Raises a TypeError when `yaml` parameter is NilClass. This method is
# similar to `safe_load` except that `Symbol` objects are allowed by default.
#
def self.load yaml, permitted_classes: [Symbol], permitted_symbols: [], aliases: false, filename: nil, fallback: nil, symbolize_names: false, freeze: false
safe_load yaml, permitted_classes: permitted_classes,
permitted_symbols: permitted_symbols,
aliases: aliases,
filename: filename,
fallback: fallback,
symbolize_names: symbolize_names,
freeze: freeze
end

###
# Parse a YAML string in +yaml+. Returns the Psych::Nodes::Document.
# +filename+ is used in the exception message if a Psych::SyntaxError is
Expand Down Expand Up @@ -577,9 +617,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 +633,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
Loading

0 comments on commit f351e51

Please sign in to comment.