Incorporate Pennyw0rth/NetExec Impacket changes into Fortra Impacket (reloaded)

[mpgn]

Fixing all the issues with PR #1715

NetExec is attempting to get added to Kali Linux as a default package, but we currently have several changes in our Impacket fork that are required to be merged into upstream.

The GKDI stuff has been used for almost a year now with no problems, and @XiaoliChan recently had some other changes merged related to NetExec changes. I just filed #1714 as well to revert some breaking changes that we require.

This is necessary because we do not want to have another impacket Kali package deployed, as that causes confusion, and we're not even sure if Kali would want that. The easiest way is to have our changes merged into upstream.

Please let me know if there are any issues testing these changes.

[NeffIsBack]

Cleanly cherry-picked the commits that matter, nice work👌🏼
If these are merged we should be ready to go for kali packaging.

[Marshall-Hallenbeck]

@mpgn can you remove f90196b from this PR? @XiaoliChan and I spoke and we think it should be removed since it's hardcoding a timeout and using a global variable. He has let me know it won't break anything (we can test, of course).

[mpgn]

set_connect_timeout is used a lot on nxc with a custom timeout, example: https://github.com/Pennyw0rth/NetExec/blob/main/nxc/connection.py#L71

Removing the commit will break nxc from what I see

[XiaoliChan]

set_connect_timeout is used a lot on nxc with a custom timeout, example: https://github.com/Pennyw0rth/NetExec/blob/main/nxc/connection.py#L71

Removing the commit will break nxc from what I see

Oh, that is rpctransport, the set_connect_timeout I added is like dcom.set_connect_timeout, not rpctransport

I can confirm it didn’t used in NXC currently (because it is not pretty much stable as dcom_firewallChecker)

[mpgn]

Should be good @Marshall-Hallenbeck :)

[anadrianmanrique]

Hello, I'll be reviewing/testing this PR changes.
Some comments/questions:

• srvs.py / smbconnection.py a.k.a Add ServerName argument to srvs.hNetrShareEnum #947 : do you guys have any idea how to reproduce the issue these changes are actually fixing?
• ntlm.py : following this up in Fix LM hash computation #1626. These changes are not going to be merged

[mpgn]

Hello @anadrianmanrique if you fix the problem of LM computing hash in another PR, i will remove the commit on this PR 😉

[anadrianmanrique]

Hello @anadrianmanrique if you fix the problem of LM computing hash in another PR, i will remove the commit on this PR 😉

created at #1723

[Marshall-Hallenbeck]

• srvs.py / smbconnection.py a.k.a Add ServerName argument to srvs.hNetrShareEnum #947 : do you guys have any idea how to reproduce the issue these changes are actually fixing?

So from my understanding of the spec, impacket defaults to NULL, which is just the localhost, but we need to define a DNS/NetBios name per: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-srvs/5f8329ee-1965-4ea1-ad35-3b29fbb63232

I'm surprised this hasn't caused more issues, actually, since I would think it would only allow connecting to the localhost.

[anadrianmanrique]

• srvs.py / smbconnection.py a.k.a Add ServerName argument to srvs.hNetrShareEnum #947 : do you guys have any idea how to reproduce the issue these changes are actually fixing?

So from my understanding of the spec, impacket defaults to NULL, which is just the localhost, but we need to define a DNS/NetBios name per: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-srvs/5f8329ee-1965-4ea1-ad35-3b29fbb63232

I'm surprised this hasn't caused more issues, actually, since I would think it would only allow connecting to the localhost.

Ok, what I'm trying to do is to replicate and trigger the bug condition. I haven't been able to do it yet. That's why I asked for any tip/help with that. In the worst case I'll merge that after sucessfully regresioning the changes

[Marshall-Hallenbeck]

Hmm yeah sorry I don't have a specific retestable scenario. @NeffIsBack did you?

[anadrianmanrique]

Hello @anadrianmanrique if you fix the problem of LM computing hash in another PR, i will remove the commit on this PR 😉

wether the problem gets fixed or not, this PR will not be approved/merged, as long as it contains changes from #1626, so please remove that commit. In this moment #1723 it's being tested and problably merged today/tomorrow ;)

[mpgn]

Hello @anadrianmanrique if you fix the problem of LM computing hash in another PR, i will remove the commit on this PR 😉

wether the problem gets fixed or not, this PR will not be approved/merged, as long as it contains changes from #1626, so please remove that commit. In this moment #1723 it's being tested and problably merged today/tomorrow ;)

I will do it tonight 👍

[mpgn]

done @anadrianmanrique :)

[anadrianmanrique]

Ok, thanks, PR is ready to merge then!

[anadrianmanrique]

Merged changes from #947 here, as the original PR needs to be rebased.

[anadrianmanrique]

Thank you guys! Let us now in comments if you find in the future PRs that you'll need integrated, in order to allow us to improve our prioritization.

[mpgn]

Thank you @anadrianmanrique !

[anadrianmanrique]

FYI #1723 was merged as well