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 payload data model #11871

Merged
merged 2 commits into from May 23, 2019

Conversation

Projects
None yet
4 participants
@asoto-r7
Copy link
Contributor

commented May 22, 2019

A recent update to metasploit_data_models removed the association between payloads and workspaces. Per @bcook, and after a discussion among a few on the team, we decided to remove the workspace association from the payload object, making payloads independent from workspaces.

I made the change to the data models gem on 10-May, not realizing that the metasploit-framework.gemspec file did not lock the version of the metasploit_data_models gem. Further, I did not realize that Jenkins would automatically upgrade the gem, incorporating the data model changes before the changes in this PR were ready and tested.

This PR addresses two changes in lib/msf/core/db_manager/payload.rb, where payloads were previously added or located via a wspace object. Now, the payloads are added and located directly via Mdm::Payload, in line with the data model changes.

Verification

To verify these changes, create a variety of payloads, making sure to explicitly set PayloadUUIDTracking on a UUID-supported payload to check multiple code paths:

Initial setup

  • Start msfconsole
  • Use db_status to confirm database connection via the remote_data_service (HTTP)

Payload generation

  • use payload/java/meterpreter/reverse_http
  • set LHOST [your local IP]
  • set PayloadUUIDTracking false
  • generate -f jar -o uuid_disabled.jar
  • set PayloadUUIDTracking true
  • generate -f jar -o uuid_enabled.jar
  • Confirm that both generate commands succeed without an exception. (Previously, they triggered an exception: Problem retrieving payload: undefined method payloads'`) A successful test looks like:
msf5 > use payload/java/meterpreter/reverse_http
msf5 payload(java/meterpreter/reverse_http) > set LHOST 192.168.199.202
LHOST => 192.168.199.202
msf5 payload(java/meterpreter/reverse_http) > set PayloadUUIDTracking false
PayloadUUIDTracking => false
msf5 payload(java/meterpreter/reverse_http) > generate -f jar -o uuid_disabled.jar
[*] Writing 5518 bytes to uuid_disabled.jar...
msf5 payload(java/meterpreter/reverse_http) > set PayloadUUIDTracking true
PayloadUUIDTracking => true
msf5 payload(java/meterpreter/reverse_http) > generate -f jar -o uuid_enabled.jar
[*] Writing 5578 bytes to uuid_enabled.jar...
msf5 payload(java/meterpreter/reverse_http) >

Payload callback

  • use exploit/multi/handler
  • set PAYLOAD java/meterpreter/reverse_http
  • set LHOST [your local IP]
  • set PayloadUUIDTracking false
  • run
  • Launch uuid_disabled.jar
  • Receive the callback and exit the Meterpreter session.
  • set PayloadUUIDTracking true
  • Launch uuid_enabled.jar
  • run
  • Receive the callback and exit the Meterpreter session.
  • Confirm that each callbacks is received successfully and does not trigger exceptions. A successful test looks like:
msf5 payload(java/meterpreter/reverse_http) > use exploit/multi/handler
msf5 exploit(multi/handler) > set PAYLOAD java/meterpreter/reverse_http
PAYLOAD => java/meterpreter/reverse_http
msf5 exploit(multi/handler) > set LHOST 192.168.199.202
LHOST => 192.168.199.202
msf5 exploit(multi/handler) > set PayloadUUIDTracking true
PayloadUUIDTracking => true
msf5 exploit(multi/handler) > run

[*] Started HTTP reverse handler on http://192.168.199.202:8080
[*] http://192.168.199.202:8080 handling request from 192.168.199.152; (UUID: jltn7avh) Staging java payload (54399 bytes) ...
[*] Meterpreter session 5 opened (192.168.199.202:8080 -> 192.168.199.152:50842) at 2019-05-22 13:52:19 -0500

Optional:

  • Disconnect from the HTTP remote_data_service (db_disconnect), to fallback to Postgres connectivity.
  • Retry the steps within Payload generation and Payload callbacks.
@@ -65,7 +65,7 @@ Gem::Specification.new do |spec|
# Metasploit::Credential database models
spec.add_runtime_dependency 'metasploit-credential'
# Database models shared between framework and Pro.
spec.add_runtime_dependency 'metasploit_data_models'
spec.add_runtime_dependency 'metasploit_data_models', '3.0.10'

This comment has been minimized.

Copy link
@acammack-r7

acammack-r7 May 22, 2019

Contributor

If we want to keep releasing new gems with this code we should probably do that under a new major version number (4.x.x). That way, we can use a version restriction here for <4.0) in case we need to pull in a bug fix for metasploit data models (and to make life for Pro easier).

This comment has been minimized.

Copy link
@busterb

busterb May 23, 2019

Member

Yep, next revision I think we should bump so we can segregate this from the 4.x-compatible stream.

This comment has been minimized.

Copy link
@jmartin-r7

jmartin-r7 May 23, 2019

Contributor

Pro is currently on metasploit_data_models 2.x stream however once msf5 reaches Pro this recommendation would be good to support.

@busterb

This comment has been minimized.

Copy link
Member

commented May 22, 2019

Does lib/metasploit/framework/data_service/proxy/payload_data_proxy.rb need to be updated to stop adding workspace to payload queries on the remote web service?

@busterb

This comment has been minimized.

Copy link
Member

commented May 22, 2019

There are still workspace queries in create_payload and update_payload. These look like no-ops. Are they?

@jmartin-r7 jmartin-r7 added the msf5 label May 23, 2019

@asoto-r7

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

There are still workspace queries in create_payload and update_payload. These look like no-ops. Are they?

