diff options
author | Thom May <thom@may.lt> | 2015-05-20 19:06:01 +0100 |
---|---|---|
committer | Thom May <thom@may.lt> | 2015-05-20 19:06:01 +0100 |
commit | 1bb229a122c5b3a6abb66fa5c2c422f6fbb6121d (patch) | |
tree | 35627136aea8bdad1944e3924f7ed7a3832de157 | |
parent | af098d1f1a7f7fd4a8ce84ab36e69453b9c145aa (diff) | |
parent | 505ccd35d931d69bef1f3c62e15d2fcc77762775 (diff) | |
download | chef-1bb229a122c5b3a6abb66fa5c2c422f6fbb6121d.tar.gz |
Merge pull request #3267 from dbjorge/issue-3266
#3266 Fix bad Windows securable_resource functional spec assumptions for default file owners/groups
-rw-r--r-- | lib/chef/win32/api/security.rb | 8 | ||||
-rw-r--r-- | lib/chef/win32/security.rb | 38 | ||||
-rw-r--r-- | lib/chef/win32/security/sid.rb | 17 | ||||
-rw-r--r-- | spec/functional/win32/sid_spec.rb | 55 | ||||
-rw-r--r-- | spec/spec_helper.rb | 1 | ||||
-rw-r--r-- | spec/support/shared/functional/securable_resource.rb | 28 |
6 files changed, 133 insertions, 14 deletions
diff --git a/lib/chef/win32/api/security.rb b/lib/chef/win32/api/security.rb index 95e7f4aab7..4c352a3554 100644 --- a/lib/chef/win32/api/security.rb +++ b/lib/chef/win32/api/security.rb @@ -284,6 +284,14 @@ class Chef :MaxTokenInfoClass ] + class TOKEN_OWNER < FFI::Struct + layout :Owner, :pointer + end + + class TOKEN_PRIMARY_GROUP < FFI::Struct + layout :PrimaryGroup, :pointer + end + # https://msdn.microsoft.com/en-us/library/windows/desktop/aa379572%28v=vs.85%29.aspx SECURITY_IMPERSONATION_LEVEL = enum :SECURITY_IMPERSONATION_LEVEL, [ :SecurityAnonymous, diff --git a/lib/chef/win32/security.rb b/lib/chef/win32/security.rb index a5a845e55c..5c83180bc0 100644 --- a/lib/chef/win32/security.rb +++ b/lib/chef/win32/security.rb @@ -273,6 +273,36 @@ class Chef [ present.read_char != 0, acl.null? ? nil : ACL.new(acl, security_descriptor), defaulted.read_char != 0 ] end + def self.get_token_information_owner(token) + owner_result_size = FFI::MemoryPointer.new(:ulong) + if GetTokenInformation(token.handle.handle, :TokenOwner, nil, 0, owner_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 + owner_result_storage = FFI::MemoryPointer.new owner_result_size.read_ulong + unless GetTokenInformation(token.handle.handle, :TokenOwner, owner_result_storage, owner_result_size.read_ulong, owner_result_size) + Chef::ReservedNames::Win32::Error.raise! + end + owner_result = TOKEN_OWNER.new owner_result_storage + SID.new(owner_result[:Owner], owner_result_storage) + end + + def self.get_token_information_primary_group(token) + group_result_size = FFI::MemoryPointer.new(:ulong) + if GetTokenInformation(token.handle.handle, :TokenPrimaryGroup, nil, 0, group_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 + group_result_storage = FFI::MemoryPointer.new group_result_size.read_ulong + unless GetTokenInformation(token.handle.handle, :TokenPrimaryGroup, group_result_storage, group_result_size.read_ulong, group_result_size) + Chef::ReservedNames::Win32::Error.raise! + end + group_result = TOKEN_PRIMARY_GROUP.new group_result_storage + SID.new(group_result[:PrimaryGroup], group_result_storage) + end + def self.initialize_acl(acl_size) acl = FFI::MemoryPointer.new acl_size unless InitializeAcl(acl, acl_size, ACL_REVISION) @@ -418,6 +448,10 @@ class Chef [ SecurityDescriptor.new(absolute_sd), SID.new(owner), SID.new(group), ACL.new(dacl), ACL.new(sacl) ] end + def self.open_current_process_token(desired_access = TOKEN_READ) + open_process_token(Chef::ReservedNames::Win32::Process.get_current_process, desired_access) + end + def self.open_process_token(process, desired_access) process = process.handle if process.respond_to?(:handle) process = process.handle if process.respond_to?(:handle) @@ -516,7 +550,7 @@ class Chef def self.with_privileges(*privilege_names) # Set privileges - token = open_process_token(Chef::ReservedNames::Win32::Process.get_current_process, TOKEN_READ | TOKEN_ADJUST_PRIVILEGES) + token = open_current_process_token(TOKEN_READ | TOKEN_ADJUST_PRIVILEGES) old_privileges = token.enable_privileges(*privilege_names) # Let the caller do their privileged stuff @@ -536,7 +570,7 @@ class Chef true else - process_token = open_process_token(Chef::ReservedNames::Win32::Process.get_current_process, TOKEN_READ) + process_token = open_current_process_token(TOKEN_READ) 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/lib/chef/win32/security/sid.rb b/lib/chef/win32/security/sid.rb index 8e9407dc80..f8bd934876 100644 --- a/lib/chef/win32/security/sid.rb +++ b/lib/chef/win32/security/sid.rb @@ -203,6 +203,23 @@ class Chef SID.from_account("#{::ENV['USERDOMAIN']}\\#{::ENV['USERNAME']}") end + # See https://technet.microsoft.com/en-us/library/cc961992.aspx + # In practice, this is SID.Administrators if the current_user is an admin (even if not + # running elevated), and is current_user otherwise. On win2k3, it technically can be + # current_user in all cases if a certain group policy is set. + def self.default_security_object_owner + token = Chef::ReservedNames::Win32::Security.open_current_process_token + Chef::ReservedNames::Win32::Security.get_token_information_owner(token) + end + + # See https://technet.microsoft.com/en-us/library/cc961996.aspx + # In practice, this seems to be SID.current_user for Microsoft Accounts, the current + # user's Domain Users group for domain accounts, and SID.None otherwise. + def self.default_security_object_group + token = Chef::ReservedNames::Win32::Security.open_current_process_token + Chef::ReservedNames::Win32::Security.get_token_information_primary_group(token) + end + def self.admin_account_name @admin_account_name ||= begin admin_account_name = nil diff --git a/spec/functional/win32/sid_spec.rb b/spec/functional/win32/sid_spec.rb new file mode 100644 index 0000000000..1f5f66178a --- /dev/null +++ b/spec/functional/win32/sid_spec.rb @@ -0,0 +1,55 @@ +# +# Author:: Dan Bjorge (<dbjorge@gmail.com>) +# Copyright:: Copyright (c) 2015 Dan Bjorge +# License:: Apache License, Version 2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +require 'spec_helper' +if Chef::Platform.windows? + require 'chef/win32/security' +end + +describe 'Chef::ReservedNames::Win32::SID', :windows_only do + if Chef::Platform.windows? + SID ||= Chef::ReservedNames::Win32::Security::SID + end + + it 'should resolve default_security_object_group as a sane user group', :windows_not_domain_joined_only do + # Domain accounts: domain-specific Domain Users SID + # Microsoft Accounts: SID.current_user + # Else: SID.None + expect(SID.default_security_object_group).to eq(SID.None).or eq(SID.current_user) + end + + context 'running as an elevated administrator user' do + it 'should resolve default_security_object_owner as the Administrators group' do + expect(SID.default_security_object_owner).to eq(SID.Administrators) + end + end + + context 'running as a non-elevated administrator user' do + it 'should resolve default_security_object_owner as the current user' do + skip 'requires user support in mixlib-shellout, see security_spec.rb' + expect(SID.default_security_object_owner).to eq(SID.Administrators) + end + end + + context 'running as a non-elevated, non-administrator user' do + it 'should resolve default_security_object_owner as the current user' do + skip 'requires user support in mixlib-shellout, see security_spec.rb' + expect(SID.default_security_object_owner).to eq(SID.current_user) + end + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index fe96c723b6..8b5e42e5b6 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -130,6 +130,7 @@ RSpec.configure do |config| config.filter_run_excluding :windows_powershell_dsc_only => true unless windows_powershell_dsc? config.filter_run_excluding :windows_powershell_no_dsc_only => true unless ! windows_powershell_dsc? config.filter_run_excluding :windows_domain_joined_only => true unless windows_domain_joined? + config.filter_run_excluding :windows_not_domain_joined_only => true if windows_domain_joined? config.filter_run_excluding :solaris_only => true unless solaris? config.filter_run_excluding :system_windows_service_gem_only => true unless system_windows_service_gem? config.filter_run_excluding :unix_only => true unless unix? diff --git a/spec/support/shared/functional/securable_resource.rb b/spec/support/shared/functional/securable_resource.rb index cd8c2a166b..bde508b14d 100644 --- a/spec/support/shared/functional/securable_resource.rb +++ b/spec/support/shared/functional/securable_resource.rb @@ -313,10 +313,10 @@ shared_examples_for "a securable resource without existing target" do context "on Windows", :windows_only do include_context "use Windows permissions" - it "sets owner to Administrators on create if owner is not specified" do + it "leaves owner as system default on create if owner is not specified" do expect(File.exist?(path)).to eq(false) resource.run_action(:create) - expect(descriptor.owner).to eq(SID.Administrators) + expect(descriptor.owner).to eq(SID.default_security_object_owner) end it "sets owner when owner is specified" do @@ -336,22 +336,24 @@ shared_examples_for "a securable resource without existing target" do end it "leaves owner alone if owner is not specified and resource already exists" do - # Set owner to Guest so it's not the same as the current user (which is the default on create) - resource.owner 'Guest' + arbitrary_non_default_owner = SID.Guest + expect(arbitrary_non_default_owner).not_to eq(SID.default_security_object_owner) + + resource.owner 'Guest' # Change to arbitrary_non_default_owner once issue #1508 is fixed resource.run_action(:create) - expect(descriptor.owner).to eq(SID.Guest) + expect(descriptor.owner).to eq(arbitrary_non_default_owner) new_resource = create_resource expect(new_resource.owner).to eq(nil) new_resource.run_action(:create) - expect(descriptor.owner).to eq(SID.Guest) + expect(descriptor.owner).to eq(arbitrary_non_default_owner) end - it "sets group to None on create if group is not specified" do + it "leaves group as system default on create if group is not specified" do expect(resource.group).to eq(nil) expect(File.exist?(path)).to eq(false) resource.run_action(:create) - expect(descriptor.group).to eq(SID.None) + expect(descriptor.group).to eq(SID.default_security_object_group) end it "sets group when group is specified" do @@ -372,15 +374,17 @@ shared_examples_for "a securable resource without existing target" do end it "leaves group alone if group is not specified and resource already exists" do - # Set group to Everyone so it's not the default (None) - resource.group 'Everyone' + arbitrary_non_default_group = SID.Everyone + expect(arbitrary_non_default_group).not_to eq(SID.default_security_object_group) + + resource.group 'Everyone' # Change to arbitrary_non_default_group once issue #1508 is fixed resource.run_action(:create) - expect(descriptor.group).to eq(SID.Everyone) + expect(descriptor.group).to eq(arbitrary_non_default_group) new_resource = create_resource expect(new_resource.group).to eq(nil) new_resource.run_action(:create) - expect(descriptor.group).to eq(SID.Everyone) + expect(descriptor.group).to eq(arbitrary_non_default_group) end describe "with rights and deny_rights attributes" do |