Always use module cache for searching #13998

adfoster-r7 · 2020-08-14T17:40:26Z

This pull request updates the slow module search to use the module cache.

Verification

1 - Console search

Open the console, and ensure that normal search functionality works as expected:

search monkey
search author:egypt arch:x64
search psexec author:egypt arch:x64

2 - rpcd

Create an instance of msfrpcd in one tab:

bundle exec ruby ./msfrpcd -P foo -f

In another create the client:

bundle exec ./ruby msfrpc -P foo -a 127.0.0.1

Ensure search works as expected, and aligns with the existing msfconsole search functionality:

rpc.call("module.search", "monkey")
rpc.call("module.search", "author:egypt arch:x64")
rpc.call("module.search", "psexec author:egypt arch:x64")

3 - json web service

The existing webservice should not be impacted by this code change, but for a sanity check - run the service with:

bundle exec thin --rackup msf-json-rpc.ru --address localhost --port 8081 --environment production --tag msf-json-rpc start

Run some search queries:

curl --request POST --url http://localhost:8081/api/v1/json-rpc --header 'content-type: application/json' --data '{ "jsonrpc": "2.0", "method": "module.search", "id": 1, "params": ["monkey"] }'
curl --request POST --url http://localhost:8081/api/v1/json-rpc --header 'content-type: application/json' --data '{ "jsonrpc": "2.0", "method": "module.search", "id": 1, "params": ["author:egypt arch:x64"] }'
curl --request POST --url http://localhost:8081/api/v1/json-rpc --header 'content-type: application/json' --data '{ "jsonrpc": "2.0", "method": "module.search", "id": 1, "params": ["psexec author:egypt arch:x64"] }'

adfoster-r7 · 2020-08-14T17:41:56Z

lib/msf/core/module.rb

@@ -57,7 +57,6 @@ class Module
    include Msf::Module::Options
    include Msf::Module::Privileged
    include Msf::Module::Ranking
-    include Msf::Module::Search


I believe this is dead, and was only being used by tests

adfoster-r7 · 2020-08-14T17:42:45Z

lib/msf/core/modules/metadata/search.rb

+  # {"platform"=>[["android"], []]} will match modules targeting the android platform
+  # {"platform"=>[[], ["android"]]} will exclude modules targeting the android platform
+  #
+  def self.parse_search_string(search_string)


This is the existing parse string functionality lifted from command_dispatcher/modules.rb

adfoster-r7 · 2020-08-14T17:45:10Z

spec/lib/msf/core/modules/metadata/search_spec.rb

-  describe '#search_filter' do
+require 'spec_helper'
+
+RSpec.describe Msf::Modules::Metadata::Search do


I've decided to port over the existing slow search tests and I've tried to keep as close to the original specification as I could, to ensure there was no unintentional regressions introduced. In a future PR I'll most likely expand/change these tests.

adfoster-r7 · 2020-08-14T17:46:47Z

spec/lib/msf/core/modules/metadata/search_spec.rb

-          it "should reject a query containing '-#{query}'" do
-            expect(subject.search_filter("-#{query}")).to be_truthy
+          it "should reject an inverse query containing of '#{query}'" do
+            expect(find(inverse_query_terms(query))).to be_empty


The new search cache has stricter validation, this test was previously producing invalid query strings which aren't supported by the search mechanism such as:

-os:osx

For the scope of these tests, the new inverse_query_terms just needs to support the translation to negating terms:

os:-osx

adfoster-r7 · 2020-08-14T17:47:20Z

spec/lib/msf/core/modules/metadata/search_spec.rb

        end

        unless opts.has_key?(:test_inverse) and not opts[:test_inverse]
-          it "should accept a query containing '-#{query}'" do
-            expect(subject.search_filter("-#{query}")).to be_truthy # what? why?


expect(subject.search_filter("-#{query}")).to be_truthy # what? why?

I've updated this to verify the exact module that's been returned, rather than using be_truthy

adfoster-r7 · 2020-08-14T17:48:01Z

spec/lib/msf/core/modules/metadata/search_spec.rb

@@ -72,65 +107,63 @@
    end

    context 'on a module with the author "joev"' do
-      let(:opts) { ({ 'Author' => ['joev'] }) }
+      let(:opts) { ({ 'author' => ['joev'] }) }


Previously these opts were used to hydrate a module, now they're hydrating the search cache - as a result I've had to update some of the keys.

adfoster-r7 · 2020-08-14T17:53:28Z

lib/msf/core/modules/metadata/search.rb

  #
  # Searches the module metadata using the passed hash of search params
  #
  def find(params, fields={})
-    raise ArgumentError if params.empty? || VALID_PARAMS.none? { |k| params.key?(k) }
+    raise ArgumentError if params.any? && VALID_PARAMS.none? { |k| params.key?(k) }


An empty search query will now result in all modules being returned, which aligns with the previous search functionality offered by framework's slow search / the old rpc search - #8606

The console never went down this path, as there's a guard clause in the dispatcher:

metasploit-framework/lib/msf/ui/console/command_dispatcher/modules.rb

Lines 411 to 418 in 2e887a8

if args.empty?

if @module_search_results.empty?

cmd_search_help

return false

end

cached = true

end

So this feels like a safe/valid change to make 🤞

dwelch-r7 · 2020-08-18T11:36:38Z