As near as I can tell. This PR is meant to fix that which was immediately broken. I did not see those two methods being called during normal workflows, but I didn't search to see if they were being used elsewhere. Is that a blocker to landing this PR?

@busterb

This comment has been minimized.

Copy link
Member

commented May 23, 2019

Not sure I follow? I'll fix it up before landing if you can't get to it in time. Testing now...

@busterb busterb self-assigned this May 23, 2019

@busterb

This comment has been minimized.

Copy link
Member

commented May 23, 2019

This also worked when I randomly killed the database service in the middle of listening. Good to go.

@busterb busterb merged commit fbd6040 into rapid7:master May 23, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Metasploit Automation - Sanity Test Execution Successfully completed all tests.
Details
Metasploit Automation - Test Execution Successfully completed all tests.
Details

busterb added a commit that referenced this pull request May 23, 2019

@busterb

This comment has been minimized.

Copy link
Member

commented May 23, 2019

Release Notes

This fixes an issue where an error would display in msfconsole when establishing a Meterpreter HTTP/S session when using a local postgresql database, preventing interaction with the session.

@busterb

This comment has been minimized.

Copy link
Member

commented May 23, 2019

Out of scope for this PR, but notes for the next, it appears that the wspace/workspace additions are load-bearing (that is, might still be some sort of workspace/payload association). A patch like this:

diff --git a/lib/metasploit/framework/data_service/proxy/payload_data_proxy.rb b/lib/metasploit/framework/data_service/proxy/payload_data_proxy.rb
index 581f025991..955ec42a24 100644
--- a/lib/metasploit/framework/data_service/proxy/payload_data_proxy.rb
+++ b/lib/metasploit/framework/data_service/proxy/payload_data_proxy.rb
@@ -3,7 +3,6 @@ module PayloadDataProxy
   def payloads(opts)
     begin
       self.data_service_operation do |data_service|
-        add_opts_workspace(opts)
         data_service.payloads(opts)
       end
     rescue => e
@@ -14,7 +13,6 @@ module PayloadDataProxy
   def create_payload(opts)
     begin
       self.data_service_operation do |data_service|
-        add_opts_workspace(opts)
         data_service.create_payload(opts)
       end
     rescue => e
diff --git a/lib/msf/core/db_manager/payload.rb b/lib/msf/core/db_manager/payload.rb
index 43596930d6..77ad8c90a2 100644
--- a/lib/msf/core/db_manager/payload.rb
+++ b/lib/msf/core/db_manager/payload.rb
@@ -7,8 +7,6 @@ module Msf::DBManager::Payload
           raise ArgumentError.new("A payload with this uuid already exists.")
         end
       end
-
-      wspace = Msf::Util::DBManager.process_opts_workspace(opts, framework)
       Mdm::Payload.create!(opts)
     end
   end
@@ -30,9 +28,6 @@ module Msf::DBManager::Payload
 
   def update_payload(opts)
     ::ActiveRecord::Base.connection_pool.with_connection do
-      wspace = Msf::Util::DBManager.process_opts_workspace(opts, framework, false)
-      opts[:workspace] = wspace if wspace
-
       id = opts.delete(:id)
       Mdm::Payload.update(id, opts)
     end
diff --git a/msfvenom b/msfvenom
index 96ce04538a..a2b8b6620e 100755
--- a/msfvenom
+++ b/msfvenom
@@ -43,7 +43,7 @@ def init_framework(create_opts={})
     type = Msf.const_get("MODULE_#{type.upcase}")
   end
 
-  @framework = ::Msf::Simple::Framework.create(create_opts.merge('DisableDatabase' => true))
+  @framework = ::Msf::Simple::Framework.create(create_opts)
 end

Throws this error on payload create with msfvenom:

[05/23/2019 11:41:11] [e(0)] core: RuntimeError : Problem creating payload: unknown attribute 'workspace' for Mdm::Payload.. See log for more details.
/Users/bcook/projects/metasploit-framework/lib/metasploit/framework/data_service/proxy/core.rb:174:in `log_error'
/Users/bcook/projects/metasploit-framework/lib/metasploit/framework/data_service/proxy/payload_data_proxy.rb:19:in `rescue in create_payload'
/Users/bcook/projects/metasploit-framework/lib/metasploit/framework/data_service/proxy/payload_data_proxy.rb:13:in `create_payload'
/Users/bcook/projects/metasploit-framework/lib/msf/core/payload/uuid/options.rb:108:in `record_payload_uuid'
/Users/bcook/projects/metasploit-framework/lib/msf/core/payload/uuid/options.rb:81:in `generate_payload_uuid'
/Users/bcook/projects/metasploit-framework/lib/msf/core/payload/uuid/options.rb:45:in `generate_uri_uuid_mode'
/Users/bcook/projects/metasploit-framework/lib/msf/core/payload/java/reverse_http.rb:59:in `generate_uri'
/Users/bcook/projects/metasploit-framework/lib/msf/core/payload/java/reverse_http.rb:66:in `stager_config'
/Users/bcook/projects/metasploit-framework/lib/msf/core/payload/java.rb:61:in `generate_jar'
/Users/bcook/projects/metasploit-framework/lib/msf/core/payload_generator.rb:344:in `generate_java_payload'
/Users/bcook/projects/metasploit-framework/lib/msf/core/payload_generator.rb:370:in `generate_payload'
./msfvenom:450:in `<main>'

With an addition longer trace into active record available in framework.log. There might still be some inheritance of workspace in payloads deeper down, or I just have a local gem out of sync; will see after this next release.

@asoto-r7 asoto-r7 deleted the asoto-r7:pr11871-fix-payload-data-model branch Jun 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.