-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
Leverage the module metadata cache in the module_sets #18704
Leverage the module metadata cache in the module_sets #18704
Conversation
Can we add an extra test slice with the feature flag enabled, similar to metasploit-framework/spec/spec_helper.rb Lines 162 to 166 in 57f97ac
I didn't implement anything generic at the time, but maybe we should? 🤷 It might make sense to add this to the acceptance tests too, your call - metasploit-framework/.github/workflows/acceptance.yml Lines 58 to 77 in 57f97ac
|
After we wire up those feature flags to be set during tests, we can regenerate the hyperfine before/after timings afterwards to confirm there's not a shift in performance |
@@ -17,6 +17,11 @@ | |||
module_type: 'encoder', | |||
modules_path: modules_path, | |||
) | |||
allow(framework.encoders).to receive(:each_module_ranked) | |||
.and_yield('x86/shikata_ga_nai', framework.encoders['x86/shikata_ga_nai']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any additional context to this change
Just wondering if it's a a workaround for an implementation change that we're fixing in tests? 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, so the test was originally just loading these 4 modules into the encoders
module set, and since those were the only 4 loaded only they would be available, I'm just updating the mocking to only have these same 4 modules being returned from each_module_ranked
which now uses the metadata to get its list of modules rather than from what modules are loaded into the module set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh so expect_to_load_module_ancestors
actually has the side effect of forcing modules to load
I don't feel super comfortable having the each_module_ranked
call mocked here, as it feels like the existing test is verifying a lot of metasploit (if we're treating this as more of an integration test). Not a hard blocker for me, it's just we don't have existing tests for each_module_ranked
from what I can see from a 3 second look 🕵️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you be more okay with mocking out the call to getting the metadata instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are your thoughts on this? As well as changing the spec/lib/msf/core/module_set_spec.rb
test to verify that ranked_modules
is actually deterministic
diff --git a/lib/msf/core/module_set.rb b/lib/msf/core/module_set.rb
index 175a32adc4..7ad1207411 100644
--- a/lib/msf/core/module_set.rb
+++ b/lib/msf/core/module_set.rb
@@ -274,7 +274,7 @@ class Msf::ModuleSet < Hash
# @return [Array<Array<String, Class>>] Array of arrays where the inner array is a pair of the module reference name
# and the module class.
def rank_modules
- module_metadata.sort_by { |refname, _metadata| module_rank(refname) }.reverse!
+ module_metadata.sort_by { |refname, _metadata| "#{module_rank(refname)}-#{refname}" }.reverse!
end
# Retrieves the rank from a loaded, not-yet-loaded, or unloadable Metasploit Module.
diff --git a/spec/lib/msf/core/encoded_payload_spec.rb b/spec/lib/msf/core/encoded_payload_spec.rb
index 88f9dcb823..cbea72498d 100644
--- a/spec/lib/msf/core/encoded_payload_spec.rb
+++ b/spec/lib/msf/core/encoded_payload_spec.rb
@@ -5,23 +5,28 @@ RSpec.describe Msf::EncodedPayload do
include_context 'Msf::Simple::Framework#modules loading'
before do
+ ancestor_reference_names = [
+ # Excellent rank
+ 'x86/shikata_ga_nai',
+ # Great rank
+ 'x86/call4_dword_xor',
+ 'x86/xor_dynamic',
+ 'generic/none',
+ ]
expect_to_load_module_ancestors(
- ancestor_reference_names: [
- # Excellent rank
- 'x86/shikata_ga_nai',
- # Great rank
- 'x86/call4_dword_xor',
- 'x86/xor_dynamic',
- 'generic/none',
- ],
+ ancestor_reference_names: ancestor_reference_names,
module_type: 'encoder',
modules_path: modules_path,
)
- allow(framework.encoders).to receive(:each_module_ranked)
- .and_yield('x86/shikata_ga_nai', framework.encoders['x86/shikata_ga_nai'])
- .and_yield('x86/xor_dynamic', framework.encoders['x86/xor_dynamic'])
- .and_yield('x86/call4_dword_xor', framework.encoders['x86/call4_dword_xor'])
- .and_yield('generic/none', framework.encoders['generic/none'])
+
+ # Improve test performance - return only the test modules that we're interested in
+ allow(framework.encoders).to receive(:rank_modules).and_wrap_original do |original, *args|
+ ranked_modules = original.call(*args)
+
+ ranked_modules.select do |ref_name, _metadata|
+ ancestor_reference_names.include?(ref_name)
+ end
+ end
end
let(:ancestor_reference_names) {
🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totally agree it's an improvement, have applied the patch
BeforeModule types relevant to the flag were not tab completed:
AfterAuto completion
|
def module_metadata(type) | ||
@mutex.synchronize do | ||
wait_for_load | ||
# TODO: Should probably figure out a way to cache this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be removed?
allow(subject).to receive(:create).with(module_refname) | ||
end | ||
|
||
# TODO: it's unexpected that `fetch` and `[]` would act this differently |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
lib/msf/core/module_set.rb
Outdated
@@ -304,33 +274,31 @@ def each_module_list(ary, opts, &block) | |||
# @return [Array<Array<String, Class>>] Array of arrays where the inner array is a pair of the module reference name | |||
# and the module class. | |||
def rank_modules | |||
self.sort_by { |pair| module_rank(*pair) }.reverse! | |||
module_metadata.sort_by { |refname, _metadata| "#{module_rank(refname)}-#{refname}" }.reverse! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is super slow now; it seems like the sort block calls off to module_rank(refname)
which then does a lookup on the module metadata - which is now behind a mutex lock and creation of an array:
# This file
def rank_modules
module_metadata.sort_by { |refname, _metadata| "#{module_rank(refname)}-#{refname}" }.reverse!
end
def module_rank(reference_name)
module_metadata[reference_name].rank || Msf::NormalRanking
end
# Elsewhere
def module_metadata(type)
@mutex.synchronize do
wait_for_load
@module_metadata_cache.filter_map { |_, metadata| [metadata.ref_name, metadata] if metadata.type == type }.to_h
end
end
I believe this should be faster, but it's worth confirming it's semantically the same:
module_metadata.sort_by { |refname, metadata| [metadata.rank, refname] }.reverse!
Tested with:
require 'benchmark/ips'
require 'msfenv'
framework = Msf::Simple::Framework.create
Benchmark.ips do |x|
# Configure the number of seconds used during
# the warmup phase (default 2) and calculation phase (default 5)
x.config(:time => 15, :warmup => 4)
# These parameters can also be configured this way
x.time = 5
x.warmup = 2
x.report("rank_modules_string") do
framework.modules.payloads.rank_modules_string.length
end
x.report("rank_modules_array") do
framework.modules.payloads.rank_modules_array.length
end
x.report("rank_modules_array_without_lock") do
framework.modules.payloads.rank_modules_array_without_lock.length
end
x.compare!
end
Results:
Warming up --------------------------------------
rank_modules_string 1.000 i/100ms
rank_modules_array 1.000 i/100ms
rank_modules_array_without_lock
15.000 i/100ms
Calculating -------------------------------------
rank_modules_string 0.392 (± 0.0%) i/s - 3.000 in 7.716404s
rank_modules_array 0.426 (± 0.0%) i/s - 3.000 in 7.055821s
rank_modules_array_without_lock
151.488 (± 5.9%) i/s - 765.000 in 5.068616s
Comparison:
rank_modules_array_without_lock: 151.5 i/s
rank_modules_array: 0.4 i/s - 355.61x slower
rank_modules_string: 0.4 i/s - 386.61x slower
Code diff that I tested with:
diff --git a/lib/msf/core/module_set.rb b/lib/msf/core/module_set.rb
index 175a32adc4..a6b297a22a 100644
--- a/lib/msf/core/module_set.rb
+++ b/lib/msf/core/module_set.rb
@@ -196,7 +196,6 @@ class Msf::ModuleSet < Hash
module_metadata.keys
end
- protected
# Enumerates the modules in the supplied array with possible limiting factors.
#
@@ -268,13 +267,25 @@ class Msf::ModuleSet < Hash
# @return [String] type of modules
attr_writer :module_type
+ def rank_modules
+ rank_modules_array_without_lock
+ end
+
# Ranks modules based on their constant rank value, if they have one. Modules without a Rank are treated as if they
# had {Msf::NormalRanking} for Rank.
#
# @return [Array<Array<String, Class>>] Array of arrays where the inner array is a pair of the module reference name
# and the module class.
- def rank_modules
- module_metadata.sort_by { |refname, _metadata| module_rank(refname) }.reverse!
+ def rank_modules_string
+ module_metadata.sort_by { |refname, _metadata| "#{module_rank(refname)}-#{refname}" }.reverse!
+ end
+
+ def rank_modules_array
+ module_metadata.sort_by { |refname, _metadata| [module_rank(refname), refname] }.reverse!
+ end
+
+ def rank_modules_array_without_lock
+ module_metadata.sort_by { |refname, metadata| [metadata.rank, refname] }.reverse!
end
# Retrieves the rank from a loaded, not-yet-loaded, or unloadable Metasploit Module.
attr_reader :platform | ||
# @return [Msf::Module::PlatformList] | ||
attr_reader :platform_list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
attr_reader :platform_list | |
attr_reader :platforms |
Edit: I see, module instance has called it platform
- but here platform
is already defined as a string
Hm. Looks like all possible options are bad 😞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can also call platforms
on a PlatformList
so calling it platforms leads to platforms.platforms
which I didn't like either 😅
@@ -28,8 +28,10 @@ class Obj | |||
attr_reader :description | |||
# @return [Array<String>] | |||
attr_reader :references | |||
# @return [Boolean] | |||
# @return [String] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker; Should we make this deprecated? 🤔
I think this might need a few more tweaks for performance, but it looks like a good stepping stone for now 👍 |
Release NotesFixes a bug with framework having 0 registered nop modules when the defer-module-loads feature was enabled. |
This PR resolves an issues introduced when the
defer-module-loads
feature was added into frameworkessentially the issue was the module sets (payloads, exploits, post...etc) weren't being populated with the the module data that several places were referencing in framework, they would only contain any modules that had been actively loaded by the user
The fix I've made for this is to make the module sets aware of the module metadata cache and to refactor some calls using the module sets so we don't accidentally force all modules to be loaded again slowing our boot times
I was pretty conservative in doing this so there's definitely some performance left on the table here but I don't think it's much
Verification steps
use
an evasion module likewindows/applocker_evasion_install_util
and dorun -n <tab><tab>
and you should see the available nop modules auto completingrun -p
for payloads andrun -e
for encodersmodule.exploits
module.nops
etc. return all the module refnames for that typeexample rpc call:
response: