From 32f06ce98ce0cef15e55514d995dd8887fc49074 Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Tue, 27 Oct 2026 16:22:10 +0900 Subject: [PATCH] Don't retag the shared fstring via TYPE_UCLASS in Marshal.load This is not a real-world problem. Marshal.dump never emits this payload. It can only be hand-crafted. Loading untrusted Marshal data is already unsafe. This fix only avoids false positives from security scanners. TYPE_UCLASS ('C') relabels the wrapped object's class with RBASIC_SET_CLASS. It assumes the object was freshly allocated during load. But with freeze: true, r_leave() interns a wrapped bare String first, and interning a bare String returns the shared global fstring. So the relabel hits that global object instead, corrupting every other reference to the same interned string. Load the wrapped object as a partial so r_leave() is deferred, and finalize it only after the class has been set. The String is then interned as a non-bare String, which keeps the result a distinct object instead of the shared fstring. Co-Authored-By: Claude Opus 4.8 --- marshal.c | 7 ++++++- test/ruby/test_marshal.rb | 20 ++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/marshal.c b/marshal.c index 4e5aac1194f..09e5936004b 100644 --- a/marshal.c +++ b/marshal.c @@ -2012,7 +2012,11 @@ r_object_for(struct load_arg *arg, bool partial, int *ivp, VALUE klass, VALUE ex hash_new_capa = rb_ident_hash_new_capa; goto type_hash; } - v = r_object_for(arg, partial, 0, c, extmod, type); + /* Load as a partial and run r_leave() only after the class is set + * below: interning a bare frozen String returns the shared global + * fstring, and retagging that shared object would corrupt every + * other reference to it. */ + v = r_object_for(arg, true, 0, c, extmod, type); if (RB_SPECIAL_CONST_P(v) || RB_TYPE_P(v, T_OBJECT) || RB_TYPE_P(v, T_CLASS) || RB_TYPE_P(v, T_MODULE)) { goto format_error; } @@ -2027,6 +2031,7 @@ r_object_for(struct load_arg *arg, bool partial, int *ivp, VALUE klass, VALUE ex rb_class_name(c)); } RBASIC_SET_CLASS(v, c); + v = r_leave(v, arg, partial); } break; diff --git a/test/ruby/test_marshal.rb b/test/ruby/test_marshal.rb index f10479b7fac..7a5e3ffab6f 100644 --- a/test/ruby/test_marshal.rb +++ b/test/ruby/test_marshal.rb @@ -1123,5 +1123,25 @@ class TestMarshal < Test::Unit::TestCase assert_same objects[1], objects[1].instance_variable_get(:@self) assert_same objects[0].instance_variable_get(:@self), objects[1].instance_variable_get(:@self) end + + class StringSubclass < String + end + + def test_uclass_fstring + interned = -("fstring".b) + assert_instance_of(String, interned) + + # TYPE_UCLASS ('C') wrapping a bare (binary, no ivar) String. + payload = "\x04\x08C".b + + Marshal.dump(StringSubclass.name.to_sym)[2..] + + Marshal.dump("fstring".b)[2..] + loaded = Marshal.load(payload, freeze: true) + + assert_instance_of(StringSubclass, loaded) + assert_predicate(loaded, :frozen?) + assert_not_same(interned, loaded) + assert_instance_of(String, interned) + assert_instance_of(String, -("fstring".b)) + end end end