~ubuntu-branches/ubuntu/trusty/puppet/trusty

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(-)

 

Index: puppet-2.7.1/lib/puppet/provider/ssh_authorized_key/parsed.rb

===================================================================

--- puppet-2.7.1.orig/lib/puppet/provider/ssh_authorized_key/parsed.rb  2011-06-27 09:50:51.000000000 -0500

+++ puppet-2.7.1/lib/puppet/provider/ssh_authorized_key/parsed.rb       2011-09-30 08:23:03.000000000 -0500

@@ -56,21 +56,22 @@

   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

Index: puppet-2.7.1/spec/unit/provider/ssh_authorized_key/parsed_spec.rb

===================================================================

--- puppet-2.7.1.orig/spec/unit/provider/ssh_authorized_key/parsed_spec.rb      2011-06-27 09:50:51.000000000 -0500

+++ puppet-2.7.1/spec/unit/provider/ssh_authorized_key/parsed_spec.rb   2011-09-30 08:23:03.000000000 -0500

@@ -114,15 +114,15 @@

         @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

 

@@ -158,11 +158,11 @@

         @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

 

@@ -173,9 +173,9 @@

         @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