CVE-2012-3865: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') (CWE-22)
value: Directory traversal vulnerability in lib/puppet/reports/store.rb in Puppet before 2.6.17 and 2.7.x before 2.7.18, and Puppet Enterprise before 2.5.2, when Delete is enabled in auth.conf, allows remote authenticated users to delete arbitrary files on the puppet master server via a .. (dot dot) in a node name.
CWE description: The product uses external input to construct a pathname that is intended to identify a file or directory that is located underneath a restricted parent directory, but the product does not properly neutralize special elements within the pathname that can cause the pathname to resolve to a location that is outside of the restricted directory.
Published: 2012-08-06T16:55Z
Repository: https://github.com/puppetlabs/puppet
Language: Ruby
Files changed: lib/puppet/reports/store.rb, spec/unit/reports/store_spec.rb
Commit date: 2012-06-28 14:13:19 -0700
Commit message: Reject directory traversal in store report processor

Diff: diff --git a/lib/puppet/reports/store.rb b/lib/puppet/reports/store.rb
index cd188fafd9..4f2d225324 100644
--- a/lib/puppet/reports/store.rb
+++ b/lib/puppet/reports/store.rb
@@ -1,9 +1,11 @@
 require 'puppet'
 require 'fileutils'
 require 'tempfile'
 
+SEPARATOR = [Regexp.escape(File::SEPARATOR.to_s), Regexp.escape(File::ALT_SEPARATOR.to_s)].join
+
 Puppet::Reports.register_report(:store) do
   desc "Store the yaml report on disk.  Each host sends its report as a YAML dump
     and this just stores the file on disk, in the `reportdir` directory.
 
     These files collect quickly -- one every half hour -- so it is a good idea
@@ -11,13 +13,15 @@ Puppet::Reports.register_report(:store) do
     default report)."
 
   def process
     # We don't want any tracking back in the fs.  Unlikely, but there
     # you go.
-    client = self.host.gsub("..",".")
+    if host =~ Regexp.union(/[#{SEPARATOR}]/, /\A\.\.?\Z/)
+      raise ArgumentError, "Invalid node name #{host.inspect}"
+    end
 
-    dir = File.join(Puppet[:reportdir], client)
+    dir = File.join(Puppet[:reportdir], host)
 
     if ! FileTest.exists?(dir)
       FileUtils.mkdir_p(dir)
       FileUtils.chmod_R(0750, dir)
     end
@@ -40,21 +44,24 @@ Puppet::Reports.register_report(:store) do
         f.close
       end
       FileUtils.mv(f.path, file)
     rescue => detail
       puts detail.backtrace if Puppet[:trace]
-      Puppet.warning "Could not write report for #{client} at #{file}: #{detail}"
+      Puppet.warning "Could not write report for #{host} at #{file}: #{detail}"
     end
 
     # Only testing cares about the return value
     file
   end
 
-  # removes all reports for a given host
+  # removes all reports for a given host?
   def self.destroy(host)
-    client = host.gsub("..",".")
-    dir = File.join(Puppet[:reportdir], client)
+    if host =~ Regexp.union(/[#{SEPARATOR}]/, /\A\.\.?\Z/)
+      raise ArgumentError, "Invalid node name #{host.inspect}"
+    end
+
+    dir = File.join(Puppet[:reportdir], host)
 
     if File.exists?(dir)
       Dir.entries(dir).each do |file|
         next if ['.','..'].include?(file)
         file = File.join(dir, file)
diff --git a/spec/unit/reports/store_spec.rb b/spec/unit/reports/store_spec.rb
index 5b752c4b7c..b4f849cdef 100755
--- a/spec/unit/reports/store_spec.rb
+++ b/spec/unit/reports/store_spec.rb
@@ -42,7 +42,35 @@ describe processor do
       file.expects(:close).in_sequence(writeseq)
       file.stubs(:path).returns(File.join(Dir.tmpdir, "foo123"))
       FileUtils.expects(:mv).in_sequence(writeseq).with(File.join(Dir.tmpdir, "foo123"), File.join(Puppet[:reportdir], @report.host, "201101061200.yaml"))
       @report.process
     end
+
+    ['..', 'hello/', '/hello', 'he/llo', 'hello/..', '.'].each do |node|
+      it "rejects #{node.inspect}" do
+        @report.host = node
+        expect { @report.process }.to raise_error(ArgumentError, /Invalid node/)
+      end
+    end
+
+    ['.hello', 'hello.', '..hi', 'hi..'].each do |node|
+      it "accepts #{node.inspect}" do
+        @report.host = node
+        @report.process
+      end
+    end
+  end
+
+  describe "::destroy" do
+    ['..', 'hello/', '/hello', 'he/llo', 'hello/..', '.'].each do |node|
+      it "rejects #{node.inspect}" do
+        expect { processor.destroy(node) }.to raise_error(ArgumentError, /Invalid node/)
+      end
+    end
+
+    ['.hello', 'hello.', '..hi', 'hi..'].each do |node|
+      it "accepts #{node.inspect}" do
+        processor.destroy(node)
+      end
+    end
   end
 end

Vulnerable code:     client = self.host.gsub("..",".")
    dir = File.join(Puppet[:reportdir], client)
      Puppet.warning "Could not write report for #{client} at #{file}: #{detail}"
  # removes all reports for a given host
    client = host.gsub("..",".")
    dir = File.join(Puppet[:reportdir], client)
Fixed code: SEPARATOR = [Regexp.escape(File::SEPARATOR.to_s), Regexp.escape(File::ALT_SEPARATOR.to_s)].join

    if host =~ Regexp.union(/[#{SEPARATOR}]/, /\A\.\.?\Z/)
      raise ArgumentError, "Invalid node name #{host.inspect}"
    end
    dir = File.join(Puppet[:reportdir], host)
      Puppet.warning "Could not write report for #{host} at #{file}: #{detail}"
  # removes all reports for a given host?
    if host =~ Regexp.union(/[#{SEPARATOR}]/, /\A\.\.?\Z/)
      raise ArgumentError, "Invalid node name #{host.inspect}"
    end

    dir = File.join(Puppet[:reportdir], host)

    ['..', 'hello/', '/hello', 'he/llo', 'hello/..', '.'].each do |node|
      it "rejects #{node.inspect}" do
        @report.host = node
        expect { @report.process }.to raise_error(ArgumentError, /Invalid node/)
      end
    end

    ['.hello', 'hello.', '..hi', 'hi..'].each do |node|
      it "accepts #{node.inspect}" do
        @report.host = node
        @report.process
      end
    end
  end

  describe "::destroy" do
    ['..', 'hello/', '/hello', 'he/llo', 'hello/..', '.'].each do |node|
      it "rejects #{node.inspect}" do
        expect { processor.destroy(node) }.to raise_error(ArgumentError, /Invalid node/)
      end
    end

    ['.hello', 'hello.', '..hi', 'hi..'].each do |node|
      it "accepts #{node.inspect}" do
        processor.destroy(node)
      end
    end