don't rely on side-effects in db util function

Author: jmartin-tech

lib/msf/core/db_manager/cred.rb

@@ -9,7 +9,7 @@ def creds(opts)
       end
 
       wspace = Msf::Util::DBManager.process_opts_workspace(opts, framework)
-      search_term = opts.delete(:search_term)
+      search_term = opts[:search_term]
 
       query = Metasploit::Credential::Core.where( workspace_id: wspace.id )
       query = query.includes(:private, :public, :logins, :realm).references(:private, :public, :logins, :realm)
@@ -108,24 +108,24 @@ def report_auth_info(opts={})
     end
 
   ::ActiveRecord::Base.connection_pool.with_connection {
-    host = opts.delete(:host)
-    ptype = opts.delete(:type) || "password"
-    token = [opts.delete(:user), opts.delete(:pass)]
-    sname = opts.delete(:sname)
-    port = opts.delete(:port)
-    proto = opts.delete(:proto) || "tcp"
-    proof = opts.delete(:proof)
-    source_id = opts.delete(:source_id)
-    source_type = opts.delete(:source_type)
-    duplicate_ok = opts.delete(:duplicate_ok)
+    host = opts[:host]
+    ptype = opts[:type] || "password"
+    token = [opts[:user], opts[:pass]]
+    sname = opts[:sname]
+    port = opts[:port]
+    proto = opts[:proto] || "tcp"
+    proof = opts[:proof]
+    source_id = opts[:source_id]
+    source_type = opts[:source_type]
+    duplicate_ok = opts[:duplicate_ok]
     # Nil is true for active.
     active = (opts[:active] || opts[:active].nil?) ? true : false
 
     wspace = Msf::Util::DBManager.process_opts_workspace(opts, framework)
 
     # Service management; assume the user knows what
     # he's talking about.
-    service = opts.delete(:service) || report_service(:host => host, :port => port, :proto => proto, :name => sname, :workspace => wspace)
+    service = opts[:service] || report_service(:host => host, :port => port, :proto => proto, :name => sname, :workspace => wspace)
 
     # Non-US-ASCII usernames are tripping up the database at the moment, this is a temporary fix until we update the tables
     if (token[0])
@@ -226,6 +226,8 @@ def update_credential(opts)
     ::ActiveRecord::Base.connection_pool.with_connection {
       # process workspace string for update if included in opts
       wspace = Msf::Util::DBManager.process_opts_workspace(opts, framework, false)
+      opts = opts.clone()
+      opts.delete(:workspace)
       opts[:workspace] = wspace if wspace
 
       if opts[:public]

lib/msf/core/db_manager/event.rb

@@ -27,6 +27,8 @@ def events(opts)
     end
 
     wspace = Msf::Util::DBManager.process_opts_workspace(opts, framework)
+    opts = opts.clone()
+    opts.delete(:workspace)
 
     order = opts.delete(:order)
     order = order.nil? ? DEFAULT_ORDER : order.to_sym
@@ -52,6 +54,9 @@ def report_event(opts)
   ::ActiveRecord::Base.connection_pool.with_connection {
     wspace = Msf::Util::DBManager.process_opts_workspace(opts, framework)
     return if not wspace # Temp fix?
+
+    opts = opts.clone()
+    opts.delete(:workspace)
     uname  = opts.delete(:username)
 
     if !opts[:host].nil? && !opts[:host].kind_of?(::Mdm::Host)
@@ -61,4 +66,4 @@ def report_event(opts)
     ::Mdm::Event.create(opts.merge(:workspace_id => wspace[:id], :username => uname))
   }
   end
-end
+end

lib/msf/core/db_manager/exploit_attempt.rb

@@ -58,7 +58,8 @@ def report_exploit_failure(opts)
     # Bail if we dont have a host object
     return if not host
 
-    opts = opts.dup
+    opts = opts.clone()
+    opts.delete(:workspace)
     opts[:service] = svc
     opts[:host] = host
 
@@ -74,14 +75,15 @@ def report_exploit_success(opts)
     host   = opts[:host] || return
 
     wspace = Msf::Util::DBManager.process_opts_workspace(opts, framework)
+    # it is rude to modify arguments in place
+    opts = opts.clone()
+    opts.delete(:workspace) # does this really need to be removed?
     port   = opts[:port]
     prot   = opts[:proto] || Msf::DBManager::DEFAULT_SERVICE_PROTO
     svc    = opts[:service]
 
     # Look up or generate the service as appropriate
     if port and svc.nil?
-      # it is rude to modify arguments in place
-      opts = opts.dup
       opts[:proto] ||= Msf::DBManager::DEFAULT_SERVICE_PROTO
       opts[:service] = report_service(
         workspace: wspace, host: host, port: port, proto: prot

lib/msf/core/db_manager/host.rb

@@ -188,6 +188,8 @@ def report_host(opts)
 
   ::ActiveRecord::Base.connection_pool.with_connection {
     wspace = Msf::Util::DBManager.process_opts_workspace(opts, framework)
+    opts = opts.clone
+    opts.delete(:workspace)
 
     begin
       retry_attempts ||= 0
@@ -280,6 +282,7 @@ def update_host(opts)
     ::ActiveRecord::Base.connection_pool.with_connection {
       # process workspace string for update if included in opts
       wspace = Msf::Util::DBManager.process_opts_workspace(opts, framework, false)
+      opts = opts.clone()
       opts[:workspace] = wspace if wspace
 
       id = opts.delete(:id)

lib/msf/core/db_manager/host_tag.rb

@@ -14,10 +14,10 @@ def report_host_tag(opts)
 
 
     host = get_host(:workspace => wspace, :address => addr)
-    desc = opts.delete(:desc)
-    summary = opts.delete(:summary)
-    detail = opts.delete(:detail)
-    crit = opts.delete(:crit)
+    desc = opts[:desc]
+    summary = opts[:summary]
+    detail = opts[:detail]
+    crit = opts[:crit]
     possible_tags = Mdm::Tag.includes(:hosts).where("hosts.workspace_id = ? and tags.name = ?", wspace.id, name).order("tags.id DESC").limit(1)
     tag = (possible_tags.blank? ? Mdm::Tag.new : possible_tags.first)
     tag.name = name

lib/msf/core/db_manager/import.rb

@@ -94,7 +94,10 @@ def import(args={}, &block)
     data = args[:data] || args['data']
     ftype = import_filetype_detect(data)
     yield(:filetype, @import_filedata[:type]) if block
-    self.send "import_#{ftype}".to_sym, args.merge(workspace: wspace.name), &block
+    # this code looks to intentionally convert workspace to a string, why?
+    opts = args.clone()
+    opts.delete(:workspace)
+    self.send "import_#{ftype}".to_sym, opts.merge(workspace: wspace.name), &block
     # post process the import here for missing default port maps
     mrefs, mports, _mservs = Msf::Modules::Metadata::Cache.instance.all_remote_exploit_maps
     # the map build above is a little expensive, another option is to do
@@ -209,10 +212,13 @@ def import_file(args={}, &block)
     # Override REXML's expansion text limit to 50k (default: 10240 bytes)
     REXML::Security.entity_expansion_text_limit = 51200
 
+    # this code looks to intentionally convert workspace to a string, why?
+    opts = args.clone()
+    opts.delete(:workspace)
     if block
-      import(args.merge(data: data, workspace: wspace.name)) { |type,data| yield type,data }
+      import(opts.merge(data: data, workspace: wspace.name)) { |type,data| yield type,data }
     else
-      import(args.merge(data: data, workspace: wspace.name))
+      import(opts.merge(data: data, workspace: wspace.name))
     end
   end
 

lib/msf/core/db_manager/import/metasploit_framework/xml.rb

@@ -229,6 +229,8 @@ def import_msf_web_vuln_element(element, options={}, &notifier)
   def import_msf_xml(args={}, &block)
     data = args[:data]
     wspace = Msf::Util::DBManager.process_opts_workspace(args, framework).name
+    args = args.clone()
+    args.delete(:workspace)
     bl = validate_ips(args[:blacklist]) ? args[:blacklist].split : []
 
     doc = Nokogiri::XML::Reader.from_memory(data)

lib/msf/core/db_manager/import/metasploit_framework/zip.rb

@@ -4,6 +4,8 @@ module Msf::DBManager::Import::MetasploitFramework::Zip
   def import_msf_collateral(args={}, &block)
     data = ::File.open(args[:filename], "rb") {|f| f.read(f.stat.size)}
     wspace = Msf::Util::DBManager.process_opts_workspace(args, framework).name
+    args = args.clone()
+    args.detele(:workspace)
     bl = validate_ips(args[:blacklist]) ? args[:blacklist].split : []
     basedir = args[:basedir] || args['basedir'] || ::File.join(Msf::Config.data_directory, "msf")
 

lib/msf/core/db_manager/login.rb

@@ -8,6 +8,7 @@ def logins(opts)
   def update_login(opts)
     ::ActiveRecord::Base.connection_pool.with_connection {
       wspace = Msf::Util::DBManager.process_opts_workspace(opts, framework, false)
+      opts = opts.clone()
       opts[:workspace] = wspace if wspace
       id = opts.delete(:id)
       login = Metasploit::Credential::Login.find(id)

lib/msf/core/db_manager/loot.rb

@@ -23,6 +23,8 @@ def loots(opts)
       data = opts.delete(:data)
 
       wspace = Msf::Util::DBManager.process_opts_workspace(opts, framework)
+      opts = opts.clone()
+      opts.delete(:workspace)
       opts[:workspace_id] = wspace.id
 
       if search_term && !search_term.empty?
@@ -46,6 +48,8 @@ def report_loot(opts)
     return if not active
   ::ActiveRecord::Base.connection_pool.with_connection {
     wspace = Msf::Util::DBManager.process_opts_workspace(opts, framework)
+    opts = opts.clone()
+    opts.delete(:workspace)
     path = opts.delete(:path) || (raise RuntimeError, "A loot :path is required")
 
     host = nil
@@ -101,6 +105,8 @@ def update_loot(opts)
       wspace = Msf::Util::DBManager.process_opts_workspace(opts, framework, false)
       # Prevent changing the data field to ensure the file contents remain the same as what was originally looted.
       raise ArgumentError, "Updating the data attribute is not permitted." if opts[:data]
+      opts = opts.clone()
+      opts.delete(:workspace)
       opts[:workspace] = wspace if wspace
 
       id = opts.delete(:id)