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

Convert all python code to python3. Fixes #12506. #12524

Merged
merged 15 commits into from Dec 23, 2019
Merged

Conversation

@xmunoz
Copy link
Contributor

xmunoz commented Oct 31, 2019

This is a breaking change that removes support for python2. Not every file in the change has been fully tested because of limited access to the exploit target environments, but running with python3 $FILE produced no syntax errors when the needed dependencies were available. These changes were made with the following command:

2to3-2.7 -f all -f ws_comma -w -n $FILE

2to3 is the recommended utility for auto translation for python 2 -> 3. I also manually edited some files where 2to3 performed unexpectedly.

Related: https://gitlab.com/kalilinux/packages/metasploit-framework/issues/1

Fixes #12506.

Verification

  • When using Python 3 (may require temporarily modifying a symlink or the shebang in the module files to force)
    • The windows shellcode builders produce the correct output
    • The teradata modules work per #10109
    • The impacket modules work per #9816 #10106 #10107
    • The haraka module works per #8178
    • exploit/windows/smb/ms17_010_eternalblue_win8 works per #10184
  • When using Python 2.6+ (may require temporarily modifying a symlink or the shebang in the module files to force)
    • The teradata modules work per #10109
    • The impacket modules work per #9816 #10106 #10107
    • The haraka module works per #8178
    • exploit/windows/smb/ms17_010_eternalblue_win8 works per #10184
xmunoz added 2 commits Oct 31, 2019
Remove utf-8 encoding declarations, as this is the default for python3.
@wvu-r7

This comment has been minimized.

Copy link
Contributor

wvu-r7 commented Nov 1, 2019

Wow, thanks!

@wvu-r7 wvu-r7 added the enhancement label Nov 1, 2019
Copy link
Contributor

acammack-r7 left a comment

Thanks for your work on this! I think we will want a somewhat different approach from this, though. First, the output of 2to3 is a bit quirky and there are some things, like extra calls to list(), that we do not need or want. Next, a couple of these files are intended to run on older systems that definitely have Python 2 but may not have Python 3, or if the user has installed it, it may not be where we expect (also, the module that runs this privilege escalation scripts would need to be updated to look for Python 3). Finally, Python 2 is still around for a bit yet, and since dual-compatibility is not a huge lift for us I am reluctant to wholesale drop support for it in modules or external module libraries that do not explicitly need Python 3 features (like the async stuff). Tools and build scripts I think are fine to move to 3, since they are intended to be run by developers.

On the shebangs, files that can run under 2 and 3 should be allowed to do so. Also, as I understand it, the encoding can now taken from the operating system's LOCALE as well, which means we still have to specify the encoding to ensure the files are read properly.

Thanks again for submitting this, there are definitely a couple places you caught that I wouldn't have thought to check!

data/exploits/CVE-2015-1130/exploit.py Outdated Show resolved Hide resolved
modules/auxiliary/admin/teradata/teradata_odbc_sql.py Outdated Show resolved Hide resolved
modules/auxiliary/scanner/http/onion_omega2_login.py Outdated Show resolved Hide resolved
modules/exploits/linux/smtp/haraka.py Outdated Show resolved Hide resolved
modules/exploits/linux/smtp/haraka.py Outdated Show resolved Hide resolved
modules/exploits/linux/smtp/haraka.py Outdated Show resolved Hide resolved
tools/hardware/killerbee_msfrelay.py Outdated Show resolved Hide resolved
data/exploits/cve-2015-5287/sosreport-rhel7.py Outdated Show resolved Hide resolved
@xmunoz

This comment has been minimized.

Copy link
Contributor Author

xmunoz commented Nov 2, 2019

Thanks for the comprehensive review! I'll clean this up shortly.

xmunoz and others added 3 commits Nov 2, 2019
- Change executable in shebang from python3 to python
- Revert changes to files that will only run as python2

Co-Authored-By: acammack-r7 <adam_cammack@rapid7.com>
- Revert files that will only run as python2.
- Remove superfluous calls to list()
- Other minor cleanup
@xmunoz xmunoz requested a review from acammack-r7 Nov 2, 2019
@acammack-r7

This comment has been minimized.

Copy link
Contributor

acammack-r7 commented Nov 5, 2019

Thanks for the cleanup, the changes look good! I haven't had a chance to test all the moving parts yet, but I have added some verification steps to the OP for anyone who wants to help out verifying these changes.

@xmunoz

This comment has been minimized.

Copy link
Contributor Author

xmunoz commented Nov 8, 2019

When I was testing the windows shellcode builders, I encountered 2 errors on master.

  1. On external/source/shellcode/windows/x64/build.py:
$ python build.py all
...
./src/block/block_reverse_https.asm:148: warning: `ptr' is not a NASM keyword [-w+ptr]
./src/block/block_reverse_https.asm:148: error: comma, colon, decorator or end of line expected after operand
[-]  [Errno 2] No such file or directory: 'bin/stager_reverse_https.bin'

According to this, you can just remove the ptr keyword. When I did that, the script ran to completion. I ran it on master and this branch, and compared the output. The results were identical.

$ diff result-master.txt result-python3.txt
1c1
< # Built on Fri Nov  8 10:47:56 2019
---
> # Built on Fri Nov  8 10:51:53 2019

Should I add this change to this PR?

$ git diff external/source/shellcode/windows/x64/src/block/block_reverse_https.asm
diff --git a/external/source/shellcode/windows/x64/src/block/block_reverse_https.asm b/external/source/shellcode/windows/x64/src/block/block_reverse_https.asm
index 7578b590c1..604d47c8ad 100644
--- a/external/source/shellcode/windows/x64/src/block/block_reverse_https.asm
+++ b/external/source/shellcode/windows/x64/src/block/block_reverse_https.asm
@@ -145,7 +145,7 @@ download_more:
   test eax,eax           ; download failed? (optional?)
   jz failure

-  mov ax, word ptr [edi]
+  mov ax, word [edi]
   add rbx, rax           ; buffer += bytes_received

   test rax,rax           ; optional?
  1. On external/source/shellcode/windows/x86/src/build.py:
$ python build.py all
...
src/stager/stager_reverse_winhttp.asm:17: fatal: unable to open include file `./src/block/block_reverse_winhttp_http.asm'
[-]  [Errno 2] No such file or directory: 'bin/stager_reverse_winhttp.bin'

Here is the change required to fix the error:

$ git diff external/source/shellcode/windows/x86/src/stager/stager_reverse_winhttp.asm
diff --git a/external/source/shellcode/windows/x86/src/stager/stager_reverse_winhttp.asm b/external/source/shellcode/windows/x86/src/stager/stager_reverse_winhttp.asm
index 7e32a9b3b8..bb8af3e470 100644
--- a/external/source/shellcode/windows/x86/src/stager/stager_reverse_winhttp.asm
+++ b/external/source/shellcode/windows/x86/src/stager/stager_reverse_winhttp.asm
@@ -14,6 +14,6 @@
 %include "./src/block/block_api.asm"
 start:                   ;
   pop ebp                ; pop off the address of 'api_call' for calling later.
-%include "./src/block/block_reverse_winhttp_http.asm"
+%include "./src/block/block_reverse_winhttp.asm"
   ; By here we will have performed the reverse_tcp connection and EDI will be our socket.

Should I add that change to this PR? Similar to above, I diff-ed the results from master and this branch after the edit was made, and the results were identical.

I think the first checkbox can be checked.

If you could give me some help for testing the other ones that would be great.

@xmunoz

This comment has been minimized.

Copy link
Contributor Author

xmunoz commented Nov 11, 2019

@acammack-r7

This comment has been minimized.

Copy link
Contributor

acammack-r7 commented Nov 12, 2019

  1. This is a NASM/MASM issue, and since it's Windows shellcode I'm inclined to leave it in the MASM style (IIRC MASM does require ptr in this case).
  2. That does look like a bug in the assembly. This PR is works great, thanks!

In Metasploit the verification steps are mostly for the person landing the PR to verify all the proposed changes have their expected result. Verifying the rest of the changes will require a Metasploit development environment and a working knowledge of how to use msfconsole (the SANS cheat sheet is a little out of date, but has the basics). Each of those PRs has a list of verification steps like this list the software that needs to be installed to test the module and how the modules are expected to run. If you want to go through all that, that's great and will definitely help us get this landed sooner! Even if you do, though, someone else will still need to verify the changes you have pushed do the right thing (a sad result of us not having good automated testing around modules).

I'll try to set aside some time to look at this since I have most of this software set up already, but the code looks good and any the team members that want to take this on can.

@busterb busterb self-assigned this Dec 19, 2019
@busterb

This comment has been minimized.

Copy link
Member

busterb commented Dec 19, 2019

Noting that a few of these don't work after the upgrade, will push fixes to the scripts.

@acammack-r7

This comment has been minimized.

Copy link
Contributor

acammack-r7 commented Dec 19, 2019

@busterb Did you the discussion the NASM/MASM difference?

This is a NASM/MASM issue, and since it's Windows shellcode I'm inclined to leave it in the MASM style (IIRC MASM does require ptr in this case).

@busterb

This comment has been minimized.

Copy link
Member

busterb commented Dec 19, 2019

Ah, thanks, maybe can make them both happy. I'm suspect the last time this was built and updated statically in framework though, it wasn't with masm.

@acammack-r7

This comment has been minimized.

Copy link
Contributor

acammack-r7 commented Dec 19, 2019

That is a good point, looking again at it now it callsnasm explicitly. Removing ptr should be fine, then.

@busterb

This comment has been minimized.

Copy link
Member

busterb commented Dec 19, 2019

It might be possible to rewrite the asm on the fly so we filter keywords to make it happy in the build process, but I think just preferring nasm is the best case, since this is an area somewhat rarely visited by most users and developers.

@@ -1,4 +1,4 @@
#!/usr/bin/env python

This comment has been minimized.

Copy link
@busterb

busterb Dec 20, 2019

Member

there is no python3 release of killerbee, making this conversion not really possible at this time.

busterb added a commit that referenced this pull request Dec 23, 2019
@busterb busterb merged commit 20e6568 into rapid7:master Dec 23, 2019
3 checks passed
3 checks passed
Metasploit Automation - Sanity Test Execution Successfully completed all tests.
Details
Metasploit Automation - Test Execution Successfully completed all tests.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@busterb

This comment has been minimized.

Copy link
Member

busterb commented Dec 23, 2019

Release Notes

This updates most Python code in Metasploit Framework to be compatible with Python 3.

msjenkins-r7 added a commit that referenced this pull request Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.