From a26f7e96b52efe0be508e223cd31f97ed04099ea Mon Sep 17 00:00:00 2001 From: Florian Frank Date: Mon, 4 Feb 2013 23:28:30 +0100 Subject: [PATCH] Security fix create_additons/JSON::GenericObject --- CHANGES | 8 ++++++ Gemfile | 1 + ext/json/ext/parser/parser.c | 2 +- ext/json/ext/parser/parser.rl | 2 +- java/src/json/ext/Parser.java | 2 +- java/src/json/ext/Parser.rl | 2 +- json.gemspec | 2 +- json_pure.gemspec | 2 +- lib/json/common.rb | 21 +++++++++----- lib/json/generic_object.rb | 7 +++++ lib/json/pure/parser.rb | 8 +++--- tests/test_json.rb | 10 +++++-- tests/test_json_addition.rb | 56 ++++++++++++++++++++++---------------- tests/test_json_generic_object.rb | 30 ++++++++++++++------ tests/test_json_string_matching.rb | 7 ++--- 15 files changed, 105 insertions(+), 55 deletions(-) diff --git a/CHANGES b/CHANGES index a8c0b35..e3d12a7 100644 --- a/CHANGES +++ b/CHANGES @@ -1,4 +1,12 @@ 2013-02-04 (1.7.7) + * Security fix for JSON create_additions default value and + JSON::GenericObject. It should not be possible to create additions unless + explicitely requested by setting the create_additions argument to true or + using the JSON.load/dump interface. If JSON::GenericObject is supposed to + be automatically deserialised, this has to be explicitely enabled by + setting + JSON::GenericObject.json_createble = true + as well. * Remove useless assert in fbuffer implementation. * Apply patch attached to https://github.com/flori/json/issues#issue/155 provided by John Shahid , Thx! diff --git a/Gemfile b/Gemfile index 98d7837..e405da2 100644 --- a/Gemfile +++ b/Gemfile @@ -8,3 +8,4 @@ gemspec :name => 'json-java' gem 'utils' gem 'test-unit' +gem 'debugger', :platform => :mri_19 diff --git a/ext/json/ext/parser/parser.c b/ext/json/ext/parser/parser.c index 8442d21..df89f2c 100644 --- a/ext/json/ext/parser/parser.c +++ b/ext/json/ext/parser/parser.c @@ -1680,7 +1680,7 @@ static VALUE cParser_initialize(int argc, VALUE *argv, VALUE self) if (option_given_p(opts, tmp)) { json->create_additions = RTEST(rb_hash_aref(opts, tmp)); } else { - json->create_additions = 1; + json->create_additions = 0; } tmp = ID2SYM(i_create_id); if (option_given_p(opts, tmp)) { diff --git a/ext/json/ext/parser/parser.rl b/ext/json/ext/parser/parser.rl index 6138a6f..ab8d318 100644 --- a/ext/json/ext/parser/parser.rl +++ b/ext/json/ext/parser/parser.rl @@ -664,7 +664,7 @@ static VALUE cParser_initialize(int argc, VALUE *argv, VALUE self) if (option_given_p(opts, tmp)) { json->create_additions = RTEST(rb_hash_aref(opts, tmp)); } else { - json->create_additions = 1; + json->create_additions = 0; } tmp = ID2SYM(i_create_id); if (option_given_p(opts, tmp)) { diff --git a/java/src/json/ext/Parser.java b/java/src/json/ext/Parser.java index ab3585e..6cb5886 100644 --- a/java/src/json/ext/Parser.java +++ b/java/src/json/ext/Parser.java @@ -166,7 +166,7 @@ public class Parser extends RubyObject { this.symbolizeNames = opts.getBool("symbolize_names", false); this.quirksMode = opts.getBool("quirks_mode", false); this.createId = opts.getString("create_id", getCreateId(context)); - this.createAdditions = opts.getBool("create_additions", true); + this.createAdditions = opts.getBool("create_additions", false); this.objectClass = opts.getClass("object_class", runtime.getHash()); this.arrayClass = opts.getClass("array_class", runtime.getArray()); this.match_string = opts.getHash("match_string"); diff --git a/java/src/json/ext/Parser.rl b/java/src/json/ext/Parser.rl index e26637d..6dd335a 100644 --- a/java/src/json/ext/Parser.rl +++ b/java/src/json/ext/Parser.rl @@ -164,7 +164,7 @@ public class Parser extends RubyObject { this.symbolizeNames = opts.getBool("symbolize_names", false); this.quirksMode = opts.getBool("quirks_mode", false); this.createId = opts.getString("create_id", getCreateId(context)); - this.createAdditions = opts.getBool("create_additions", true); + this.createAdditions = opts.getBool("create_additions", false); this.objectClass = opts.getClass("object_class", runtime.getHash()); this.arrayClass = opts.getClass("array_class", runtime.getArray()); this.match_string = opts.getHash("match_string"); diff --git a/json.gemspec b/json.gemspec index fb52be8..8d7c693 100644 --- a/json.gemspec +++ b/json.gemspec @@ -6,7 +6,7 @@ Gem::Specification.new do |s| s.required_rubygems_version = Gem::Requirement.new(">= 0") if s.respond_to? :required_rubygems_version= s.authors = ["Florian Frank"] - s.date = "2013-02-04" + s.date = "2013-02-10" s.description = "This is a JSON implementation as a Ruby extension in C." s.email = "flori@ping.de" s.extensions = ["ext/json/ext/generator/extconf.rb", "ext/json/ext/parser/extconf.rb"] diff --git a/json_pure.gemspec b/json_pure.gemspec index 1d4b4c0..0d696c9 100644 --- a/json_pure.gemspec +++ b/json_pure.gemspec @@ -6,7 +6,7 @@ Gem::Specification.new do |s| s.required_rubygems_version = Gem::Requirement.new(">= 0") if s.respond_to? :required_rubygems_version= s.authors = ["Florian Frank"] - s.date = "2013-02-04" + s.date = "2013-02-10" s.description = "This is a JSON implementation in pure Ruby." s.email = "flori@ping.de" s.extra_rdoc_files = ["README.rdoc"] diff --git a/lib/json/common.rb b/lib/json/common.rb index 03892d9..65a74a1 100644 --- a/lib/json/common.rb +++ b/lib/json/common.rb @@ -299,21 +299,28 @@ module JSON attr_accessor :load_default_options end self.load_default_options = { - :max_nesting => false, - :allow_nan => true, - :quirks_mode => true, + :max_nesting => false, + :allow_nan => true, + :quirks_mode => true, + :create_additions => true, } # Load a ruby data structure from a JSON _source_ and return it. A source can # either be a string-like object, an IO-like object, or an object responding # to the read method. If _proc_ was given, it will be called with any nested - # Ruby object as an argument recursively in depth first order. The default - # options for the parser can be changed via the load_default_options method. + # Ruby object as an argument recursively in depth first order. To modify the + # default options pass in the optional _options_ argument as well. + # + # BEWARE: This method is meant to serialise data from trusted user input, + # like from your own database server or clients under your control, it could + # be dangerous to allow untrusted users to pass JSON sources into it. The + # default options for the parser can be changed via the load_default_options + # method. # # This method is part of the implementation of the load/dump interface of # Marshal and YAML. - def load(source, proc = nil) - opts = load_default_options + def load(source, proc = nil, options = {}) + opts = load_default_options.merge options if source.respond_to? :to_str source = source.to_str elsif source.respond_to? :to_io diff --git a/lib/json/generic_object.rb b/lib/json/generic_object.rb index cd93e1a..8b1074c 100644 --- a/lib/json/generic_object.rb +++ b/lib/json/generic_object.rb @@ -5,6 +5,12 @@ module JSON class << self alias [] new + def json_creatable? + @json_creatable + end + + attr_writer :json_creatable + def json_create(data) data = data.dup data.delete JSON.create_id @@ -26,6 +32,7 @@ module JSON end end end + self.json_creatable = false def to_hash table diff --git a/lib/json/pure/parser.rb b/lib/json/pure/parser.rb index cb249b2..a41d1ee 100644 --- a/lib/json/pure/parser.rb +++ b/lib/json/pure/parser.rb @@ -63,9 +63,9 @@ module JSON # * *symbolize_names*: If set to true, returns symbols for the names # (keys) in a JSON object. Otherwise strings are returned, which is also # the default. - # * *create_additions*: If set to false, the Parser doesn't create - # additions even if a matchin class and create_id was found. This option - # defaults to true. + # * *create_additions*: If set to true, the Parser creates + # additions when if a matching class and create_id was found. This + # option defaults to false. # * *object_class*: Defaults to Hash # * *array_class*: Defaults to Array # * *quirks_mode*: Enables quirks_mode for parser, that is for example @@ -88,7 +88,7 @@ module JSON if opts.key?(:create_additions) @create_additions = !!opts[:create_additions] else - @create_additions = true + @create_additions = false end @create_id = @create_additions ? JSON.create_id : nil @object_class = opts[:object_class] || Hash diff --git a/tests/test_json.rb b/tests/test_json.rb index be974cd..6af6b32 100755 --- a/tests/test_json.rb +++ b/tests/test_json.rb @@ -329,12 +329,12 @@ class TestJSON < Test::Unit::TestCase def test_generate_core_subclasses_with_new_to_json obj = SubHash2["foo" => SubHash2["bar" => true]] obj_json = JSON(obj) - obj_again = JSON(obj_json) + obj_again = JSON.parse(obj_json, :create_additions => true) assert_kind_of SubHash2, obj_again assert_kind_of SubHash2, obj_again['foo'] assert obj_again['foo']['bar'] assert_equal obj, obj_again - assert_equal ["foo"], JSON(JSON(SubArray2["foo"])) + assert_equal ["foo"], JSON(JSON(SubArray2["foo"]), :create_additions => true) end def test_generate_core_subclasses_with_default_to_json @@ -493,6 +493,12 @@ EOT assert_equal nil, JSON.load('') end + def test_load_with_options + small_hash = JSON("foo" => 'bar') + symbol_hash = { :foo => 'bar' } + assert_equal symbol_hash, JSON.load(small_hash, nil, :symbolize_names => true) + end + def test_dump too_deep = '[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]' assert_equal too_deep, JSON.dump(eval(too_deep)) diff --git a/tests/test_json_addition.rb b/tests/test_json_addition.rb index 707aa32..a30f06a 100755 --- a/tests/test_json_addition.rb +++ b/tests/test_json_addition.rb @@ -73,11 +73,19 @@ class TestJSONAddition < Test::Unit::TestCase a = A.new(666) assert A.json_creatable? json = generate(a) - a_again = JSON.parse(json) + a_again = JSON.parse(json, :create_additions => true) assert_kind_of a.class, a_again assert_equal a, a_again end + def test_extended_json_default + a = A.new(666) + assert A.json_creatable? + json = generate(a) + a_hash = JSON.parse(json) + assert_kind_of Hash, a_hash + end + def test_extended_json_disabled a = A.new(666) assert A.json_creatable? @@ -104,7 +112,7 @@ class TestJSONAddition < Test::Unit::TestCase c = C.new assert !C.json_creatable? json = generate(c) - assert_raises(ArgumentError, NameError) { JSON.parse(json) } + assert_raises(ArgumentError, NameError) { JSON.parse(json, :create_additions => true) } end def test_raw_strings @@ -122,7 +130,7 @@ class TestJSONAddition < Test::Unit::TestCase assert_match(/\A\{.*\}\z/, json) assert_match(/"json_class":"String"/, json) assert_match(/"raw":\[0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33,34,35,36,37,38,39,40,41,42,43,44,45,46,47,48,49,50,51,52,53,54,55,56,57,58,59,60,61,62,63,64,65,66,67,68,69,70,71,72,73,74,75,76,77,78,79,80,81,82,83,84,85,86,87,88,89,90,91,92,93,94,95,96,97,98,99,100,101,102,103,104,105,106,107,108,109,110,111,112,113,114,115,116,117,118,119,120,121,122,123,124,125,126,127,128,129,130,131,132,133,134,135,136,137,138,139,140,141,142,143,144,145,146,147,148,149,150,151,152,153,154,155,156,157,158,159,160,161,162,163,164,165,166,167,168,169,170,171,172,173,174,175,176,177,178,179,180,181,182,183,184,185,186,187,188,189,190,191,192,193,194,195,196,197,198,199,200,201,202,203,204,205,206,207,208,209,210,211,212,213,214,215,216,217,218,219,220,221,222,223,224,225,226,227,228,229,230,231,232,233,234,235,236,237,238,239,240,241,242,243,244,245,246,247,248,249,250,251,252,253,254,255\]/, json) - raw_again = JSON.parse(json) + raw_again = JSON.parse(json, :create_additions => true) assert_equal raw, raw_again end @@ -130,17 +138,17 @@ class TestJSONAddition < Test::Unit::TestCase def test_core t = Time.now - assert_equal t, JSON(JSON(t)) + assert_equal t, JSON(JSON(t), :create_additions => true) d = Date.today - assert_equal d, JSON(JSON(d)) + assert_equal d, JSON(JSON(d), :create_additions => true) d = DateTime.civil(2007, 6, 14, 14, 57, 10, Rational(1, 12), 2299161) - assert_equal d, JSON(JSON(d)) - assert_equal 1..10, JSON(JSON(1..10)) - assert_equal 1...10, JSON(JSON(1...10)) - assert_equal "a".."c", JSON(JSON("a".."c")) - assert_equal "a"..."c", JSON(JSON("a"..."c")) + assert_equal d, JSON(JSON(d), :create_additions => true) + assert_equal 1..10, JSON(JSON(1..10), :create_additions => true) + assert_equal 1...10, JSON(JSON(1...10), :create_additions => true) + assert_equal "a".."c", JSON(JSON("a".."c"), :create_additions => true) + assert_equal "a"..."c", JSON(JSON("a"..."c"), :create_additions => true) s = MyJsonStruct.new 4711, 'foot' - assert_equal s, JSON(JSON(s)) + assert_equal s, JSON(JSON(s), :create_additions => true) struct = Struct.new :foo, :bar s = struct.new 4711, 'foot' assert_raises(JSONError) { JSON(s) } @@ -148,41 +156,41 @@ class TestJSONAddition < Test::Unit::TestCase raise TypeError, "test me" rescue TypeError => e e_json = JSON.generate e - e_again = JSON e_json + e_again = JSON e_json, :create_additions => true assert_kind_of TypeError, e_again assert_equal e.message, e_again.message assert_equal e.backtrace, e_again.backtrace end - assert_equal(/foo/, JSON(JSON(/foo/))) - assert_equal(/foo/i, JSON(JSON(/foo/i))) + assert_equal(/foo/, JSON(JSON(/foo/), :create_additions => true)) + assert_equal(/foo/i, JSON(JSON(/foo/i), :create_additions => true)) end def test_utc_datetime now = Time.now - d = DateTime.parse(now.to_s) # usual case - assert_equal d, JSON.parse(d.to_json) + d = DateTime.parse(now.to_s, :create_additions => true) # usual case + assert_equal d, JSON.parse(d.to_json, :create_additions => true) d = DateTime.parse(now.utc.to_s) # of = 0 - assert_equal d, JSON.parse(d.to_json) + assert_equal d, JSON.parse(d.to_json, :create_additions => true) d = DateTime.civil(2008, 6, 17, 11, 48, 32, Rational(1,24)) - assert_equal d, JSON.parse(d.to_json) + assert_equal d, JSON.parse(d.to_json, :create_additions => true) d = DateTime.civil(2008, 6, 17, 11, 48, 32, Rational(12,24)) - assert_equal d, JSON.parse(d.to_json) + assert_equal d, JSON.parse(d.to_json, :create_additions => true) end def test_rational_complex - assert_equal Rational(2, 9), JSON(JSON(Rational(2, 9))) - assert_equal Complex(2, 9), JSON(JSON(Complex(2, 9))) + assert_equal Rational(2, 9), JSON.parse(JSON(Rational(2, 9)), :create_additions => true) + assert_equal Complex(2, 9), JSON.parse(JSON(Complex(2, 9)), :create_additions => true) end def test_bigdecimal - assert_equal BigDecimal('3.141', 23), JSON(JSON(BigDecimal('3.141', 23))) - assert_equal BigDecimal('3.141', 666), JSON(JSON(BigDecimal('3.141', 666))) + assert_equal BigDecimal('3.141', 23), JSON(JSON(BigDecimal('3.141', 23)), :create_additions => true) + assert_equal BigDecimal('3.141', 666), JSON(JSON(BigDecimal('3.141', 666)), :create_additions => true) end def test_ostruct o = OpenStruct.new # XXX this won't work; o.foo = { :bar => true } o.foo = { 'bar' => true } - assert_equal o, JSON(JSON(o)) + assert_equal o, JSON.parse(JSON(o), :create_additions => true) end end diff --git a/tests/test_json_generic_object.rb b/tests/test_json_generic_object.rb index 83093b8..77ef22e 100644 --- a/tests/test_json_generic_object.rb +++ b/tests/test_json_generic_object.rb @@ -20,17 +20,22 @@ class TestJSONGenericObject < Test::Unit::TestCase end def test_generate_json - assert_equal @go, JSON(JSON(@go)) + switch_json_creatable do + assert_equal @go, JSON(JSON(@go), :create_additions => true) + end end def test_parse_json - assert_equal @go, l = JSON('{ "json_class": "JSON::GenericObject", "a": 1, "b": 2 }') - assert_equal 1, l.a - assert_equal @go, l = JSON('{ "a": 1, "b": 2 }', :object_class => GenericObject) - assert_equal 1, l.a - assert_equal GenericObject[:a => GenericObject[:b => 2]], - l = JSON('{ "a": { "b": 2 } }', :object_class => GenericObject) - assert_equal 2, l.a.b + assert_kind_of Hash, JSON('{ "json_class": "JSON::GenericObject", "a": 1, "b": 2 }', :create_additions => true) + switch_json_creatable do + assert_equal @go, l = JSON('{ "json_class": "JSON::GenericObject", "a": 1, "b": 2 }', :create_additions => true) + assert_equal 1, l.a + assert_equal @go, l = JSON('{ "a": 1, "b": 2 }', :object_class => GenericObject) + assert_equal 1, l.a + assert_equal GenericObject[:a => GenericObject[:b => 2]], + l = JSON('{ "a": { "b": 2 } }', :object_class => GenericObject) + assert_equal 2, l.a.b + end end def test_from_hash @@ -43,4 +48,13 @@ class TestJSONGenericObject < Test::Unit::TestCase assert_equal true, result.foo.quux.first.foobar assert_equal true, GenericObject.from_hash(true) end + + private + + def switch_json_creatable + JSON::GenericObject.json_creatable = true + yield + ensure + JSON::GenericObject.json_creatable = false + end end diff --git a/tests/test_json_string_matching.rb b/tests/test_json_string_matching.rb index 2ddedfa..c233df8 100644 --- a/tests/test_json_string_matching.rb +++ b/tests/test_json_string_matching.rb @@ -27,14 +27,13 @@ class TestJSONStringMatching < Test::Unit::TestCase t = TestTime.new t_json = [ t ].to_json assert_equal [ t ], - JSON.parse(t_json, + JSON.parse(t_json, :create_additions => true, :match_string => { /\A\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}[+-]\d{4}\z/ => TestTime }) assert_equal [ t.strftime('%FT%T%z') ], - JSON.parse(t_json, + JSON.parse(t_json, :create_additions => true, :match_string => { /\A\d{3}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}[+-]\d{4}\z/ => TestTime }) assert_equal [ t.strftime('%FT%T%z') ], JSON.parse(t_json, - :match_string => { /\A\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}[+-]\d{4}\z/ => TestTime }, - :create_additions => false) + :match_string => { /\A\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}[+-]\d{4}\z/ => TestTime }) end end -- 1.8.1.2