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

Add windows credhist plugin #566

Merged
merged 14 commits into from
Apr 19, 2024
Merged

Conversation

JSCU-CNI
Copy link
Contributor

@JSCU-CNI JSCU-CNI commented Mar 5, 2024

This PR adds support for decrypting CREDHIST entries on Windows targets.

Currently supports decrypting full and partial CREDHIST chains and can tell you if the decryption worked or not. Does not (yet) provide decrypted SHA1 hashes to the DPAPI plugin. Perhaps @cobyge can tell us where it's best to implement this.

@Horofic Horofic self-assigned this Mar 10, 2024
@cobyge
Copy link
Contributor

cobyge commented Mar 14, 2024

RE: Passing the output to DPAPI,
There are a few options I can think of:

  1. I could add support to the DPAPI plugin to read from the "keychain"
  2. I could try to call the CREDHIST plugin from the DPAPI plugin.

I think I'd prefer the first option, as it is a nice way to interface credentials between plugins, but I'd like to hear one of the maintainers thoughts about this first.

@Schamper
Copy link
Member

@cobyge option 1 is at least preferred for providing the "initial"/current password, as is currently in review in #541. While it would be nice, I don't immediately see a nice way how CREDHIST could provide passwords to the keychain (it would need to be invoked somehow). You could make something similar to how we have "child plugins", i.e. "keychain plugins" and bind a keychain to a target (instead of being global right now). But that feels a bit overkill, at least for the moment where we would only have a single plugin for it.

I'm more than happy to hear some ideas for this, but for complexity sake, option 2 might otherwise be preferred.

@JSCU-CNI JSCU-CNI requested a review from Schamper April 8, 2024 10:40
Copy link
Member

@Schamper Schamper left a comment

Choose a reason for hiding this comment

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

Can you also rebase onto main.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 97.08738% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 73.37%. Comparing base (2c14aa5) to head (c45be8b).

Files Patch % Lines
dissect/target/plugins/os/windows/credhist.py 97.08% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #566      +/-   ##
==========================================
- Coverage   74.99%   73.37%   -1.63%     
==========================================
  Files         288      289       +1     
  Lines       24192    24295     +103     
==========================================
- Hits        18143    17826     -317     
- Misses       6049     6469     +420     
Flag Coverage Δ
unittests 73.37% <97.08%> (-1.63%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Horofic Horofic removed their assignment Apr 17, 2024
@JSCU-CNI JSCU-CNI requested a review from Schamper April 18, 2024 15:37
@Schamper Schamper merged commit 34c4aeb into fox-it:main Apr 19, 2024
16 checks passed
@JSCU-CNI JSCU-CNI deleted the feature/add-credhist-plugin branch April 24, 2024 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants