From 9674b1f7c1cdd3d1b732a7fe2174ef84b8524f77 Mon Sep 17 00:00:00 2001
From: Matthaus Litteken <matthaus@puppetlabs.com>
Date: Wed, 4 Apr 2012 14:37:18 -0700
Subject: [PATCH] 2.6 - Fixes for CVEs 2012-1906, 2012-1986 to 2012-1989

Squashed commit of the following:

commit f7829ec1f1b2c3def8e0eda09c22c3c1fed3a27f
Author: Patrick Carlisle <patrick@puppetlabs.com>
Date:   Wed Apr 4 13:36:06 2012 -0700

    Stub mktmpdir and remove_entry_secure in os x package providers

commit 7ac1ec8f75db05c55037556bdbb0be8ded9eb53e
Author: Patrick Carlisle <patrick@puppetlabs.com>
Date:   Tue Apr 3 16:17:18 2012 -0700

    (#13260) Spec test to verify that mktmpdir is used

commit 018020095cdaa39a869f0b0fede97e5f4f36d973
Author: Patrick Carlisle <patrick@puppetlabs.com>
Date:   Fri Mar 16 14:44:44 2012 -0700

    Refactor pkgdmg specs

    Refactor to a more current spec style. Several of these specs didn't actually
    test anything. They have either been deleted or made more specific.

    Conflicts:

    	lib/puppet/provider/package/pkgdmg.rb

commit c51447dfa81c9751fdc7663e0e91a9c9238abcaa
Author: Patrick Carlisle <patrick@puppetlabs.com>
Date:   Fri Mar 23 12:20:40 2012 -0700

    (#13260) Use mktmpdir when downloading packages

    This fixes a security vulnerability in the appdmg and pkgdmg providers where
    they would curl packages directly into /tmp and the install them, allowing an
    attacker to craft a symlink and overwrite arbitrary files or install arbitrary
    packages.

    Conflicts:

    	lib/puppet/provider/package/appdmg.rb
    	lib/puppet/provider/package/pkgdmg.rb

commit 568ded50ec6cc498ad32ff7f086d9f73b5d24c14
Author: Andrew Parker <andy@puppetlabs.com>
Date:   Mon Apr 2 11:47:17 2012 -0700

    Fix for bucket_path security vulnerability

    This is a fix for Bugs #13553, #13418, #13511. The bucket_path parameter
    allowed control over where the filebucket will try to read and write to.
    The only place available to stop this parameter is in the resolution
    from a URI to an indirectory terminus. The bucket_path is used
    internally for local filebuckets and so cannot be removed completely
    without a larger change to the design.

    Conflicts:

    	lib/puppet/network/http/api/v1.rb
    	spec/unit/network/http/api/v1_spec.rb

    Conflicts resolved by modifying the patch to fit the use of
    Puppet::Indirector::Request in the code.

commit 6bef2e622584d71e6643aeb6f852bb4773b84e9c
Author: Andrew Parker <andy@puppetlabs.com>
Date:   Mon Apr 2 10:44:26 2012 -0700

    Removed text/marshal support

    Removing text/marshal support in order to close the security
    vulnerability described in Bug #13552.
---
 lib/puppet/network/formats.rb             |   27 --------
 lib/puppet/network/http/api/v1.rb         |    1 +
 lib/puppet/provider/package/appdmg.rb     |   26 ++++----
 lib/puppet/provider/package/pkgdmg.rb     |   39 +++++------
 spec/unit/network/formats_spec.rb         |   43 ------------
 spec/unit/network/http/api/v1_spec.rb     |    8 +++
 spec/unit/provider/package/appdmg_spec.rb |   42 ++++++++++++
 spec/unit/provider/package/pkgdmg_spec.rb |  102 +++++++++++++++--------------
 8 files changed, 133 insertions(+), 155 deletions(-)
 create mode 100755 spec/unit/provider/package/appdmg_spec.rb

diff --git a/lib/puppet/network/formats.rb b/lib/puppet/network/formats.rb
index 4ca3240..dd19692 100644
--- a/lib/puppet/network/formats.rb
+++ b/lib/puppet/network/formats.rb
@@ -77,33 +77,6 @@ Puppet::Network::FormatHandler.create_serialized_formats(:b64_zlib_yaml) do
   end
 end
 
-
-Puppet::Network::FormatHandler.create(:marshal, :mime => "text/marshal") do
-  # Marshal doesn't need the class name; it's serialized.
-  def intern(klass, text)
-    Marshal.load(text)
-  end
-
-  # Marshal doesn't need the class name; it's serialized.
-  def intern_multiple(klass, text)
-    Marshal.load(text)
-  end
-
-  def render(instance)
-    Marshal.dump(instance)
-  end
-
-  # Marshal monkey-patches Array, so this works.
-  def render_multiple(instances)
-    Marshal.dump(instances)
-  end
-
-  # Everything's supported
-  def supported?(klass)
-    true
-  end
-end
-
 Puppet::Network::FormatHandler.create(:s, :mime => "text/plain", :extension => "txt")
 
 # A very low-weight format so it'll never get chosen automatically.
diff --git a/lib/puppet/network/http/api/v1.rb b/lib/puppet/network/http/api/v1.rb
index 9e51aae..219e0cb 100644
--- a/lib/puppet/network/http/api/v1.rb
+++ b/lib/puppet/network/http/api/v1.rb
@@ -28,6 +28,7 @@ module Puppet::Network::HTTP::API::V1
     method = indirection_method(http_method, indirection)
 
     params[:environment] = environment
+    params.delete(:bucket_path)
 
     raise ArgumentError, "No request key specified in #{uri}" if key == "" or key.nil?
 
diff --git a/lib/puppet/provider/package/appdmg.rb b/lib/puppet/provider/package/appdmg.rb
index a49700e..439043c 100644
--- a/lib/puppet/provider/package/appdmg.rb
+++ b/lib/puppet/provider/package/appdmg.rb
@@ -50,23 +50,24 @@ Puppet::Type.type(:package).provide(:appdmg, :parent => Puppet::Provider::Packag
 
   def self.installpkgdmg(source, name)
     unless source =~ /\.dmg$/i
-      self.fail "Mac OS X PKG DMG's must specificy a source string ending in .dmg"
+      self.fail "Mac OS X PKG DMG's must specify a source string ending in .dmg"
     end
     require 'open-uri'
     require 'facter/util/plist'
     cached_source = source
-    if %r{\A[A-Za-z][A-Za-z0-9+\-\.]*://} =~ cached_source
-      cached_source = "/tmp/#{name}"
-      begin
-        curl "-o", cached_source, "-C", "-", "-k", "-s", "--url", source
-        Puppet.debug "Success: curl transfered [#{name}]"
-      rescue Puppet::ExecutionFailure
-        Puppet.debug "curl did not transfer [#{name}].  Falling back to slower open-uri transfer methods."
-        cached_source = source
+    tmpdir = Dir.mktmpdir
+    begin
+      if %r{\A[A-Za-z][A-Za-z0-9+\-\.]*://} =~ cached_source
+        cached_source = File.join(tmpdir, name)
+        begin
+          curl "-o", cached_source, "-C", "-", "-k", "-L", "-s", "--url", source
+          Puppet.debug "Success: curl transfered [#{name}]"
+        rescue Puppet::ExecutionFailure
+          Puppet.debug "curl did not transfer [#{name}].  Falling back to slower open-uri transfer methods."
+          cached_source = source
+        end
       end
-    end
 
-    begin
       open(cached_source) do |dmg|
         xml_str = hdiutil "mount", "-plist", "-nobrowse", "-readonly", "-mountrandom", "/tmp", dmg.path
           ptable = Plist::parse_xml xml_str
@@ -87,8 +88,7 @@ Puppet::Type.type(:package).provide(:appdmg, :parent => Puppet::Provider::Packag
           end
       end
     ensure
-      # JJM Remove the file if open-uri didn't already do so.
-      File.unlink(cached_source) if File.exist?(cached_source)
+      FileUtils.remove_entry_secure(tmpdir, force=true)
     end
   end
 
diff --git a/lib/puppet/provider/package/pkgdmg.rb b/lib/puppet/provider/package/pkgdmg.rb
index 39e377d..8f6c39c 100644
--- a/lib/puppet/provider/package/pkgdmg.rb
+++ b/lib/puppet/provider/package/pkgdmg.rb
@@ -50,14 +50,7 @@ Puppet::Type.type(:package).provide :pkgdmg, :parent => Puppet::Provider::Packag
 
   def self.instances
     instance_by_name.collect do |name|
-
-      new(
-
-        :name => name,
-        :provider => :pkgdmg,
-
-        :ensure => :installed
-      )
+      new(:name => name, :provider => :pkgdmg, :ensure => :installed)
     end
   end
 
@@ -72,22 +65,23 @@ Puppet::Type.type(:package).provide :pkgdmg, :parent => Puppet::Provider::Packag
 
   def self.installpkgdmg(source, name)
     unless source =~ /\.dmg$/i || source =~ /\.pkg$/i
-      raise Puppet::Error.new("Mac OS X PKG DMG's must specificy a source string ending in .dmg or flat .pkg file")
+      raise Puppet::Error.new("Mac OS X PKG DMG's must specify a source string ending in .dmg or flat .pkg file")
     end
     require 'open-uri'
     cached_source = source
-    if %r{\A[A-Za-z][A-Za-z0-9+\-\.]*://} =~ cached_source
-      cached_source = "/tmp/#{name}"
-      begin
-        curl "-o", cached_source, "-C", "-", "-k", "-s", "--url", source
-        Puppet.debug "Success: curl transfered [#{name}]"
-      rescue Puppet::ExecutionFailure
-        Puppet.debug "curl did not transfer [#{name}].  Falling back to slower open-uri transfer methods."
-        cached_source = source
+    tmpdir = Dir.mktmpdir
+    begin
+      if %r{\A[A-Za-z][A-Za-z0-9+\-\.]*://} =~ cached_source
+        cached_source = File.join(tmpdir, name)
+        begin
+          curl "-o", cached_source, "-C", "-", "-k", "-L", "-s", "--url", source
+          Puppet.debug "Success: curl transfered [#{name}]"
+        rescue Puppet::ExecutionFailure
+          Puppet.debug "curl did not transfer [#{name}].  Falling back to slower open-uri transfer methods."
+          cached_source = source
+        end
       end
-    end
 
-    begin
       if source =~ /\.dmg$/i
         File.open(cached_source) do |dmg|
           xml_str = hdiutil "mount", "-plist", "-nobrowse", "-readonly", "-noidme", "-mountrandom", "/tmp", dmg.path
@@ -110,14 +104,11 @@ Puppet::Type.type(:package).provide :pkgdmg, :parent => Puppet::Provider::Packag
             end
           end
         end
-      elsif source =~ /\.pkg$/i
-        installpkg(cached_source, name, source)
       else
-        raise Puppet::Error.new("Mac OS X PKG DMG's must specificy a source string ending in .dmg or flat .pkg file")
+        installpkg(cached_source, name, source)
       end
     ensure
-      # JJM Remove the file if open-uri didn't already do so.
-      File.unlink(cached_source) if File.exist?(cached_source)
+      FileUtils.remove_entry_secure(tmpdir, force=true)
     end
   end
 
diff --git a/spec/unit/network/formats_spec.rb b/spec/unit/network/formats_spec.rb
index 2c58a05..c6f2b32 100755
--- a/spec/unit/network/formats_spec.rb
+++ b/spec/unit/network/formats_spec.rb
@@ -163,49 +163,6 @@ describe "Puppet Network Format" do
 
   end
 
-  it "should include a marshal format" do
-    Puppet::Network::FormatHandler.format(:marshal).should_not be_nil
-  end
-
-  describe "marshal" do
-    before do
-      @marshal = Puppet::Network::FormatHandler.format(:marshal)
-    end
-
-    it "should have its mime type set to text/marshal" do
-      Puppet::Network::FormatHandler.format(:marshal).mime.should == "text/marshal"
-    end
-
-    it "should be supported on Strings" do
-      @marshal.should be_supported(String)
-    end
-
-    it "should render by calling 'Marshal.dump' on the instance" do
-      instance = mock 'instance'
-      Marshal.expects(:dump).with(instance).returns "foo"
-      @marshal.render(instance).should == "foo"
-    end
-
-    it "should render multiple instances by calling 'to_marshal' on the array" do
-      instances = [mock('instance')]
-
-      Marshal.expects(:dump).with(instances).returns "foo"
-      @marshal.render_multiple(instances).should == "foo"
-    end
-
-    it "should intern by calling 'Marshal.load'" do
-      text = "foo"
-      Marshal.expects(:load).with("foo").returns "bar"
-      @marshal.intern(String, text).should == "bar"
-    end
-
-    it "should intern multiples by calling 'Marshal.load'" do
-      text = "foo"
-      Marshal.expects(:load).with("foo").returns "bar"
-      @marshal.intern_multiple(String, text).should == "bar"
-    end
-  end
-
   describe "plaintext" do
     before do
       @text = Puppet::Network::FormatHandler.format(:s)
diff --git a/spec/unit/network/http/api/v1_spec.rb b/spec/unit/network/http/api/v1_spec.rb
index 9a8780c..e3eaab6 100644
--- a/spec/unit/network/http/api/v1_spec.rb
+++ b/spec/unit/network/http/api/v1_spec.rb
@@ -43,6 +43,14 @@ describe Puppet::Network::HTTP::API::V1 do
       @tester.uri2indirection("GET", "/env/foo/bar", {:environment => "otherenv"}).environment.should == Puppet::Node::Environment.new("env")
     end
 
+    it "should not pass a buck_path parameter through (See Bugs #13553, #13518, #13511)" do
+      @tester.uri2indirection("GET", "/env/foo/bar", { :bucket_path => "/malicious/path" }).options.should_not include({ :bucket_path => "/malicious/path" })
+    end
+
+    it "should pass allowed parameters through" do
+      @tester.uri2indirection("GET", "/env/foo/bar", { :allowed_param => "value" }).options.should include({ :allowed_param => "value" })
+    end
+
     it "should use the second field of the URI as the indirection name" do
       @tester.uri2indirection("GET", "/env/foo/bar", {}).indirection_name.should == :foo
     end
diff --git a/spec/unit/provider/package/appdmg_spec.rb b/spec/unit/provider/package/appdmg_spec.rb
new file mode 100755
index 0000000..bde9efc
--- /dev/null
+++ b/spec/unit/provider/package/appdmg_spec.rb
@@ -0,0 +1,42 @@
+#!/usr/bin/env rspec
+require 'spec_helper'
+
+describe Puppet::Type.type(:package).provider(:appdmg) do
+  let(:resource) { Puppet::Type.type(:package).new(:name => 'foo', :provider => :appdmg) }
+  let(:provider) { described_class.new(resource) }
+
+  describe "when installing an appdmg" do
+    let(:fake_mountpoint) { "/tmp/dmg.foo" }
+    let(:empty_hdiutil_plist) { Plist::Emit.dump({}) }
+    let(:fake_hdiutil_plist) { Plist::Emit.dump({"system-entities" => [{"mount-point" => fake_mountpoint}]}) }
+
+    before do
+      fh = mock 'filehandle'
+      fh.stubs(:path).yields "/tmp/foo"
+      resource[:source] = "foo.dmg"
+      described_class.stubs(:open).yields fh
+      Dir.stubs(:mktmpdir).returns "/tmp/testtmp123"
+      FileUtils.stubs(:remove_entry_secure)
+    end
+
+    describe "from a remote source" do
+      let(:tmpdir) { "/tmp/good123" }
+
+      before :each do
+        resource[:source] = "http://fake.puppetlabs.com/foo.dmg"
+      end
+
+      it "should call tmpdir and use the returned directory" do
+        Dir.expects(:mktmpdir).returns tmpdir
+        Dir.stubs(:entries).returns ["foo.app"]
+        described_class.expects(:curl).with do |*args|
+          args[0] == "-o" and args[1].include? tmpdir
+        end
+        described_class.stubs(:hdiutil).returns fake_hdiutil_plist
+        described_class.expects(:installapp)
+
+        provider.install
+      end
+    end
+  end
+end
diff --git a/spec/unit/provider/package/pkgdmg_spec.rb b/spec/unit/provider/package/pkgdmg_spec.rb
index 1fd5b4a..6365bda 100755
--- a/spec/unit/provider/package/pkgdmg_spec.rb
+++ b/spec/unit/provider/package/pkgdmg_spec.rb
@@ -2,83 +2,89 @@
 
 Dir.chdir(File.dirname(__FILE__)) { (s = lambda { |f| File.exist?(f) ? require(f) : Dir.chdir("..") { s.call(f) } }).call("spec/spec_helper.rb") }
 
-provider = Puppet::Type.type(:package).provider(:pkgdmg)
+describe Puppet::Type.type(:package).provider(:pkgdmg) do
+  let(:resource) { Puppet::Type.type(:package).new(:name => 'foo', :provider => :pkgdmg) }
+  let(:provider) { described_class.new(resource) }
 
-describe provider do
-  before do
-    @resource = stub 'resource', :[] => "dummypkgdmg"
-    @provider = provider.new(@resource)
-
-    @fakemountpoint   = "/tmp/dmg.foo"
-    @fakepkgfile      = "/tmp/test.pkg"
-    @fakehdiutilinfo  = {"system-entities" => [{"mount-point" => @fakemountpoint}] }
-    @fakehdiutilplist = Plist::Emit.dump(@fakehdiutilinfo)
-
-    @hdiutilmountargs = ["mount", "-plist", "-nobrowse", "-readonly",
-      "-noidme", "-mountrandom", "/tmp"]
-  end
-
-  it "should not be versionable" do
-    provider.versionable?.should be_false
-  end
-
-  it "should not be uninstallable" do
-    provider.uninstallable?.should be_false
-  end
+  it { should_not be_versionable }
+  it { should_not be_uninstallable }
 
   describe "when installing it should fail when" do
-    it "no source is specified" do
-      @resource.stubs(:[]).with(:source).returns nil
-      lambda { @provider.install }.should raise_error(Puppet::Error)
+    before :each do
+      Puppet::Util.expects(:execute).never
     end
 
-    it "no name is specified" do
-      @resource.stubs(:[]).with(:name).returns nil
-      lambda { @provider.install }.should raise_error(Puppet::Error)
+    it "no source is specified" do
+      expect { provider.install }.should raise_error(Puppet::Error, /must specify a package source/)
     end
 
     it "the source does not end in .dmg or .pkg" do
-      @resource.stubs(:[]).with(:source).returns "notendingindotdmgorpkg"
-      lambda { @provider.install }.should raise_error(Puppet::Error)
-    end
-
-    it "a disk image with no system entities is mounted" do
-      @provider.stubs(:[]).with(:hdiutil).returns ""
-      lambda { @provider.install }.should raise_error(Puppet::Error)
+      resource[:source] = "bar"
+      expect { provider.install }.should raise_error(Puppet::Error, /must specify a source string ending in .*dmg.*pkg/)
     end
   end
 
   # These tests shouldn't be this messy. The pkgdmg provider needs work...
   describe "when installing a pkgdmg" do
+    let(:fake_mountpoint) { "/tmp/dmg.foo" }
+    let(:empty_hdiutil_plist) { Plist::Emit.dump({}) }
+    let(:fake_hdiutil_plist) { Plist::Emit.dump({"system-entities" => [{"mount-point" => fake_mountpoint}]}) }
+
     before do
       fh = mock 'filehandle'
       fh.stubs(:path).yields "/tmp/foo"
-      @resource.stubs(:[]).with(:source).returns "foo.dmg"
+      resource[:source] = "foo.dmg"
       File.stubs(:open).yields fh
+      Dir.stubs(:mktmpdir).returns "/tmp/testtmp123"
+      FileUtils.stubs(:remove_entry_secure)
+    end
+
+    it "should fail when a disk image with no system entities is mounted" do
+      described_class.stubs(:hdiutil).returns(empty_hdiutil_plist)
+      expect { provider.install }.should raise_error(Puppet::Error, /No disk entities/)
     end
 
     it "should call hdiutil to mount and eject the disk image" do
       Dir.stubs(:entries).returns []
-      @provider.class.expects(:hdiutil).with("eject", @fakemountpoint).returns 0
-      @provider.class.expects(:hdiutil).with("mount", "-plist", "-nobrowse", "-readonly", "-noidme", "-mountrandom", "/tmp", nil).returns @fakehdiutilplist
-      @provider.install
+      provider.class.expects(:hdiutil).with("eject", fake_mountpoint).returns 0
+      provider.class.expects(:hdiutil).with("mount", "-plist", "-nobrowse", "-readonly", "-noidme", "-mountrandom", "/tmp", nil).returns fake_hdiutil_plist
+      provider.install
     end
 
     it "should call installpkg if a pkg/mpkg is found on the dmg" do
       Dir.stubs(:entries).returns ["foo.pkg"]
-      @provider.class.stubs(:hdiutil).returns @fakehdiutilplist
-      @provider.class.expects(:installpkg).with("#{@fakemountpoint}/foo.pkg", @resource[:name], "foo.dmg").returns ""
-      @provider.install
+      provider.class.stubs(:hdiutil).returns fake_hdiutil_plist
+      provider.class.expects(:installpkg).with("#{fake_mountpoint}/foo.pkg", resource[:name], "foo.dmg").returns ""
+      provider.install
+    end
+
+    describe "from a remote source" do
+      let(:tmpdir) { "/tmp/good123" }
+
+      before :each do
+        resource[:source] = "http://fake.puppetlabs.com/foo.dmg"
+      end
+
+      it "should call tmpdir and use the returned directory" do
+        Dir.expects(:mktmpdir).returns tmpdir
+        Dir.stubs(:entries).returns ["foo.pkg"]
+        described_class.expects(:curl).with do |*args|
+          args[0] == "-o" and args[1].include? tmpdir
+        end
+        described_class.stubs(:hdiutil).returns fake_hdiutil_plist
+        described_class.expects(:installpkg)
+
+        provider.install
+      end
     end
   end
 
   describe "when installing flat pkg file" do
     it "should call installpkg if a flat pkg file is found instead of a .dmg image" do
-      @resource.stubs(:[]).with(:source).returns "/tmp/test.pkg"
-      @resource.stubs(:[]).with(:name).returns "testpkg"
-        @provider.class.expects(:installpkgdmg).with("#{@fakepkgfile}", "testpkg").returns ""
-        @provider.install
-        end
+      resource[:source] = "/tmp/test.pkg"
+      resource[:name] = "testpkg"
+      provider.class.expects(:installpkgdmg).with("/tmp/test.pkg", "testpkg").returns ""
+      provider.install
+    end
   end
-
 end
-- 
1.7.9.2


