(#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 <jacob@puppetlabs.com>

1 files changed, 86 insertions, 11 deletions

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