Skip to content

Comments

(PUP-5640)(PUP-5591) Fix Win32 Registry Reads which cause errors#4534

Closed
glennsarti wants to merge 1 commit intopuppetlabs:stablefrom
glennsarti:ticket/stable/PUP-5591-5640-registry
Closed

(PUP-5640)(PUP-5591) Fix Win32 Registry Reads which cause errors#4534
glennsarti wants to merge 1 commit intopuppetlabs:stablefrom
glennsarti:ticket/stable/PUP-5591-5640-registry

Conversation

@glennsarti
Copy link
Contributor

@glennsarti glennsarti commented Dec 29, 2015

  • Added rescue clause around pointer read for out of bounds errors to trap
    corrupted reg values
  • Fixed REG_BINARY reads referring to a non existent local variable

glennsarti
glennsarti reviewed Dec 30, 2015
View reviewed changes
spec/unit/util/windows/registry_spec.rb Outdated
Copy link
Contributor Author

@glennsarti glennsarti Dec 30, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried using REG.EXE and Ruby API calls but they all guarded against invalid values. Eventually found I could craft a .REG file to make a bad value. Need to test on 2008 though

@ferventcoder
Copy link
Contributor

ferventcoder commented Dec 30, 2015

Does this supersede my initial PR?

@glennsarti glennsarti force-pushed the ticket/stable/PUP-5591-5640-registry branch from 315654a to 9f33bcd Compare December 30, 2015 00:33
@glennsarti
Copy link
Contributor Author

glennsarti commented Dec 30, 2015

That is the intent yes. I wanted to get it working before I commented on your PR

@glennsarti glennsarti force-pushed the ticket/stable/PUP-5591-5640-registry branch from 9f33bcd to d573631 Compare December 30, 2015 00:39
ferventcoder
ferventcoder reviewed Dec 30, 2015
View reviewed changes
lib/puppet/util/windows/registry.rb
Copy link
Contributor

@ferventcoder ferventcoder Dec 30, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it always an IndexError?

Copy link
Contributor Author

@glennsarti glennsarti Dec 30, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The underlying call is to FFI MemoryPointer get_bytes

http://www.rubydoc.info/github/ffi/ffi/FFI%2FAbstractMemory%3Aget_bytes
"(IndexError) — if length is too great"

Copy link
Contributor

@Iristyle Iristyle Dec 31, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FFI impl is https://github.com/ffi/ffi/blob/master/ext/ffi_c/AbstractMemory.c#L450-L472

Looks like IndexError is indeed the only thing that we can deal with...

@glennsarti glennsarti force-pushed the ticket/stable/PUP-5591-5640-registry branch 3 times, most recently from 03cbc45 to 0ad1798 Compare December 30, 2015 21:27
@glennsarti
Copy link
Contributor Author

glennsarti commented Dec 30, 2015

This PR supersedes #4488

@glennsarti glennsarti changed the title {WIP}(PUP-5640)(PUP-5591) Fix Win32 Registry reads (PUP-5640)(PUP-5591) Fix Win32 Registry Reads which cause errors Dec 30, 2015
@glennsarti glennsarti force-pushed the ticket/stable/PUP-5591-5640-registry branch 2 times, most recently from ef1b0d7 to 34a7766 Compare December 30, 2015 21:45
Iristyle
Iristyle reviewed Dec 31, 2015
View reviewed changes
spec/integration/util/windows/registry_spec.rb Outdated
Copy link
Contributor

@Iristyle Iristyle Dec 31, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Puppet already has temp file helpers... they track the file, and later clean up afterwards.

https://github.com/puppetlabs/puppet/blob/master/spec/lib/puppet_spec/files.rb#L27-L37

I wonder if we can do this without files and calling out to regedit.exe though

Copy link
Contributor Author

@glennsarti glennsarti Jan 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will look at using the helper class instead of the current method.

I tried creating invalid values using API calls, WMI and REG.EXE and they all protected me from making a bad call. REGEDIT.EXE was the only method that I could find that would allow me to inject a bad DWORD. I do not believe regedit will accept STDIN so the only method left was a crafted temporary reg file to import.

@ferventcoder ferventcoder mentioned this pull request Dec 31, 2015
Closed
@ferventcoder
Copy link
Contributor

ferventcoder commented Dec 31, 2015

Some of the original commit message from #4488 should probably make it into this commit message:

Previously, the read method was using an older variable name when
reading data for binary values. It would result in errors such as the
following when reading a binary value from the registry.

Error: Could not run: undefined local variable or method `data' for
Puppet::Provider::Package::Windows::Package:Class
C:/Program Files/Puppet
Labs/Puppet/puppet/lib/puppet/util/windows/registry.rb:216:in `block in
read'

This was introduced in b46ede7 (Puppet v4.0.0).

Without this fix, any registry enumerations that contain a binary
value will fail.

@glennsarti glennsarti force-pushed the ticket/stable/PUP-5591-5640-registry branch 6 times, most recently from e02cf6f to 47384ca Compare January 13, 2016 01:28
@glennsarti
Copy link
Contributor Author

glennsarti commented Jan 13, 2016

@Iristyle Bad data injection has been changed from REG.EXE to using raw Win32 API

@glennsarti
(PUP-5640)(PUP-5591) Fix Win32 Registry Reads which cause errors
47384ca 
Without this patch applied, reading REG_BINARY or corrupted registry
values would cause fatal errors to be raised and the error messages
did not contain enough information for users to remediate.

This commit fixes a typo in the REG_BINARY logic which was referencing
a now non-existent local variable.  Also, the read function
now has a wrapping begin-rescue clause to catch buffer overrun error
messages caused by corrupted registry values e.g. a DWORD having less
than 4 bytes of data.  When a corrupted registry key is found, a warning
is raised stating the key name, not value name, where the value is
located and returns nil for the data of the value.

The tests for this class have been moved from unit to integration as
they were not true unit tests and required actual read/write operations
on the Windows registry.  Tests for the REG_BINARY and corrupted registry
values scenarios were added.
@Iristyle Iristyle closed this in d125c70 Jan 13, 2016
ferventcoder referenced this pull request Jan 13, 2016
@glennsarti @Iristyle
(PUP-5640)(PUP-5591) Fix Win32 Registry Reads
649df14 
Without this patch applied, reading REG_BINARY or corrupted registry
values would cause fatal errors to be raised and the error messages
did not contain enough information for users to remediate.

This commit fixes a typo in the REG_BINARY logic which was referencing
a now non-existent local variable.  Also, the read function
now has a wrapping begin-rescue clause to catch buffer overrun error
messages caused by corrupted registry values e.g. a DWORD having less
than 4 bytes of data.  When a corrupted registry key is found, a
warning is raised stating the key name, not value name, where the value
is located and returns nil for the data of the value.

The tests for this class have been moved from unit to integration as
they were not true unit tests and required actual read/write operations
on the Windows registry.  Tests for the REG_BINARY and corrupted
registry values scenarios were added.
@glennsarti glennsarti deleted the ticket/stable/PUP-5591-5640-registry branch February 11, 2016 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@glennsarti @ferventcoder @Iristyle