From 8d9575775737c08c6cbfdf7f9a22f2ea4ab21b20 Mon Sep 17 00:00:00 2001 From: Ricky Zhou 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 for discovering this and supplying the security fix. Awesome work. Signed-off-by: Daniel Pittman --- 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