fixes #3521 correcting format of user name passed to policy apis and does not clobber existing service rights of other usersservice_user

7 files changed, 137 insertions, 87 deletions

diff --git a/lib/chef/provider/service/windows.rb b/lib/chef/provider/service/windows.rb
index 355ffafc2a..e41e815f2e 100644
--- a/lib/chef/provider/service/windows.rb
+++ b/lib/chef/provider/service/windows.rb
@@ -47,6 +47,8 @@ class Chef::Provider::Service::Windows < Chef::Provider::Service
 
   TIMEOUT  = 60
 
+  SERVICE_RIGHT = 'SeServiceLogonRight'
+
   def whyrun_supported?
     false
   end
@@ -78,10 +80,10 @@ class Chef::Provider::Service::Windows < Chef::Provider::Service
       Win32::Service.configure(new_config)
       Chef::Log.info "#{@new_resource} configured with #{new_config.inspect}"
 
-      # it would be nice to check if the user already has the logon privilege, but that turns out to be
-      # nontrivial.
       if new_config.has_key?(:service_start_name)
-        grant_service_logon(new_config[:service_start_name])
+        unless Chef::ReservedNames::Win32::Security.get_account_right(canonicalize_username(new_config[:service_start_name])).include?(SERVICE_RIGHT)
+          grant_service_logon(new_config[:service_start_name])
+        end
       end
 
       state = current_state
@@ -236,62 +238,15 @@ class Chef::Provider::Service::Windows < Chef::Provider::Service
   end
 
   private
-  def make_policy_text(username)
-    text = <<-EOS
-[Unicode]
-Unicode=yes
-[Privilege Rights]
-SeServiceLogonRight = \\\\#{canonicalize_username(username)},*S-1-5-80-0
-[Version]
-signature="$CHICAGO$"
-Revision=1
-EOS
-  end
-
-  def grant_logfile_name(username)
-    Chef::Util::PathHelper.canonical_path("#{Dir.tmpdir}/logon_grant-#{clean_username_for_path(username)}-#{$$}.log", prefix=false)
-  end
-
-  def grant_policyfile_name(username)
-    Chef::Util::PathHelper.canonical_path("#{Dir.tmpdir}/service_logon_policy-#{clean_username_for_path(username)}-#{$$}.inf", prefix=false)
-  end
-
-  def grant_dbfile_name(username)
-    "#{ENV['TEMP']}\\secedit.sdb"
-  end
-
   def grant_service_logon(username)
-    logfile = grant_logfile_name(username)
-    policy_file = ::File.new(grant_policyfile_name(username), 'w')
-    policy_text = make_policy_text(username)
-    dbfile = grant_dbfile_name(username)        # this is just an audit file.
-
     begin
-      Chef::Log.debug "Policy file text:\n#{policy_text}"
-      policy_file.puts(policy_text)
-      policy_file.close   # need to flush the buffer.
-
-      # it would be nice to do this with APIs instead, but the LSA_* APIs are
-      # particularly onerous and life is short.
-      cmd = %Q{secedit.exe /configure /db "#{dbfile}" /cfg "#{policy_file.path}" /areas USER_RIGHTS SECURITYPOLICY SERVICES /log "#{logfile}"}
-      Chef::Log.debug "Granting logon-as-service privilege with: #{cmd}"
-      runner = shell_out(cmd)
-
-      if runner.exitstatus != 0
-        Chef::Log.fatal "Logon-as-service grant failed with output: #{runner.stdout}"
-        raise Chef::Exceptions::Service, <<-EOS
-Logon-as-service grant failed with policy file #{policy_file.path}.
-You can look at #{logfile} for details, or do `secedit /analyze #{dbfile}`.
-The failed command was `#{cmd}`.
-EOS
-      end
-
-      Chef::Log.info "Grant logon-as-service to user '#{username}' successful."
-
-      ::File.delete(dbfile) rescue nil
-      ::File.delete(policy_file)
-      ::File.delete(logfile) rescue nil     # logfile is not always present at end.
+      Chef::ReservedNames::Win32::Security.add_account_right(canonicalize_username(username), SERVICE_RIGHT)
+    rescue Chef::Exceptions::Win32APIError => err
+      Chef::Log.fatal "Logon-as-service grant failed with output: #{err}"
+      raise Chef::Exceptions::Service, "Logon-as-service grant failed for #{username}: #{err}"
     end
