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

[Draft] POC ldap3 implementation for schannel authentication #412

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LightxR
Copy link

@LightxR LightxR commented Sep 2, 2024

POC for ldap3 implementation.

The main goal of this PR is to provide schannel authentication support to Netexec through ldap3 implementation.

Future improvements could be ldap signing and channel binding support (available on ldap3 dev branch at the moment). Great article about it here : https://offsec.almond.consulting/ldap-authentication-in-active-directory-environments.html

A workaround is possible right now to bypass channel binding with a fallback on port 389 and StartTLS, see another great article here : https://offsec.almond.consulting/bypassing-ldap-channel-binding-with-starttls.html. This code logic can be added into the actual code.

Another feature worth to consider is the pkinit function from Certipy.

@LightxR LightxR changed the title [POC ldap3 [Draft] POC ldap3 implementation Sep 2, 2024
@LightxR LightxR changed the title [Draft] POC ldap3 implementation [Draft] POC ldap3 implementation for schannel authentication Sep 2, 2024
@mpgn
Copy link
Collaborator

mpgn commented Sep 2, 2024

The problem with ldap3 is that kerberos auth is not native, you need to manually install using apt or whatever libkrb5-dev krb5-config (it's not even default on kali). If you are on linux without admin rights, you just can't use kerberos anymore now and therefore netexec with kerberos

@LightxR
Copy link
Author

LightxR commented Sep 2, 2024

I get your point but not sure to understand if it will be a problem for this PR : I only replace the impacket.ldap import and the code is still using impacket.krb5 so this should be OK.
I tested it on a fresh new debian VM without libkrb5-dev krb5-config packages and kerberos is working fine.
Let me know if I miss something

image

@mpgn
Copy link
Collaborator

mpgn commented Sep 2, 2024

I get your point but not sure to understand if it will be a problem for this PR : I only replace the impacket.ldap import and the code is still using impacket.krb5 so this should be OK. I tested it on a fresh new debian VM without libkrb5-dev krb5-config packages and kerberos is working fine. Let me know if I miss something

image

Ok, i read the code a little bit to fast then, if this is working without any additional package this is an awesome PR !

@NeffIsBack NeffIsBack marked this pull request as draft September 2, 2024 23:22
@NeffIsBack
Copy link
Contributor

As you already wrote it into the title i converted the PR into a draft :)

@NeffIsBack
Copy link
Contributor

And of course thanks for the PR!
I will take a look at it when i got the time

Copy link
Contributor

@NeffIsBack NeffIsBack left a comment

Choose a reason for hiding this comment

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

First of all thanks for the PR! This would be really cool to have!

There are many changes in the PR that seems to revert previous changes. Could it be that they were accidentally commited?
With them in here it is nearly impossible to track what have been changed (and should have an impact on the new feature) and what not.

The same applies to the patch from the impacket ldap file. Currently i don't get a diff because the whole file is added. I think it would be best if we PR&merge it into the Pennyw0rth impacket fork so we can test it with this PR. Could you open up a PR to the fork?

When both changes are done i can review the PR and give proper feedback.

Fyi if you want to use a local version (or just some other repo) for impacket you can change that in the pyproject.toml file.
This is how you can use a local repo:

[tool.poetry.dependencies]
my-package = { path = "../my/path" }

Comment on lines +252 to +256
if is_admin:
self.context.log.debug("User is admin!")
self.admin_privs = True
return True
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

Something is off here. Multiple lines are fixes introduced in the last months. Perhaps the commit containing this change reverted them.

Comment on lines +445 to +448
if is_admin:
self.admin_privs = True
return True
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same with this

@NeffIsBack NeffIsBack added the enhancement New feature or request label Nov 22, 2024
@NeffIsBack NeffIsBack added this to the v1.4.0 milestone Nov 22, 2024
@NeffIsBack
Copy link
Contributor

@LightxR if you need help with git, hit me up on discord :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants