-
Notifications
You must be signed in to change notification settings - Fork 54
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
Improve DPAPI plugin #711
Improve DPAPI plugin #711
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments. I will need to take a deeper dive into the key provider thing at a later stage, as I do think that can be done nicer.
dissect/target/plugins/os/windows/dpapi/keyprovider/credhist.py
Outdated
Show resolved
Hide resolved
dissect/target/plugins/os/windows/dpapi/keyprovider/keychain.py
Outdated
Show resolved
Hide resolved
* separate LSA logic from DPAPI in separate plugin * move SAM and CREDHIST plugins to plugins.os.windows.credential * add Windows XP support to DPAPI plugin (RC4 and DES3) * fix bug in _SHA256 identifier (0x800C instead of 0x8004) * add providers for DPAPI masterkey passphrases
* Implement review feedback * Improved test coverage for DPAPI (XP, vista, 7, 10) * Added several helper functions for DPAPI tests * LSA plugin now returns records * Fixed DES3 DPAPI for WinXP * Small rewrite of DPAPIPlugin master key discovery logic * Added DefaultPassword LSA secret as DPAPI keyprovider
9e70b0b
to
fde58ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need to have a think about the key provider plugins, but you can have some fun with these comments already 😉.
Co-authored-by: Erik Schamper <[email protected]>
Co-authored-by: Erik Schamper <[email protected]>
Is this PR good to go? :) |
I'm still a little bit torn on the keyprovider thing. I don't think it's reasonable to implement a proper solution in this PR, so I'm willing to have a temporary one instead, but I'd at least want to make the For reference, the idea I was leaning towards the most as a proper solution to this was nested namespaces, but that will take a smidge more work 😅. I'm a bit swamped the coming days, so if you're not willing to wait on my solution to the internal namespace stuff, feel free to have a go at it. Another idea I had was a fancy "target keychain" that you can plug "password/key material providers" into. For example, also dump password databases into it. But I'm not sure if there's actually a real use case for that outside of this specific one. So I don't think that's a good way to go. |
I've made a few changes, let me know if those work for you @JSCU-CNI. Unfortunately |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #711 +/- ##
==========================================
+ Coverage 75.52% 75.59% +0.06%
==========================================
Files 305 311 +6
Lines 26363 26540 +177
==========================================
+ Hits 19911 20062 +151
- Misses 6452 6478 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Co-authored-by: Erik Schamper <[email protected]>
That looks like a neat solution, thanks. I guess we can update the namespaces to
Is that blocking for now? I can see that |
Yes.
No, not blocking. I'd prefer #763 instead of trying to fix this. There were some other things broken as well, but it doesn't really matter 😄. |
That should fix the tests. |
This PR improves several DPAPI related features:
dissect.target.plugins.os.windows.credential
_SHA256
identifier (0x800C
instead of0x8004
)The latter "dpapi provider" feature is experimental and we are keen to discuss a better
InternalNamespacePlugin
implementation.