Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

winregistry.py: handle value name containting backslash character #1767

Merged
merged 6 commits into from
Aug 19, 2024

Conversation

DidierA
Copy link
Contributor

@DidierA DidierA commented Jun 25, 2024

winregistry.Registry.getValue() splits keyValue on backslash to separate key and value. But value names can contain backslash, and calling getValue on such a value will cause a ValueError exception.

This commit corrects this bug while keeping compatibility with old code.

@anadrianmanrique anadrianmanrique added the bug Unexpected problem or unintended behavior label Jun 27, 2024
@gabrielg5 gabrielg5 self-assigned this Jul 29, 2024
Copy link
Collaborator

@gabrielg5 gabrielg5 left a comment

Choose a reason for hiding this comment

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

Hey @DidierA, thanks for your changes!

I've been reviewing them and posted some suggestions. Think they are cleaner this way according to our current use cases.

examples/registry-read.py Outdated Show resolved Hide resolved
impacket/winregistry.py Outdated Show resolved Hide resolved
impacket/winregistry.py Outdated Show resolved Hide resolved
impacket/winregistry.py Outdated Show resolved Hide resolved
impacket/winregistry.py Outdated Show resolved Hide resolved
@gabrielg5 gabrielg5 added the waiting for response Further information is needed from people who opened the issue or pull request label Jul 30, 2024
@gabrielg5
Copy link
Collaborator

Applied suggestions to move forward with the bug resolution

@anadrianmanrique anadrianmanrique self-requested a review August 16, 2024 14:56
@anadrianmanrique anadrianmanrique merged commit f98c987 into fortra:master Aug 19, 2024
8 checks passed
@DidierA
Copy link
Contributor Author

DidierA commented Aug 20, 2024

Hi, Sorry for the late reply, I was on leave. The idea behind accepting a REG_NK was to allow multiple calls on the same key to reuse the result of findKey(), such as when you dump an entire tree. However I don't have a problem with the reviewed changes.

gabrielg5 added a commit that referenced this pull request Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unexpected problem or unintended behavior waiting for response Further information is needed from people who opened the issue or pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants