Merge pull request #6687 from MsysTechnologiesllc/nim/logonSession_user_permission

[MSYS-724] Chef::Util::Windows::LogonSession should allow having only the prescribed users permissions

9 files changed, 167 insertions, 15 deletions

diff --git a/lib/chef/mixin/user_context.rb b/lib/chef/mixin/user_context.rb
index 526d6b0f3f..f5c2da1389 100644
--- a/lib/chef/mixin/user_context.rb
+++ b/lib/chef/mixin/user_context.rb
@@ -22,7 +22,10 @@ class Chef
   module Mixin
     module UserContext
 
-      def with_user_context(user, password, domain = nil, &block)
+      # valid values for authentication => :remote, :local
+      # When authentication = :local, we use the credentials to create a logon session against the local system, and then try to access the files.
+      # When authentication = :remote, we continue with the current user but pass the provided credentials to the remote system.
+      def with_user_context(user, password, domain = nil, authentication = :remote, &block)
         unless Chef::Platform.windows?
           raise Exceptions::UnsupportedPlatform, "User context impersonation is supported only on the Windows platform"
         end
@@ -31,11 +34,11 @@ class Chef
           raise ArgumentError, "You must supply a block to `with_user_context`"
         end
 
-        login_session = nil
+        logon_session = nil
 
         begin
           if user
-            logon_session = Chef::Util::Windows::LogonSession.new(user, password, domain)
+            logon_session = Chef::Util::Windows::LogonSession.new(user, password, domain, authentication)
             logon_session.open
             logon_session.set_user_context
           end
diff --git a/lib/chef/provider/remote_file/network_file.rb b/lib/chef/provider/remote_file/network_file.rb
index a08bfd2453..f9dc7b0e7b 100644
--- a/lib/chef/provider/remote_file/network_file.rb
+++ b/lib/chef/provider/remote_file/network_file.rb
@@ -43,7 +43,7 @@ class Chef
             tempfile = Chef::FileContentManagement::Tempfile.new(new_resource).tempfile
             Chef::Log.debug("#{new_resource} staging #{@source} to #{tempfile.path}")
 
-            with_user_context(new_resource.remote_user, new_resource.remote_password, new_resource.remote_domain) do
+            with_user_context(new_resource.remote_user, new_resource.remote_password, new_resource.remote_domain, new_resource.authentication) do
               ::File.open(@source, "rb") do |remote_file|
                 while data = remote_file.read(TRANSFER_CHUNK_SIZE)
                   tempfile.write(data)
diff --git a/lib/chef/resource/remote_file.rb b/lib/chef/resource/remote_file.rb
index 4db055a20d..d2c2622524 100644
--- a/lib/chef/resource/remote_file.rb
+++ b/lib/chef/resource/remote_file.rb
@@ -139,6 +139,8 @@ class Chef
 
       property :remote_password, String, sensitive: true
 
+      property :authentication, equal_to: [:remote, :local], default: :remote
+
       def after_created
         validate_identity_platform(remote_user, remote_password, remote_domain)
         identity = qualify_user(remote_user, remote_password, remote_domain)
diff --git a/lib/chef/util/windows/logon_session.rb b/lib/chef/util/windows/logon_session.rb
index ef80b113b1..afe58ae4f9 100644
--- a/lib/chef/util/windows/logon_session.rb
+++ b/lib/chef/util/windows/logon_session.rb
@@ -25,7 +25,7 @@ class Chef
       class LogonSession
         include Chef::Mixin::WideString
 
-        def initialize(username, password, domain = nil)
+        def initialize(username, password, domain = nil, authentication = :remote)
           if username.nil? || password.nil?
             raise ArgumentError, "The logon session must be initialize with non-nil user name and password parameters"
           end
@@ -33,6 +33,7 @@ class Chef
           @original_username = username
           @original_password = password
           @original_domain = domain
+          @authentication = authentication
           @token = FFI::Buffer.new(:pointer)
           @session_opened = false
           @impersonating = false
@@ -47,7 +48,8 @@ class Chef
           password = wstring(original_password)
           domain = wstring(original_domain)
 
