Fix metasploit-framework#15968 (Multiple Socket Abstraction Bugs) #20

zeroSteiner · 2022-01-24T22:18:09Z

This fixes multiple issues I ran into while working on rapid7/metasploit-framework#15968. I also ran rubocop on the two files which accounts for quite a few of their changes. Each was done in a dedicated commit, so hopefully that will help isolate those changes from the actual fixes.

The core of the issues results in a users inability to open a reverse_http Meterpreter session over a pivot/port-forward. A user should be able to either start a local handler and portforward to it via Meterpreter or start a handler via a Meterpreter session directly (using the ReverseListenerComm datastore option). Without these changes, the Meterpreter session would consistently fail to be established. The user would see that the stage was being sent, but the session would never be fully opened and responsive. I tracked the issue down to the socket abstraction that would close the connection before the entire HTTP response had been sent due to a race condition.

Testing

Open a Meterpreter session or an SSH session, one that's capable of starting port forwards (GatewayPorts must be enabled for SSH)
Use a Meterpreter payload with a reverse_http stager (Windows and Python were tested while working on this PR)
Configure the payload to start a handler using the session opened in step 1
- Set LHOST to an IP address on the promised host
- Set ReverseListenerComm to the session ID (note that the -1 shortcut does not work yet)
- Start the handler and generate a payload
- Execute the payload and see a functioning Meterpreter session has been established
  - At this point, without these changes, the sending stage message would be shown but the session would never work. The original bug can be reproduced in this way.

The original issue mentioned using an explicit portfwd. While not strictly necessary to exercise the applicable code path, you can do so by modifying the steps to star the handler on the local Metasploit host and using a Meterpreter session opened in step 1 to start a reverse portforward. An SSH session won't work for this purpose because a single port can be forward since the portfwd command is a Meterpreter-specific command.

smcintyre-r7 · 2022-01-24T22:19:51Z

lib/rex/io/stream_server.rb

+          begin
+            cli = accept
+            unless cli
+              elog('The accept() returned nil in stream server listener monitor')


Dropped the fd.inspect part because fd isn't available in Meterpreter channels which caused an exception to be thrown.

smcintyre-r7 · 2022-01-24T22:20:49Z

lib/rex/io/socket_abstraction.rb

+
+      module MonitoredRSock
+        def close
+          @close_requested = true


This only ever goes from true to false so I don't think there's a synchronization problem here but if another solution is preferred happy to go a different direction.

smcintyre-r7 · 2022-01-24T22:21:42Z

lib/rex/io/socket_abstraction.rb

          end
-          break
+
+          rsock.sysclose


This is now the one and only location where a monitored rsock will actually be closed. Other attempts to close it will result in the request being made and it's up to this loop to honor it and exit.

adfoster-r7 · 2022-01-24T23:02:35Z

Thanks for looking into this! 🎉

Would you mind throwing up a separate PR for that first rubocop commit and I can ship that ASAP, it would just help make the git history a bit cleaner. Or I can cherry-pick your first commit and create a separate PR as well if that's easier - just let me know which you'd prefer 👍

zeroSteiner · 2022-01-25T13:30:42Z

Would you mind throwing up a separate PR for that first rubocop commit and I can ship that ASAP,

Sure thing, see #21. I can rebase this one once that's landed.

adfoster-r7 · 2022-01-25T15:41:14Z

@zeroSteiner Merged, thanks!

zeroSteiner · 2022-01-25T15:58:50Z

Thanks, this has been rebased now so it's a lot easier to review.

adfoster-r7 · 2022-01-26T11:32:45Z

Sorry, didn't realize there was a second commit related to running rubocop - grabbed that into master now too: #22

Also added Github actions to this repository to ensure that the test suite runs, although it looks like we don't have a lot of tests for this specific functionality that's being changed

The monitor thread will notice that the lsock has been closed so wait for it to exit after processing all of the data that needed to be sent.

This means that if the rsock is closed from another thread like a DIO handler that the monitor thread can ensure that it has processed all data before closing it.

