From 00c4b25010068eeb57a15104fb178a72af733fda Mon Sep 17 00:00:00 2001 From: Nick Lewis Date: Thu, 11 Aug 2011 15:47:37 -0700 Subject: (#8662) Migrate suidmanager test case to rspec We're trying to move away from the legacy Test::Unit tests, and toward rspec specs, so rewrite this file as specs. Reviewed-By: Jacob Helwig --- spec/unit/util/suidmanager_spec.rb | 113 ++++++++++++++++++++++++++++++++++ test/puppet/tc_suidmanager.rb | 120 ------------------------------------- 2 files changed, 113 insertions(+), 120 deletions(-) create mode 100755 spec/unit/util/suidmanager_spec.rb delete mode 100755 test/puppet/tc_suidmanager.rb diff --git a/spec/unit/util/suidmanager_spec.rb b/spec/unit/util/suidmanager_spec.rb new file mode 100755 index 000000000..19ddc2b4d --- /dev/null +++ b/spec/unit/util/suidmanager_spec.rb @@ -0,0 +1,113 @@ +#!/usr/bin/env rspec + +require 'spec_helper' + +describe Puppet::Util::SUIDManager do + let :user do + Puppet::Type.type(:user).new(:name => 'name', :uid => 42, :gid => 42) + end + + let :xids do + Hash.new {|h,k| 0} + end + + before :each do + Puppet::Util::SUIDManager.stubs(:convert_xid).returns(42) + Puppet::Util::SUIDManager.stubs(:initgroups) + + [:euid, :egid, :uid, :gid, :groups].each do |id| + Process.stubs("#{id}=").with {|value| xids[id] = value} + end + end + + describe "#uid" do + it "should allow setting euid/egid" do + Puppet::Util::SUIDManager.egid = user[:gid] + Puppet::Util::SUIDManager.euid = user[:uid] + + xids[:egid].should == user[:gid] + xids[:euid].should == user[:uid] + end + end + + describe "#asuser" do + it "should set euid/egid when root" do + Process.stubs(:uid).returns(0) + + Process.stubs(:euid).returns(50) + Process.stubs(:egid).returns(50) + + yielded = false + Puppet::Util::SUIDManager.asuser(user[:uid], user[:gid]) do + xids[:egid].should == user[:gid] + xids[:euid].should == user[:uid] + yielded = true + end + + xids[:egid].should == 50 + xids[:euid].should == 50 + + # It's possible asuser could simply not yield, so the assertions in the + # block wouldn't fail. So verify those actually got checked. + yielded.should be_true + end + + it "should not get or set euid/egid when not root" do + Process.stubs(:uid).returns(1) + + Process.stubs(:euid).returns(50) + Process.stubs(:egid).returns(50) + + Puppet::Util::SUIDManager.asuser(user[:uid], user[:gid]) {} + + xids.should be_empty + end + end + + describe "when running commands" do + before :each do + # We want to make sure $CHILD_STATUS is set + Kernel.system '' if $CHILD_STATUS.nil? + end + + describe "with #system" do + it "should set euid/egid when root" do + Process.stubs(:uid).returns(0) + Process.stubs(:groups=) + Process.expects(:euid).returns(99997) + Process.expects(:egid).returns(99996) + + Process.expects(:euid=).with(uid) + Process.expects(:egid=).with(gid) + + Kernel.expects(:system).with('blah') + Puppet::Util::SUIDManager.system('blah', user[:uid], user[:gid]) + + xids[:egid].should == 99996 + xids[:euid].should == 99997 + end + + it "should not get or set euid/egid when not root" do + Process.stubs(:uid).returns(1) + Kernel.expects(:system).with('blah') + + Puppet::Util::SUIDManager.system('blah', user[:uid], user[:gid]) + + xids.should be_empty + end + end + + describe "with #run_and_capture" do + it "should capture the output and return process status" do + Puppet::Util. + expects(:execute). + with('yay', :combine => true, :failonfail => false, :uid => user[:uid], :gid => user[:gid]). + returns('output') + output = Puppet::Util::SUIDManager.run_and_capture 'yay', user[:uid], user[:gid] + + output.first.should == 'output' + output.last.should be_a(Process::Status) + end + end + end +end diff --git a/test/puppet/tc_suidmanager.rb b/test/puppet/tc_suidmanager.rb deleted file mode 100755 index eeb56f2c9..000000000 --- a/test/puppet/tc_suidmanager.rb +++ /dev/null @@ -1,120 +0,0 @@ -#!/usr/bin/env ruby - -require File.dirname(__FILE__) + '/../lib/puppettest' - -require 'puppet' -require 'puppettest' -require 'test/unit' -require 'mocha' - -class TestSUIDManager < Test::Unit::TestCase - include PuppetTest - - def setup - the_id = 42 - Puppet::Util::SUIDManager.stubs(:convert_xid).returns(the_id) - Puppet::Util::SUIDManager.stubs(:initgroups) - @user = stub('user', :uid => the_id, :gid => the_id, :name => 'name') - super - end - - def test_metaprogramming_function_additions - # NOTE: the way that we are dynamically generating the methods in - # SUIDManager for the UID/GID calls was causing problems due to the - # modification of a closure. Should the bug rear itself again, this - # test will fail. - Process.expects(:uid).times(2) - - assert_nothing_raised do - Puppet::Util::SUIDManager.uid - Puppet::Util::SUIDManager.uid - end - end - - def test_id_set - Process.expects(:euid=).with(@user.uid) - Process.expects(:egid=).with(@user.gid) - - assert_nothing_raised do - Puppet::Util::SUIDManager.egid = @user.gid - Puppet::Util::SUIDManager.euid = @user.uid - end - end - - def test_utiluid - assert_not_equal(nil, Puppet::Util.uid(nonrootuser.name)) - end - - def test_asuser_as_root - Process.stubs(:uid).returns(0) - expects_id_set_and_revert @user.uid, @user.gid - Puppet::Util::SUIDManager.asuser @user.uid, @user.gid do end - rescue Errno::EPERM - end - - def test_asuser_as_nonroot - Process.stubs(:uid).returns(1) - expects_no_id_set - Puppet::Util::SUIDManager.asuser @user.uid, @user.gid do end - end - - - def test_system_as_root - Process.stubs(:uid).returns(0) - set_exit_status! - expects_id_set_and_revert @user.uid, @user.gid - Kernel.expects(:system).with('blah') - Puppet::Util::SUIDManager.system('blah', @user.uid, @user.gid) - end - - def test_system_as_nonroot - Process.stubs(:uid).returns(1) - set_exit_status! - expects_no_id_set - Kernel.expects(:system).with('blah') - Puppet::Util::SUIDManager.system('blah', @user.uid, @user.gid) - end - - def test_run_and_capture - if (RUBY_VERSION <=> "1.8.4") < 0 - warn "Cannot run this test on ruby < 1.8.4" - else - set_exit_status! - Puppet::Util. - expects(:execute). - with('yay',:combine => true, :failonfail => false, :uid => @user.uid, :gid => @user.gid). - returns('output') - output = Puppet::Util::SUIDManager.run_and_capture 'yay', @user.uid, @user.gid - - assert_equal 'output', output.first - assert_kind_of Process::Status, output.last - end - end - - private - - def expects_id_set_and_revert(uid, gid) - Process.stubs(:groups=) - Process.expects(:euid).returns(99997) - Process.expects(:egid).returns(99996) - - Process.expects(:euid=).with(uid) - Process.expects(:egid=).with(gid) - - Process.expects(:euid=).with(99997) - Process.expects(:egid=).with(99996) - end - - def expects_no_id_set - Process.expects(:egid).never - Process.expects(:euid).never - Process.expects(:egid=).never - Process.expects(:euid=).never - end - - def set_exit_status! - # We want to make sure $CHILD_STATUS is set, this is the only way I know how. - Kernel.system '' if $CHILD_STATUS.nil? - end -end - -- cgit From 2a0de12af5305ed54dd99391ee17533e71e0d88e Mon Sep 17 00:00:00 2001 From: Nick Lewis Date: Tue, 9 Aug 2011 17:48:33 -0700 Subject: (#8770) Always fully drop privileges when changing user On Mac OS X, it is only possible to directly change the euid of a process, and not the uid. Thus, when a puppet master started as root on OS X would change to the service user (puppet), it would leave the uid of its process set to 0. This allowed any type of Ruby plugin executed on the master (a type, provider, function, etc.) to trivially regain root privileges (by setting the euid of its process back to 0) and potentially compromise the master. Now, when permanently changing user, we will first try Process::UID.change_privilege, before falling back to setting the euid/uid ourselves. change_privilege correctly sets the uid of the process to the desired new uid, preventing the process from later escalating itself back to root. Similar behavior is also used when changing group. This has no effect on the behavior when temporarily changing user/group (for instance, to execute a single command or create a file as a particular user). Reviewed-By: Jacob Helwig --- lib/puppet/util.rb | 56 +++++++--------------- lib/puppet/util/suidmanager.rb | 54 +++++++++++++++------ spec/unit/util/suidmanager_spec.rb | 97 +++++++++++++++++++++++++++++++++----- 3 files changed, 142 insertions(+), 65 deletions(-) diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb index 34c6ec1ed..72c1f5f91 100644 --- a/lib/puppet/util.rb +++ b/lib/puppet/util.rb @@ -46,35 +46,24 @@ module Util # Change the process to a different user def self.chuser if group = Puppet[:group] - group = self.gid(group) - raise Puppet::Error, "No such group #{Puppet[:group]}" unless group - unless Puppet::Util::SUIDManager.gid == group - begin - Puppet::Util::SUIDManager.egid = group - Puppet::Util::SUIDManager.gid = group - rescue => detail - Puppet.warning "could not change to group #{group.inspect}: #{detail}" - $stderr.puts "could not change to group #{group.inspect}" - - # Don't exit on failed group changes, since it's - # not fatal - #exit(74) - end + begin + Puppet::Util::SUIDManager.change_group(group, true) + rescue => detail + Puppet.warning "could not change to group #{group.inspect}: #{detail}" + $stderr.puts "could not change to group #{group.inspect}" + + # Don't exit on failed group changes, since it's + # not fatal + #exit(74) end end if user = Puppet[:user] - user = self.uid(user) - raise Puppet::Error, "No such user #{Puppet[:user]}" unless user - unless Puppet::Util::SUIDManager.uid == user - begin - Puppet::Util::SUIDManager.initgroups(user) - Puppet::Util::SUIDManager.uid = user - Puppet::Util::SUIDManager.euid = user - rescue => detail - $stderr.puts "Could not change to user #{user}: #{detail}" - exit(74) - end + begin + Puppet::Util::SUIDManager.change_user(user, true) + rescue => detail + $stderr.puts "Could not change to user #{user}: #{detail}" + exit(74) end end end @@ -89,18 +78,14 @@ module Util if useself Puppet::Util::Log.create( - :level => level, :source => self, - :message => args ) else Puppet::Util::Log.create( - :level => level, - :message => args ) end @@ -261,9 +246,6 @@ module Util Puppet.debug "Executing '#{str}'" end - arguments[:uid] = Puppet::Util::SUIDManager.convert_xid(:uid, arguments[:uid]) if arguments[:uid] - arguments[:gid] = Puppet::Util::SUIDManager.convert_xid(:gid, arguments[:gid]) if arguments[:gid] - if execution_stub = Puppet::Util::ExecutionStub.current_value return execution_stub.call(command, arguments) end @@ -305,14 +287,8 @@ module Util $stderr.reopen(error_file) 3.upto(256){|fd| IO::new(fd).close rescue nil} - if arguments[:gid] - Process.egid = arguments[:gid] - Process.gid = arguments[:gid] unless @@os == "Darwin" - end - if arguments[:uid] - Process.euid = arguments[:uid] - Process.uid = arguments[:uid] unless @@os == "Darwin" - end + Puppet::Util::SUIDManager.change_group(arguments[:gid], true) if arguments[:gid] + Puppet::Util::SUIDManager.change_user(arguments[:uid], true) if arguments[:uid] ENV['LANG'] = ENV['LC_ALL'] = ENV['LC_MESSAGES'] = ENV['LANGUAGE'] = 'C' if command.is_a?(Array) Kernel.exec(*command) diff --git a/lib/puppet/util/suidmanager.rb b/lib/puppet/util/suidmanager.rb index 6633de002..2e12b220f 100644 --- a/lib/puppet/util/suidmanager.rb +++ b/lib/puppet/util/suidmanager.rb @@ -36,12 +36,6 @@ module Puppet::Util::SUIDManager end module_function :groups= - if Facter['kernel'].value == 'Darwin' - # Cannot change real UID on Darwin so we set euid - alias :uid :euid - alias :gid :egid - end - def self.root? Process.uid == 0 end @@ -50,23 +44,55 @@ module Puppet::Util::SUIDManager def asuser(new_uid=nil, new_gid=nil) return yield if Puppet.features.microsoft_windows? or !root? - # We set both because some programs like to drop privs, i.e. bash. - old_uid, old_gid = self.uid, self.gid old_euid, old_egid = self.euid, self.egid - old_groups = self.groups begin - self.egid = convert_xid :gid, new_gid if new_gid - self.initgroups(convert_xid(:uid, new_uid)) if new_uid - self.euid = convert_xid :uid, new_uid if new_uid + change_group(new_gid) if new_gid + change_user(new_uid) if new_uid yield ensure - self.euid, self.egid = old_euid, old_egid - self.groups = old_groups + change_group(old_egid) + change_user(old_euid) end end module_function :asuser + def change_group(group, permanently=false) + gid = convert_xid(:gid, group) + raise Puppet::Error, "No such group #{group}" unless gid + + if permanently + begin + Process::GID.change_privilege(gid) + rescue NotImplementedError + Process.egid = gid + Process.gid = gid + end + else + Process.egid = gid + end + end + module_function :change_group + + def change_user(user, permanently=false) + uid = convert_xid(:uid, user) + raise Puppet::Error, "No such user #{user}" unless uid + + if permanently + begin + Process::UID.change_privilege(uid) + rescue NotImplementedError + initgroups(uid) + Process.euid = uid + Process.uid = uid + end + else + initgroups(uid) + Process.euid = uid + end + end + module_function :change_user + # Make sure the passed argument is a number. def convert_xid(type, id) map = {:gid => :group, :uid => :user} diff --git a/spec/unit/util/suidmanager_spec.rb b/spec/unit/util/suidmanager_spec.rb index 19ddc2b4d..f58f6c708 100755 --- a/spec/unit/util/suidmanager_spec.rb +++ b/spec/unit/util/suidmanager_spec.rb @@ -34,8 +34,11 @@ describe Puppet::Util::SUIDManager do it "should set euid/egid when root" do Process.stubs(:uid).returns(0) + Process.stubs(:egid).returns(51) Process.stubs(:euid).returns(50) - Process.stubs(:egid).returns(50) + + Puppet::Util::SUIDManager.stubs(:convert_xid).with(:gid, 51).returns(51) + Puppet::Util::SUIDManager.stubs(:convert_xid).with(:uid, 50).returns(50) yielded = false Puppet::Util::SUIDManager.asuser(user[:uid], user[:gid]) do @@ -44,7 +47,7 @@ describe Puppet::Util::SUIDManager do yielded = true end - xids[:egid].should == 50 + xids[:egid].should == 51 xids[:euid].should == 50 # It's possible asuser could simply not yield, so the assertions in the @@ -55,8 +58,8 @@ describe Puppet::Util::SUIDManager do it "should not get or set euid/egid when not root" do Process.stubs(:uid).returns(1) + Process.stubs(:egid).returns(51) Process.stubs(:euid).returns(50) - Process.stubs(:egid).returns(50) Puppet::Util::SUIDManager.asuser(user[:uid], user[:gid]) {} @@ -64,6 +67,76 @@ describe Puppet::Util::SUIDManager do end end + describe "#change_group" do + describe "when changing permanently" do + it "should try to change_privilege if it is supported" do + Process::GID.expects(:change_privilege).with do |gid| + Process.gid = gid + Process.egid = gid + end + + Puppet::Util::SUIDManager.change_group(42, true) + + xids[:egid].should == 42 + xids[:gid].should == 42 + end + + it "should change both egid and gid if change_privilege isn't supported" do + Process::GID.stubs(:change_privilege).raises(NotImplementedError) + + Puppet::Util::SUIDManager.change_group(42, true) + + xids[:egid].should == 42 + xids[:gid].should == 42 + end + end + + describe "when changing temporarily" do + it "should change only egid" do + Puppet::Util::SUIDManager.change_group(42, false) + + xids[:egid].should == 42 + xids[:gid].should == 0 + end + end + end + + describe "#change_user" do + describe "when changing permanently" do + it "should try to change_privilege if it is supported" do + Process::UID.expects(:change_privilege).with do |uid| + Process.uid = uid + Process.euid = uid + end + + Puppet::Util::SUIDManager.change_user(42, true) + + xids[:euid].should == 42 + xids[:uid].should == 42 + end + + it "should change euid and uid and groups if change_privilege isn't supported" do + Process::UID.stubs(:change_privilege).raises(NotImplementedError) + + Puppet::Util::SUIDManager.expects(:initgroups).with(42) + + Puppet::Util::SUIDManager.change_user(42, true) + + xids[:euid].should == 42 + xids[:uid].should == 42 + end + end + + describe "when changing temporarily" do + it "should change only euid and groups" do + Puppet::Util::SUIDManager.change_user(42, false) + + xids[:euid].should == 42 + xids[:uid].should == 0 + end + end + end + describe "when running commands" do before :each do # We want to make sure $CHILD_STATUS is set @@ -73,18 +146,20 @@ describe Puppet::Util::SUIDManager do describe "with #system" do it "should set euid/egid when root" do Process.stubs(:uid).returns(0) - Process.stubs(:groups=) - Process.expects(:euid).returns(99997) - Process.expects(:egid).returns(99996) + Process.stubs(:egid).returns(51) + Process.stubs(:euid).returns(50) + + Puppet::Util::SUIDManager.stubs(:convert_xid).with(:gid, 51).returns(51) + Puppet::Util::SUIDManager.stubs(:convert_xid).with(:uid, 50).returns(50) - Process.expects(:euid=).with(uid) - Process.expects(:egid=).with(gid) + Puppet::Util::SUIDManager.expects(:change_group).with(user[:uid]) + Puppet::Util::SUIDManager.expects(:change_user).with(user[:uid]) + + Puppet::Util::SUIDManager.expects(:change_group).with(51) + Puppet::Util::SUIDManager.expects(:change_user).with(50) Kernel.expects(:system).with('blah') Puppet::Util::SUIDManager.system('blah', user[:uid], user[:gid]) - - xids[:egid].should == 99996 - xids[:euid].should == 99997 end it "should not get or set euid/egid when not root" do -- cgit