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

Implement MS-GKDI and LAPSv2 password extraction #1556

Merged
merged 8 commits into from
Mar 20, 2024
Merged

Conversation

zblurx
Copy link
Contributor

@zblurx zblurx commented May 18, 2023

This code implements MS-GKDI.

I added a script to extract every LAPSv2 passwords.

image

There is also a fix on Endpoint Mapper on max_tower value

dru1d-foofus added a commit to dru1d-foofus/GetLAPSPassword that referenced this pull request May 20, 2023
…packet#1556 fork of impacket as it contains an implementation of MS-GKDI. Shout out to @zblurx for 99.9999% of the code I used.
* Added GetLAPSPassword to examples.

* update GetLapsPassword

* Add LAPSv2 column

---------

Co-authored-by: dru1d <[email protected]>
Co-authored-by: zblurx <[email protected]>
@zblurx
Copy link
Contributor Author

zblurx commented May 20, 2023

Thanks to @dru1d-foofus we added LAPSv1 support to the example script

@dru1d-foofus
Copy link
Contributor

Updated script output looks like this. It should be noted that LAPSv1 shows password expiration, but LAPSv2 currently does not have it implemented and therefore it will always return N/A.

image

dru1d-foofus added a commit to dru1d-foofus/GetLAPSPassword that referenced this pull request May 22, 2023
fortra/impacket#1556 fork of impacket as it
contains an implementation of MS-GKDI. Shout out to @zblurx for
99.9999% of the code I used."

This reverts commit 5c22d10 as it was
added to the fork to impacket and I want this to be available for folks
who don't have that version yet.
@zblurx
Copy link
Contributor Author

zblurx commented May 22, 2023

After some modifications:
image

The LAPSv2 column store True if the account use LAPSv2, False if not.

@anadrianmanrique anadrianmanrique added the in review This issue or pull request is being analyzed label Aug 17, 2023
@CaledoniaProject
Copy link
Contributor

@anadrianmanrique Can we get this merged by now?

@anadrianmanrique
Copy link
Contributor

@CaledoniaProject of course not. First conflicts needs to be solved in setup.py, also new dependence should be added in requirements.txt . Then it will be ready to review and then testing if review is ok. Thanks

@zblurx
Copy link
Contributor Author

zblurx commented Mar 7, 2024

Hey @anadrianmanrique, just resolved the merge conflicts and updated the requirements file, can you start reviewing the PR please ?

@anadrianmanrique
Copy link
Contributor

Hi, we've just merged readLAPS.py example. Do you think that your changes could be integrated in that example instead?

@zblurx
Copy link
Contributor Author

zblurx commented Mar 8, 2024

Sad, we created GetLAPSPasswords.py example script like 10 months ago...
This could be integrated, but I won't have the time for it right now.

@anadrianmanrique
Copy link
Contributor

Yes I understand. readLAPS.py got merged within the 3 months period where we had no visibility on how and when this PR was going to be fixed. Bad timing :/
Anyway, I could try to integrate these changes in a separate PR, and then request your review, if you are not able to do it before the 0.12 release ( jul/aug 2024 ). Let me know what you think
Thanks

@zblurx
Copy link
Contributor Author

zblurx commented Mar 12, 2024

I'll try to do it before may. Thanks

@Marshall-Hallenbeck
Copy link
Contributor

Can we merge this ASAP so NetExec can use upstream Impacket and be packaged into Kali? If there needs to be example scripts updated later that can be another PR, but we need the core functionality merged.

@gabrielg5 gabrielg5 self-assigned this Mar 18, 2024
@gabrielg5
Copy link
Collaborator

hi @zblurx,

I'll be checking your PR as is now. Will keep you updated how it goes

thank you!

@gabrielg5
Copy link
Collaborator

Today will be merging this PR

Just a few notes for us in the future @anadrianmanrique :

  • there's an issue with the --dump-laps flag in ntlmrelayx as it's not considering LAPSv2. Will create an issue to have it documented in the backlog
  • we may need to rollback readLAPS example added in GetADComputers.py and readLAPS.py #1673 as GetLAPSPassword is already solving the same use case as the other one and also handling both LAPS versions. @F-Masood do you like to give it a try to this example please?

thank you!!

@F-Masood
Copy link
Contributor

sure, ill try this script and see if its better than my one. and update here.

@dru1d-foofus
Copy link
Contributor

Yes I understand. readLAPS.py got merged within the 3 months period where we had no visibility on how and when this PR was going to be fixed. Bad timing :/ Anyway, I could try to integrate these changes in a separate PR, and then request your review, if you are not able to do it before the 0.12 release ( jul/aug 2024 ). Let me know what you think Thanks

I appreciate this finally getting looked at. It is really unfortunate that we weren't really provided any feedback between August of 2023 (when this was assigned) and December of 2023 when another user (@CaledoniaProject) asked about the status of our initial PR.

When this was initially submitted for review, we were extremely active and would've been able to address any concerns in a timely manner and prevent anyone from doing extraneous/duplicate work. All around it seems like everyone's schedules just didn't line up here. Everyone is busy and has their own stuff going on.

I believe that rolling back readLAPS.py and going with GetLAPSPassword.py makes the most sense because it is the most feature rich example:

  • It adds support for MS-GKDI.
  • It supports both LAPSv1/LAPSv2.
  • It supports saving output to a delimited file.

It really doesn't make sense to rewrite the most recent example to support all of this functionality when it already exists in this PR. I am admittedly biased here because I was a contributor, but I'll state my case anyway.

@F-Masood
Copy link
Contributor

@dru1d-foofus agree, this GetLAPSPassword.py has more features, Moderators, you can roll back my readLAPS.
no worries.

@dru1d-foofus
Copy link
Contributor

@dru1d-foofus agree, this GetLAPSPassword.py has more features, Moderators, you can roll back my readLAPS.

no worries.

If you think there are changes that can be made to improve GetLAPSPassword, I think we should take those into account. That way we can make sure it meets your needs.

@gabrielg5
Copy link
Collaborator

Awesome, merging!
Let's move forward from here

Will rollback the other example later on

Thank you all!!

@gabrielg5 gabrielg5 merged commit 7e25245 into fortra:master Mar 20, 2024
9 checks passed
gabrielg5 added a commit that referenced this pull request Jun 26, 2024
Removed `readLAPS` example as talked in #1556
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review This issue or pull request is being analyzed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants