Skip to content

Fix fake user addition to the config because of Mimikatz #1902

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 20, 2022
Merged

Fix fake user addition to the config because of Mimikatz #1902

merged 4 commits into from
Apr 20, 2022

Conversation

shreyamalviya
Copy link
Contributor

What does this PR do?

Fixes #1860

PR Checklist

  • Have you added an explanation of what your changes do and why you'd like to include them?
  • Is the TravisCI build passing?
  • Was the CHANGELOG.md updated to reflect the changes?
  • Was the documentation framework updated to reflect the changes?
  • Have you checked that you haven't introduced any duplicate code?

Testing Checklist

  • Added relevant unit tests?
  • Have you successfully tested your changes locally? Elaborate:

    Tested by running on Windows

  • If applicable, add screenshots or log transcripts of the feature working

@codecov
Copy link

codecov bot commented Apr 20, 2022 •
edited
Loading

Codecov Report

Merging #1902 (9f78e0d) into develop (e91087f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 9f78e0d differs from pull request most recent head a335f30. Consider uploading reports for the commit a335f30 to get more accurate results

@@           Coverage Diff            @@
##           develop    #1902   +/-   ##
========================================
  Coverage    54.93%   54.94%           
========================================
  Files          446      447    +1     
  Lines        12624    12626    +2     
========================================
+ Hits          6935     6937    +2     
  Misses        5689     5689
Impacted Files Coverage Δ
monkey/infection_monkey/consts.py 100.00% <100.00%> (ø)
...imikatz_collector/mimikatz_credential_collector.py 100.00% <100.00%> (ø)
...ost_breach/actions/communicate_as_backdoor_user.py 63.46% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e91087f...a335f30. Read the comment docs.

VakarisZ
VakarisZ reviewed Apr 20, 2022
View reviewed changes
monkey/infection_monkey/consts.py Outdated
@@ -0,0 +1 @@
USERNAME_PREFIX = "somenewuser"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's worth creating a new file just for this. I suppose mimikatz credential collector should just import from PBA, since that's the source of users we don't want to add.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't couple the mimikatz collector to the PBAs!

VakarisZ
VakarisZ reviewed Apr 20, 2022
View reviewed changes
...y/infection_monkey/credential_collectors/mimikatz_collector/mimikatz_credential_collector.py

# Mimikatz picks up users created by the Monkey even if they're successfully deleted
# since it picks up creds from the registry. The newly created users are not removed
# from the registry until a reboot of the system, hence this check.
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you checked? We need to make sure these users aren't left there forever

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, checked

VakarisZ
VakarisZ requested changes Apr 20, 2022
View reviewed changes
Copy link
Contributor

@VakarisZ VakarisZ left a comment

Choose a reason for hiding this comment

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

Small changes/tests

mssalvatore
mssalvatore requested changes Apr 20, 2022
View reviewed changes
Copy link
Collaborator

@mssalvatore mssalvatore left a comment

Choose a reason for hiding this comment

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

I'm concerned about the coupling this introduces between the PBAs and CredentialsCollectors. We may not be able to properly solve this issue without some more infrastructure in place. Ideally, both the PBA and the MimikatzCollector would be configurable so that the user can configure which users MimikatzCollector ignores.

At this time, I don't know that this issue is a significant enough nuisance to warrant coupling the PBAs and the CredentialsCollectors.

@VakarisZ
Agent: Move username const to model
a335f30 
This const is used by PBA and mimikatz collectors as describes the username prefix for users created by IM
@mssalvatore mssalvatore merged commit 526448c into develop Apr 20, 2022
@SarmadBytes SarmadBytes mentioned this pull request May 16, 2022
Open
@shreyamalviya shreyamalviya deleted the 1860-fake-users-mimikatz branch June 2, 2022 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mssalvatore mssalvatore mssalvatore requested changes

@VakarisZ VakarisZ VakarisZ approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fake users get picked up by mimikatz
3 participants
@shreyamalviya @mssalvatore @VakarisZ