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

LDAP Channel binding implementation #1697

Closed

Conversation

FrankSpierings
Copy link

Implemented LDAP channel binding as cleanly as I could, based on https://github.com/ly4k/ldap3.

The fact that I had to add a parameter to computeResponse/computeResponseNTLMv2/getNTLMSSPType3, is not great.

This works for GetUserSPNs.py, since it uses Impackets version of ldap.py. I have seen that addcomputer.py uses https://github.com/cannatag/ldap3. So that stills needs work.

@anadrianmanrique anadrianmanrique self-assigned this Apr 11, 2024
@anadrianmanrique anadrianmanrique added the medium Medium priority item label Apr 11, 2024
@anadrianmanrique
Copy link
Contributor

@FrankSpierings Hi, I'm reviewing your changes. So far, code seems to work as expected. I suggested some minor changes in the code. In order to approve the PR those should be applied. Thanks!

@anadrianmanrique anadrianmanrique added the waiting for response Further information is needed from people who opened the issue or pull request label Apr 24, 2024
@FrankSpierings
Copy link
Author

@FrankSpierings Hi, I'm reviewing your changes. So far, code seems to work as expected. I suggested some minor changes in the code. In order to approve the PR those should be applied. Thanks!

I might be a n00b; but I don't see your suggestions. Should these just show up in my pull request?

@@ -37,10 +37,10 @@


def computeResponse(flags, serverChallenge, clientChallenge, serverName, domain, user, password, lmhash='', nthash='',
use_ntlmv2=USE_NTLMv2):
use_ntlmv2=USE_NTLMv2, channel_binding_value=''):
Copy link
Contributor

Choose a reason for hiding this comment

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

set channel_binding_value default value as b''

@@ -594,7 +594,7 @@ def getNTLMSSPType1(workstation='', domain='', signingRequired = False, use_ntlm

return auth

def getNTLMSSPType3(type1, type2, user, password, domain, lmhash = '', nthash = '', use_ntlmv2 = USE_NTLMv2):
def getNTLMSSPType3(type1, type2, user, password, domain, lmhash = '', nthash = '', use_ntlmv2 = USE_NTLMv2, channel_binding_value = ''):
Copy link
Contributor

Choose a reason for hiding this comment

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

set channel_binding_value default value as b''

@@ -898,7 +898,7 @@ def LMOWFv2( user, password, domain, lmhash = ''):


def computeResponseNTLMv2(flags, serverChallenge, clientChallenge, serverName, domain, user, password, lmhash='',
nthash='', use_ntlmv2=USE_NTLMv2):
nthash='', use_ntlmv2=USE_NTLMv2, channel_binding_value=''):
Copy link
Contributor

Choose a reason for hiding this comment

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

set channel_binding_value default value as b''

# The following variable length AvPairs must be terminated like so
av_pairs[NTLMSSP_AV_EOL] = b''

# Format according to:
Copy link
Contributor

Choose a reason for hiding this comment

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

Good! code is now more readable (Y)

serverName + b'\x00' * 4

if channel_binding_value is None:
channel_binding_value = b''
Copy link
Contributor

Choose a reason for hiding this comment

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

remove 924,925,926, as channel_binding_value should be always be either b'' or a binary buffer

@anadrianmanrique
Copy link
Contributor

@FrankSpierings sorry, my bad, please check now

@NeffIsBack
Copy link
Contributor

Hi guys, huge fan of this PR as this would also enable NetExec to still function using NTLM, despite LDAP Signing & LDAPS channel binding being enabled (copy&pasted the code into the nxc env, poetry uses the unpatched version):
image

@FrankSpierings any updates on this?
The requested changes seems to be pretty straight forward. If you don't have the time i can quickly fix it and open up a PR so this can get merged.

@anadrianmanrique
Copy link
Contributor

continued in #1844. Notice that these changes break test_ntlm.py as computeResponseNTLMv2 changed it's internal implementation

anadrianmanrique pushed a commit that referenced this pull request Nov 25, 2024
* Implemented LDAP channel binding as cleanly as I could, based on https://github.com/ly4k/ldap3.

* Set channel binding to bytes value as requested in the review

* Fix test sessionBaseKey

* Fix test ntResponse

* Fix test encryptedSessionKey

* Fix test ntlmChallengeResponse

* Fix test ntlmChallengeResponse

* Removing leftover print statement

* Remove unnecessary AV_EOL, this is done by impackets struct anyway

---------

Co-authored-by: frank <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium Medium priority item waiting for response Further information is needed from people who opened the issue or pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants