the-useless-one / pywerview

A (partial) Python rewriting of PowerSploit's PowerView
GNU General Public License v3.0
908 stars 113 forks source link

Migration to python3 #34

Closed mpgn closed 3 years ago

mpgn commented 5 years ago

Hello,

I migrate the project to python3, I didn't have tested all the functions so I set this Pull Request as Draft for now until i'm sure everything is migrated to python3.

byt3bl33d3r commented 5 years ago

Also @the-useless-one , could we get Pywerview published to Pypi? I'll gladly help if needed :)

Thanks !

ThePirateWhoSmellsOfSunflowers commented 4 years ago

I think you forgot to migrate encode('hex') ;) btw, nice PR

Traceback (most recent call last):
  File "pywerview.py", line 24, in <module>
    main()
  File "/xxx/pywerview/pywerview/cli/main.py", line 466, in main
    x = str(x)
  File "/xxx/pywerview/pywerview/objects/adobjects.py", line 130, in __str__
    member_value = '{}...'.format(member[1].encode('hex')[:100])
LookupError: 'hex' is not a text encoding; use codecs.encode() to handle arbitrary codecs

:sunflower:

mpgn commented 4 years ago

@ThePirateWhoSmellsOfSunflowers can you check again ?

ThePirateWhoSmellsOfSunflowers commented 4 years ago

Better but now you forgot a coma :smile:

  File "/xxx/pywerview/pywerview/objects/adobjects.py", line 130
    member_value = '{}...'.format(codecs.encode(member[1]'hex')[:100])
                                                             ^
SyntaxError: invalid syntax
ThePirateWhoSmellsOfSunflowers commented 4 years ago

:man_shrugging:

Traceback (most recent call last):
  File "/usr/lib/python3.6/encodings/hex_codec.py", line 15, in hex_encode
    return (binascii.b2a_hex(input), len(input))
TypeError: a bytes-like object is required, not 'str'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/xxx/pywerview/pywerview/cli/main.py", line 466, in main
    x = str(x)
  File "/xxx/pywerview/pywerview/objects/adobjects.py", line 130, in __str__
    member_value = '{}...'.format(codecs.encode(member[1],'hex')[:100])
TypeError: encoding with 'hex' codec failed (TypeError: a bytes-like object is required, not 'str')

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/lib/python3.6/encodings/hex_codec.py", line 15, in hex_encode
    return (binascii.b2a_hex(input), len(input))
TypeError: a bytes-like object is required, not 'str'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "pywerview.py", line 24, in <module>
    main()
  File "/xxx/pywerview/pywerview/cli/main.py", line 471, in main
    print(results)
  File "/xxx/pywerview/pywerview/objects/adobjects.py", line 139, in __repr__
    return str(self)
  File "/xxx/pywerview/pywerview/objects/adobjects.py", line 130, in __str__
    member_value = '{}...'.format(codecs.encode(member[1],'hex')[:100])
TypeError: encoding with 'hex' codec failed (TypeError: a bytes-like object is required, not 'str')

:sunflower:

mpgn commented 4 years ago

I clean up and remove binascii. Let's try one more time :dagger:

ThePirateWhoSmellsOfSunflowers commented 4 years ago

Eeeeeeet...non

Traceback (most recent call last):
  File "/xxx/pywerview/pywerview/cli/main.py", line 466, in main
    x = str(x)
  File "/xxx/pywerview/pywerview/objects/adobjects.py", line 129, in __str__
    member_value = '{}...'.format(codecs.encode(bytes(member[1]),'hex')[:100])
TypeError: string argument without an encoding

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "pywerview.py", line 24, in <module>
    main()
  File "/xxx/pywerview/pywerview/cli/main.py", line 471, in main
    print(results)
  File "/xxx/pywerview/pywerview/objects/adobjects.py", line 138, in __repr__
    return str(self)
  File "/xxx/pywerview/pywerview/objects/adobjects.py", line 129, in __str__
    member_value = '{}...'.format(codecs.encode(bytes(member[1]),'hex')[:100])
TypeError: string argument without an encoding

:sunflower:

mpgn commented 4 years ago

I do it blindly, not ideal :'(

We will squash this at the end so all the commit will be merged in one aha

ThePirateWhoSmellsOfSunflowers commented 4 years ago

Hooray :tada: Seems to work now, binary attributes are prefixed with a b but I think it's OK since these attributes are not very useful (and striped in the tostring method).

Screenshot from 2019-11-13 17-45-35

Maybe I'll have enough time to test others functions this week.

:sunflower:

ThePirateWhoSmellsOfSunflowers commented 4 years ago
Traceback (most recent call last):
  File "/xxx/pywerview/pywerview/cli/main.py", line 466, in main
    x = str(x)
  File "/xxx/pywerview/pywerview/objects/adobjects.py", line 108, in __str__
    member_value = (',\n' + ' ' * (max_length + 2)).join(codecs.encode(bytes(x, encoding='utf8'),'hex') for x in member[1])
TypeError: sequence item 0: expected str instance, bytes found

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "pywerview.py", line 24, in <module>
    main()
  File "/xxx/pywerview/pywerview/cli/main.py", line 471, in main
    print(results)
  File "/xxx/pywerview/pywerview/objects/adobjects.py", line 138, in __repr__
    return str(self)
  File "/xxx/pywerview/pywerview/objects/adobjects.py", line 108, in __str__
    member_value = (',\n' + ' ' * (max_length + 2)).join(codecs.encode(bytes(x, encoding='utf8'),'hex') for x in member[1])
TypeError: sequence item 0: expected str instance, bytes found
ThePirateWhoSmellsOfSunflowers commented 4 years ago

The error is gone but now the output is f*cked up because \n are no longer interpreted.

[...]
msmqdigests:                b'c29XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX,\n                            c29fYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYYY55,\n                          
[...]
mpgn commented 4 years ago

let's try this one

ThePirateWhoSmellsOfSunflowers commented 4 years ago

Yes, this one is ok ! No more b in front of "binary" strings.

mpgn commented 4 years ago

Yeah, it's better :)

the-useless-one commented 4 years ago

Hi all,

Thanks a lot for contributing to pywerview! As you can see, I don't really take the time to support this project, as my colleagues like to remind me (hi @ThePirateWhoSmellsOfSunflowers :heart:), so i'm glad to see that people still find it useful, and contribute to it :)

@byt3bl33d3r pywerview is in fact published to PyPI, but it's reeeally possible that it's not up to date with the latest additions. I'll try to find some time update everything. Also, I was wondering whether I should rename the functions, as they did in the dev version of PowerView (renaming every Get-Net* to Get-Domain* etc.). What do you think? They can easily create aliases in PowerShell, but it might be messy in Python. I don't really think a lot of users use pywerview's API so there shouldn't be a lot of breaking, but I don't know :man_shrugging:

Cheers,

Y

mpgn commented 4 years ago

Any update on this @the-useless-one ?

the-useless-one commented 4 years ago

Hi @mpgn, @ThePirateWhoSmellsOfSunflowers and I are performing some internal tests later in the month. We'll have the occasion to test your PR, and I'll merge it if everything works properly.

Thanks a lot for your work!

the-useless-one commented 4 years ago

Hi @mpgn,

I had some errors running functions that call the get_gpttmpl function. For example, with find-gpolocation, get-netgpogroup, or get-domainpolicy:

$ ./pywerview.py find-gpolocation -t $dc_ip -w $domain -u $user -p $password --username $queried_user
Traceback (most recent call last):
  File "./pywerview.py", line 24, in <module>
    main()
  File "/home/myhome/tools/pywerview/pywerview/cli/main.py", line 458, in main
    results = args.func(**parsed_args)
  File "/home/myhome/tools/pywerview/pywerview/cli/helpers.py", line 245, in find_gpolocation
    queried_domain=queried_domain)
  File "/home/myhome/tools/pywerview/pywerview/functions/gpo.py", line 438, in find_gpolocation
    for gpo_group in self.get_netgpogroup(queried_domain=queried_domain):
  File "/home/myhome/tools/pywerview/pywerview/functions/gpo.py", line 262, in get_netgpogroup
    results += self._get_groupsgpttmpl(gpttmpl_path, gpo_display_name)
  File "/home/myhome/tools/pywerview/pywerview/functions/gpo.py", line 205, in _get_groupsgpttmpl
    gpt_tmpl = self.get_gpttmpl(gpttmpl_path)
  File "/home/myhome/tools/pywerview/pywerview/functions/gpo.py", line 61, in get_gpttmpl
    smb_connection.getFile(share, file_name, content_io.write)
  File "/usr/local/lib/python3.6/dist-packages/impacket-0.9.21.dev0-py3.6.egg/impacket/smbconnection.py", line 792, in getFile
  File "/usr/local/lib/python3.6/dist-packages/impacket-0.9.21.dev0-py3.6.egg/impacket/smb3.py", line 1597, in retrieveFile
