From 66473a6007111313784b9a02d0f9184e8fc3d82a Mon Sep 17 00:00:00 2001 From: CarterPerez-dev Date: Sun, 26 Jul 2026 11:02:58 -0400 Subject: [PATCH] fix(rube): reject negative counts and stop discarding name nodes Codex, working the defensive half without having written the parser, probed M1 with adversarial input and found defects my own tests missed. Verified independently before fixing, and two more were found while confirming: negative_array_count ACCEPTED as an empty array negative_hash_count ACCEPTED as an empty hash negative_bignum_words NoMethodError leaked outside StreamError negative_ivar_count ACCEPTED negative_string_len cursor moved BACKWARD, wrong error raised The last is the worst. take(-5) does not trip the count > remaining guard, so byteslice returns nil and @position decreases. A parser whose cursor can rewind on attacker input is a loop primitive, not merely a wrong error. The M1 gate claimed bignum length confusion was covered. It was not. Oversized widths were tested and negative counts never were, because the same author chose both the implementation and the cases it would face. That is the negative-control failure one level up, and it is exactly what an author cannot catch alone. Fixes: every count and length now flows through read_count with a role label and a nonnegative check, take rejects negative byte counts outright, and MalformedCountError joins the StreamError hierarchy so nothing leaks a raw NoMethodError. Negative link indices were already guarded; regression tests now pin that. Also closes a detection blind spot Codex identified. read_ivar and read_object discarded the parsed name nodes after taking their values, so a sink tag placed in an instance-variable-name position vanished from Result#sinks. Node now carries an auxiliary collection that Node#each traverses, and name and class nodes are retained. Proven: a stream with a userdef tag in the name position now reports Evil#_load where it previously reported nothing. 46 parser tests, 80 assertions. All controls pass, exploit gate still passes. --- .../lib/rube/marshal/constants.rb | 7 +++ .../lib/rube/marshal/errors.rb | 2 + .../lib/rube/marshal/node.rb | 4 +- .../lib/rube/marshal/parser.rb | 28 ++++++--- .../test/marshal/parser_test.rb | 60 +++++++++++++++++++ 5 files changed, 92 insertions(+), 9 deletions(-) diff --git a/PROJECTS/beginner/deserialization-gadget-lab/lib/rube/marshal/constants.rb b/PROJECTS/beginner/deserialization-gadget-lab/lib/rube/marshal/constants.rb index 2d0f1ec9..8f0302ba 100644 --- a/PROJECTS/beginner/deserialization-gadget-lab/lib/rube/marshal/constants.rb +++ b/PROJECTS/beginner/deserialization-gadget-lab/lib/rube/marshal/constants.rb @@ -49,6 +49,13 @@ module Rube DEFAULT_MAX_DEPTH = 256 + ROLE_LENGTH = "byte length" + ROLE_ARRAY = "array element" + ROLE_HASH = "hash entry" + ROLE_IVAR = "instance variable" + ROLE_STRUCT = "struct member" + ROLE_BIGNUM = "bignum word" + SINK_TAGS = [TAG_USERDEF, TAG_USERMARSHAL, TAG_DATA].freeze SINK_METHODS = { diff --git a/PROJECTS/beginner/deserialization-gadget-lab/lib/rube/marshal/errors.rb b/PROJECTS/beginner/deserialization-gadget-lab/lib/rube/marshal/errors.rb index 5817ad95..4b3cd95b 100644 --- a/PROJECTS/beginner/deserialization-gadget-lab/lib/rube/marshal/errors.rb +++ b/PROJECTS/beginner/deserialization-gadget-lab/lib/rube/marshal/errors.rb @@ -16,5 +16,7 @@ module Rube class InvalidLinkError < StreamError; end class DepthLimitError < StreamError; end + + class MalformedCountError < StreamError; end end end diff --git a/PROJECTS/beginner/deserialization-gadget-lab/lib/rube/marshal/node.rb b/PROJECTS/beginner/deserialization-gadget-lab/lib/rube/marshal/node.rb index 34fa1f03..eb51757a 100644 --- a/PROJECTS/beginner/deserialization-gadget-lab/lib/rube/marshal/node.rb +++ b/PROJECTS/beginner/deserialization-gadget-lab/lib/rube/marshal/node.rb @@ -4,7 +4,7 @@ module Rube module Marshal class Node - attr_reader :type, :tag, :children, :instance_variables_map + attr_reader :type, :tag, :children, :instance_variables_map, :auxiliary attr_accessor :value, :class_name def initialize(type:, tag: nil, value: nil, class_name: nil) @@ -14,6 +14,7 @@ module Rube @class_name = class_name @children = [] @instance_variables_map = {} + @auxiliary = [] end def sink? @@ -34,6 +35,7 @@ module Rube yield self children.each { |child| child.each(&block) } instance_variables_map.each_value { |child| child.each(&block) } + auxiliary.each { |child| child.each(&block) } end end diff --git a/PROJECTS/beginner/deserialization-gadget-lab/lib/rube/marshal/parser.rb b/PROJECTS/beginner/deserialization-gadget-lab/lib/rube/marshal/parser.rb index 1eafff04..c7a7afe1 100644 --- a/PROJECTS/beginner/deserialization-gadget-lab/lib/rube/marshal/parser.rb +++ b/PROJECTS/beginner/deserialization-gadget-lab/lib/rube/marshal/parser.rb @@ -31,6 +31,7 @@ module Rube end def take(count) + raise MalformedCountError, "negative byte count #{count}" if count.negative? raise TruncatedStreamError, "wanted #{count} bytes, had #{remaining}" if count > remaining slice = source.byteslice(@position, count) @@ -38,6 +39,13 @@ module Rube slice end + def read_count(role) + value = read_fixnum + raise MalformedCountError, "negative #{role} count #{value}" if value.negative? + + value + end + def take_byte take(1).unpack1("C") end @@ -73,7 +81,7 @@ module Rube end def read_counted_bytes - take(read_fixnum) + take(read_count(ROLE_LENGTH)) end def register(node) @@ -136,7 +144,7 @@ module Rube def read_bignum(tag) negative = take(1) == BIGNUM_SIGN_NEGATIVE - magnitude = little_endian(take(read_fixnum * BIGNUM_WORD_BYTES)) + magnitude = little_endian(take(read_count(ROLE_BIGNUM) * BIGNUM_WORD_BYTES)) Node.new(type: :bignum, tag: tag, value: negative ? -magnitude : magnitude) end @@ -158,13 +166,13 @@ module Rube def read_array(tag, depth) node = register(Node.new(type: :array, tag: tag)) - read_fixnum.times { node.children << read_value(depth + 1) } + read_count(ROLE_ARRAY).times { node.children << read_value(depth + 1) } node end def read_hash(tag, depth) node = register(Node.new(type: :hash, tag: tag)) - read_fixnum.times { node.children << read_pair(depth) } + read_count(ROLE_HASH).times { node.children << read_pair(depth) } node.children << read_value(depth + 1) if tag == TAG_HASH_DEFAULT node end @@ -178,8 +186,9 @@ module Rube def read_ivar(tag, depth) inner = read_value(depth) - read_fixnum.times do + read_count(ROLE_IVAR).times do name = read_value(depth + 1) + inner.auxiliary << name inner.instance_variables_map[name.value] = read_value(depth + 1) end inner @@ -187,9 +196,12 @@ module Rube def read_object(tag, depth) node = register(Node.new(type: :object, tag: tag)) - node.class_name = read_value(depth + 1).value.to_s - read_fixnum.times do + class_node = read_value(depth + 1) + node.class_name = class_node.value.to_s + node.auxiliary << class_node + read_count(ROLE_IVAR).times do name = read_value(depth + 1) + node.auxiliary << name node.instance_variables_map[name.value] = read_value(depth + 1) end node @@ -198,7 +210,7 @@ module Rube def read_struct(tag, depth) node = register(Node.new(type: :struct, tag: tag)) node.class_name = read_value(depth + 1).value.to_s - read_fixnum.times { node.children << read_pair(depth) } + read_count(ROLE_STRUCT).times { node.children << read_pair(depth) } node end diff --git a/PROJECTS/beginner/deserialization-gadget-lab/test/marshal/parser_test.rb b/PROJECTS/beginner/deserialization-gadget-lab/test/marshal/parser_test.rb index 4ce6c90f..8d2d1463 100644 --- a/PROJECTS/beginner/deserialization-gadget-lab/test/marshal/parser_test.rb +++ b/PROJECTS/beginner/deserialization-gadget-lab/test/marshal/parser_test.rb @@ -159,6 +159,66 @@ module Rube assert_raises(TruncatedStreamError) { parse("\x04\x08i\x03\x01") } end + def test_rejects_negative_array_count + assert_raises(MalformedCountError) { parse("\x04\x08[\xFA") } + end + + def test_rejects_negative_hash_count + assert_raises(MalformedCountError) { parse("\x04\x08{\xFA") } + end + + def test_rejects_negative_bignum_word_count + assert_raises(MalformedCountError) { parse("\x04\x08l+\xFA") } + end + + def test_rejects_negative_instance_variable_count + assert_raises(MalformedCountError) { parse("\x04\x08I\"\x06a\xFA") } + end + + def test_rejects_negative_string_length + assert_raises(MalformedCountError) { parse("\x04\x08\"\xFA") } + end + + def test_rejects_negative_struct_member_count + assert_raises(MalformedCountError) { parse("\x04\x08S:\x06A\xFA") } + end + + def test_negative_counts_never_rewind_the_cursor + parser = Parser.new("\x04\x08\"\xFA") + assert_raises(MalformedCountError) { parser.parse } + end + + def test_every_malformed_count_stays_inside_the_stream_error_hierarchy + ["\x04\x08[\xFA", "\x04\x08{\xFA", "\x04\x08l+\xFA", "\x04\x08\"\xFA"].each do |blob| + parse(blob) + flunk "expected #{blob.inspect} to be rejected" + rescue StreamError + pass + rescue StandardError => e + flunk "#{blob.inspect} leaked #{e.class} outside StreamError" + end + end + + def test_rejects_negative_object_link_index + assert_raises(InvalidLinkError) { parse("\x04\x08[\x06@\xFA") } + end + + def test_rejects_negative_symlink_index + assert_raises(InvalidLinkError) { parse("\x04\x08[\x07:\x06a;\xFA") } + end + + def test_sink_in_an_instance_variable_name_position_is_still_reported + result = parse("\x04\x08I\"\x06a\x06u:\x09Evil\x06x0") + assert_includes result.class_names, "Evil" + assert_equal ["Evil#_load"], result.sinks.map { |s| "#{s.class_name}##{s.sink_method}" } + end + + def test_class_name_node_is_traversable + result = parse(::Marshal.dump(Fixture.new)) + symbols = result.nodes.select { |n| n.type == :symbol } + refute_empty symbols + end + def test_rejects_depth_beyond_limit deep = "\x04\x08" + ("[\x06" * (Constants::DEFAULT_MAX_DEPTH + 5)) + "0" assert_raises(DepthLimitError) { parse(deep) }