From 3bb52c0682797e716b7030ef0af6cb885e68cf25 Mon Sep 17 00:00:00 2001 From: Matt Wrock Date: Thu, 17 Dec 2015 18:32:56 -0800 Subject: fixes #3521 correcting format of user name passed to policy apis and does not clobber existing service rights of other users --- lib/chef/provider/service/windows.rb | 67 ++++-------------------- lib/chef/win32/api.rb | 1 + lib/chef/win32/api/security.rb | 38 ++++++++++++++ lib/chef/win32/error.rb | 3 +- lib/chef/win32/security.rb | 63 ++++++++++++++++++++++ spec/functional/resource/windows_service_spec.rb | 7 ++- spec/unit/provider/service/windows_spec.rb | 45 +++++++--------- 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 -- cgit v1.2.1