TypeError: string argument expected, got 'bytes'

I also got an error with get-netlocalgroup:

./pywerview.py get-netlocalgroup -t $dc_ip -w $domain -u $user -p $password --computername $target_computer
Traceback (most recent call last):
  File "/home/myhome/tools/pywerview/pywerview/cli/main.py", line 466, in main
    x = str(x)
TypeError: __str__ returned non-string (type bytes)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "./pywerview.py", line 24, in <module>
    main()
  File "/home/myhome/tools/pywerview/pywerview/cli/main.py", line 471, in main
    print(results)
  File "/home/myhome/tools/pywerview/pywerview/objects/rpcobjects.py", line 65, in __repr__
    return str(self)
TypeError: __str__ returned non-string (type bytes)

Any ideas?

mpgn commented 4 years ago

I will look tomorrow :)

mpgn commented 4 years ago

Hi @the-useless-one check the commit.

I didn't fix the error with get-netlocalgroup since on my side, this command doesn't return any output so I cannot test it

python pywerview.py get-netlocalgroup -t 172.16.60.199 -w demo.local -u Administrator -p P@ssword --computername DEMO-DC
-> empty
the-useless-one commented 4 years ago

Hi, thanks for the commit! I'll check it tomorrow.

Regarding get-netlocalgroup, this is weird. By default, it should return the members of the local Administrators group (and it's based on the RID, so it shouldn't be a localization problem). Could you check with the Python 2 version?

mpgn commented 4 years ago

Same result on pywerview python2 against a windows server 2012 R2 64bits

the-useless-one commented 4 years ago

Nevermind, I managed to get a correct result for get-netlocalgroup. Almost everything seems to work out!

However, I noticed a problem when building LDAP search filters with non-ascii characters. Here's an example:

Using my Python 2 branch :

$ ./pywerview.py get-netgroupmember -t $dc_ip -w $domain -u $user -p $password --groupname 'Administrateurs du schéma'
groupdomain:  $domain
groupname:    Administrateurs du schéma
isgroup:      False
memberdn:     CN=blahblahbla
memberdomain: $domain
membername:   $user
membersid:    S-1-5-21-XXX

Using the Python 3 version :

$ ./pywerview.py get-netgroupmember -t $dc_ip -w $domain -u $user -p $password --groupname 'Administrateurs du schéma'
<no result>

By taking a look at the network traffic in Wireshark, I noticed that the é is encoded as \xc3\xa9 in the Python 2 version. However, it's encoded as \xe9 in Python 3. Any ideas?

I think that once we fix this, I'll merge to master.

Thanks a lot for your contribution!

mpgn commented 4 years ago

I fix the problem with groupname LDAP search the most efficient way to avoid code duplication. If this problem is generalized to all LDAP queries, we may have a bigger problem :'(

check https://github.com/mpgn/pywerview/commit/2fd83cf7f048752f8545320cb3d4fa67d71540f8

mpgn commented 4 years ago

Any update ?

the-useless-one commented 4 years ago

Hi @mpgn, sorry about the delay. I think the problem is indeed generalized to every LDAP parameter, so I'm thinking of a way to fix the problem once and for all, in an easy, yet elegant way :stuck_out_tongue:

the-useless-one commented 4 years ago

I noticed another problem: when I'm on the Python 3 branch, the LDAP results are limited to 1000, but not when I'm on the Python 2 branch. Is it something that you are able to reproduce @mpgn? Or is it just my pywerview/impacket installation that is messy?

blshkv commented 4 years ago

I can see that you are straggling with encoding/decoding between latin1/utf-8.

That should not be the case. Everything should be just in unicode, starting from the console (ENV LANG "C.UTF-8", ENV LC_ALL "C.UTF-8"), received text, output and so on. https://blog.emacsos.com/unicode-in-python.html

the-useless-one commented 4 years ago

Hi @blshkv,

Indeed, everything should be in UTF8. My console is encoded in UTF8, and so are the Python files. If you know why we're observing this behaviour, or how to fix this, I'd be glad for the help!

yellow-starburst commented 4 years ago

When is this going to be merged? @the-useless-one @ThePirateWhoSmellsOfSunflowers

the-useless-one commented 4 years ago

HI @yellow-starburst,

This PR is almost functional. As said before, we're still experiencing weird problems regarding encoding in our LDAP requests. If you know how to fix this, then we can quickly merge this PR!

mpgn commented 4 years ago

Should be a lot better, time to review @the-useless-one :)

the-useless-one commented 4 years ago

Hi @mpgn,

First of all, thanks a lot for all of your work on pywerview, I really appreciate it! My colleagues now consider you pywerview's main dev ;)

Regarding your modifications, I'm sure that they would work for the modified parameters. However, there are many command line arguments that are then used to build LDAP requests. So we'd have to change the type for each of these arguments. What's more, I wonder if the use of latin-1 is because our systems are installed in French (is your system installed in French?). But what happens on a system installed in another language, or if we're making requests against a Domain Controller installed in Japanese?

I really hesitate to implement your fix for every command line parameter, as a kind of workaround, but I fear that this temporary solution becomes permanent and that it causes problems in the cases I mentioned before.

I'll try to quickly do some tests (or ask a colleague to) on different systems to understand this problem, so that we can find a solution.

Cheers,

Y

mpgn commented 4 years ago

No problem on my side as we already use this PR on cme, getting pywerview merged is just a bonus on our side anyway.

Btw if you find a better way I would be pleased, don't forgot to put the new version on pypi :)

blshkv commented 4 years ago

@mpgn I can see that you have changed the requirements to impacket>=0.9.22 but I did not find a code which would require this version (or higher) specifically.

Please do not rise requirements unnecessary.

mpgn commented 4 years ago

Feel free to push a PR @blshkv, I'm not following this PR anymore on my side as I think there is a better way to resolve the encoding problem but I don't have the solution. From my point of view I only do worst and worst trying to solve them, so I'm backing away from this PR ^^

ThePirateWhoSmellsOfSunflowers commented 4 years ago

Hi all. For information, I've submitted a PR to impacket regarding the encoding issue, just to determine where is the best place to fix the issue (pywerview or impacket).

https://github.com/SecureAuthCorp/impacket/pull/832

Not really sure if it will be quickly merged (or rejected).

:sunflower:

mpgn commented 4 years ago

Nice, it seems to be the proper way for me, I'm glad you dig into this issue, it's much cleaner. Finger crossed !

ThePirateWhoSmellsOfSunflowers commented 4 years ago

For the record, the last three known bugs to fix before an hypothetical merge are:

https://github.com/the-useless-one/pywerview/blob/2fd83cf7f048752f8545320cb3d4fa67d71540f8/pywerview/objects/adobjects.py#L56-L58

:sunflower:

mpgn commented 4 years ago

I'm not sure if it's possible to merge only specific commit but some of my commit like the last one https://github.com/the-useless-one/pywerview/pull/34/commits/2fd83cf7f048752f8545320cb3d4fa67d71540f8 maybe need to be removed because it will be irrelevant (if impacket merge PR)

cherry-pick from git should allow this type of PR but maybe closing and working on a new much cleaner PR can be also the solution, I don't mind, the goal is to have this project working on python3 and pushed into pip :)

ThePirateWhoSmellsOfSunflowers commented 4 years ago

Hi all! For information, I've submitted a new PR to impacket regarding the 1k results issue

https://github.com/SecureAuthCorp/impacket/pull/840

:sunflower:

EDIT: Merged :wink: