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

Implement payload size caching, speeding up framework loads #4894

Merged
merged 18 commits into from
Mar 10, 2015

Conversation

hdm
Copy link
Contributor

@hdm hdm commented Mar 8, 2015

Quick and dirty benchmark:

$ for i in `seq 1 3`; do time ./msfconsole -q -x 'exit'; done

real  0m5.829s
user  0m5.185s
sys 0m0.530s

real  0m5.897s
user  0m5.358s
sys 0m0.434s

real  0m5.593s
user  0m5.073s
sys 0m0.424s

Generate the cache:

$ ./tools/update_payload_cached_sizes.rb 

[*] /data/work/msf-hdm/modules/payloads/singles/aix/ppc/shell_bind_tcp.rb has size 264, updating cache...
[*] /data/work/msf-hdm/modules/payloads/singles/aix/ppc/shell_find_port.rb has size 220, updating cache...
[*] /data/work/msf-hdm/modules/payloads/singles/aix/ppc/shell_interact.rb has size 56, updating cache...
[*] /data/work/msf-hdm/modules/payloads/singles/aix/ppc/shell_reverse_tcp.rb has size 204, updating cache...
[*] /data/work/msf-hdm/modules/payloads/stagers/android/reverse_http.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/stagers/android/reverse_https.rb has a dynamic size, skipping...
[..]

Verify the cache:

$ ./tools/update_payload_cached_sizes.rb 

[*] /data/work/msf-hdm/modules/payloads/stagers/android/reverse_http.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/stagers/android/reverse_https.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/stagers/android/reverse_tcp.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/stagers/android/reverse_http.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/stagers/android/reverse_https.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/stagers/android/reverse_tcp.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/singles/cmd/unix/bind_netcat.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/singles/cmd/unix/reverse_bash.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/singles/cmd/unix/reverse_bash_telnet_ssl.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/singles/cmd/unix/reverse_netcat.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/singles/cmd/unix/reverse_python.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/singles/cmd/windows/download_eval_vbs.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/singles/cmd/windows/download_exec_vbs.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/singles/firefox/exec.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/singles/firefox/shell_bind_tcp.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/singles/firefox/shell_reverse_tcp.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/singles/php/bind_php.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/singles/php/bind_php_ipv6.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/singles/php/download_exec.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/singles/php/exec.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/singles/php/reverse_perl.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/singles/php/reverse_php.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/singles/php/shell_findsock.rb has a dynamic size, skipping...
[*] /data/work/msf-hdm/modules/payloads/singles/solaris/sparc/shell_find_port.rb has a dynamic size, skipping...

Bask in the glory:

$ for i in `seq 1 3`; do time ./msfconsole -q -x 'exit'; done

real  0m5.100s
user  0m4.511s
sys 0m0.483s

real  0m4.895s
user  0m4.371s
sys 0m0.438s

real  0m5.056s
user  0m4.549s
sys 0m0.427s

Spec coverage is still a TODO and will handle build-time verification of cached sizes.

This change would let us move to Metasm payload generators without a startup-time performance penalty.

@hdm hdm changed the title This implements payload size caching, speeding up framework loads Implement payload size caching, speeding up framework loads Mar 8, 2015
@bcook-r7
Copy link
Contributor

bcook-r7 commented Mar 9, 2015

I presume you're writing an automated test to compare the cached value to the real one for all the non-dynamic payloads and make sure its not stale? Maybe msftidy could be updated to notice this?

@bcook-r7
Copy link
Contributor

bcook-r7 commented Mar 9, 2015

That said, it worked well - the is_dynamic_size? check seemed like it might false-negative, e.g. if a payload rounded a string length to multiples of 32 or something, but I didn't find any actual counter-examples.

@bcook-r7
Copy link
Contributor

bcook-r7 commented Mar 9, 2015

The regex needs a little work - there is some whitespace drifting

--- a/modules/payloads/stagers/windows/reverse_http_proxy_pstore.rb
+++ b/modules/payloads/stagers/windows/reverse_http_proxy_pstore.rb
@@ -8,6 +8,16 @@ require 'msf/core/handler/reverse_http'


 module Metasploit3
+  CachedSize = 650
+
+
+
+
+
+
+
+
+

@bcook-r7
Copy link
Contributor

bcook-r7 commented Mar 9, 2015

Does this seem like a reasonable update? When we remove the previous value, we need to remove the whole line.

diff --git a/tools/update_payload_cached_sizes.rb b/tools/update_payload_cached_sizes.rb
index f6db518..699e3d7 100755
--- a/tools/update_payload_cached_sizes.rb
+++ b/tools/update_payload_cached_sizes.rb
@@ -41,8 +41,8 @@ end
 def update_cache_size(mod)
   data = ''
   File.open(mod.file_path, 'rb'){|fd| data = fd.read(fd.stat.size)}
-  data = data.gsub(/^\s*CachedSize\s*=\s*\d+.*/, '')
-  data = data.gsub(/^(module Metasploit\d+)/) {|m| "#{m}\n  CachedSize = #{mod.new.size}\n" }
+  data = data.gsub(/^\s*CachedSize\s*=\s*\d+.*\n/, '')
+  data = data.gsub(/^(module Metasploit\d+)/) {|m| "#{m}\n  CachedSize = #{mod.new.size}" }
   File.open(mod.file_path, 'wb'){|fd| fd.write(data) }
 end

Result is a little prettier:

--- a/modules/payloads/stagers/windows/reverse_http_proxy_pstore.rb
+++ b/modules/payloads/stagers/windows/reverse_http_proxy_pstore.rb
@@ -8,6 +8,7 @@ require 'msf/core/handler/reverse_http'


 module Metasploit3
+  CachedSize = 650

@hdm
Copy link
Contributor Author

hdm commented Mar 9, 2015

Fixing this and some other open issues in an imminent update. Still missing specs too.

@hdm
Copy link
Contributor Author

hdm commented Mar 9, 2015

The previous commit should fail the build, as ./tools/update_payload_cached_sizes.rb has not been run across the modules.

@hdm
Copy link
Contributor Author

hdm commented Mar 9, 2015

Builds fail because datastore defaults like LHOST change based on the system they are running on. This has been a problem for a while, so fixing it will be a nice improvement all around.

@hdm
Copy link
Contributor Author

hdm commented Mar 9, 2015

The previous commits handle cases where the datastore defaults to something based on the host, but it doesn't solve the real problem, which is that the generate() call during startup needs a default datastore that represents the largest-size output.

I will file a separate ticket for that soon, in the meantime, this is good to land.

@bcook-r7
Copy link
Contributor

Sounds good.

Solving for the maximum size will be interesting. For instance, the max LHOST for the reverse_http stagers above would actually be [0000:0000:0000:0000:0000:0000:0000:0000], since they work for ipv6 too. Some metadata for max parameter lengths would be nice, since currently that info currently hard to introspect, e.g. reverse_tcp_dns.rb's exception when LHOST > 63.

@bcook-r7 bcook-r7 merged commit 618fbf0 into rapid7:master Mar 10, 2015
bcook-r7 pushed a commit that referenced this pull request Mar 10, 2015
Avoid the hit of regenerating all of the static-size payloads when
loading the framework. This will facilitate conversion of payloads to
use metasm later.
@hdm
Copy link
Contributor Author

hdm commented Mar 10, 2015

@bcook-r7 Thanks! Definitely agree, we need to do more work on byte-size prediction for payloads. We have only seen this result in bugs in corner cases (options bring a payload + encoder just over the line), but definitely worth solving.

hdm pushed a commit to hdm/metasploit-framework that referenced this pull request Mar 10, 2015
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 618fbf0 on hdm:feature/payload-cached-size into * on rapid7:master*.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants