From 2b16db5eb7b10aced53756686747f2cd2c94ec3d Mon Sep 17 00:00:00 2001 From: "Ethan J. Brown" Date: Fri, 30 Nov 2018 17:13:01 -0800 Subject: [PATCH 1/3] (PUP-9312) Fix errant registry read message - When failing to retrieve a registry value, the API called to read the pointer of the name of the subkey was used improperly, resulting in a messsage like: Error: Could not run: wrong number of arguments (given 0, expected 1..3) C:/source/puppet-PUP-9312/lib/puppet/util/windows/api_types.rb:56:in `read_wide_string' Fix this, so the message is correctly emitted instead like: Error: Could not run: Failed to read registry value QuietDisplayName at Software\Microsoft\Windows\CurrentVersion\Uninstall\7-Zip: The operation completed successfully. --- lib/puppet/util/windows/registry.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/puppet/util/windows/registry.rb b/lib/puppet/util/windows/registry.rb index 0d689cebe69..812785caa43 100644 --- a/lib/puppet/util/windows/registry.rb +++ b/lib/puppet/util/windows/registry.rb @@ -247,7 +247,9 @@ def query_value_ex(key, name_ptr, &block) buffer_ptr, length_ptr) if result != FFI::ERROR_SUCCESS - msg = _("Failed to read registry value %{value} at %{key}") % { value: name_ptr.read_wide_string, key: key.keyname } + # buffer is raw bytes, *not* chars - less a NULL terminator + name_length = (name_ptr.size / FFI.type_size(:wchar)) - 1 if name_ptr.size > 0 + msg = _("Failed to read registry value %{value} at %{key}") % { value: name_ptr.read_wide_string(name_length), key: key.keyname } raise Puppet::Util::Windows::Error.new(msg) end From 084d783a4b26c423c2ab3ea9a9774e9ab7de2210 Mon Sep 17 00:00:00 2001 From: "Ethan J. Brown" Date: Mon, 3 Dec 2018 14:05:49 -0800 Subject: [PATCH 2/3] (PUP-9312) Fix Windows Registry error handling - Previous error handling for Win32 registry API calls were, in some cases, relying on FFI to call GetLastError behind the scenes when raising a new Puppet::Util::Windows::Error. However, Registry API calls do not use GetLastError and instead return error codes directly from their API calls. " If the function fails, the return value is a nonzero error code defined in Winerror.h. You can use the FormatMessage function with the FORMAT_MESSAGE_FROM_SYSTEM flag to get a generic description of the error. " The above is what Puppet::Util::Windows::Error does when given an explicit error code. - This fixes multiple problems with being able to inspect actual error codes from raised exceptions. In particular, it's helpful to know when RegQueryValueExW has returned a value of "2" which is ERROR_FILE_NOT_FOUND, meaning a registry value does not exist with the given name. This is important for the next commit. --- lib/puppet/util/windows/registry.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/puppet/util/windows/registry.rb b/lib/puppet/util/windows/registry.rb index 812785caa43..51a1b02533a 100644 --- a/lib/puppet/util/windows/registry.rb +++ b/lib/puppet/util/windows/registry.rb @@ -104,7 +104,7 @@ def reg_enum_key(key, index, max_key_length = Win32::Registry::Constants::MAX_KE if result != FFI::ERROR_SUCCESS msg = _("Failed to enumerate %{key} registry keys at index %{index}") % { key: key.keyname, index: index } - raise Puppet::Util::Windows::Error.new(msg) + raise Puppet::Util::Windows::Error.new(msg, result) end filetime = FFI::WIN32::FILETIME.new(filetime_ptr) @@ -135,7 +135,7 @@ def reg_enum_value(key, index, max_value_length = Win32::Registry::Constants::MA if result != FFI::ERROR_SUCCESS msg = _("Failed to enumerate %{key} registry values at index %{index}") % { key: key.keyname, index: index } - raise Puppet::Util::Windows::Error.new(msg) + raise Puppet::Util::Windows::Error.new(msg, result) end subkey_length = subkey_length_ptr.read_dword @@ -165,7 +165,7 @@ def reg_query_info_key_max_lengths(key) if status != FFI::ERROR_SUCCESS msg = _("Failed to query registry %{key} for sizes") % { key: key.keyname } - raise Puppet::Util::Windows::Error.new(msg) + raise Puppet::Util::Windows::Error.new(msg, status) end result = [ @@ -250,7 +250,7 @@ def query_value_ex(key, name_ptr, &block) # buffer is raw bytes, *not* chars - less a NULL terminator name_length = (name_ptr.size / FFI.type_size(:wchar)) - 1 if name_ptr.size > 0 msg = _("Failed to read registry value %{value} at %{key}") % { value: name_ptr.read_wide_string(name_length), key: key.keyname } - raise Puppet::Util::Windows::Error.new(msg) + raise Puppet::Util::Windows::Error.new(msg, result) end # allows caller to use FFI MemoryPointer helpers to read / shape From 65d135544e98e07bd29667904dc0673f8415213a Mon Sep 17 00:00:00 2001 From: "Ethan J. Brown" Date: Fri, 30 Nov 2018 17:19:55 -0800 Subject: [PATCH 3/3] (PUP-9312) Explicitly load Windows package reg vals - Previously, the Windows package provider would enumerate all registry values affiliated with a given product. Products are enumerated under HKLM\SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall Where each individual product resides in a subkey, like 7-Zip This could result in reading quite a bit of extraneous data. - In reality, there are only 9 total key values that the package provider is interested in for either MSI or EXE packages, so whitelist those values when enumerating for a given product. - This also fixes an issue where some products may install bad data into registry values (one such product stores binary data inside of a REG_SZ which breaks Puppets abiliy to interpret the value as a string). While this is really the fault of the product and not of Puppet, Puppet must be resilient to such bad data. --- .../provider/package/windows/exe_package.rb | 13 +++++++ .../provider/package/windows/msi_package.rb | 8 ++++ .../provider/package/windows/package.rb | 10 ++++- lib/puppet/util/windows/registry.rb | 22 +++++++++++ .../integration/util/windows/registry_spec.rb | 39 +++++++++++++++++++ 5 files changed, 91 insertions(+), 1 deletion(-) diff --git a/lib/puppet/provider/package/windows/exe_package.rb b/lib/puppet/provider/package/windows/exe_package.rb index 9adf5b732e3..dd0942c0d92 100644 --- a/lib/puppet/provider/package/windows/exe_package.rb +++ b/lib/puppet/provider/package/windows/exe_package.rb @@ -4,6 +4,19 @@ class Puppet::Provider::Package::Windows class ExePackage < Puppet::Provider::Package::Windows::Package attr_reader :uninstall_string + # registry values to load under each product entry in + # HKLM\SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall + # for this provider + REG_VALUE_NAMES = [ + 'DisplayVersion', + 'UninstallString', + 'ParentKeyName', + 'Security Update', + 'Update Rollup', + 'Hotfix', + 'WindowsInstaller', + ] + # Return an instance of the package from the registry, or nil def self.from_registry(name, values) if valid?(name, values) diff --git a/lib/puppet/provider/package/windows/msi_package.rb b/lib/puppet/provider/package/windows/msi_package.rb index 8de58ac3348..78eed6c7bcc 100644 --- a/lib/puppet/provider/package/windows/msi_package.rb +++ b/lib/puppet/provider/package/windows/msi_package.rb @@ -8,6 +8,14 @@ class MsiPackage < Puppet::Provider::Package::Windows::Package INSTALLSTATE_DEFAULT = 5 # product is installed for the current user INSTALLUILEVEL_NONE = 2 # completely silent installation + # registry values to load under each product entry in + # HKLM\SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall + # for this provider + REG_VALUE_NAMES = [ + 'DisplayVersion', + 'WindowsInstaller' + ] + # Get the COM installer object, it's in a separate method for testing def self.installer # REMIND: when does the COM release happen? diff --git a/lib/puppet/provider/package/windows/package.rb b/lib/puppet/provider/package/windows/package.rb index 6dc9113411e..efccc369830 100644 --- a/lib/puppet/provider/package/windows/package.rb +++ b/lib/puppet/provider/package/windows/package.rb @@ -11,6 +11,14 @@ class Package attr_reader :name, :version + REG_DISPLAY_VALUE_NAMES = [ 'DisplayName', 'QuietDisplayName' ] + + def self.reg_value_names_to_load + REG_DISPLAY_VALUE_NAMES | + MsiPackage::REG_VALUE_NAMES | + ExePackage::REG_VALUE_NAMES + end + # Enumerate each package. The appropriate package subclass # will be yielded. def self.each(&block) @@ -37,7 +45,7 @@ def self.with_key(&block) open(hive, 'Software\Microsoft\Windows\CurrentVersion\Uninstall', mode) do |uninstall| each_key(uninstall) do |name, wtime| open(hive, "#{uninstall.keyname}\\#{name}", mode) do |key| - yield key, values(key) + yield key, values_by_name(key, reg_value_names_to_load) end end end diff --git a/lib/puppet/util/windows/registry.rb b/lib/puppet/util/windows/registry.rb index 51a1b02533a..0f8952988b6 100644 --- a/lib/puppet/util/windows/registry.rb +++ b/lib/puppet/util/windows/registry.rb @@ -65,6 +65,28 @@ def values(key) vals end + # Retrieve a set of values from a registry key given their names + # Value names listed but not found in the registry will not be added to the + # resultant Hashtable + # + # @param key [RegistryKey] An open handle to a Registry Key + # @param names [String[]] An array of names of registry values to return if they exist + # @return [Hashtable] A hashtable of all of the found values in the registry key + def values_by_name(key, names) + vals = {} + names.each do |name| + FFI::Pointer.from_string_to_wide_string(name) do |subkeyname_ptr| + begin + _, vals[name] = read(key, subkeyname_ptr) + rescue Puppet::Util::Windows::Error => e + # ignore missing names, but raise other errors + raise e unless e.code == Puppet::Util::Windows::Error::ERROR_FILE_NOT_FOUND + end + end + end + vals + end + def each_value(key, &block) index = 0 subkey = nil diff --git a/spec/integration/util/windows/registry_spec.rb b/spec/integration/util/windows/registry_spec.rb index 43e6dc57d9c..65e7a1fd20d 100644 --- a/spec/integration/util/windows/registry_spec.rb +++ b/spec/integration/util/windows/registry_spec.rb @@ -253,5 +253,44 @@ def expects_registry_value(array) end end end + + context "#values_by_name" do + let(:hkey) { stub 'hklm' } + let(:subkey) { stub 'subkey' } + + before :each do + subject.stubs(:root).returns(hkey) + end + + context "when reading values" do + let (:hklm) { Win32::Registry::HKEY_LOCAL_MACHINE } + let (:puppet_key) { "SOFTWARE\\Puppet Labs"} + let (:subkey_name) { "PuppetRegistryTest#{SecureRandom.uuid}" } + + before(:each) do + hklm.create("#{puppet_key}\\#{subkey_name}", Win32::Registry::KEY_ALL_ACCESS) do |reg| + reg.write('valuename1', Win32::Registry::REG_SZ, 'value1') + reg.write('valuename2', Win32::Registry::REG_SZ, 'value2') + end + end + + after(:each) do + hklm.open(puppet_key, Win32::Registry::KEY_ALL_ACCESS) do |reg| + subject.delete_key(reg, subkey_name) + end + end + + it "should return only the values for the names specified" do + hklm.open("#{puppet_key}\\#{subkey_name}", Win32::Registry::KEY_ALL_ACCESS) do |reg_key| + vals = subject.values_by_name(reg_key, ['valuename1', 'missingname']) + + expect(vals).to have_key('valuename1') + expect(vals).to_not have_key('valuename2') + expect(vals['valuename1']).to eq('value1') + expect(vals['missingname']).to be_nil + end + end + end + end end end