lib/msf/core/modules/metadata/search.rb

+  # {"platform"=>[[], ["android"]]} will exclude modules targeting the android platform
+  #
+  def self.parse_search_string(search_string)
+    search_string += ' '


not sure why you're adding a space to the end of this only to split on spaces on the next line, it doesn't make a difference right?

This was copied from the original implementation, I noticed this problem on the RPC service, with the input:

author:egypt arch:x64

This parsing method gives the following result:

=> {"author"=>[["egypt arch:x64"], []]}

With the extra space appended, it gives the expected result of:

=> {"author"=>[["egypt"], []], "arch"=>[["x64"], []]}

I'll add a unit test to make sure this scenario is caught correctly.

Tests added. The tests were red without this line:

Failure/Error: it { expect(described_class.parse_search_string("author:egypt arch:x64")).to eq({"author"=>[["egypt"], []], "arch"=>[["x64"], []]}) } expected: {"arch"=>[["x64"], []], "author"=>[["egypt"], []]} got: {"author"=>[["egypt arch:x64"], []]} (compared using ==) Diff: @@ -1,3 +1,2 @@ -"arch" => [["x64"], []], -"author" => [["egypt"], []], +"author" => [["egypt arch:x64"], []],

And now it's green with the line added back in 👍

metasploit-framework/lib/msf/ui/console/command_dispatcher/modules.rb

Line 496 in 9bd98f9

def parse_search_string(search_string)

isn't this where you got the code from? unless I'm missing something no space is being added to the search term

The addition of the space is from the slow search that I'm unifying:

metasploit-framework/lib/msf/core/module/search.rb

Lines 12 to 16 in 9bd98f9

def search_filter(search_string)

return false if not search_string

search_string += " "

ah gotcha, that one has a guard clause, does that need added in here too? or are we stopping nil being passed in from elsewhere

It's technically possible to provide nil via a message pack rpc call. I've updated this to handle nil, and added tests for an entirely empty string too. A nil search string will now result in empty search params, and thus return all modules.

adfoster-r7 · 2020-08-19T14:02:51Z

Release notes

Greatly improved the performance of Metasploit Framework's module.search rpc call by searching the module cache instead of Framework's previous slow search functionality.

adfoster-r7 commented Aug 14, 2020

View reviewed changes

Always use module cache for searching

baa33df

adfoster-r7 force-pushed the always-use-module-cache-for-searching branch from ad020b2 to baa33df Compare August 14, 2020 18:15

dwelch-r7 reviewed Aug 18, 2020

View reviewed changes

adfoster-r7 force-pushed the always-use-module-cache-for-searching branch 2 times, most recently from 341ca70 to 0491d34 Compare August 19, 2020 08:35

Add additional tests for edge cases

f8523cb

adfoster-r7 force-pushed the always-use-module-cache-for-searching branch from 0491d34 to f8523cb Compare August 19, 2020 08:37

dwelch-r7 merged commit 3d1eba2 into rapid7:master Aug 19, 2020

dwelch-r7 added enhancement rn-no-release-notes no release notes labels Aug 19, 2020

adfoster-r7 deleted the always-use-module-cache-for-searching branch August 19, 2020 14:00

adfoster-r7 added rn-enhancement release notes enhancement and removed rn-no-release-notes no release notes labels Aug 19, 2020

dwelch-r7 mentioned this pull request Dec 2, 2020

RPC API module.search is significantly slower than msfconsole's search command #10586

Closed

adfoster-r7 mentioned this pull request Mar 23, 2021

Add time command #14938

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always use module cache for searching #13998

Always use module cache for searching #13998

adfoster-r7 commented Aug 14, 2020 •

edited

Loading

adfoster-r7 Aug 14, 2020

adfoster-r7 Aug 14, 2020

adfoster-r7 Aug 14, 2020

adfoster-r7 Aug 14, 2020 •

edited

Loading

adfoster-r7 Aug 14, 2020

adfoster-r7 Aug 14, 2020

adfoster-r7 Aug 14, 2020

dwelch-r7 Aug 18, 2020

adfoster-r7 Aug 18, 2020

adfoster-r7 Aug 18, 2020

dwelch-r7 Aug 18, 2020

adfoster-r7 Aug 18, 2020 •

edited

Loading

dwelch-r7 Aug 18, 2020

adfoster-r7 Aug 19, 2020

adfoster-r7 commented Aug 19, 2020 •

edited by pbarry-r7

Loading

	if args.empty?
	if @module_search_results.empty?
	cmd_search_help
	return false
	end

	cached = true
	end

	def search_filter(search_string)
	return false if not search_string

	search_string += " "

Always use module cache for searching #13998

Always use module cache for searching #13998

Conversation

adfoster-r7 commented Aug 14, 2020 • edited Loading

Verification

1 - Console search

2 - rpcd

3 - json web service

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adfoster-r7 Aug 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adfoster-r7 Aug 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adfoster-r7 commented Aug 19, 2020 • edited by pbarry-r7 Loading

Release notes

adfoster-r7 commented Aug 14, 2020 •

edited

Loading

adfoster-r7 Aug 14, 2020 •

edited

Loading

adfoster-r7 Aug 18, 2020 •

edited

Loading

adfoster-r7 commented Aug 19, 2020 •

edited by pbarry-r7

Loading