Attachment 839246 Details for Bug 1045212 – CVE-2013-4969-3.3.x-temp-file.patch

[patch] CVE-2013-4969-3.3.x-temp-file.patch

CVE-2013-4969-3.3.x-temp-file.patch (text/plain), 10.56 KB, created by Kurt Seifried on 2013-12-19 21:49:22 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Kurt Seifried

Created: 2013-12-19 21:49:22 UTC

Size: 10.56 KB

patch

obsolete

>From 2bcd29c8bd42f3d82bfb8e75bab0b18dc55ae825 Mon Sep 17 00:00:00 2001
>From: Andrew Parker <andy@puppetlabs.com>
>Date: Tue, 3 Dec 2013 12:13:26 -0800
>Subject: [PATCH] (#23343) Use `replace_file` to update a file's contents
>
>The previous code had an unsafe use of temp files by looking for a name
>it can use in a directory and then later opening the file and writing to
>it. An attacker could, through some lucky timing and good access, make
>the selected name a symlink to another file and thereby cause puppet to
>overwrite something that it did not intend to.
>
>The temporary file is used to make the file update atomic and also to
>check that the contents have been written correctly. This updates the
>file type to use the Puppet::Util#replace_file method to do a safe,
>atomic, content swap. Since replace_file requires a mode to be provided,
>the code was also updated to use the assumed default of 0644, which is
>used when figuring out the absolute mode integer from symbolic modes.
>
>In order to get this change to work on windows, there were a few
>modifications that needed to be made as well:
>
> - New file must have binmode set to generate checksums
>   appropriately across operating sytems that results from newline
>   differences
> - replace_file has been adjusted so that passing a mode is now
>   optional, and mode is only applied when the passed value is not nil
> - Windows code has been cleaned up / refactored to reduce unnecessary
>   confusing code paths.  The simple mechanism for ensuring Windows
>   ReplaceFile will always succeed is to first create a file by
>   touching it (yielding default permissions) if it does not yet exist.
> - replace_file is now allowed to take a nil `default_mode` so that when
>   the file type is not attempting to manage the mode of the file we
>   will end up with the system default.
>
>diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
>index 862ab41..fca8ac7 100644
>--- a/lib/puppet/type/file.rb
>+++ b/lib/puppet/type/file.rb
>@@ -704,36 +704,25 @@ Puppet::Type.newtype(:file) do
>   def write(property)
>     remove_existing(:file)
> 
>-    use_temporary_file = write_temporary_file?
>-    if use_temporary_file
>-      path = "#{self[:path]}.puppettmp_#{rand(10000)}"
>-      path = "#{self[:path]}.puppettmp_#{rand(10000)}" while ::File.exists?(path) or ::File.symlink?(path)
>-    else
>-      path = self[:path]
>-    end
>+    assumed_default_mode = 0644
> 
>     mode = self.should(:mode) # might be nil
>-    umask = mode ? 000 : 022
>-    mode_int = mode ? symbolic_mode_to_int(mode, 0644) : nil
>-
>-    content_checksum = Puppet::Util.withumask(umask) { ::File.open(path, 'wb', mode_int ) { |f| write_content(f) } }
>-
>-    # And put our new file in place
>-    if use_temporary_file # This is only not true when our file is empty.
>-      begin
>-        fail_if_checksum_is_wrong(path, content_checksum) if validate_checksum?
>-        ::File.rename(path, self[:path])
>-      rescue => detail
>-        fail "Could not rename temporary file #{path} to #{self[:path]}: #{detail}"
>-      ensure
>-        # Make sure the created file gets removed
>-        ::File.unlink(path) if FileTest.exists?(path)
>+    mode_int = mode ? symbolic_mode_to_int(mode, assumed_default_mode) : nil
>+
>+    if write_temporary_file?
>+      Puppet::Util.replace_file(self[:path], mode_int) do |file|
>+        file.binmode
>+        content_checksum = write_content(file)
>+        file.flush
>+        fail_if_checksum_is_wrong(file.path, content_checksum) if validate_checksum?
>       end
>+    else
>+      umask = mode ? 000 : 022
>+      Puppet::Util.withumask(umask) { ::File.open(self[:path], 'wb', mode_int ) { |f| write_content(f) } }
>     end
> 
>     # make sure all of the modes are actually correct
>     property_fix
>-
>   end
> 
>   private
>diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb
>index c1a8234..6f028a9 100644
>--- a/lib/puppet/util.rb
>+++ b/lib/puppet/util.rb
>@@ -404,10 +404,13 @@ module Util
>   def replace_file(file, default_mode, &block)
>     raise Puppet::DevError, "replace_file requires a block" unless block_given?
> 
>-    unless valid_symbolic_mode?(default_mode)
>-      raise Puppet::DevError, "replace_file default_mode: #{default_mode} is invalid"
>+    if default_mode
>+      unless valid_symbolic_mode?(default_mode)
>+        raise Puppet::DevError, "replace_file default_mode: #{default_mode} is invalid"
>+      end
>+
>+      mode = symbolic_mode_to_int(normalize_symbolic_mode(default_mode))
>     end
>-    mode = symbolic_mode_to_int(normalize_symbolic_mode(default_mode))
> 
>     file     = Pathname(file)
>     tempfile = Tempfile.new(file.basename.to_s, file.dirname.to_s)
>@@ -420,16 +423,21 @@ module Util
>     # and specifically handle the platform, which has all sorts of magic.
>     # So, unlike Unix, we don't pre-prep security; we use the default "quite
>     # secure" tempfile permissions instead.  Magic happens later.
>-    unless Puppet.features.microsoft_windows?
>+    if !Puppet.features.microsoft_windows?
>       # Grab the current file mode, and fall back to the defaults.
>-      stat = file.lstat rescue OpenStruct.new(:mode => mode,
>-                                              :uid  => Process.euid,
>-                                              :gid  => Process.egid)
>-
>-      # We only care about the bottom four slots, which make the real mode,
>-      # and not the rest of the platform stat call fluff and stuff.
>-      tempfile.chmod(stat.mode & 07777)
>-      tempfile.chown(stat.uid, stat.gid)
>+      if file.exist?
>+        stat = file.lstat
>+        tempfile.chown(stat.uid, stat.gid)
>+        effective_mode = stat.mode
>+      else
>+        effective_mode = mode
>+      end
>+
>+      if effective_mode
>+        # We only care about the bottom four slots, which make the real mode,
>+        # and not the rest of the platform stat call fluff and stuff.
>+        tempfile.chmod(effective_mode & 07777)
>+      end
>     end
> 
>     # OK, now allow the caller to write the content of the file.
>@@ -452,41 +460,17 @@ module Util
>     tempfile.close
> 
>     if Puppet.features.microsoft_windows?
>-      # This will appropriately clone the file, but only if the file we are
>-      # replacing exists.  Which is kind of annoying; thanks Microsoft.
>-      #
>-      # So, to avoid getting into an infinite loop we will retry once if the
>-      # file doesn't exist, but only the once...
>-      have_retried = false
>-
>-      begin
>-        # Yes, the arguments are reversed compared to the rename in the rest
>-        # of the world.
>-        Puppet::Util::Windows::File.replace_file(file, tempfile.path)
>-      rescue Puppet::Util::Windows::Error
>-        # This might race, but there are enough possible cases that there
>-        # isn't a good, solid "better" way to do this, and the next call
>-        # should fail in the same way anyhow.
>-        raise if have_retried or File.exist?(file)
>-        have_retried = true
>-
>-        # OK, so, we can't replace a file that doesn't exist, so let us put
>-        # one in place and set the permissions.  Then we can retry and the
>-        # magic makes this all work.
>-        #
>-        # This is the least-worst option for handling Windows, as far as we
>-        # can determine.
>-        File.open(file, 'a') do |fh|
>-          # this space deliberately left empty for auto-close behaviour,
>-          # append mode, and not actually changing any of the content.
>+      # Windows ReplaceFile needs a file to exist, so touch handles this
>+      if !file.exist?
>+        FileUtils.touch(file.to_s)
>+        if mode
>+          Puppet::Util::Windows::Security.set_mode(mode, file.to_s)
>         end
>-
>-        # Set the permissions to what we want.
>-        Puppet::Util::Windows::Security.set_mode(mode, file.to_s)
>-
>-        # ...and finally retry the operation.
>-        retry
>       end
>+      # Yes, the arguments are reversed compared to the rename in the rest
>+      # of the world.
>+      Puppet::Util::Windows::File.replace_file(file, tempfile.path)
>+
>     else
>       File.rename(tempfile.path, file.to_s)
>     end
>diff --git a/spec/integration/type/file_spec.rb b/spec/integration/type/file_spec.rb
>index 5377251..55e6cc2 100755
>--- a/spec/integration/type/file_spec.rb
>+++ b/spec/integration/type/file_spec.rb
>@@ -495,20 +495,6 @@ describe Puppet::Type.type(:file) do
>       bucket.bucket.getfile(foomd5).should == "fooyay"
>       bucket.bucket.getfile(barmd5).should == "baryay"
>     end
>-
>-    it "should propagate failures encountered when renaming the temporary file" do
>-      file = described_class.new :path => path, :content => "foo"
>-      file.stubs(:perform_backup).returns(true)
>-
>-      catalog.add_resource file
>-
>-      File.open(path, "w") { |f| f.print "bar" }
>-
>-      File.expects(:rename).raises ArgumentError
>-
>-      expect { file.write(:content) }.to raise_error(Puppet::Error, /Could not rename temporary file/)
>-      File.read(path).should == "bar"
>-    end
>   end
> 
>   describe "when recursing" do
>@@ -993,6 +979,7 @@ describe Puppet::Type.type(:file) do
>     describe "on Windows systems", :if => Puppet.features.microsoft_windows? do
>       it "should provide valid default values when ACLs are not supported" do
>         Puppet::Util::Windows::Security.stubs(:supports_acl?).with(source).returns false
>+        Puppet::Util::Windows::Security.stubs(:supports_acl?).with(path).returns true
> 
>         file = described_class.new(
>           :path   => path,
>diff --git a/spec/unit/type/file_spec.rb b/spec/unit/type/file_spec.rb
>index dd695ee..d79271a 100755
>--- a/spec/unit/type/file_spec.rb
>+++ b/spec/unit/type/file_spec.rb
>@@ -1071,35 +1071,6 @@ describe Puppet::Type.type(:file) do
>   end
> 
>   describe "#write" do
>-    it "should propagate failures encountered when renaming the temporary file" do
>-      File.stubs(:open)
>-      File.expects(:rename).raises ArgumentError
>-
>-      file[:backup] = 'puppet'
>-
>-      file.stubs(:validate_checksum?).returns(false)
>-
>-      property = stub('content_property', :actual_content => "something", :length => "something".length)
>-      file.stubs(:property).with(:content).returns(property)
>-
>-      expect { file.write(:content) }.to raise_error(Puppet::Error)
>-    end
>-
>-    it "should delegate writing to the content property" do
>-      filehandle = stub_everything 'fh'
>-      File.stubs(:open).yields(filehandle)
>-      File.stubs(:rename)
>-      property = stub('content_property', :actual_content => "something", :length => "something".length)
>-      file[:backup] = 'puppet'
>-
>-      file.stubs(:validate_checksum?).returns(false)
>-      file.stubs(:property).with(:content).returns(property)
>-
>-      property.expects(:write).with(filehandle)
>-
>-      file.write(:content)
>-    end
>-
>     describe "when validating the checksum" do
>       before { file.stubs(:validate_checksum?).returns(true) }
> 
>-- 
>1.8.4.3
>

Actions: View | Diff

Attachments on bug 1045212: 839245 | 839246