-          status = Chef::ReservedNames::Win32::API::Security.LogonUserW(username, domain, password, Chef::ReservedNames::Win32::API::Security::LOGON32_LOGON_NEW_CREDENTIALS, Chef::ReservedNames::Win32::API::Security::LOGON32_PROVIDER_DEFAULT, token)
+          logon_type = (authentication == :local) ? (Chef::ReservedNames::Win32::API::Security::LOGON32_LOGON_NETWORK) : (Chef::ReservedNames::Win32::API::Security::LOGON32_LOGON_NEW_CREDENTIALS)
+          status = Chef::ReservedNames::Win32::API::Security.LogonUserW(username, domain, password, logon_type, Chef::ReservedNames::Win32::API::Security::LOGON32_PROVIDER_DEFAULT, token)
 
           if !status
             last_error = FFI::LastError.error
@@ -110,6 +112,7 @@ class Chef
         attr_reader :original_username
         attr_reader :original_password
         attr_reader :original_domain
+        attr_reader :authentication
 
         attr_reader :token
         attr_reader :session_opened
diff --git a/lib/chef/win32/api/security.rb b/lib/chef/win32/api/security.rb
index a6f79f5d7d..6620f321aa 100644
--- a/lib/chef/win32/api/security.rb
+++ b/lib/chef/win32/api/security.rb
@@ -303,6 +303,17 @@ class Chef
              :SecurityDelegation,
         ]
 
+        # https://msdn.microsoft.com/en-us/library/windows/desktop/bb530718%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396
+        ELEVATION_TYPE = enum :ELEVATION_TYPE, [
+            :TokenElevationTypeDefault, 1,
+            :TokenElevationTypeFull,
+            :TokenElevationTypeLimited
+        ]
+
+        class TOKEN_ELEVATION_TYPE < FFI::Struct
+          layout :ElevationType, :ELEVATION_TYPE
+        end
+
         # SECURITY_DESCRIPTOR is an opaque structure whose contents can vary.  Pass the
         # pointer around and free it with LocalFree.
         # http://msdn.microsoft.com/en-us/library/windows/desktop/aa379561(v=vs.85).aspx
diff --git a/lib/chef/win32/security.rb b/lib/chef/win32/security.rb
index c7d3f55a40..63b626b1d7 100644
--- a/lib/chef/win32/security.rb
+++ b/lib/chef/win32/security.rb
@@ -341,6 +341,22 @@ class Chef
         SID.new(group_result[:PrimaryGroup], group_result_storage)
       end
 
+      def self.get_token_information_elevation_type(token)
+        token_result_size = FFI::MemoryPointer.new(:ulong)
+        if GetTokenInformation(token.handle.handle, :TokenElevationType, nil, 0, token_result_size)
+          raise "Expected ERROR_INSUFFICIENT_BUFFER from GetTokenInformation, and got no error!"
+        elsif FFI::LastError.error != ERROR_INSUFFICIENT_BUFFER
+          Chef::ReservedNames::Win32::Error.raise!
+        end
+        info_ptr = FFI::MemoryPointer.new(:pointer)
+        token_info_pointer = TOKEN_ELEVATION_TYPE.new info_ptr
+        token_info_length = 4
+        unless GetTokenInformation(token.handle.handle, :TokenElevationType, token_info_pointer, token_info_length, token_result_size)
+          Chef::ReservedNames::Win32::Error.raise!
+        end
+        token_info_pointer[:ElevationType]
+      end
+
       def self.initialize_acl(acl_size)
         acl = FFI::MemoryPointer.new acl_size
         unless InitializeAcl(acl, acl_size, ACL_REVISION)
@@ -632,7 +648,19 @@ class Chef
 
           true
         else
-          process_token = open_current_process_token(TOKEN_READ)
+          # a regular user doesn't have privileges to call Chef::ReservedNames::Win32::Security.OpenProcessToken
+          # hence we return false if the open_current_process_token fails with `Access is denied.` error message.
+          begin
+            process_token = open_current_process_token(TOKEN_READ)
+          rescue Exception => run_error
+            return false if run_error.message =~ /Access is denied/
+            Chef::ReservedNames::Win32::Error.raise!
+          end
+
+          # display token elevation details
+          token_elevation_type = get_token_information_elevation_type(process_token)
+          Chef::Log.debug("Token Elevation Type: #{token_elevation_type}")
+
           elevation_result = FFI::Buffer.new(:ulong)
           elevation_result_size = FFI::MemoryPointer.new(:uint32)
           success = GetTokenInformation(process_token.handle.handle, :TokenElevation, elevation_result, 4, elevation_result_size)
diff --git a/spec/functional/win32/security_spec.rb b/spec/functional/win32/security_spec.rb
index 40ae99bfa4..6c24cbec08 100644
--- a/spec/functional/win32/security_spec.rb
+++ b/spec/functional/win32/security_spec.rb
@@ -17,6 +17,8 @@
 #
 
 require "spec_helper"
+require "mixlib/shellout"
+require "chef/mixin/user_context"
 if Chef::Platform.windows?
   require "chef/win32/security"
 end
@@ -26,13 +28,37 @@ describe "Chef::Win32::Security", :windows_only do
     expect(Chef::ReservedNames::Win32::Security.has_admin_privileges?).to eq(true)
   end
 
-  # We've done some investigation adding a negative test and it turned
-  # out to be a lot of work since mixlib-shellout doesn't have user
-  # support for windows.
-  #
-  # TODO - Add negative tests once mixlib-shellout has user support
-  it "has_admin_privileges? returns false when running as non-admin" do
-    skip "requires user support in mixlib-shellout"
+  describe "running as non admin user" do
+    include Chef::Mixin::UserContext
+    let(:user) { "security_user" }
+    let(:password) { "Security@123" }
+
+    let(:domain) do
+      whoami = Mixlib::ShellOut.new("whoami")
+      whoami.run_command
+      whoami.error!
+      whoami.stdout.split("\\")[0]
+    end
+
+    before do
+      allow_any_instance_of(Chef::Mixin::UserContext).to receive(:node).and_return({ "platform_family" => "windows" })
+      allow(Chef::Platform).to receive(:windows_server_2003?).and_return(false)
+      add_user = Mixlib::ShellOut.new("net user #{user} #{password} /ADD")
+      add_user.run_command
+      add_user.error!
+    end
+
+    after do
+      delete_user = Mixlib::ShellOut.new("net user #{user} /delete")
+      delete_user.run_command
+      delete_user.error!
+    end
+    it "has_admin_privileges? returns false" do
+      has_admin_privileges = with_user_context(user, password, domain, :local) do
+        Chef::ReservedNames::Win32::Security.has_admin_privileges?
+      end
+      expect(has_admin_privileges).to eq(false)
+    end
   end
 
   describe "get_file_security" do
@@ -97,4 +123,31 @@ describe "Chef::Win32::Security", :windows_only do
       end
     end
   end
+
+  describe ".get_token_information_elevation_type" do
+    let(:token_rights) { Chef::ReservedNames::Win32::Security::TOKEN_READ }
+
+    let(:token) do
+      Chef::ReservedNames::Win32::Security.open_process_token(
+        Chef::ReservedNames::Win32::Process.get_current_process,
+        token_rights)
+    end
+
+    context "when the token is valid" do
+      let(:token_elevation_type) { [:TokenElevationTypeDefault, :TokenElevationTypeFull, :TokenElevationTypeLimited] }
+
+      it "returns the token elevation type" do
+        elevation_type = Chef::ReservedNames::Win32::Security.get_token_information_elevation_type(token)
+        expect(token_elevation_type).to include(elevation_type)
+      end
+    end
+
+    context "when the token is invalid" do
+      it "raises `handle invalid` error" do
+        # If `OpenProcessToken` is stubbed, `open_process_token` returns an invalid token
+        allow(Chef::ReservedNames::Win32::Security).to receive(:OpenProcessToken).and_return(true)
+        expect { Chef::ReservedNames::Win32::Security.get_token_information_elevation_type(token) }.to raise_error(Chef::Exceptions::Win32APIError)
+      end
+    end
+  end
 end
diff --git a/spec/unit/util/windows/logon_session_spec.rb b/spec/unit/util/windows/logon_session_spec.rb
index b9b6c458b6..8a94802bf6 100644
--- a/spec/unit/util/windows/logon_session_spec.rb
+++ b/spec/unit/util/windows/logon_session_spec.rb
@@ -27,7 +27,8 @@ describe ::Chef::Util::Windows::LogonSession do
     stub_const("Chef::ReservedNames::Win32::API::System", Class.new )
   end
 
-  let(:session) { ::Chef::Util::Windows::LogonSession.new(session_user, password, session_domain) }
+  let(:session) { ::Chef::Util::Windows::LogonSession.new(session_user, password, session_domain, authentication) }
+  let(:authentication) { :remote }
 
   shared_examples_for "it received syntactically invalid credentials" do
     it "does not raisees an exception when it is initialized" do
diff --git a/spec/unit/win32/security_spec.rb b/spec/unit/win32/security_spec.rb
index 1c755061ce..6e4441a482 100644
--- a/spec/unit/win32/security_spec.rb
+++ b/spec/unit/win32/security_spec.rb
@@ -63,4 +63,55 @@ describe "Chef::Win32::Security", :windows_only do
       end
     end
   end
+
+  describe "self.has_admin_privileges?" do
+    it "returns true for windows server 2003" do
+      allow(Chef::Platform).to receive(:windows_server_2003?).and_return(true)
+      expect(Chef::ReservedNames::Win32::Security.has_admin_privileges?).to be true
+    end
+
+    context "when the user doesn't have admin privileges" do
+      it "returns false" do
+        allow(Chef::Platform).to receive(:windows_server_2003?).and_return(false)
+        allow(Chef::ReservedNames::Win32::Security).to receive(:open_current_process_token).and_raise("Access is denied.")
+        expect(Chef::ReservedNames::Win32::Security.has_admin_privileges?).to be false
+      end
+    end
+
+    context "when open_current_process_token fails with some other error than `Access is Denied`" do
+      it "raises error" do
+        allow(Chef::Platform).to receive(:windows_server_2003?).and_return(false)
+        allow(Chef::ReservedNames::Win32::Security).to receive(:open_current_process_token).and_raise("boom")
+        expect { Chef::ReservedNames::Win32::Security.has_admin_privileges? }.to raise_error(Chef::Exceptions::Win32APIError)
+      end
+    end
+
+    context "when the user has admin privileges" do
+      it "returns true" do
+        allow(Chef::Platform).to receive(:windows_server_2003?).and_return(false)
+        allow(Chef::ReservedNames::Win32::Security).to receive(:open_current_process_token)
+        token = Chef::ReservedNames::Win32::Security.open_current_process_token
+        allow(token).to receive_message_chain(:handle, :handle)
+        allow(Chef::ReservedNames::Win32::Security).to receive(:get_token_information_elevation_type)
+        allow(Chef::ReservedNames::Win32::Security).to receive(:GetTokenInformation).and_return(true)
+        allow_any_instance_of(FFI::Buffer).to receive(:read_ulong).and_return(1)
+        expect(Chef::ReservedNames::Win32::Security.has_admin_privileges?).to be true
+      end
+    end
+  end
+
+  describe "self.get_token_information_elevation_type" do
+    let(:token_rights) { Chef::ReservedNames::Win32::Security::TOKEN_READ }
+
+    let(:token) do
+      Chef::ReservedNames::Win32::Security.open_process_token(
+        Chef::ReservedNames::Win32::Process.get_current_process,
+        token_rights)
+    end
+
+    it "raises error if GetTokenInformation fails" do
+      allow(Chef::ReservedNames::Win32::Security).to receive(:GetTokenInformation).and_return(false)
+      expect { Chef::ReservedNames::Win32::Security.get_token_information_elevation_type(token) }.to raise_error(Chef::Exceptions::Win32APIError)
+    end
+  end
 end