File 2.6.x-9791-TOCTOU-in-ssh-auth-keys-type.patch of Package puppet.import5746

Overview Repositories Revisions Requests Users Attributes Meta

File 2.6.x-9791-TOCTOU-in-ssh-auth-keys-type.patch of Package puppet.import5746

From 8d9575775737c08c6cbfdf7f9a22f2ea4ab21b20 Mon Sep 17 00:00:00 2001
From: Ricky Zhou <ricky@fedoraproject.org>
Date: Mon, 29 Aug 2011 16:01:12 -0400
Subject: [PATCH] Drop privileges before creating and chmodding SSH keys.

Previously, potentially abusable chown and chmod calls were performed as
root.  This tries to moves as much as possible into code which is run
after privileges have been dropped.

Huge thanks to Ricky Zhou <ricky@fedoraproject.org> for discovering this and
supplying the security fix.  Awesome work.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
---
 lib/puppet/provider/ssh_authorized_key/parsed.rb   |   19 ++++++++++---------
 .../provider/ssh_authorized_key/parsed_spec.rb     |   16 ++++++++--------
 2 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/lib/puppet/provider/ssh_authorized_key/parsed.rb b/lib/puppet/provider/ssh_authorized_key/parsed.rb
index 6a3855c..5243477 100644
--- a/lib/puppet/provider/ssh_authorized_key/parsed.rb
+++ b/lib/puppet/provider/ssh_authorized_key/parsed.rb
@@ -56,21 +56,22 @@ require 'puppet/provider/parsedfile'
   def flush
     raise Puppet::Error, "Cannot write SSH authorized keys without user"    unless @resource.should(:user)
     raise Puppet::Error, "User '#{@resource.should(:user)}' does not exist" unless uid = Puppet::Util.uid(@resource.should(:user))
-    unless File.exist?(dir = File.dirname(target))
-      Puppet.debug "Creating #{dir}"
-      Dir.mkdir(dir, dir_perm)
-      File.chown(uid, nil, dir)
-    end
-
     # ParsedFile usually calls backup_target much later in the flush process,
     # but our SUID makes that fail to open filebucket files for writing.
     # Fortunately, there's already logic to make sure it only ever happens once,
     # so calling it here supresses the later attempt by our superclass's flush method.
     self.class.backup_target(target)
 
-    Puppet::Util::SUIDManager.asuser(@resource.should(:user)) { super }
-    File.chown(uid, nil, target)
-    File.chmod(file_perm, target)
+    Puppet::Util::SUIDManager.asuser(@resource.should(:user)) do
+        unless File.exist?(dir = File.dirname(target))
+          Puppet.debug "Creating #{dir}"
+          Dir.mkdir(dir, dir_perm)
+        end
+
+        super
+
+        File.chmod(file_perm, target)
+    end
   end
 
   # parse sshv2 option strings, wich is a comma separated list of
diff --git a/spec/unit/provider/ssh_authorized_key/parsed_spec.rb b/spec/unit/provider/ssh_authorized_key/parsed_spec.rb
index 2e5be16..64935df 100755
--- a/spec/unit/provider/ssh_authorized_key/parsed_spec.rb
+++ b/spec/unit/provider/ssh_authorized_key/parsed_spec.rb
@@ -133,15 +133,15 @@ describe provider_class do
         @provider.flush
       end
 
-      it "should chown the directory to the user" do
+      it "should absolutely not chown the directory to the user" do
         uid = Puppet::Util.uid("random_bob")
-        File.expects(:chown).with(uid, nil, "/tmp/.ssh_dir")
+        File.expects(:chown).never
         @provider.flush
       end
 
-      it "should chown the key file to the user" do
+      it "should absolutely not chown the key file to the user" do
         uid = Puppet::Util.uid("random_bob")
-        File.expects(:chown).with(uid, nil, "/tmp/.ssh_dir/place_to_put_authorized_keys")
+        File.expects(:chown).never
         @provider.flush
       end
 
@@ -177,11 +177,11 @@ describe provider_class do
         @provider.flush
       end
 
-      it "should chown the directory to the user if it creates it" do
+      it "should absolutely not chown the directory to the user if it creates it" do
         File.stubs(:exist?).with(@dir).returns false
         Dir.stubs(:mkdir).with(@dir,0700)
         uid = Puppet::Util.uid("nobody")
-        File.expects(:chown).with(uid, nil, @dir)
+        File.expects(:chown).never
         @provider.flush
       end
 
@@ -192,9 +192,9 @@ describe provider_class do
         @provider.flush
       end
 
-      it "should chown the key file to the user" do
+      it "should absolutely not chown the key file to the user" do
         uid = Puppet::Util.uid("nobody")
-        File.expects(:chown).with(uid, nil, File.expand_path("~nobody/.ssh/authorized_keys"))
+        File.expects(:chown).never
         @provider.flush
       end
 
-- 
1.7.6.4

Places

File 2.6.x-9791-TOCTOU-in-ssh-auth-keys-type.patch of Package puppet.import5746

Places