Permalink
Browse files

Fix for temporary file security whole

We create temporary files in /tmp/ with predictable names.  These
could be used by an attacker to DoS a box by setting a symlink to
some other file (say, /etc/shadow) and waiting for us to overwrite
it.

The minimalistic solution employed by this patch is to wrap all such
file writing with a paranoid wrapper that:

1) Check to see if the target exists
2) Issues a warning if it was a symlink
3) Deletes it
4) Waits (0.1 seconds if it was a file, 5 seconds if it was a symlink)
5) Opens the file with EXCL, which will fail if the file has come back.

If this succeeds (as it normally will) it has exactly the same semantics
as the original code (a must, as we are right at a release boundary).
However, under no circumstances will it follow a preexisting symlink (the
operating system guarantees this with EXCL) so the danger of an exploit
has been converted into the possibility of a failure, with an appropriate
warning.
  • Loading branch information...
1 parent 87136b4 commit 0e9db34ba0afe0ea0bee3afd005a011e3099d055 @MarkusQ committed Jan 4, 2010
Showing with 32 additions and 7 deletions.
  1. +2 −2 lib/puppet/daemon.rb
  2. +1 −1 lib/puppet/network/server.rb
  3. +1 −1 lib/puppet/rails/benchmark.rb
  4. +22 −0 lib/puppet/util.rb
  5. +6 −3 lib/puppet/util/reference.rb
View
@@ -31,10 +31,10 @@ def daemonize
$stderr.reopen $stdout
Puppet::Util::Log.reopen
rescue => detail
- File.open("/tmp/daemonout", "w") { |f|
+ Puppet.err "Could not start %s: %s" % [Puppet[:name], detail]
+ Puppet::Util::secure_open("/tmp/daemonout", "w") { |f|
f.puts "Could not start %s: %s" % [Puppet[:name], detail]
}
- Puppet.err "Could not start %s: %s" % [Puppet[:name], detail]
exit(12)
end
end
@@ -22,7 +22,7 @@ def daemonize
$stderr.reopen $stdout
Puppet::Util::Log.reopen
rescue => detail
- File.open("/tmp/daemonout", "w") { |f|
+ Puppet::Util.secure_open("/tmp/daemonout", "w") { |f|
f.puts "Could not start %s: %s" % [Puppet[:name], detail]
}
raise "Could not start %s: %s" % [Puppet[:name], detail]
@@ -64,6 +64,6 @@ def write_benchmarks
data = {}
end
data[branch] = $benchmarks
- File.open(file, "w") { |f| f.print YAML.dump(data) }
+ Puppet::Util.secure_open(file, "w") { |f| f.print YAML.dump(data) }
end
end
View
@@ -407,6 +407,28 @@ def thinmark
end
module_function :memory, :thinmark
+
+ def secure_open(file,must_be_w,&block)
+ raise Puppet::DevError,"secure_open only works with mode 'w'" unless must_be_w == 'w'
+ raise Puppet::DevError,"secure_open only requires a block" unless block_given?
+ Puppet.warn "#{file} was a symlink to #{File.readlink(file)}" if File.symlink?(file)
+ if File.exists?(file) or File.symlink?(file)
+ wait = File.symlink?(file) ? 5.0 : 0.1
+ File.delete(file)
+ sleep wait # give it a chance to reappear, just in case someone is actively trying something.
+ end
+ begin
+ File.open(file,File::CREAT|File::EXCL|File::TRUNC|File::WRONLY,&block)
+ rescue Errno::EEXIST
+ desc = File.symlink?(file) ? "symlink to #{File.readlink(file)}" : File.stat(file).ftype
+ puts "Warning: #{file} was apparently created by another process (as"
+ puts "a #{desc}) as soon as it was deleted by this process. Someone may be trying"
+ puts "to do something objectionable (such as tricking you into overwriting system"
+ puts "files if you are running as root)."
+ raise
+ end
+ end
+ module_function :secure_open
end
end
@@ -36,7 +36,7 @@ def self.page(*sections)
def self.pdf(text)
puts "creating pdf"
- File.open("/tmp/puppetdoc.txt", "w") do |f|
+ Puppet::Util.secure_open("/tmp/puppetdoc.txt", "w") do |f|
f.puts text
end
rst2latex = %x{which rst2latex}
@@ -48,6 +48,9 @@ def self.pdf(text)
end
rst2latex.chomp!
cmd = %{#{rst2latex} /tmp/puppetdoc.txt > /tmp/puppetdoc.tex}
+ Puppet::Util.secure_open("/tmp/puppetdoc.tex","w") do |f|
+ # If we get here without an error, /tmp/puppetdoc.tex isn't a tricky cracker's symlink
+ end
output = %x{#{cmd}}
unless $? == 0
$stderr.puts "rst2latex failed"
@@ -67,7 +70,7 @@ def self.markdown(name, text)
puts "Creating markdown for #{name} reference."
dir = "/tmp/" + Puppet::PUPPETVERSION
FileUtils.mkdir(dir) unless File.directory?(dir)
- File.open(dir + "/" + "#{name}.rst", "w") do |f|
+ Puppet::Util.secure_open(dir + "/" + "#{name}.rst", "w") do |f|
f.puts text
end
pandoc = %x{which pandoc}
@@ -190,7 +193,7 @@ def to_trac(with_contents = true)
end
def trac
- File.open("/tmp/puppetdoc.txt", "w") do |f|
+ Puppet::Util.secure_open("/tmp/puppetdoc.txt", "w") do |f|
f.puts self.to_trac
end

0 comments on commit 0e9db34

Please sign in to comment.