zeroSteiner · 2022-01-26T15:55:42Z

lib/rex/io/socket_abstraction.rb

@@ -182,14 +202,18 @@ def monitor_rsock(threadname = 'SocketMonitorRemote')

            begin
              close_write if respond_to?('close_write')
-            rescue IOError
+            rescue StandardError


Forgot to mention, the Meterpreter error raised when calling close_write on an invalid handle is not derived from IOError so I changed this to StandardError.

Pull in the changes from rapid7/rex-core#20

smcintyre-r7 requested a review from adfoster-r7 January 24, 2022 22:18

smcintyre-r7 added the bug label Jan 24, 2022

smcintyre-r7 reviewed Jan 24, 2022

View reviewed changes

zeroSteiner force-pushed the fix/msf-15968 branch from 15eabe0 to ecb278f Compare January 25, 2022 15:57

adfoster-r7 mentioned this pull request Jan 26, 2022

Rubocop stream_server #22

Merged

zeroSteiner added 3 commits January 26, 2022 10:54

Wait for the monitor thread before closing

ee9e212

The monitor thread will notice that the lsock has been closed so wait for it to exit after processing all of the data that needed to be sent.

fd isn't always defined

846bfd2

Force the monitor thread to close the rsock

caa6027

This means that if the rsock is closed from another thread like a DIO handler that the monitor thread can ensure that it has processed all data before closing it.

zeroSteiner force-pushed the fix/msf-15968 branch from ecb278f to caa6027 Compare January 26, 2022 15:54

zeroSteiner commented Jan 26, 2022

View reviewed changes

adfoster-r7 merged commit 3c1fafc into rapid7:master Jan 26, 2022

zeroSteiner added a commit to zeroSteiner/metasploit-framework that referenced this pull request Jan 26, 2022

Bump rex-core to 0.1.25

a741a80

Pull in the changes from rapid7/rex-core#20

zeroSteiner added a commit to zeroSteiner/metasploit-framework that referenced this pull request Jan 26, 2022

Bump rex-core to 0.1.25

e10203a

Pull in the changes from rapid7/rex-core#20

zeroSteiner mentioned this pull request Jan 26, 2022

Bump rex-core to 0.1.25 rapid7/metasploit-framework#16104

Merged

audibleblink pushed a commit to audibleblink/metasploit-framework that referenced this pull request Jan 28, 2022

Bump rex-core to 0.1.25

013bafa

Pull in the changes from rapid7/rex-core#20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix metasploit-framework#15968 (Multiple Socket Abstraction Bugs) #20

Fix metasploit-framework#15968 (Multiple Socket Abstraction Bugs) #20

zeroSteiner commented Jan 24, 2022

smcintyre-r7 Jan 24, 2022

smcintyre-r7 Jan 24, 2022

smcintyre-r7 Jan 24, 2022

adfoster-r7 commented Jan 24, 2022 •

edited

Loading

zeroSteiner commented Jan 25, 2022

adfoster-r7 commented Jan 25, 2022

zeroSteiner commented Jan 25, 2022

adfoster-r7 commented Jan 26, 2022

zeroSteiner Jan 26, 2022

Fix metasploit-framework#15968 (Multiple Socket Abstraction Bugs) #20

Fix metasploit-framework#15968 (Multiple Socket Abstraction Bugs) #20

Conversation

zeroSteiner commented Jan 24, 2022

Testing

smcintyre-r7 Jan 24, 2022

Choose a reason for hiding this comment

smcintyre-r7 Jan 24, 2022

Choose a reason for hiding this comment

smcintyre-r7 Jan 24, 2022

Choose a reason for hiding this comment

adfoster-r7 commented Jan 24, 2022 • edited Loading

zeroSteiner commented Jan 25, 2022

adfoster-r7 commented Jan 25, 2022

zeroSteiner commented Jan 25, 2022

adfoster-r7 commented Jan 26, 2022

zeroSteiner Jan 26, 2022

Choose a reason for hiding this comment

adfoster-r7 commented Jan 24, 2022 •

edited

Loading