Skip to content

Commit

Permalink
Fix for temporary file security whole
Browse files Browse the repository at this point in the history
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
MarkusQ authored and jamtur01 committed Jan 5, 2010
1 parent e7d98cc commit 6111ba8
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 7 deletions.
4 changes: 2 additions & 2 deletions lib/puppet/daemon.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion lib/puppet/network/server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
2 changes: 1 addition & 1 deletion lib/puppet/rails/benchmark.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
22 changes: 22 additions & 0 deletions lib/puppet/util.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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.warning "#{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

Expand Down
9 changes: 6 additions & 3 deletions lib/puppet/util/reference.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand All @@ -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"
Expand All @@ -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}
Expand Down Expand Up @@ -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

Expand Down

0 comments on commit 6111ba8

Please # to comment.