LDAP Channel binding implementation from #1697

[NeffIsBack]

Hi folks,
following up on #1697 (comment), this is a fixed version to finish up the review of @anadrianmanrique. No changes besides the one requested, so this can get merged as soon as possible 🚀

Tested on the NetExec side of things:

[NeffIsBack]

I updated the ntlmv2 values sessionBaseKey, ntResponse and encryptedSessionKey , because this changed due to the additional "temp" variable from the channel binding:

impacket/impacket/ntlm.py

Lines 937 to 941 in 835e175

	ntProofStr = hmac_md5(responseKeyNT, serverChallenge + temp)
	ntChallengeResponse = ntProofStr + temp
	lmChallengeResponse = hmac_md5(responseKeyNT, serverChallenge + clientChallenge) + clientChallenge
	sessionBaseKey = hmac_md5(responseKeyNT, ntProofStr)

@anadrianmanrique could you help me put together the final ntlmChallengeResponse?

Edit:
Managed to fix it :)

[mpgn]

Hello @anadrianmanrique hope you are doing well :)

Can you review this one ? this is fixing a big problem on nxc 🎉

[anadrianmanrique]

Hello guys, first, thank you for taking care of #1697, hope we will finally integrate it :)/
Second, I'm currently on vacactions, so access to the repo ( and internet as well ) it's a bit limited for me, until my return on next Monday.
Third, I see some changes in ntlm unit tests. As far as I understand ntlm.computeResponseNTLMv2 shouldn't change behavior ( as it would break clients not using channel binding ), so I don't understand why this has to be updated. ntlm.computeResponseNTLMv2 calculation changes only if channelbinding data it's being passed to the function ( sorry I'm not being able to test the code at the moment ). If you can clarify this to me, it would be great. Thank you!

[NeffIsBack]

Enjoy the vacation!

That is a good question. As far as my tests go the current implementation should also work for Channel binding not being active, but I will test that again.

I assumed that the new variable introduced bytes that are ignored by non channel binding connections, but I will dig into the Microsoft documentation to find out what is happening here!

[NeffIsBack]

So apparently NTLMv2 still works, even when channel binding is completely disabled:

Going on the hunt now why the response calculcation changes.

[NeffIsBack]

So, according to the microsoft documentation for the NTLMv2_CLIENT_CHALLENGE the object has the AvPairs variable as last parameter. These AvPairs (attribute-value) are terminated by MsvAvEOL which is effectively just 4 Null bytes. Before this PR this was done by:

1. The AvPair Struct:
   

   impacket/impacket/ntlm.py

   Lines 248 to 257 in 835e175

   	def getData(self):
   	if NTLMSSP_AV_EOL in self.fields:
   	del self.fields[NTLMSSP_AV_EOL]
   	ans = b''
   	for i in list(self.fields.keys()):
   	ans+= struct.pack('<HH', i, self[i][0])
   	ans+= self[i][1]
   	# end with a NTLMSSP_AV_EOL
   	ans += struct.pack('<HH', NTLMSSP_AV_EOL, 0)

2. Manually adding 4 null bytes to the end:
   

   impacket/impacket/ntlm.py

   Lines 934 to 935 in 835e175

   	temp = responseServerVersion + hiResponseServerVersion + b'\x00' * 6 + aTime + clientChallenge + b'\x00' * 4 + \
   	serverName + b'\x00' * 4

Effectively resulting in 8 null bytes:

I would assume that on the Microsoft side of things the unnecessary 4 null bytes just got thrown away.

With this PR we just add the AvPair to the, which does the termination of the object with MsvAvEOL (4 null bytes) by itself. As this seems to work against servers with and without channel binding enabled i would recommend to go with the new method (which looks to match the microsoft spec as far as i can tell).
As we remove 4 null bytes the tests obviously don't pass anymore and have to get updated.

[NeffIsBack]

Example of the 8 null bytes in the NetExec connection:

[anadrianmanrique]

@NeffIsBack I confirm what you said. I'll run some remote testcases regarding ntlm. If that's ok, the PR will be ready
to be merged. Thanks!

[NeffIsBack]

Awesome! Really looking forward to this being merged🚀

[mpgn]

Thanks @anadrianmanrique ! 🔥

[anadrianmanrique]

Merging, Thank you all!!