+
+    Chef::Log.info "Grant logon-as-service to user '#{username}' successful."
     true
   end
 
diff --git a/lib/chef/win32/api.rb b/lib/chef/win32/api.rb
index 4786222bd4..acc3980f81 100644
--- a/lib/chef/win32/api.rb
+++ b/lib/chef/win32/api.rb
@@ -147,6 +147,7 @@ class Chef
         host.typedef :long,    :LRESULT # Signed result of message processing. WinDef.h: host.typedef LONG_PTR LRESULT;
         host.typedef :pointer, :LPWIN32_FIND_DATA # Pointer to WIN32_FIND_DATA struct
         host.typedef :pointer, :LPBY_HANDLE_FILE_INFORMATION # Point to a BY_HANDLE_FILE_INFORMATION struct
+        host.typedef :ulong,   :NTSTATUS # An NTSTATUS code returned by an LSA function call.
         host.typedef :pointer, :PBOOL # Pointer to a BOOL.
         host.typedef :pointer, :PBOOLEAN # Pointer to a BOOL.
         host.typedef :pointer, :PBYTE # Pointer to a BYTE.
diff --git a/lib/chef/win32/api/security.rb b/lib/chef/win32/api/security.rb
index 4c352a3554..9f5302fdd1 100644
--- a/lib/chef/win32/api/security.rb
+++ b/lib/chef/win32/api/security.rb
@@ -207,6 +207,21 @@ class Chef
         LOGON32_PROVIDER_WINNT40 = 2;
         LOGON32_PROVIDER_WINNT50 = 3;
 
+        # LSA access policy
+        POLICY_VIEW_LOCAL_INFORMATION = 0x00000001
+        POLICY_VIEW_AUDIT_INFORMATION = 0x00000002
+        POLICY_GET_PRIVATE_INFORMATION = 0x00000004
+        POLICY_TRUST_ADMIN = 0x00000008
+        POLICY_CREATE_ACCOUNT = 0x00000010
+        POLICY_CREATE_SECRET = 0x00000020
+        POLICY_CREATE_PRIVILEGE = 0x00000040
+        POLICY_SET_DEFAULT_QUOTA_LIMITS = 0x00000080
+        POLICY_SET_AUDIT_REQUIREMENTS = 0x00000100
+        POLICY_AUDIT_LOG_ADMIN = 0x00000200
+        POLICY_SERVER_ADMIN = 0x00000400
+        POLICY_LOOKUP_NAMES = 0x00000800
+        POLICY_NOTIFICATION = 0x00001000
+
         ###############################################
         # Win32 API Bindings
         ###############################################
@@ -381,6 +396,23 @@ class Chef
           end
         end
 
+        # https://msdn.microsoft.com/en-us/library/windows/desktop/ms721829(v=vs.85).aspx
+        class LSA_OBJECT_ATTRIBUTES < FFI::Struct
+          layout :Length, :ULONG,
+                 :RootDirectory, :HANDLE,
+                 :ObjectName, :pointer,
+                 :Attributes, :ULONG,
+                 :SecurityDescriptor, :PVOID,
+                 :SecurityQualityOfService, :PVOID
+        end
+
+        # https://msdn.microsoft.com/en-us/library/windows/desktop/ms721841(v=vs.85).aspx
+        class LSA_UNICODE_STRING < FFI::Struct
+          layout :Length, :USHORT,
+                 :MaximumLength, :USHORT,
+                 :Buffer, :PWSTR
+        end
+
         ffi_lib "advapi32"
 
         safe_attach_function :AccessCheck, [:pointer, :HANDLE, :DWORD, :pointer, :pointer, :pointer, :pointer, :pointer], :BOOL
@@ -415,6 +447,12 @@ class Chef
         safe_attach_function :LookupPrivilegeNameW, [ :LPCWSTR, :PLUID, :LPWSTR, :LPDWORD ], :BOOL
         safe_attach_function :LookupPrivilegeDisplayNameW, [ :LPCWSTR, :LPCWSTR, :LPWSTR, :LPDWORD, :LPDWORD ], :BOOL
         safe_attach_function :LookupPrivilegeValueW, [ :LPCWSTR, :LPCWSTR, :PLUID ], :BOOL
+        safe_attach_function :LsaAddAccountRights, [ :pointer, :pointer, :pointer, :ULONG ], :NTSTATUS
+        safe_attach_function :LsaClose, [ :pointer ], :NTSTATUS
+        safe_attach_function :LsaEnumerateAccountRights, [ :pointer, :pointer, :pointer, :pointer ], :NTSTATUS
+        safe_attach_function :LsaFreeMemory, [ :pointer ], :NTSTATUS
+        safe_attach_function :LsaNtStatusToWinError, [ :NTSTATUS ], :ULONG
+        safe_attach_function :LsaOpenPolicy, [ :pointer, :pointer, :DWORD, :pointer ], :NTSTATUS
         safe_attach_function :MakeAbsoluteSD, [ :pointer, :pointer, :LPDWORD, :pointer, :LPDWORD, :pointer, :LPDWORD, :pointer, :LPDWORD, :pointer, :LPDWORD], :BOOL
         safe_attach_function :MapGenericMask, [ :PDWORD, :PGENERICMAPPING ], :void
         safe_attach_function :OpenProcessToken, [ :HANDLE, :DWORD, :PHANDLE ], :BOOL
diff --git a/lib/chef/win32/error.rb b/lib/chef/win32/error.rb
index 2175608eeb..c638773d2a 100644
--- a/lib/chef/win32/error.rb
+++ b/lib/chef/win32/error.rb
@@ -57,8 +57,7 @@ class Chef
       # nil::: always returns nil when it does not raise
       # === Raises
       # Chef::Exceptions::Win32APIError:::
-      def self.raise!(message = nil)
-        code = get_last_error
+      def self.raise!(message = nil, code = get_last_error)
         msg = format_message(code).strip
         formatted_message = ""
         formatted_message << message if message
diff --git a/lib/chef/win32/security.rb b/lib/chef/win32/security.rb
index bc80517d80..38694c9fec 100644
--- a/lib/chef/win32/security.rb
+++ b/lib/chef/win32/security.rb
@@ -104,6 +104,22 @@ class Chef
         end
       end
 
+      def self.add_account_right(name, privilege)
+        privilege_pointer = FFI::MemoryPointer.new LSA_UNICODE_STRING, 1
+        privilege_lsa_string = LSA_UNICODE_STRING.new(privilege_pointer)
+        privilege_lsa_string[:Buffer] = FFI::MemoryPointer.from_string(privilege.to_wstring)
+        privilege_lsa_string[:Length] = privilege.length * 2
+        privilege_lsa_string[:MaximumLength] = (privilege.length + 1) * 2
+
+        with_lsa_policy(name) do |policy_handle, sid|
+          result = LsaAddAccountRights(policy_handle.read_pointer, sid, privilege_pointer, 1)
+          win32_error = LsaNtStatusToWinError(result)
+          if win32_error != 0
+            Chef::ReservedNames::Win32::Error.raise!(nil, win32_error)
+          end
+        end
+      end
+
       def self.adjust_token_privileges(token, privileges)
         token = token.handle if token.respond_to?(:handle)
         old_privileges_size = FFI::Buffer.new(:long).write_long(privileges.size_with_privileges)
@@ -165,6 +181,29 @@ class Chef
         end
       end
 
+      def self.get_account_right(name)
+        privileges = []
+        privilege_pointer = FFI::MemoryPointer.new(:pointer)
+        privilege_length = FFI::MemoryPointer.new(:ulong)
+
+        with_lsa_policy(name) do |policy_handle, sid|
+          result = LsaEnumerateAccountRights(policy_handle.read_pointer, sid, privilege_pointer, privilege_length)
+          win32_error = LsaNtStatusToWinError(result)
+          return [] if win32_error == 2 # FILE_NOT_FOUND - No rights assigned
+          if win32_error != 0
+            Chef::ReservedNames::Win32::Error.raise!(nil, win32_error)
+          end
+
+          privilege_length.read_ulong.times do |i|
+            privilege = LSA_UNICODE_STRING.new(privilege_pointer.read_pointer + i * LSA_UNICODE_STRING.size)
+            privileges << privilege[:Buffer].read_wstring
+          end
+          LsaFreeMemory(privilege_pointer)
+        end
+
+        privileges
+      end
+
       def self.get_ace(acl, index)
         acl = acl.pointer if acl.respond_to?(:pointer)
         ace = FFI::Buffer.new :pointer
@@ -548,6 +587,30 @@ class Chef
         end
       end
 
+      def self.with_lsa_policy(username)
+        sid = lookup_account_name(username)[1]
+
+        access = 0
+        access |= POLICY_CREATE_ACCOUNT
+        access |= POLICY_LOOKUP_NAMES
+
+        policy_handle = FFI::MemoryPointer.new(:pointer)
+        result = LsaOpenPolicy(nil, LSA_OBJECT_ATTRIBUTES.new, access, policy_handle)
+        win32_error = LsaNtStatusToWinError(result)
+        if win32_error != 0
+          Chef::ReservedNames::Win32::Error.raise!(nil, win32_error)
+        end
+
+        begin
+          yield policy_handle, sid.pointer
+        ensure
+          win32_error = LsaNtStatusToWinError(LsaClose(policy_handle.read_pointer))
+          if win32_error != 0
+            Chef::ReservedNames::Win32::Error.raise!(nil, win32_error)
+          end
+        end
+      end
+
       def self.with_privileges(*privilege_names)
         # Set privileges
         token = open_current_process_token(TOKEN_READ | TOKEN_ADJUST_PRIVILEGES)
diff --git a/spec/functional/resource/windows_service_spec.rb b/spec/functional/resource/windows_service_spec.rb
index 90545429e5..9ee4adde87 100644
--- a/spec/functional/resource/windows_service_spec.rb
+++ b/spec/functional/resource/windows_service_spec.rb
@@ -23,7 +23,7 @@ describe Chef::Resource::WindowsService, :windows_only, :system_windows_service_
   include_context "using Win32::Service"
 
   let(:username) { "service_spec_user"}
-  let(:qualified_username) { ".\\#{username}"}
+  let(:qualified_username) { "#{ENV['COMPUTERNAME']}\\#{username}"}
   let(:password) { "1a2b3c4X!&narf"}
 
   let(:user_resource) {
@@ -93,6 +93,9 @@ describe Chef::Resource::WindowsService, :windows_only, :system_windows_service_
       service_resource.run_action(:start)
     end
 
-    it "raises an exception when it can't grant the logon privilege"
+    it "grants the user the log on as service right" do
+      service_resource.run_action(:start)
+      expect(Chef::ReservedNames::Win32::Security.get_account_right(qualified_username)).to include("SeServiceLogonRight")
+    end
   end
 end
diff --git a/spec/unit/provider/service/windows_spec.rb b/spec/unit/provider/service/windows_spec.rb
index 4c9f5b3377..34140fdd7b 100644
--- a/spec/unit/provider/service/windows_spec.rb
+++ b/spec/unit/provider/service/windows_spec.rb
@@ -30,6 +30,7 @@ describe Chef::Provider::Service::Windows, "load_current_resource" do
     prvdr.current_resource = Chef::Resource::WindowsService.new("current-chef")
     prvdr
   end
+  let(:service_right) { Chef::Provider::Service::Windows::SERVICE_RIGHT }
 
   before(:all) do
     Win32::Service = Class.new
@@ -46,6 +47,7 @@ describe Chef::Provider::Service::Windows, "load_current_resource" do
       double("ConfigStruct", :start_type => "auto start"))
     allow(Win32::Service).to receive(:exists?).and_return(true)
     allow(Win32::Service).to receive(:configure).and_return(Win32::Service)
+    allow(Chef::ReservedNames::Win32::Security).to receive(:get_account_right).and_return([])
   end
 
   after(:each) do
@@ -164,6 +166,13 @@ describe Chef::Provider::Service::Windows, "load_current_resource" do
         expect(provider).to receive(:grant_service_logon).and_return(true)
         provider.start_service
       end
+
+      it "does not grant user SeServiceLogonRight if it already has it" do
+        expect(Win32::Service).to receive(:start)
+        expect(Chef::ReservedNames::Win32::Security).to receive(:get_account_right).with("wallace").and_return([service_right])
+        expect(Chef::ReservedNames::Win32::Security).not_to receive(:add_account_right).with("wallace", service_right)
+        provider.start_service
+      end
     end
   end
 
@@ -417,37 +426,19 @@ describe Chef::Provider::Service::Windows, "load_current_resource" do
     include_context "testing private methods"
 
     let(:username) { "unit_test_user" }
-    let(:success_string) { "The task has completed successfully.\r\nSee logfile etc." }
-    let(:failure_string) { "Look on my works, ye Mighty, and despair!" }
-    let(:command) {
-      dbfile = provider.grant_dbfile_name(username)
-      policyfile = provider.grant_policyfile_name(username)
-      logfile = provider.grant_logfile_name(username)
-
-      %Q{secedit.exe /configure /db "#{dbfile}" /cfg "#{policyfile}" /areas USER_RIGHTS SECURITYPOLICY SERVICES /log "#{logfile}"}
-    }
-    let(:shellout_env) { {:environment=>{"LC_ALL"=>"en_US.UTF-8"}} }
 
-    before {
-      expect_any_instance_of(described_class).to receive(:shell_out).with(command).and_call_original
-      expect_any_instance_of(Mixlib::ShellOut).to receive(:run_command).and_return(nil)
-    }
-
-    after {
-      # only needed for the second test.
-      ::File.delete(provider.grant_policyfile_name(username)) rescue nil
-      ::File.delete(provider.grant_logfile_name(username)) rescue nil
-      ::File.delete(provider.grant_dbfile_name(username)) rescue nil
-    }
-
-    it "calls Mixlib::Shellout with the correct command string" do
-      expect_any_instance_of(Mixlib::ShellOut).to receive(:exitstatus).and_return(0)
+    it "calls win32 api to grant user SeServiceLogonRight" do
+      expect(Chef::ReservedNames::Win32::Security).to receive(:add_account_right).with(username, service_right)
       expect(provider.grant_service_logon(username)).to equal true
     end
 
-    it "raises an exception when the grant command fails" do
-      expect_any_instance_of(Mixlib::ShellOut).to receive(:exitstatus).and_return(1)
-      expect_any_instance_of(Mixlib::ShellOut).to receive(:stdout).and_return(failure_string)
+    it "strips '.\' from user name when sending to win32 api" do
+      expect(Chef::ReservedNames::Win32::Security).to receive(:add_account_right).with(username, service_right)
+      expect(provider.grant_service_logon(".\\#{username}")).to equal true
+    end
+
+    it "raises an exception when the grant fails" do
+      expect(Chef::ReservedNames::Win32::Security).to receive(:add_account_right).and_raise(Chef::Exceptions::Win32APIError, "barf")
       expect { provider.grant_service_logon(username) }.to raise_error(Chef::Exceptions::Service)
     end
   end