Permalink
Browse files

Reject directory traversal in store report processor

  • Loading branch information...
pcarlisle committed Jun 28, 2012
1 parent 34b9c0b commit d80478208d79a3e6d6cb1fbc525e24817fe8c4c6
Showing with 41 additions and 6 deletions.
  1. +13 −6 lib/puppet/reports/store.rb
  2. +28 −0 spec/unit/reports/store_spec.rb
@@ -2,6 +2,8 @@
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.
@@ -13,9 +15,11 @@
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)
@@ -42,17 +46,20 @@ def process
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|
@@ -44,5 +44,33 @@
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

0 comments on commit d804782

Please sign in to comment.