From 50640852a08733e0bee1dc7d9f691b353d31a4c9 Mon Sep 17 00:00:00 2001
From: Matt Brictson <matt@mattbrictson.com>
Date: Tue, 21 Feb 2023 10:14:34 -0800
Subject: Allow `Marshal.load` to be disabled for `Plist.parse_xml` (#61)

* Allow `Marshal.load` to be disabled for `Plist.parse_xml`

* Document the new :marshal option
---
 README.rdoc         |  9 +++++++--
 lib/plist/parser.rb | 29 ++++++++++++++++++-----------
 test/test_parser.rb | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+), 13 deletions(-)

diff --git a/README.rdoc b/README.rdoc
index 689a512..2fd54ee 100644
--- a/README.rdoc
+++ b/README.rdoc
@@ -9,12 +9,17 @@ Plist is a library to manipulate Property List files, also known as plists.  It
 
 === Security considerations
 
-Plist.parse_xml uses Marshal.load for <data/> attributes. If the <data/> attribute contains malicious data, an attacker can gain code execution.
-You should never use Plist.parse_xml with untrusted plists!
+By default, Plist.parse_xml uses Marshal.load for <data/> attributes. If the <data/> attribute contains malicious data, an attacker can gain code execution.
+
+You should never use the default Plist.parse_xml with untrusted plists!
+
+To disable the Marshal.load behavior, use <tt>marshal: false</tt>. This will return the raw binary <data> contents as an IO object instead of attempting to unmarshal it.
 
 === Parsing
 
   result = Plist.parse_xml('path/to/example.plist')
+  # or
+  result = Plist.parse_xml('path/to/example.plist', marshal: false)
 
   result.class
   => Hash
diff --git a/lib/plist/parser.rb b/lib/plist/parser.rb
index f325988..6b83ed4 100755
--- a/lib/plist/parser.rb
+++ b/lib/plist/parser.rb
@@ -26,8 +26,13 @@ module Plist
   # can't be parsed into a Time object, please create an issue
   # attaching your plist file at https://github.com/patsplat/plist/issues
   # so folks can implement the proper support.
-  def self.parse_xml(filename_or_xml)
-    listener = Listener.new
+  #
+  # By default, <data> will be assumed to be a marshaled Ruby object and
+  # interpreted with <tt>Marshal.load</tt>. Pass <tt>marshal: false</tt>
+  # to disable this behavior and return the raw binary data as an IO
+  # object instead.
+  def self.parse_xml(filename_or_xml, options={})
+    listener = Listener.new(options)
     # parser = REXML::Parsers::StreamParser.new(File.new(filename), listener)
     parser = StreamParser.new(filename_or_xml, listener)
     parser.parse
@@ -39,13 +44,14 @@ module Plist
 
     attr_accessor :result, :open
 
-    def initialize
+    def initialize(options={})
       @result = nil
       @open   = []
+      @options = { :marshal => true }.merge(options).freeze
     end
 
     def tag_start(name, attributes)
-      @open.push PTag.mappings[name].new
+      @open.push PTag.mappings[name].new(@options)
     end
 
     def text(contents)
@@ -154,9 +160,10 @@ module Plist
       mappings[key] = sub_class
     end
 
-    attr_accessor :text, :children
-    def initialize
+    attr_accessor :text, :children, :options
+    def initialize(options)
       @children = []
+      @options = options
     end
 
     def to_ruby
@@ -244,13 +251,13 @@ module Plist
     def to_ruby
       data = Base64.decode64(text.gsub(/\s+/, '')) unless text.nil?
       begin
-        return Marshal.load(data)
+        return Marshal.load(data) if options[:marshal]
       rescue Exception
-        io = StringIO.new
-        io.write data
-        io.rewind
-        return io
       end
+      io = StringIO.new
+      io.write data
+      io.rewind
+      io
     end
   end
 end
diff --git a/test/test_parser.rb b/test/test_parser.rb
index 7b692a5..2ff4db5 100755
--- a/test/test_parser.rb
+++ b/test/test_parser.rb
@@ -122,4 +122,56 @@ class TestParser < Test::Unit::TestCase
       Plist.parse_xml('<string>Fish &amp; Chips</tring>')
     end
   end
+
+  def test_marshal_is_enabled_by_default_meaning_data_is_passed_to_marshal_load
+    plist = <<-PLIST.strip
+      <plist version="1.0">
+      <dict>
+        <key>Token</key>
+        <data>
+        BANUb2tlbg==
+        </data>
+      </dict>
+      </plist>
+    PLIST
+
+    data = Plist.parse_xml(plist)
+    # "BANUb2tlbg==" is interpreted as `true` when base64 decoded and passed to Marshal.load
+    assert_equal(true, data["Token"])
+  end
+
+  def test_data_unrecognized_by_marshal_load_is_returned_as_raw_binary
+    jpeg = File.read(File.expand_path("../assets/example_data.jpg", __FILE__))
+    plist = <<-PLIST.strip
+      <plist version="1.0">
+      <dict>
+        <key>Token</key>
+        <data>
+        #{Base64.encode64(jpeg)}
+        </data>
+      </dict>
+      </plist>
+    PLIST
+
+    data = Plist.parse_xml(plist)
+    assert_kind_of(StringIO, data["Token"])
+    assert_equal(jpeg, data["Token"].read)
+  end
+
+  def test_marshal_can_be_disabled_so_that_data_is_always_returned_as_raw_binary
+    plist = <<-PLIST.strip
+      <plist version="1.0">
+      <dict>
+        <key>Token</key>
+        <data>
+        BANUb2tlbg==
+        </data>
+      </dict>
+      </plist>
+    PLIST
+
+    data = Plist.parse_xml(plist, marshal: false)
+    assert_kind_of(StringIO, data["Token"])
+    assert_equal("\x04\x03Token", data["Token"].read)
+  end
 end
-- 
cgit v1.2.1