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

Cleanup Refactor and QoL Improvements #6

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

monoxgas
Copy link

First off, thanks so much for dropping all this code! I was working on an internal project for these attacks before the release, and decided to bail on on that, bring over some of my code, and work to prep this code base for some long term support. This PR is quite large, but the majority of the changes are basic code cleanup and re-organization. I've also tried to add some bug fixes and stability improvements, but quality is in the eye of the beholder. I'll try to break down everything I went through, but there might be small things here/there that I forget about.

Code Cleanup

Most of this is relates to organizing the Interop code layer, structure definitions, breaking out component files, and cleaning up all the DllImport calls so they are more consistent. There were quite a few instances of structures being re-defined in different places under various names. I've also moved some code files around into the following rough layout:

/KrbRelay
   /Clients - [left this alone for the most part]
   /Com - Migrated the IStorage folder into here as it only relates to COM
   /Common
      /Helpers.cs - [left this alone for the most part]
      /Hooks.cs - Centralized class for hooking SSPI functions
      /State.cs - Extracted out the shared global vars here
   /Interop
      /Interop.cs - Centralized all the DllImport calls in here
      /*.cs - Remainder of the CS files are pure structs/enums in the root namespace by category

Some random specifics:

  • There is now an .editorconfig file for OmniSharp, C# in VSCode, or Visual Studio so code formatting stays more consistent.
  • With the point above, I ran a fair bit of the code against https://github.com/belav/csharpier, but there really aren't any opinionated formatters for csharp ATM, so I imagine this will degrade over time.
  • Added a .gitignore file to exclude common VS stuff
  • I ran through and expanded/cleaned a lot of the enums, trying to follow the standard of EnumThing.Value where it made sense
  • I cleaned out most of the code from Program.cs so it only focuses on parsing arguments, preparing for run tasks, validating conflicts
  • I stripped various commented code blocks, but tried to wrap any useful WriteLine statements with an #ifdef DEBUG so it still applies when debugging. This can probably be done to many more statements
  • Worked to make console output more consistent in terms of spacing/styling, but there is probably a lot of improvement still to make
  • When using NtQueryProcessInformation APIs or the like, we can simply use (IntPtr)(-1) for the current process pseudo value, this cleans up the PEB parsing code quite a bit.

SSPI Hooks / Buffers

A fair bit of my time was spent bringing over better implementations of SecurityBuffer and SecurityBufferDescriptor from my code. This includes a lot of helper functions for enumerating buffers, their types, extracting the SEC_TOKEN buffer value, and replacing it as well. This essentially means we no longer have to use byte searching code and hard-coded offsets when working with AcceptSecurityContext. I haven't dug too deep into their memory management when external buffers are supplied, so we might need to revisit some of the Dispose functions.

https://github.com/monoxgas/KrbRelay/blob/65bed1a77f1400fff00a162d39a780224e1ded6d/KrbRelay/Common/Hooks.cs#L150

In addition, I migrated the hooking to it's own SSPIHooks class, which handles installing the hooks, and uninstalling them when the class disposes. It will also not only hook in sspicli.dll!SecTableW, but also go hunting through module data sections that might have already stored the address to SSPI code. This is part of a larger effort to make the code compatible with in-memory assembly loads. Essentially removing the assumption that the code will load only once, in it's own process, and terminate that process after finishing. To the point above, I've also stripped out all Environment.Exit calls, but it's a bit unclear to me what other issues this might introduce in edge cases.

LDAP Client / Features

  • Expanded many of the Generics helpers for LDAP attacks, adding support for querying objects by different name, sid, or DN values. I also clarified the difference between Adding (Replacing), Removing, or Listing attributes on objects.
  • Added a check to make sure -ssl is passed when doing gMSA accounts, otherwise the LDAP query will fail to pull back the msDs-ManagedPassword attribute
  • Expanded the ShadowCredentials attack to support adding credentials multiple times. My current take is to have a GUID prefix (4 bytes) that gets applied to the DeviceId for any credentials we add, and those are identified and removed if we every do the same attack again. We could also strip this and have the code simple ignore any previously added credentials.
  • Added a raw hex password dump to the gMSA extraction as well, as NTLM isn't ideal for some situations
  • Cleaned up the support for passing RBCD targets as samAccountName values

This is getting quite large, but I hope I've communicated the main points. I tried to strike a balance with better organization for long term support, and keeping the general project framework in tact. I didn't touch much of the spoofing code files.

Feel free to ping specific code blocks for comment, request changes, or ask any questions. I have (as I'm sure you do as well) as large TODO list of things I'd like to look at, but I didn't want stuff to get too far from the original fork.

Update hooks system to support post-load setups
Remove explicit Environment.Exit calls
Testing + Fixing various LDAP attacks
Adding better support for updating ShadowCredentials if they already exist
@cube0x0
Copy link
Owner

cube0x0 commented Feb 17, 2022

Hey!
Thank you for your hard work!
But it get a invalid apReq using the same syntax as the main branch, I'll have a closer look later!

@monoxgas
Copy link
Author

Sounds good, it might be related to Visual Studio racing for CoInitializeSecurity before we can set the SPNs + Auth methods. I read that removing StaThread might help in this regard, so I've tried that. When you try again, check the return code from CoInitializeSecurity and see what it says.

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.

2 participants