(PUP-9312) Whitelist registry reads for Windows package providers #7266
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- 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.
Iristyle
commented
Dec 1, 2018
|
CLA signed by all contributors. |
glennsarti
force-pushed
the
PUP-9312-packages-when-registry-corrupt
branch
from
508f8db to
bd2b5f2
Compare
|
@Iristyle I've updated the PR. Due the error handling in
I also added a simple test for the I don't think this requires any test changes for the package provider code, only the registry portions. |
|
CLA signed by all contributors. |
|
@glennsarti so I was intentionally trying to avoid enumerating the keys given there's an API for retrieving by name - this ends up adding a fair amount more code. I initially looked at doing something like you ended up with, but decided on the direction in my PR. I'll take a look at your concern about |
Iristyle
commented
Dec 3, 2018
- 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.
Iristyle
force-pushed
the
PUP-9312-packages-when-registry-corrupt
branch
3 times, most recently
from
6397070 to
c3163fd
Compare
glennsarti
reviewed
Dec 3, 2018
glennsarti
reviewed
Dec 3, 2018
| vals[name] = data | ||
| 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 |
- 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.
Iristyle
force-pushed
the
PUP-9312-packages-when-registry-corrupt
branch
from
c3163fd to
65d1355
Compare
michaeltlombardi
approved these changes
Dec 4, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 key values that the package provider
is interested in, 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.
Alternate approach to #7244