From 9888a02e864564150718d1b912dce027bbff9af8 Mon Sep 17 00:00:00 2001 From: davutselcuk Date: Thu, 10 Sep 2026 18:41:43 +0300 Subject: [PATCH] Cap nesting depth in Gem::SafeMarshal::Reader `Reader#read_element` recurses once per nested element and counts nothing, so a marshal stream of nested arrays exhausts the stack before any other limit applies. Marshal spends two bytes per level, so ~4.8 KB is enough. `SystemStackError` descends from `Exception`, not `StandardError`, so it walks past callers that already guard this parse. `Gem::Source#fetch_spec` is written to tolerate a bad spec and carry on: spec = begin Gem::SafeMarshal.safe_load(spec) rescue StandardError nil end That rescue cannot see a `SystemStackError`, so the intended "ignore the bad spec" path never runs. This adds a depth cap symmetric with the one `yaml_serializer.rb` already applies to the other parser in this tree: MAX_NESTING_DEPTH = 1_000 Past the cap the reader raises `Reader::TooDeeplyNestedError`, which is a `Reader::Error` and therefore a `StandardError`, so existing rescues behave as written. Depth is counted per `read_element`, so width is unaffected: an array of 5,000 siblings still parses, and so does a 998-level nest. Tests cover the cap, a nest just under it, and the wide-but-shallow case. Co-Authored-By: Claude Opus 5 (1M context) --- lib/rubygems/safe_marshal/reader.rb | 12 +++++++++++ test/rubygems/test_gem_safe_marshal.rb | 29 ++++++++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/lib/rubygems/safe_marshal/reader.rb b/lib/rubygems/safe_marshal/reader.rb index ac33f91988c9..b523c2c6a6eb 100644 --- a/lib/rubygems/safe_marshal/reader.rb +++ b/lib/rubygems/safe_marshal/reader.rb @@ -29,10 +29,14 @@ class NegativeLengthError < Error class LengthTooLongError < Error end + class TooDeeplyNestedError < Error + end + def initialize(io) @io = io @object_links = {} @symbol_links = {} + @depth = 0 end def read! @@ -47,6 +51,9 @@ def read! MARSHAL_VERSION = [Marshal::MAJOR_VERSION, Marshal::MINOR_VERSION].map(&:chr).join.freeze private_constant :MARSHAL_VERSION + MAX_NESTING_DEPTH = 1_000 + private_constant :MAX_NESTING_DEPTH + def read_header v = @io.read(2) raise UnsupportedVersionError, "Unsupported marshal version #{v.bytes.map(&:ord).join(".")}, expected #{Marshal::MAJOR_VERSION}.#{Marshal::MINOR_VERSION}" unless v == MARSHAL_VERSION @@ -109,6 +116,9 @@ def read_count end def read_element + @depth += 1 + raise TooDeeplyNestedError, "exceeded maximum nesting depth (#{MAX_NESTING_DEPTH})" if @depth > MAX_NESTING_DEPTH + type = read_byte case type when 34 then read_string # ?" @@ -139,6 +149,8 @@ def read_element else raise Error, "Unknown marshal type discriminator #{type.chr.inspect} (#{type})" end + ensure + @depth -= 1 end STRING_E_SYMBOL = Elements::Symbol.new("E").freeze diff --git a/test/rubygems/test_gem_safe_marshal.rb b/test/rubygems/test_gem_safe_marshal.rb index c34d8570c6ec..1434fa21995e 100644 --- a/test/rubygems/test_gem_safe_marshal.rb +++ b/test/rubygems/test_gem_safe_marshal.rb @@ -478,6 +478,35 @@ def test_unexpected_eof assert_equal e.message, "expected 1 bytes, got EOF" end + def test_nesting_depth_is_capped + # 2 bytes per level on the wire, so this is a small payload + payload = "\x04\x08".b + ("[\x06".b * 2_400) + "0".b + + e = assert_raise(Gem::SafeMarshal::Reader::TooDeeplyNestedError) do + Gem::SafeMarshal.safe_load(payload) + end + assert_equal "exceeded maximum nesting depth (1000)", e.message + + # the cap raises a StandardError, so callers that already rescue + # StandardError around safe_load keep working + assert_kind_of StandardError, e + end + + def test_nesting_below_the_cap_still_parses + payload = "\x04\x08".b + ("[\x06".b * 998) + "0".b + # deliberately not asserting on the value itself: inspecting a 998-deep + # array is what blows the stack, not parsing it + assert_equal ::Array, Gem::SafeMarshal.safe_load(payload).class + end + + def test_repeated_siblings_do_not_count_as_depth + # 5_000 elements, all at depth 2 -- must not trip the cap + payload = "\x04\x08[".b + "\x02\x88\x13".b + ("0".b * 5_000) + parsed = Gem::SafeMarshal.safe_load(payload) + assert_equal ::Array, parsed.class + assert_equal 5_000, parsed.size + end + def test_negative_length assert_raise(Gem::SafeMarshal::Reader::NegativeLengthError) do Gem::SafeMarshal.safe_load("\004\010}\325")