diff --git a/internal/vm/yaml.go b/internal/vm/yaml.go index 0e2d4e08..78b51301 100644 --- a/internal/vm/yaml.go +++ b/internal/vm/yaml.go @@ -43,18 +43,31 @@ func (vm *VM) registerYAML() { psych.smethods[m] = &Method{name: m, owner: psych, native: notImpl(m)} } - // YAML.load(source[, ...]) / Psych.load parse a YAML document string to a tree - // of Ruby values. unsafe_load shares the same implementation. Leading + // YAML.load(source[, ...]) / Psych.load is RESTRICTED, and does not share an + // implementation with unsafe_load. Psych 4 inverted the two: psych.rb:369 is + // `def self.load yaml, permitted_classes: [Symbol], ...` delegating straight + // to safe_load, which is the change Psych made precisely because `load` is + // the call people habitually write. Binding it to the unrestricted loader was + // right for Psych 3 and wrong from Psych 4 on. See psychLoadArgs. Leading // keyword/positional options Psych accepts (filename, symbolize_names, …) are // tolerated and ignored. loadFn := func(vm *VM, _ object.Value, args []object.Value, _ *Proc) object.Value { if len(args) == 0 { raise("ArgumentError", "wrong number of arguments (given 0, expected 1..)") } - return yamlLoad(vm, yamlSourceArg(args[0])) + return yamlSafeLoad(vm, yamlSourceArg(args[0]), psychLoadArgs(args[1:])) } psych.smethods["load"] = &Method{name: "load", owner: psych, native: loadFn} - psych.smethods["unsafe_load"] = &Method{name: "unsafe_load", owner: psych, native: loadFn} + + // YAML.unsafe_load is the unrestricted entry point (psych.rb:272), and the one + // a caller that genuinely wants arbitrary classes back must now name. + unsafeLoadFn := func(vm *VM, _ object.Value, args []object.Value, _ *Proc) object.Value { + if len(args) == 0 { + raise("ArgumentError", "wrong number of arguments (given 0, expected 1..)") + } + return yamlLoad(vm, yamlSourceArg(args[0])) + } + psych.smethods["unsafe_load"] = &Method{name: "unsafe_load", owner: psych, native: unsafeLoadFn} // YAML.safe_load(source[, permitted_classes: [...], permitted_symbols: [...]]) // restricts which classes a document may materialise. Psych's signature is @@ -69,15 +82,31 @@ func (vm *VM) registerYAML() { } psych.smethods["safe_load"] = &Method{name: "safe_load", owner: psych, native: safeLoadFn} - // YAML.load_file(path[, ...]) reads the file and parses its contents. + // YAML.load_file(path[, ...]) reads the file and parses its contents. It + // delegates to load (psych.rb:716), so it carries load's restriction and its + // keyword defaults -- measured: load_file over ":s\n" with + // permitted_classes: [G] raises Psych::DisallowedClass under MRI, exactly as + // load does, because the explicit list replaces the [Symbol] default. loadFileFn := func(vm *VM, _ object.Value, args []object.Value, _ *Proc) object.Value { if len(args) == 0 { raise("ArgumentError", "wrong number of arguments (given 0, expected 1..)") } - return yamlLoad(vm, yamlReadFile(args[0])) + return yamlSafeLoad(vm, yamlReadFile(args[0]), psychLoadArgs(args[1:])) } psych.smethods["load_file"] = &Method{name: "load_file", owner: psych, native: loadFileFn} + // YAML.unsafe_load_file(path) is unsafe_load over a file (psych.rb:694). It + // is added here with the restriction, not before it: without it a caller that + // legitimately wants objects back from a file -- which is what load_file used + // to give them -- would have no spelling at all to migrate to. + unsafeLoadFileFn := func(vm *VM, _ object.Value, args []object.Value, _ *Proc) object.Value { + if len(args) == 0 { + raise("ArgumentError", "wrong number of arguments (given 0, expected 1..)") + } + return yamlLoad(vm, yamlReadFile(args[0])) + } + psych.smethods["unsafe_load_file"] = &Method{name: "unsafe_load_file", owner: psych, native: unsafeLoadFileFn} + // YAML.safe_load_file(path[, permitted_classes: [...]]) is safe_load over a file. safeLoadFileFn := func(vm *VM, _ object.Value, args []object.Value, _ *Proc) object.Value { if len(args) == 0 { @@ -216,6 +245,43 @@ func psychSafeArgs(rest []object.Value) psychSafeOpts { return o } +// psychLoadArgs is psychSafeArgs with Psych.load's own default allow-list. +// +// psych.rb:369 declares `def self.load yaml, permitted_classes: [Symbol], ...` +// and delegates to safe_load, so load differs from safe_load ONLY in that +// default. An explicit permitted_classes REPLACES it rather than adding to it, +// because it is an ordinary keyword default -- measured against MRI 4.0.5: +// +// load(":s") => :s +// load(":s", permitted_classes: [G]) => Psych::DisallowedClass +// load(":s", permitted_classes: [G, Symbol]) => :s +// +// which is why the seeding below is conditional on the keyword being ABSENT +// rather than on the resulting list being empty. Absent and empty are different +// policies here: absent means "use load's default", empty means "deny +// everything", and psychSafeOpts exists to keep them distinguishable. +func psychLoadArgs(rest []object.Value) psychSafeOpts { + o := psychSafeArgs(rest) + if !psychHasKeyword(rest, "permitted_classes") { + o.classes["Symbol"] = true + } + return o +} + +// psychHasKeyword reports whether the trailing keyword hash carries key at all, +// which is not the same question as whether its value is empty. +func psychHasKeyword(rest []object.Value, key string) bool { + if len(rest) == 0 { + return false + } + h, ok := rest[len(rest)-1].(*object.Hash) + if !ok { + return false + } + _, ok = h.Get(object.Symbol(key)) + return ok +} + // psychNameList reads one keyword from h as a list of names. A Class / Module // element contributes its name and any other element its to_s, matching how // Psych accepts `[Date]` and `["Date"]` alike (`permitted_classes.map(&:to_s)`). diff --git a/internal/vm/yaml_load_test.go b/internal/vm/yaml_load_test.go index 0d4a7db0..967f8812 100644 --- a/internal/vm/yaml_load_test.go +++ b/internal/vm/yaml_load_test.go @@ -136,11 +136,14 @@ func TestYAMLLoadBlockScalar(t *testing.T) { } // TestYAMLLoadTime covers the ISO-8601 timestamp the emitter writes loading back -// to a Time, in both the "Z" and numeric-offset forms. +// to a Time, in both the "Z" and numeric-offset forms. It goes through +// unsafe_load because what is under test is the LOADER's ability to build a +// Time, not load's policy: load permits only Symbol by default, and MRI refuses +// a timestamp through a bare load for the same reason. func TestYAMLLoadTime(t *testing.T) { cases := []struct{ src, want string }{ - {`t = YAML.load("--- 1970-01-01 00:00:00.000000000 Z\n"); p t.class; p t.to_i`, "Time\n0\n"}, - {`t = YAML.load("--- 1970-01-01 01:00:00.000000000 +01:00\n"); p t.to_i`, "0\n"}, + {`t = YAML.unsafe_load("--- 1970-01-01 00:00:00.000000000 Z\n"); p t.class; p t.to_i`, "Time\n0\n"}, + {`t = YAML.unsafe_load("--- 1970-01-01 01:00:00.000000000 +01:00\n"); p t.to_i`, "0\n"}, } for _, c := range cases { if got := eval(t, c.src); got != c.want { @@ -149,12 +152,19 @@ func TestYAMLLoadTime(t *testing.T) { } } +// The allow-list is not a concession to our own restriction: MRI refuses this +// document through a bare YAML.load too, because Time is not in load's [Symbol] +// default. Measured on 4.0.5 -- load(doc) raises Psych::DisallowedClass, while +// load(doc, permitted_classes: [Symbol, Time]) returns the hash. Since this is +// Puppet's state shape, Puppet cannot be reading it with a bare load either: +// Puppet::Util::Yaml passes its own allow-list, which is what this mirrors. +// // TestYAMLLoadStateRoundTrip covers the exact shape Puppet's state.yaml uses (a // String -> {Symbol -> Time} mapping), which must round-trip through dump/load. func TestYAMLLoadStateRoundTrip(t *testing.T) { src := `require "yaml" h = {"File[/x]" => {:checked => Time.at(0).utc, :synced => Time.at(0).utc}} -r = YAML.load(YAML.dump(h)) +r = YAML.load(YAML.dump(h), permitted_classes: [Symbol, Time]) p r.keys p r["File[/x]"].keys p r["File[/x]"][:checked].to_i` @@ -164,6 +174,10 @@ p r["File[/x]"][:checked].to_i` } } +// These go through unsafe_load because the subject is the LOADER's ability to +// rebuild a tagged object, not load's policy: since Psych 4, load permits only +// Symbol by default and refuses !ruby/object: exactly as MRI does. +// // TestYAMLLoadRubyObject covers loading a !ruby/object: mapping back into an // instance of the named class, with the mapping entries as instance variables — // including a class the program already defines and one it does not (which the @@ -172,26 +186,26 @@ func TestYAMLLoadRubyObject(t *testing.T) { cases := []struct{ src, want string }{ // Known class: ivars are restored. {`class Foo; attr_reader :a, :b; end -o = YAML.load("--- !ruby/object:Foo\na: 1\nb: hi\n") +o = YAML.unsafe_load("--- !ruby/object:Foo\na: 1\nb: hi\n") p o.class p o.a p o.b`, "Foo\n1\n\"hi\"\n"}, // Bare !ruby/object loads as an Object instance. - {`o = YAML.load("--- !ruby/object\na: 1\n") + {`o = YAML.unsafe_load("--- !ruby/object\na: 1\n") p o.instance_variable_get(:@a)`, "1\n"}, // Unknown class: a placeholder class of that name is created. - {`o = YAML.load("--- !ruby/object:Quux\nx: 5\n") + {`o = YAML.unsafe_load("--- !ruby/object:Quux\nx: 5\n") p o.class.name p o.instance_variable_get(:@x)`, "\"Quux\"\n5\n"}, // A qualified unknown class name. - {`o = YAML.load("--- !ruby/object:A::B\nx: 1\n") + {`o = YAML.unsafe_load("--- !ruby/object:A::B\nx: 1\n") p o.class.name`, "\"A::B\"\n"}, // An empty-bodied object (the "tag {}" inline form). {`class Empty; end -o = YAML.load("--- !ruby/object:Empty {}\n") +o = YAML.unsafe_load("--- !ruby/object:Empty {}\n") p o.class`, "Empty\n"}, // A symbol-keyed object mapping (Psych object ivars). - {`o = YAML.load("--- !ruby/object:Quux2\n:x: 1\n") + {`o = YAML.unsafe_load("--- !ruby/object:Quux2\n:x: 1\n") p o.instance_variable_get(:@x)`, "1\n"}, } for _, c := range cases { @@ -206,7 +220,7 @@ p o.instance_variable_get(:@x)`, "1\n"}, func TestYAMLLoadAnchors(t *testing.T) { src := `class Node; attr_reader :n; end doc = "---\n- &1 !ruby/object:Node\n n: 7\n- *1\n" -a = YAML.load(doc) +a = YAML.unsafe_load(doc) p a[0].equal?(a[1]) p a[1].n` want := "true\n7\n" diff --git a/internal/vm/yaml_object_test.go b/internal/vm/yaml_object_test.go index 83b22327..1d222589 100644 --- a/internal/vm/yaml_object_test.go +++ b/internal/vm/yaml_object_test.go @@ -201,7 +201,7 @@ p0 = Person.new p0.instance_variable_set(:@name, "Ada") p0.instance_variable_set(:@age, 36) doc = [p0, p0].to_yaml -r = YAML.load(doc) +r = YAML.unsafe_load(doc) p r[0].class p r[0].name p r[0].age diff --git a/internal/vm/yaml_safe_load_test.go b/internal/vm/yaml_safe_load_test.go index eed0eccf..f0eb86fc 100644 --- a/internal/vm/yaml_safe_load_test.go +++ b/internal/vm/yaml_safe_load_test.go @@ -253,24 +253,52 @@ rescue Psych::DisallowedClass => e puts "refused: #{e.message}" end puts YAML.safe_load_file(` + rubyStr(path) + `, permitted_classes: [Gadget]).class -puts YAML.load_file(` + rubyStr(path) + `).class` +puts YAML.unsafe_load_file(` + rubyStr(path) + `).class` want := "refused: Tried to load unspecified class: Gadget\nGadget\nGadget\n" if got := eval(t, src); got != want { t.Errorf("got %q want %q", got, want) } } -// TestYAMLLoadStaysUnrestricted checks the fix did not leak into the unsafe -// entry points. load / unsafe_load are documented as unsafe and Puppet's -// persistence calls them; a restriction there would be a different bug. -func TestYAMLLoadStaysUnrestricted(t *testing.T) { +// TestYAMLLoadIsRestrictedAndUnsafeLoadIsNot replaces a test that asserted the +// defect. Its predecessor, TestYAMLLoadStaysUnrestricted, REQUIRED +// YAML.load("--- !ruby/object:Gadget") to return a Gadget, reasoning that load +// is "documented as unsafe" and that Puppet's persistence calls it. +// +// Psych 4 inverted that: psych.rb:369 makes load delegate to safe_load with +// permitted_classes: [Symbol] -- the change Psych made precisely because load is +// the call people habitually write. The Puppet premise does not hold either: +// MRI refuses Puppet's own state shape through a bare load (measured; see +// TestYAMLLoadStateRoundTrip), so Puppet passes its own allow-list rather than +// relying on load being unrestricted. +// +// The invariant is the pair. load restricts to Symbol; unsafe_load does not +// restrict at all, so a caller who genuinely wants objects back still has a +// spelling to name; and an explicit permitted_classes REPLACES the Symbol +// default rather than adding to it, which is the subtlety a merge-shaped +// implementation would get wrong. +func TestYAMLLoadIsRestrictedAndUnsafeLoadIsNot(t *testing.T) { src := `require "yaml" class Gadget; attr_accessor :foo; end -puts YAML.load("--- !ruby/object:Gadget\nfoo: bar\n").class -puts YAML.unsafe_load("--- !ruby/object:Gadget\nfoo: bar\n").foo +g = "--- !ruby/object:Gadget\nfoo: bar\n" +begin + YAML.load(g) + puts "load: NOT REFUSED" +rescue Psych::DisallowedClass => e + puts "load: #{e.message}" +end +puts YAML.unsafe_load(g).foo puts YAML.load(":sym").class -puts YAML.load("--- !ruby/regexp /ab/i\n").class` - want := "Gadget\nbar\nSymbol\nRegexp\n" +puts YAML.load(g, permitted_classes: [Gadget]).class +begin + YAML.load(":sym", permitted_classes: [Gadget]) + puts "explicit list: NOT REFUSED" +rescue Psych::DisallowedClass + puts "explicit list replaces the Symbol default" +end` + want := "load: Tried to load unspecified class: Gadget\n" + + "bar\nSymbol\nGadget\n" + + "explicit list replaces the Symbol default\n" if got := eval(t, src); got != want { t.Errorf("got %q want %q", got, want) } @@ -325,3 +353,45 @@ puts YAML.safe_load("` + tag + `", permitted_classes: [Object]).class` } } } + +// TestYAMLLoadArgShapesTheCoverageGateNamed covers two arms the per-function +// gate reported as newly below 100% after the restriction landed. Both were +// missing tests rather than dead code, which is the question worth asking +// first: each is reachable from ordinary Ruby. +// +// - psychHasKeyword returns false when the trailing argument is not a keyword +// hash at all. The binding tolerates and ignores leading positional options +// Psych accepts, so `YAML.load(doc, "anything")` takes that arm and the +// document still loads under load's [Symbol] default. +// - the arity guard inside unsafe_load_file, the entry point this change +// added. MRI raises there too, with its own wording. +// +// Noted while measuring, and deliberately not changed here: MRI refuses +// `YAML.load(doc, "notahash")` outright with `wrong number of arguments +// (given 2, expected 1)`, where this binding tolerates it. That divergence is +// the binding's documented choice and predates this change. +func TestYAMLLoadArgShapesTheCoverageGateNamed(t *testing.T) { + path := tmpDirSlash(t) + "/doc.yaml" + writeFileT(t, path, "a: 1\n") + src := `require "yaml" +p YAML.load("a: 1\n", "a positional that is not a keyword hash") +p YAML.load("a: 1\n", 42) +begin + YAML.unsafe_load_file +rescue ArgumentError => e + puts "unsafe_load_file: #{e.message}" +end +begin + YAML.unsafe_load +rescue ArgumentError => e + puts "unsafe_load: #{e.message}" +end +p YAML.unsafe_load_file(` + rubyStr(path) + `)` + want := "{\"a\" => 1}\n{\"a\" => 1}\n" + + "unsafe_load_file: wrong number of arguments (given 0, expected 1..)\n" + + "unsafe_load: wrong number of arguments (given 0, expected 1..)\n" + + "{\"a\" => 1}\n" + if got := eval(t, src); got != want { + t.Errorf("got %q want %q", got, want) + } +}