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

Reduce Unknown commands handling from 1 second to 0.5 seconds #13148

Merged
merged 3 commits into from
Apr 1, 2020

Conversation

agalway-r7
Copy link
Contributor

@agalway-r7 agalway-r7 commented Mar 26, 2020

Cuts down unknown command time from 1 second to 0.5 seconds

Creates a class instance key for signing JARs. This cuts down on creation time for new android modules and as a result speeds up boot and incorrect command reset times by 0.5 seconds.

Verification

  • Start msfconsole
  • jimbob
  • [-] Unknown command: jimbob. Should be output instantly
  • exploit/windows/misc/crosschex_device_bof
  • [-] Unknown command: exploit/windows/misc/crosschex_device_bof.
  • This is a module we can load. Do you want to use exploit/windows/misc/crosschex_device_bof? [y/N] Should be output instantly

TL;DR

  • Implementing a constant signing key of size 2048 cuts down incorrect command time down by half. Still feels a little slow but it’s a definite improvement.
  • Constant signing key of size 1024 seems to improve speed by roughly 0.1s over size 2048. Whether it’s worth it depends on what we get out of the larger key. IMO I’d keep the larger key, with all the difference cutting down the size makes in time.
  • Using the key lookup is by far the fastest method, by virtue of avoiding module creation and JAR signing altogether. However I’ve no idea if that breaks any functionality so can’t recommend it atm. Most likely another PR.

@@ -96,7 +96,7 @@ def create(name, aliased_as: nil)
module_instance = create(aliased, aliased_as: "#{type}/#{name}")
else
module_reference_name = names.join("/")
module_instance = set.create(module_reference_name)
module_instance = set.has_key?(module_reference_name) ? set.create(module_reference_name) : nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function that takes ~1 second to execute is the PayloadSet's recalculate method. There's a comment on the code that this approach may not work as we expect:

###
#
# This class is a special case of the generic module set class because
# payloads are generated in terms of combinations between various
# components, such as a stager and a stage. As such, the payload set
# needs to be built on the fly and cannot be simply matched one-to-one
# with a payload module. Yeah, the term module is kind of overloaded
# here, but eat it!
#
###
class PayloadSet < ModuleSet

We might need a different approach to the above, but it's worth double checking.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Poking further at this; I'm concerned that this approach breaks the law of demeter, and may not work well with some of the assumptions that are already in place within framework. In particular it seems like module set already has some nil handling in place:

# If there is no module associated with this class, then try to demand
# load it.
if klass.nil? or klass == Msf::SymbolicModule
framework.modules.load_cached_module(module_type, reference_name)
recalculate
klass = fetch(reference_name, nil)
end

Which is actually why the slow recalculate method gets called in the first place. It might be worth doing a bit of git fu to learn why that logic was added in the first place, to make sure this approach doesn't break existing logic etc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cross referencing some discussion on bootup/missing command performance - #7292 (comment)

@@ -9,6 +9,8 @@ module Msf::Payload::Android
include Msf::Payload::TransportConfig
include Msf::Payload::UUID::Options

@@jar_signing_key = OpenSSL::PKey::RSA.new(2048)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on making this lazy instantiated? i.e. Loading this module doesn't create a key, but the first access will:

def signing_key
  @@signing_key ||= OpenSSL::PKey::RSA.new(2048)
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, will add it in 👍

@@ -65,11 +68,13 @@ def generate_config(opts={})
config
end

def sign_jar(jar)
def sign_jar(jar, new_key)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm; Maybe drop the new_key argument here until there's a valid use case for it? 🤔

Just thinking: If we let this becomes configurable, and every android payload starts generating its own key - then we're back to the same position as before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true ^^ I'll save it for when it's 100% needed

@adfoster-r7 adfoster-r7 added the blocked Blocked by one or more additional tasks label Mar 30, 2020
@agalway-r7
Copy link
Contributor Author

agalway-r7 commented Mar 30, 2020

Using this to time creation of all module types:

start = Time.now
module_instance = set.create(module_reference_name)
puts Time.now - start

These are the unchanged times from master:

0.001121
1.009482 <-- Payload Set (Includes JAR Signing Code)
0.002953
0.001575
0.001481
0.001276
0.000813

Time with cached 2048 JAR signing key:

0.000993
0.497179 <-- Payload Set (Includes JAR Signing Code)
0.001081
0.000687
0.000395
0.000522
0.000399

Time with cached 1024 JAR signing key:

0.001161
0.350745 <-- Payload Set (Includes JAR Signing Code)
0.001392
0.000904
0.000475
0.000554
0.000436

Time with using keys in payload_set and module_set to determine if a path points to a module (Signing Key size doesn’t matter here):

4.0e-06
3.0e-06 <-- Payload Set (Includes JAR Signing Code)
2.0e-06
2.0e-06
2.0e-06
2.0e-06
1.0e-06

Conclusions:

  • Implementing a constant signing key of size 2048 cuts down incorrect command time down by half. Still feels a little slow but it’s a definite improvement.
  • Constant signing key of size 1024 seems to improve speed by roughly 0.1s over size 2048. Whether it’s worth it depends on what we get out of the larger key. IMO I’d keep the larger key, with all the difference cutting down the size makes in time.
  • Using the key lookup is by far the fastest method, by virtue of avoiding module creation and JAR signing altogether. However I’ve no idea if that breaks any functionality so can’t recommend it atm. Most likely another PR.

@adfoster-r7 adfoster-r7 changed the title Unknown commands now instantly return output Reduce Unknown commands handling from 1 second to 0.5 seconds Mar 31, 2020
@adfoster-r7 adfoster-r7 removed the blocked Blocked by one or more additional tasks label Mar 31, 2020
@timwr
Copy link
Contributor

timwr commented Apr 1, 2020

Great work! This seems to reduce the startup time (but it's similar to #13171 )

$ git checkout fb06bc0
$ time for i in $(seq 1 5); do msfconsole -qx "exit"; done

real    0m29.137s
user    0m25.340s
sys     0m3.491s
$ git checkout master
Previous HEAD position was fb06bc09f5 remove new_key option and lazy loads static key
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.
$ time for i in $(seq 1 5); do msfconsole -qx "exit"; done

real    0m33.000s
user    0m28.664s
sys     0m4.001s
$ git checkout fix_android_pkey
Switched to branch 'fix_android_pkey'
$ time for i in $(seq 1 5); do msfconsole -qx "exit"; done

real    0m29.917s
user    0m25.692s
sys     0m3.917s
$

I'm going to land this. If after landing this #13171 still makes a noticeable difference we can consider landing that too.

@timwr timwr merged commit 90d4351 into rapid7:master Apr 1, 2020
@cdelafuente-r7
Copy link
Contributor

cdelafuente-r7 commented Apr 3, 2020

Release Notes

This reduces unknown commands handling from 1 second to 0.5 seconds for Android payloads.

@tperry-r7 tperry-r7 added the rn-enhancement release notes enhancement label Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy enhancement rn-enhancement release notes enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants